Add checkup for return statement outside of a function by GuillaumeGomez · Pull Request #37780 · rust-lang/rust

GuillaumeGomez · 2016-11-15T15:12:10Z

Fixes #37778.

r? @eddyb (don't know who is in charge. Don't hesitate to set another one)

cc @jonathandturner

eddyb · 2016-11-15T16:09:39Z

Please use an informative title, the error number means nothing.
LGTM. r? @nikomatsakis on the choice of fix.

GuillaumeGomez · 2016-11-15T16:25:41Z

Didn't pay attention, I thought it took first commit's message by default. My bad.

durka · 2016-11-15T18:08:26Z

returning a value in a function that returns ()

nikomatsakis · 2016-11-15T20:00:33Z

How about "return can only be used inside a function body"?

It seems a bit odd to do this in regionck ... but I see that for (e.g.) a constant we use the type of the constant as the return type. That seems strange to me! I wonder how hard it would be to change that -- I'd rather have FnCtxt have a Option<Ty<'tcx>> as the return type, or at least do the flag in librustc_typeck/check/mod.rs and not regionck.

What do you think @eddyb ?

I'm fine with banning it earlier - return for constant values only makes sense in MIR anyway.

So I guess it's not getting merged any time soon... :-/

Just have to move it to ExprRet type-checking, should be straight-forward (except for the condition not being region-related).

durka · 2016-11-15T20:07:50Z

return type mismatch now :)

leoyvens · 2016-11-15T20:55:55Z

Wording is not the best here. return foo is an expression according to the reference, and in the example you want to remove only the return and not the entire statement.

Considering the statement/expression grammatical categories, return is an expression according to the reference. So to say "remove the return statement" is imprecise because it's an expression, and misleading because the user might try to remove the entire expression when he should only remove the return keyword.

Oh I see now. I really didn't understand the first time. :-/

Thanks! :)

nikomatsakis

Looking better! :) I left some suggestions on wording and also a request to move the ret_ty as a parameter to new

nikomatsakis · 2016-11-16T23:33:12Z

Nit: "return statement outside of function body" feels like clearer wording to me. "cannot be out of a function scope" doesn't quite seem grammatical; also, the term "fn scope" is not (I think) quite so standard.

nikomatsakis · 2016-11-16T23:33:31Z

Nit: "A return statement was found outside of a function body."

nikomatsakis · 2016-11-16T23:33:49Z

(This would have to be updated too)

nikomatsakis · 2016-11-16T23:35:32Z

Huh. It feels weird to me to not have the ret_ty get passed into the constructor. I see that we (one time) reassigned it later, but now it seems like we almost always do. I think I'd rather we continue to pass in a Option<> into new as an argument.

It has to be computed after the fcx is created anyway, AFAICT (not here though).

From my changes, it was always updated outside so it doesn't change much AFAICT.

The majority of cases do not need to recompute it, from what I can tell. Basically 3/4 :). In one of those cases, the default of None suffices. So I think it's better to pass it as an argument -- harder to forget that you may want to assign it.

It may well be though that in those other cases, None would also suffice-- I suspect so.

bors · 2016-11-17T12:42:17Z

☔ The latest upstream changes (presumably #37717) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2016-11-17T16:14:54Z

Updated.

nikomatsakis · 2016-11-17T19:29:01Z

here it does not need to be recomputed

nikomatsakis · 2016-11-17T19:29:09Z

here it does not need to be recomputed

nikomatsakis · 2016-11-17T19:29:52Z

The majority of cases do not need to recompute it, from what I can tell. Basically 3/4 :). In one of those cases, the default of None suffices. So I think it's better to pass it as an argument -- harder to forget that you may want to assign it.

nikomatsakis · 2016-11-17T19:30:25Z

It may well be though that in those other cases, None would also suffice-- I suspect so.

GuillaumeGomez · 2016-11-17T21:58:22Z

@bors: r=nikomatsakis

bors · 2016-11-17T21:58:22Z

📌 Commit dd938bc has been approved by nikomatsakis

bors · 2016-11-19T02:19:12Z

⌛ Testing commit dd938bc with merge f42e53f...

bors · 2016-11-19T03:21:46Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

GuillaumeGomez · 2016-11-19T15:47:38Z

@nikomatsakis: @eddyb: We have quite a huge issue here in src/test/ui/span/loan-extend.rs:

warning: broken MIR (_0 = ()): bad assignment (impl std::marker::Copy = ()): Sorts(ExpectedFound { expected: impl std::marker::Copy, found: () })
  --> src/test/ui/span/loan-extend.rs:14:47
   |
14 | fn borrow<'a, T>(_: &'a mut T) -> impl Copy { () }
   |                                               ^^

eddyb · 2016-11-19T15:59:21Z

I wonder if the MIR sanity checker should use Reveal::All for that (or just special-case impl Trait).

GuillaumeGomez · 2016-11-24T15:37:12Z

@eddyb: Reveal::All? Is it something at a lower level?

eddyb · 2016-11-24T15:50:09Z

@rust-lang/compiler What should we do about MIR typeck and impl Trait?

nikomatsakis · 2016-11-29T16:20:36Z

