Detect missing `;` on methods with return type `()` #36409

estebank · 2016-09-12T00:00:49Z

On a given file foo.rs:

fn foo() {
    return 1;
}

fn main() {
    3
}

Provide the following output:

error[E0308]: mismatched types
 --> foo.rs:2:12
  |
2 |     return 1;
  |            ^ expected (), found integral variable
  |
  = note: expected type `()`
  = note:    found type `{integer}`

error[E0308]: mismatched types
 --> foo.rs:6:5
  |
6 |     3
  |     ^ expected (), found integral variable
  |
  = note: expected type `()`
  = note:    found type `{integer}`
help: maybe you meant to have a statement ending in `;` here
 --> foo.rs:6:6
  |
6 |     3
  |      ^

error: aborting due to 2 previous errors

Fixes: #25133

rust-highfive · 2016-09-12T00:00:55Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

eddyb · 2016-09-12T04:00:44Z

r? @jonathandturner

sophiajt · 2016-09-12T15:28:39Z

Thanks for the PR!

You know, in looking at this I wonder if we can combine the two. Something like:

error[E0308]: mismatched types
 --> foo.rs:6:5
  |
6 |     3
  |     ^ expected (), possibly missing `;` at end
  |
  = note: expected type `()`
  = note:    found type `{integer}`

sophiajt · 2016-09-12T16:13:51Z

Or maybe

error[E0308]: mismatched types
 --> foo.rs:6:5
  |
6 |     3
  |     ^ expected type `()`, missing `;` at end?
  |
  = note: expected type `()`
  = note:    found type `{integer}`

nikomatsakis · 2016-09-12T16:17:14Z

One last possibility might be

error[E0308]: mismatched types
 --> foo.rs:6:5
  |
6 |     3
  |     ^- possibly missing `;` here?
  |     expected type `()`
  |
  = note: expected type `()`
  = note:    found type `{integer}`

that is, add another label -- of course this runs afoul of the case where our rendering looks wacky.

sophiajt · 2016-09-12T16:42:43Z

Of the three I vote for #36409 (comment), but take your pick :)

estebank · 2016-09-12T16:52:44Z

@jonathandturner I'll use that one, seems to be the least verbose of the options. I'll update this later today.

estebank · 2016-09-13T15:52:50Z

@jonathandturner I went with @nikomatsakis' style as it lend itself to a cleaner diff and didn't look half bad :)

error[E0308]: mismatched types
 --> unit-error.rs:6:5
  |
6 |     3
  |     ^
  |     |
  |     possibly `;` missing here?
  |     expected (), found integral variable
  |
  = note: expected type `()`
  = note:    found type `{integer}`

sophiajt · 2016-09-13T16:23:31Z

nit: I think it reads slightly better in the opposite order

error[E0308]: mismatched types
 --> unit-error.rs:6:5
  |
6 |     3
  |     ^
  |     |
  |     expected (), found integral variable
  |     possibly `;` missing here?
  |
  = note: expected type `()`
  = note:    found type `{integer}`

sophiajt · 2016-09-13T16:23:59Z

Actually, scratch that, either way is fine

estebank · 2016-09-29T01:23:42Z

@nikomatsakis

that is, add another label -- of course this runs afoul of the case where our rendering looks wacky.

I think it doesn't look half bad in this case:

error[E0308]: mismatched types
  --> /Users/ekuber/personal/rust/src/test/compile-fail/issue-20862.rs:12:5
   |
12 |     |y| x + y
   |     ^^^^^^^^^
   |     |
   |     possibly `;` missing here?
   |     expected (), found closure

@jonathandturner I'm puzzled by the test failures. For the test compile-fail/issue-20862.rs, runtest.rs on this branch sees the following errors being emitted:

[
Error { line_num: 12, kind: Some(Error), msg: "12:5: 12:14: mismatched types [E0308]" },
Error { line_num: 12, kind: Some(Error), msg: "12:5: 12:14: mismatched types [E0308]" },
Error { line_num: 12, kind: Some(Note), msg: "possibly `;` missing here?" },
Error { line_num: 12, kind: Some(Note), msg: "expected (), found closure" },
Error { line_num: 12, kind: Some(Note), msg: "12:5: 12:14: expected type `()`" },
Error { line_num: 12, kind: Some(Note), msg: "12:5: 12:14: expected type `()`" },
Error { line_num: 12, kind: Some(Note), msg: "12:5: 12:14:    found type `[closure@/Users/ekuber/personal/rust/src/test/compile-fail/issue-20862.rs:12:5: 12:14 x:_]`" },
Error { line_num: 12, kind: Some(Note), msg: "12:5: 12:14:    found type `[closure@/Users/ekuber/personal/rust/src/test/compile-fail/issue-20862.rs:12:5: 12:14 x:_]`" },
Error { line_num: 17, kind: Some(Error), msg: "17:13: 17:22: expected function, found `()`" }]

for the following compiler visual output:

error[E0308]: mismatched types
  --> /Users/ekuber/personal/rust/src/test/compile-fail/issue-20862.rs:12:5
   |
12 |     |y| x + y
   |     ^^^^^^^^^
   |     |
   |     possibly `;` missing here?
   |     expected (), found closure
   |
   = note: expected type `()`
   = note:    found type `[closure@/Users/ekuber/personal/rust/src/test/compile-fail/issue-20862.rs:12:5: 12:14 x:_]`

error: expected function, found `()`
  --> /Users/ekuber/personal/rust/src/test/compile-fail/issue-20862.rs:17:13
   |
17 |     let x = foo(5)(2);
   |             ^^^^^^^^^

error: aborting due to 2 previous errors

While in master for the same file, the errors emitted are:

[
Error { line_num: 12, kind: Some(Error), msg: "12:5: 12:14: mismatched types [E0308]" }, 
Error { line_num: 12, kind: Some(Note), msg: "expected (), found closure" }, 
Error { line_num: 12, kind: Some(Note), msg: "12:5: 12:14: expected type `()`" }, 
Error { line_num: 12, kind: Some(Note), msg: "12:5: 12:14:    found type `[closure@/Users/ekuber/personal/rust/src/test/compile-fail/issue-20862.rs:12:5: 12:14 x:_]`" }, 
Error { line_num: 17, kind: Some(Error), msg: "17:13: 17:22: expected function, found `()`" }
]

That made me think that I might have introduced a double emit() call to the same DiagnosticBuilder somewhere in my PR, but couldn't find it.

By commenting out src/librustc/infer/error_reporting.rs:554-558 (the new span_label() call), the emitted errors are the same as in master, which leads me to believe that there might be some quirk in the handling of multiple labels on the same span.

If this is indeed the case, does it sound ok if I work around it for now by duplicating the expected error messages in the affected unittests and file a ticket to fix this (either by changing how the emitter works or by introducing deduplication in runtest.rs)?

Also, compile-fail/issue-20862.rs shows a situation where this help message would be wrong, as the problem was with the original fn definition lacking the return type. Is it ok if I leave the effort to check the usages for that possibility for a different PR?

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

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

estebank · 2016-10-25T16:03:15Z

r? @jonathandturner

eddyb · 2016-10-25T16:16:29Z

src/librustc/infer/error_reporting.rs

@@ -523,7 +523,8 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                         origin: TypeOrigin,
                         secondary_span: Option<(Span, String)>,
                         values: Option<ValuePairs<'tcx>>,
-                         terr: &TypeError<'tcx>)
+                         terr: &TypeError<'tcx>,
+                         implicit_return: bool)


Could you rename this, and all other instances, to is_block_tail? It doesn't seem to be related to return at all.

@eddyb, renamed the field as requested.

eddyb · 2016-11-06T18:40:11Z

src/librustc/infer/error_reporting.rs

-                values.expected.is_primitive() && values.found.is_primitive()
+            let (is_simple_error, expected_unit) = if let &TypeError::Sorts(ref values) = terr {
+                (values.expected.is_primitive() && values.found.is_primitive(),
+                    &format!("{}", values.expected) == "()")


Doesn't .is_unit() (or I suppose .is_nil()) work here?

is_nil seems like it would the appropriate method. Running the tests now.

eddyb · 2016-11-07T07:32:26Z

src/librustc_typeck/check/demand.rs

+                Node::NodeBlock(_) => true,
+                _ => false,
+            };
+            self.report_mismatched_types_for_return(origin, expected, expr_ty, e, implicit_return);


