-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect missing ;
on methods with return type ()
#42850
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
4d018ee
to
f2f6fdb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
| | ||
11 | fn main() { | ||
| - expected `()` because of this default return type | ||
12 | while true { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the main function spanned on this? while
etc. can't return a value via tail expression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code now walks up until it finds a fn
block for success or a while
or for
block for failure, avoiding this.
src/librustc_typeck/check/mod.rs
Outdated
name, node: hir::ItemFn(ref decl, ..), .. | ||
}) = parent { | ||
decl.clone().and_then(|decl| { | ||
Some((decl, name == Symbol::intern("main"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's rather unrobust: this compiles and runs:
fn main() {
println!("{}", foo::main());
}
mod foo {
pub fn main() -> u32 { 4 }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
--> $DIR/block-must-not-have-result-do.rs:13:9 | ||
| | ||
11 | fn main() { | ||
| - expected `()` because of this default return type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the main function spanned on this? loop
, while
etc. can't return a value via tail expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the previous one.
src/librustc_typeck/check/mod.rs
Outdated
"default " | ||
} else { | ||
"" | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do resolve_type_vars_if_possible
here or somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
- 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
f2f6fdb
to
ecde91a
Compare
@Mark-Simulacrum moved the |
Don't specify a suggested return type for `TyAnon`, `TyFnDef`, `TyFnPtr`, `TyDynamic`, `TyClosure` and `TyProjection`.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clearly not right.
@@ -0,0 +1,4 @@ | |||
error[E0601]: main function not found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really testing what we expect it to test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits, but this looks pretty good to me.
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh. This is fine for now, but it'd be nice to make this an iterator, I think.
src/librustc/hir/map/mod.rs
Outdated
@@ -626,6 +632,34 @@ impl<'hir> Map<'hir> { | |||
} | |||
} | |||
|
|||
pub fn get_return_block(&self, id: NodeId) -> Option<NodeId> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment explaining what this function does would be good. Ideally with an example.
src/librustc_typeck/check/mod.rs
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/wether/whether/
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
@bors r=nikomatsakis |
📌 Commit 7dad295 has been approved by |
⌛ Testing commit 7dad295 with merge 962840a15d3468f254a66d49aa1ff469ed611c41... |
💔 Test failed - status-travis |
Timed out while building Cargo. @bors retry |
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)
☀️ Test successful - status-appveyor, status-travis |
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.
of a method
()
andthe implicit return makes sense as a statement
fn main()