@eddyb Hmm. I am not crazy about having the MIR type-checker use Reveal::All, but I guess if we don't, we would need to add some sort of cast into and out of the impl Trait existential.

I am not crazy about it because my plan is to use the MIR type-checker as the basis for region checking, and I guess that in that mode, we would want to behave like the real region checker.

I think we should use Reveal::All but add a FIXME and open an associated issue. We can fix it later.

GuillaumeGomez · 2016-12-04T07:19:48Z

So, what should I do about this?

bors · 2016-12-06T10:54:42Z

☔ The latest upstream changes (presumably #38097) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2016-12-06T23:34:15Z

@nikomatsakis: Seems like the last master update fixed the issue. Should be ready to get merged!

GuillaumeGomez · 2016-12-09T05:04:58Z

Ok, so someone added an E0571 meanwhile so I had to update mine to E0572. Should be good now.

GuillaumeGomez · 2016-12-09T17:46:37Z

cc @nikomatsakis

nikomatsakis · 2016-12-10T00:34:37Z

@GuillaumeGomez still has an X...do you see why?

jseyfried · 2016-12-10T00:44:27Z

Travis is broken -- it fails this way on all PRs before running most of the tests.

GuillaumeGomez · 2016-12-10T13:10:22Z

Just like @jseyfried said. New rustbuild issue if I'm not mistaken. I r+ in your name, don't hesitate to r- if you see anything.

@bors: r=nikomatsakis

bors · 2016-12-10T13:10:23Z

📌 Commit ed3c483 has been approved by nikomatsakis

bors · 2016-12-10T16:13:05Z

⌛ Testing commit ed3c483 with merge 576a361...

@eddyb

Add checkup for return statement outside of a function Fixes #37778. r? @eddyb (don't know who is in charge. Don't hesitate to set another one) cc @jonathandturner

bors · 2016-12-10T19:07:45Z

rust-highfive assigned eddyb Nov 15, 2016

GuillaumeGomez force-pushed the E0571 branch from 4a4a471 to a519c40 Compare November 15, 2016 15:35

rust-highfive assigned nikomatsakis and unassigned eddyb Nov 15, 2016

GuillaumeGomez changed the title ~~E0571~~ Add checkup for return statement outside of a function Nov 15, 2016

durka reviewed Nov 15, 2016

View reviewed changes

Comment thread src/librustc_typeck/diagnostics.rs Outdated

Copy link
Copy Markdown

Contributor

durka Nov 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning a value in a function that returns ()

GuillaumeGomez force-pushed the E0571 branch from a519c40 to 94d21b5 Compare November 15, 2016 19:38

nikomatsakis reviewed Nov 15, 2016

View reviewed changes

durka reviewed Nov 15, 2016

View reviewed changes

leoyvens reviewed Nov 15, 2016

View reviewed changes

GuillaumeGomez force-pushed the E0571 branch 2 times, most recently from 6442ec8 to c843fa2 Compare November 16, 2016 11:48

nikomatsakis requested changes Nov 16, 2016

View reviewed changes

GuillaumeGomez force-pushed the E0571 branch from c843fa2 to 34fe31d Compare November 17, 2016 16:14

nikomatsakis reviewed Nov 17, 2016

View reviewed changes

nikomatsakis approved these changes Nov 17, 2016

View reviewed changes

eddyb added the I-nominated label Nov 24, 2016

eddyb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 24, 2016

nikomatsakis removed the I-nominated label Dec 1, 2016

GuillaumeGomez force-pushed the E0571 branch from dd938bc to 4474f92 Compare December 6, 2016 23:33

GuillaumeGomez added 5 commits December 8, 2016 20:49

Add checkup for return statement outside of a function

5a06564

Add E0571 test

8dee5ab

Set back Ty parameter to FnCntxt

64de44e

Update to last master

b1dd793

Change error to E0572

ed3c483

GuillaumeGomez force-pushed the E0571 branch from 4474f92 to ed3c483 Compare December 9, 2016 05:04

GuillaumeGomez mentioned this pull request Dec 9, 2016

Improve E0389 "cannot borrow data mutably in a & reference" error message #38147

Closed

bors merged commit ed3c483 into rust-lang:master Dec 10, 2016

GuillaumeGomez deleted the E0571 branch December 11, 2016 21:13

Uh oh!

Conversation

GuillaumeGomez commented Nov 15, 2016

Uh oh!

eddyb commented Nov 15, 2016

Uh oh!

GuillaumeGomez commented Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Nov 17, 2016

Uh oh!

GuillaumeGomez commented Nov 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Nov 17, 2016

Uh oh!

bors commented Nov 17, 2016

Uh oh!

bors commented Nov 19, 2016

Uh oh!

bors commented Nov 19, 2016

Uh oh!

GuillaumeGomez commented Nov 19, 2016

Uh oh!

eddyb commented Nov 19, 2016

Uh oh!

GuillaumeGomez commented Nov 24, 2016

Uh oh!

GuillaumeGomez commented Nov 15, 2016 •

edited

Loading

eddyb Nov 15, 2016 •

edited

Loading

eddyb Nov 16, 2016 •

edited

Loading