Hmm, thinking about it more, could the logic be moved here? Or even better, in check_block_with_expectation?
~~See #37412 for how I moved a note there. Maybe I should get that PR merged first, would make this easier.~~ #37412 is merged now!

brson · 2016-11-09T01:39:51Z

I feel a need to question the premise of this change.

There's been a lot of debate here about how to tell the user to put a semicolon at the end of this expression. But in the running example, putting a semicolon after the literal "3" is not correct. It results in nonsense code.

Maybe we should think deeper about what might be going wrong when a block that is supposed to be terminated with () is not. Is it ever correct to just add a semicolon? I'm not sure, but I'd guess that is less likely to be the right solution than others because it's rare in Rust to not use a return value, so any value produced by the trailing expression is probably intended to be used, not thrown away.

nikomatsakis · 2016-11-09T01:45:48Z

@brson it happens to me ~~pretty regularly~~ that the right answer is to add a semicolon. Admittedly, not with an expression like 3. Much more common is something like map.insert(key, value). I think it will depend a lot on the kind of expression.

EDIT: I don't know how often it happens compared to other things. So I weakened the language. =) But it definitely happens.

Two caveats though:

A lot of times, this happens in closure expressions, like foo.bar(|elem| map.insert(elem, 1)), in which case I have to change it to foo.bar(|elem| { map.insert(elem, 1); }), so just adding a semicolon is not enough.
The other very common case for me personally is that I forgot to write the return type, most commonly in fn new(...) { SomeStruct { ... } }.

This last example again suggests that we might examine the kind of expression to see whether to suggest changing the return type or adding a semicolon.

estebank · 2016-11-09T01:51:42Z

@brson it happens to be pretty regularly that the right answer is to add a semicolon. Admittedly, not with an expression like 3. Much more common is something like map.insert(key, value). I think it will depend a lot on the kind of expression.

I agree with this (unsurprisingly, as I wrote the PR in the first place).

This last example again suggests that we might examine the kind of expression to see whether to suggest changing the return type or adding a semicolon.

I was just thinking about that very same case, and I think that a reasonable approach to this would be to point out both options, given that we cannot really find a single recommendation that will be right 100% of the time:

error[E0308]: mismatched types
 --> foo.rs:6:5
  |
