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

estebank · 2017-06-23T04:45:22Z

Point out the origin of a type requirement when it is the return type
of a method
Point out possibly missing semicolon when the return type is () and
the implicit return makes sense as a statement
Suggest changing the return type of methods with default return type
Don't suggest changing the return type on fn main()
Don't suggest changing the return type on impl fn
Suggest removal of semicolon (instead of being help)

rust-highfive · 2017-06-23T04:45:32Z

(rust_highfive has picked a reviewer for you, use r? to override)

estebank · 2017-06-23T04:46:36Z

Previous discussion: #39231, #36409. Re #42823.

Mark-Simulacrum

Could you move the compile-fail tests to UI tests before the actual changes are implemented, in a separate commit so we can see the differences this PR introduces?

arielb1 · 2017-06-23T21:09:56Z

src/test/ui/block-result/block-must-not-have-result-while.stderr

+   |
+11 | fn main() {
+   |           - expected `()` because of this default return type
+12 |     while true {


Why is the main function spanned on this? while etc. can't return a value via tail expression

The code now walks up until it finds a fn block for success or a while or for block for failure, avoiding this.

arielb1 · 2017-06-23T21:13:41Z

src/librustc_typeck/check/mod.rs

+            name, node: hir::ItemFn(ref decl, ..), ..
+        }) = parent {
+            decl.clone().and_then(|decl| {
+                Some((decl, name == Symbol::intern("main")))


that's rather unrobust: this compiles and runs:

fn main() { println!("{}", foo::main()); } mod foo { pub fn main() -> u32 { 4 } }

It seemed good enough at the time, given that the failure mode is not suggesting a return type when it could, but point taken. Is there a way to figure out wether the current method is the main method during typeck?

arielb1 · 2017-06-23T21:16:21Z

src/test/ui/block-result/block-must-not-have-result-do.stderr

+  --> $DIR/block-must-not-have-result-do.rs:13:9
+   |
+11 | fn main() {
+   |           - expected `()` because of this default return type


Why is the main function spanned on this? loop, while etc. can't return a value via tail expression.

Same as the previous one.

arielb1 · 2017-06-23T21:20:21Z

src/librustc_typeck/check/mod.rs

+            "default "
+        } else {
+            ""
+        };


Please do resolve_type_vars_if_possible here or somewhere.

- Point out the origin of a type requirement when it is the return type of a method - Point out possibly missing semicolon when the return type is () and the implicit return makes sense as a statement - Suggest changing the return type of methods with default return type - Don't suggest changing the return type on fn main() - Don't suggest changing the return type on impl fn

estebank · 2017-06-24T17:21:39Z

@Mark-Simulacrum moved the ui tests on a commit on its own, and separated the changes onto two other commits.

Don't specify a suggested return type for `TyAnon`, `TyFnDef`, `TyFnPtr`, `TyDynamic`, `TyClosure` and `TyProjection`.

ollie27 · 2017-06-24T21:27:12Z

src/test/ui/impl-trait/equality.stderr

@@ -1,6 +1,9 @@
 error[E0308]: mismatched types
  --> $DIR/equality.rs:25:5
   |
+21 | fn two(x: bool) -> impl Foo {
+   |                    -------- expected `i32` because of this return type


This doesn't seem right. It's the return 1_i32; which is causing this error and ideally that's what would be pointed to.

ollie27 · 2017-06-24T21:28:01Z

src/test/ui/resolve/token-error-correct-3.stderr

@@ -34,8 +34,13 @@ error[E0425]: cannot find function `is_directory` in this scope
 error[E0308]: mismatched types
  --> $DIR/token-error-correct-3.rs:25:13
   |
+20 |                                                                -> io::Result<bool> {
+   |                                                                   ---------------- expected `()` because of this return type


This is clearly not right.

nikomatsakis · 2017-06-27T17:05:31Z

src/test/ui/block-result/expected-return-on-unit.stderr

@@ -0,0 +1,4 @@
+error[E0601]: main function not found


is this really testing what we expect it to test?

nikomatsakis

Left a few nits, but this looks pretty good to me.

nikomatsakis · 2017-06-27T17:06:45Z

src/librustc/hir/map/mod.rs

@@ -594,8 +594,12 @@ impl<'hir> Map<'hir> {
    /// last good node id we found. Note that reaching the crate root (id == 0),
    /// is not an error, since items in the crate module have the crate root as
    /// parent.
-    fn walk_parent_nodes<F>(&self, start_id: NodeId, found: F) -> Result<NodeId, NodeId>
-        where F: Fn(&Node<'hir>) -> bool
+    fn walk_parent_nodes<F, F2>(&self,


Meh. This is fine for now, but it'd be nice to make this an iterator, I think.

nikomatsakis · 2017-06-27T17:07:04Z

src/librustc/hir/map/mod.rs

@@ -626,6 +632,34 @@ impl<'hir> Map<'hir> {
        }
    }

+    pub fn get_return_block(&self, id: NodeId) -> Option<NodeId> {


Comment explaining what this function does would be good. Ideally with an example.

nikomatsakis · 2017-06-27T17:08:27Z

src/librustc_typeck/check/mod.rs

@@ -4210,6 +4214,130 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
        ty
    }

+    /// Given a `NodeId`, return the `FnDecl` of the method it is enclosed by and wether it is


Nit: s/wether/whether/

nikomatsakis · 2017-06-27T17:42:24Z

r=me with nits addressed (not the iterator thing, though if you want to, that's good too)

- Fix typo - Add docstring - Remove spurious test output file

estebank · 2017-06-27T22:13:07Z

@bors r=nikomatsakis

bors · 2017-06-27T22:13:08Z

📌 Commit 7dad295 has been approved by nikomatsakis

bors · 2017-06-28T17:08:08Z

⌛ Testing commit 7dad295 with merge 962840a15d3468f254a66d49aa1ff469ed611c41...

bors · 2017-06-28T19:06:05Z

💔 Test failed - status-travis

Mark-Simulacrum · 2017-06-28T19:08:52Z

Timed out while building Cargo.

@bors retry

bors · 2017-06-28T20:16:19Z

⌛ Testing commit 7dad295 with merge 69c65d2...

Detect missing `;` on methods with return type `()` - Point out the origin of a type requirement when it is the return type of a method - Point out possibly missing semicolon when the return type is `()` and the implicit return makes sense as a statement - Suggest changing the return type of methods with default return type - Don't suggest changing the return type on `fn main()` - Don't suggest changing the return type on impl fn - Suggest removal of semicolon (instead of being help)

bors · 2017-06-28T22:42:57Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 69c65d2 to master...

Before this, the diagnostic errors would only point at the return type when changing it would be a possible solution to a type error. Add a label to the return type without a suggestion to change in order to make the source of the expected type obvious. Follow up to rust-lang#42850, fixes rust-lang#25133, fixes rust-lang#41897.

Point at return type always when type mismatch against it Before this, the diagnostic errors would only point at the return type when changing it would be a possible solution to a type error. Add a label to the return type without a suggestion to change in order to make the source of the expected type obvious. Follow up to #42850, fixes #25133, fixes #41897.

rust-highfive assigned nikomatsakis Jun 23, 2017

estebank force-pushed the unwanted-return-rotj branch from 4d018ee to f2f6fdb Compare June 23, 2017 05:39

Mark-Simulacrum reviewed Jun 23, 2017

View reviewed changes

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 23, 2017

arielb1 reviewed Jun 23, 2017

View reviewed changes

estebank added 3 commits June 23, 2017 18:01

Move tests to ui

8e11189

Suggest removal of semicolon (instead of being help)

ecde91a

estebank force-pushed the unwanted-return-rotj branch from f2f6fdb to ecde91a Compare June 24, 2017 17:09

Do not specify return type in suggestion for some Tys

27d4b31

Don't specify a suggested return type for `TyAnon`, `TyFnDef`, `TyFnPtr`, `TyDynamic`, `TyClosure` and `TyProjection`.

ollie27 reviewed Jun 24, 2017

View reviewed changes

Don't naively point to return type on type error

c13a913

estebank mentioned this pull request Jun 25, 2017

Make suggestion include the line number #42904

Merged

nikomatsakis reviewed Jun 27, 2017

View reviewed changes

nikomatsakis requested changes Jun 27, 2017

View reviewed changes

Review comments

7dad295

- Fix typo - Add docstring - Remove spurious test output file

bors merged commit 7dad295 into rust-lang:master Jun 28, 2017

This was referenced Jun 28, 2017

Unsized tuple coercions #42527

Merged

Change some notes into suggestions #42033

Merged

estebank mentioned this pull request Jul 26, 2017

Point at return type always when type mismatch against it #43484

Merged

kennytm mentioned this pull request Jan 28, 2018

Incorrect use of traits::SizedReturnType #47830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

estebank commented Jun 23, 2017

rust-highfive commented Jun 23, 2017

estebank commented Jun 23, 2017 •

edited

Loading

Mark-Simulacrum left a comment

arielb1 Jun 23, 2017

estebank Jun 24, 2017

arielb1 Jun 23, 2017

estebank Jun 23, 2017

arielb1 Jun 23, 2017 •

edited

Loading

estebank Jun 24, 2017

arielb1 Jun 23, 2017

estebank Jun 24, 2017

estebank commented Jun 24, 2017

ollie27 Jun 24, 2017

ollie27 Jun 24, 2017

nikomatsakis Jun 27, 2017

nikomatsakis left a comment

nikomatsakis Jun 27, 2017

nikomatsakis Jun 27, 2017

nikomatsakis Jun 27, 2017

nikomatsakis commented Jun 27, 2017

estebank commented Jun 27, 2017

bors commented Jun 27, 2017

bors commented Jun 28, 2017

bors commented Jun 28, 2017

Mark-Simulacrum commented Jun 28, 2017

bors commented Jun 28, 2017

bors commented Jun 28, 2017

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

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

Conversation

estebank commented Jun 23, 2017

rust-highfive commented Jun 23, 2017

estebank commented Jun 23, 2017 • edited Loading

Mark-Simulacrum left a 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

arielb1 Jun 23, 2017 • edited Loading

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

estebank commented Jun 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a 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

nikomatsakis commented Jun 27, 2017

estebank commented Jun 27, 2017

bors commented Jun 27, 2017

bors commented Jun 28, 2017

bors commented Jun 28, 2017

Mark-Simulacrum commented Jun 28, 2017

bors commented Jun 28, 2017

bors commented Jun 28, 2017

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

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

estebank commented Jun 23, 2017 •

edited

Loading

arielb1 Jun 23, 2017 •

edited

Loading