5 | fn foo() {
  | -------- possibly return type `{integer}` missing here?
6 |     3
  |     ^
  |     |
  |     expected (), found integral variable
  |     possibly `;` missing here?
  |
  = note: expected type `()`
  = note:    found type `{integer}`

sophiajt · 2016-11-12T13:12:17Z

@estebank - yeah I'm okay with that. An alternative would be to use a suggestion:

error[E0308]: mismatched types
 --> foo.rs:6:5
  |
5 | fn foo() {
  | -------- possibly return type `{integer}` missing here?
6 |     3
  |     ^
  |     |
  |     expected (), found integral variable
  |
  = note: expected type `()`
  = note:    found type `{integer}`
suggestion:
  |
6 |     3
  |      ^ possibly missing ';' here

estebank · 2016-11-13T03:07:32Z

I've been looking around and by the time the type error is raised I have no longer access to any NodeId, which I need to find the Span for fn foo(). Is it ok if we merge this without that feature while I try to refactor the code to allow for that suggestion?

eddyb · 2016-11-13T10:20:45Z

@estebank I believe that if you follow #36409 (comment) you'll regain that ability quite readily.
I don't want to have this merged in this state if it can be simpler (as it will interact with other changes).

EDIT: Here's what I did. You'd probably want to do something similar to this line.

GuillaumeGomez · 2016-11-13T11:22:49Z

Just to say:

possibly ; missing here?

👎

possibly missing ';' here?

👍

estebank · 2016-11-14T06:24:29Z

@eddyb, Oh, my. I completely missed that comment when I got back to this PR. Just pushed a change implementing that recommendation and adding the method return type suggestion. I still need to work on this to avoid suggesting adding a return type when one is already provided, maybe suggesting changing it to the supplied type, and not to suggest anything when it is implementing a trait.

estebank · 2016-11-14T06:24:43Z

@GuillaumeGomez done.

GuillaumeGomez · 2016-11-14T09:56:21Z

src/test/compile-fail/block-must-not-have-result-do.rs

+        //~| NOTE: possibly missing `;` here?
+        //~| NOTE: expected (), found bool
+        //~| NOTE: expected type `()`
+        //~| NOTE: expected type `()`


I wonder: why these lines are always duplicated?

I didn't dig too deeply into this when I noticed it, but we should probably create a ticket for it. Could the same DiagnosticBuilder be emitter multiple times? Maybe that's what's happening...

eddyb · 2016-11-14T10:32:14Z

src/librustc/infer/mod.rs

        let trace = TypeTrace {
            origin: origin,
            values: Types(ExpectedFound {
                expected: expected,
                found: actual
            })
        };
-        self.report_and_explain_type_error(trace, &err).emit();
+        self.report_and_explain_type_error(trace, &err)
    }


If you added this function you should use it wherever report_and_explain_type_error was used for the same effect.

eddyb · 2016-11-14T10:34:23Z

src/librustc_typeck/check/demand.rs

+            let mut diag = self.mismatched_types_diag(origin, expected, expr_ty, e.clone());
+
+            if let Node::NodeBlock(block) = self.tcx.map.get(self.tcx.map.get_parent_node(expr.id)) {
+                if let TypeError::Sorts(ref values) = e {


You can move this to check_block_with_expected and it will be simpler.

eddyb · 2016-11-14T10:37:00Z

src/librustc_typeck/check/demand.rs

+                        {
+                            if let Node::NodeItem(func) = self.tcx.map
+                                .get(self.tcx.map.get_parent_node(item.id))
+                            {


I don't like this, it's too arbitrary. If you want to do this properly you'd have to add something to Expectation to indicate that the type comes from a function return type.

Take a look at the latest code , where I am still against the block's parent to see wether it is a function or a trait's method for the recommendation. Let me know if it still looks too out of place.

I also changed the label to be consistent with the existing phrasing "consider removing this semicolon"/"consider adding a semicolon here".

bors · 2016-11-17T12:38:41Z

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

On a given file `foo.rs`: ```rust fn foo() { return 1; } fn main() { 3 } ``` Provide the following output: ```bash error[E0308]: mismatched types --> foo.rs:2:12 | 1 | fn foo() { | ^ possibly return type `{integer}` missing in this fn? 2 | return 1; | ^ expected (), found integral variable | = note: expected type `()` = note: found type `{integer}` error[E0308]: mismatched types --> foo.rs:6:5 | 6 | 3 | ^ | | | possibly `;` missing here? | expected (), found integral variable | = note: expected type `()` = note: found type `{integer}` error: aborting due to 2 previous errors ```

estebank · 2016-11-21T18:46:48Z

Output for file.rs:

fn foo(a: u32, b: u32) {
        a + b
}

fn bar(a: u32, b: u32) -> () {
        a + b
}

fn qux(a: u32, b: u32) {
    3
}

fn main() {
    3
}

eddyb · 2016-11-21T20:16:12Z

@estebank @jonathandturner Would it be possible to put the label about ; just after the expression?

eddyb · 2016-11-21T20:20:35Z

Btw, two of my comments (#36409 (comment) and #36409 (comment)) appear to still be relevant.

estebank · 2016-11-22T02:00:48Z

Would it be possible to put the label about ; just after the expression?

I believe that I can create a span right after the last char for the expression.

Btw, two of my comments (#36409 (comment) and #36409 (comment)) appear to still be relevant.

You're right, I didn't have the time to incorporate them over the weekend, just to fix the merge conflict and remove unused code. I will be doing so within the next few days.

eddyb · 2016-11-23T21:19:43Z

src/librustc_typeck/check/mod.rs

+                    // Is the block part of a fn?
+                    let parent = self.tcx.map.get(self.tcx.map.get_parent(blk.id));
+                    let fn_decl = if let Node::NodeItem(&hir::Item {
+                        name, node: hir::ItemFn(ref decl, ..), ..


So this is the only part I'm wary about anymore: doing it based solely on syntactical nesting is bound to be either too specific, or wrong. The only way I can see this working as intended is if ExpectHasType carried the extra information that it corresponds to a () implicit return type. cc @rust-lang/compiler

bors · 2016-11-30T06:31:37Z

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

steveklabnik · 2017-01-03T16:26:06Z

Closing due to inactivity; @estebank if you end up rebasing this, please let us know and we can re-open!

rust-highfive assigned eddyb Sep 12, 2016

estebank force-pushed the unwanted-return branch 2 times, most recently from 16b0f74 to a805189 Compare September 12, 2016 00:03

rust-highfive assigned sophiajt and unassigned eddyb Sep 12, 2016

estebank mentioned this pull request Sep 13, 2016

Possible improvement in lack-of-semicolon error messages #36413

Closed

estebank force-pushed the unwanted-return branch from a805189 to 96f0ccf Compare September 13, 2016 15:47

estebank force-pushed the unwanted-return branch from 96f0ccf to ca3ca82 Compare September 13, 2016 16:10

estebank force-pushed the unwanted-return branch from ca3ca82 to 3b4414e Compare September 30, 2016 00:00

estebank force-pushed the unwanted-return branch from 3b4414e to 64be372 Compare October 24, 2016 01:07

eddyb reviewed Oct 25, 2016

View reviewed changes

estebank force-pushed the unwanted-return branch from a55a299 to 6af8104 Compare October 30, 2016 08:18

eddyb reviewed Nov 6, 2016

View reviewed changes

eddyb reviewed Nov 7, 2016

View reviewed changes

estebank force-pushed the unwanted-return branch from 6f282e1 to 47d50e7 Compare November 14, 2016 06:22

GuillaumeGomez reviewed Nov 14, 2016

View reviewed changes

eddyb reviewed Nov 14, 2016

View reviewed changes

estebank force-pushed the unwanted-return branch from 47d50e7 to 555fb3a Compare November 21, 2016 02:33

Move check to FnCtxt::check_block_with_expected

a45366c

estebank force-pushed the unwanted-return branch from 88b4826 to a45366c Compare November 23, 2016 21:10

eddyb reviewed Nov 23, 2016

View reviewed changes

steveklabnik closed this Jan 3, 2017

estebank mentioned this pull request Jan 21, 2017

Detect missing ; on methods with return type () #39231

Closed

estebank mentioned this pull request Jun 23, 2017

Detect missing ; on methods with return type () #42850

Merged

Detect missing ; on methods with return type () #36409

Detect missing ; on methods with return type () #36409

Conversation

estebank commented Sep 12, 2016 • edited Loading

rust-highfive commented Sep 12, 2016

eddyb commented Sep 12, 2016

sophiajt commented Sep 12, 2016

sophiajt commented Sep 12, 2016

nikomatsakis commented Sep 12, 2016 • edited Loading

sophiajt commented Sep 12, 2016

estebank commented Sep 12, 2016

estebank commented Sep 13, 2016 • edited Loading

sophiajt commented Sep 13, 2016

sophiajt commented Sep 13, 2016

estebank commented Sep 29, 2016

bors commented Oct 12, 2016

estebank commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

brson commented Nov 9, 2016 • edited Loading

nikomatsakis commented Nov 9, 2016 • edited Loading

estebank commented Nov 9, 2016

sophiajt commented Nov 12, 2016

estebank commented Nov 13, 2016

eddyb commented Nov 13, 2016 • edited Loading

GuillaumeGomez commented Nov 13, 2016

estebank commented Nov 14, 2016

estebank commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 17, 2016

estebank commented Nov 21, 2016

eddyb commented Nov 21, 2016

eddyb commented Nov 21, 2016

estebank commented Nov 22, 2016

Choose a reason for hiding this comment

bors commented Nov 30, 2016

steveklabnik commented Jan 3, 2017

Detect missing `;` on methods with return type `()` #36409

Detect missing `;` on methods with return type `()` #36409

estebank commented Sep 12, 2016 •

edited

Loading

nikomatsakis commented Sep 12, 2016 •

edited

Loading

estebank commented Sep 13, 2016 •

edited

Loading

eddyb Nov 7, 2016 •

edited

Loading

brson commented Nov 9, 2016 •

edited

Loading

nikomatsakis commented Nov 9, 2016 •

edited

Loading

eddyb commented Nov 13, 2016 •

edited

Loading