Skip to content
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

suggest return type for main() #62384

Closed
wants to merge 11 commits into from

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Jul 4, 2019

This adds a suggestion for main() methods with default () return if the resulting expression has a type that implements std::process::Termination.

This fixes #61277.

r? @estebank

This adds a suggestion for main() methods with default () return
if the resulting expression has a type that implements
`std::process::Termination`.

This fixes rust-lang#61277.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2019
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Can you please also add tests to exercise this behavior?

src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

LGTM, please add tests.

src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/mod.rs Show resolved Hide resolved
err.span_suggestion(
span,
"try adding a return type",
format!("-> {} ", ret_ty),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the whitespace in the code suggestion might be subtly wrong. I believe this should be " -> {} ", but without the stderr` test output I can't say for sure.

Copy link
Contributor Author

@llogiq llogiq Jul 6, 2019

Choose a reason for hiding this comment

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

I just added a UI test, but the suggestion doesn't appear. Any idea why?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. Could you add some debug statements to verify the contents of fn_decl.output, found.is_suggestable(), can_suggest, expected and ret_ty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing.

@scottmcm scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2019
@estebank
Copy link
Contributor

estebank commented Jul 6, 2019

tidy error: /checkout/src/librustc_typeck/check/mod.rs:3943: line longer than 100 chars

@llogiq
Copy link
Contributor Author

llogiq commented Jul 11, 2019

It appears that the on_unimplemented check is executed before it can get to the return type suggestion – at least that's what the debug logs show (I never got a debug message from rustc_typeck, I had to RUSTC_LOG=debug to find out). So I probably need to defer that check (or even better omit it if I can prove that the later suggestion will help). If I get rid of the ?, I still get the error without the typeck-debug messages. However, I get the following logs:

 INFO (timestamp elided): rustc::traits::error_reporting: skipping ErrorDescriptor { predicate: Binder(TraitPredicate(<std::result::Result<[type error], ()> as std::marker::Sized>)), index: Some(3) } (implied by ErrorDescriptor { predicate: Binder(TraitPredicate(<[type error] as std::marker::Sized>)), index: Some(1) })
 INFO (timestamp elided): rustc::traits::error_reporting: skipping ErrorDescriptor { predicate: Binder(TraitPredicate(<std::result::Result<(), [type error]> as std::marker::Sized>)), index: Some(2) } (implied by ErrorDescriptor { predicate: Binder(TraitPredicate(<[type error] as std::marker::Sized>)), index: Some(0) })

@estebank
Copy link
Contributor

estebank commented Jul 11, 2019

I see. The problem is that found is already poisoned with TyErr which makes predicate_for_trait_def not do what you expect it to do. I'll try to take a look at this during the weekend.

@llogiq
Copy link
Contributor Author

llogiq commented Jul 12, 2019

Thanks, you're a hero!

@estebank
Copy link
Contributor

estebank commented Jul 17, 2019

@llogiq the reason the suggestion doesn't trigger is because the found type references FreshTy. If you change the code to use a fully qualified type, the suggestion triggers:

error[E0308]: mismatched types
 --> src/test/ui/suggestions/missing_return_type.rs:6:5
  |
6 |     Result::<(), ()>::Ok(())
  |     ^^^^^^^^^^^^^^^^^^^^^^^^ expected (), found enum `std::result::Result`
  |
  = note: expected type `()`
             found type `std::result::Result<(), ()>`
help: try adding a semicolon
  |
6 |     Result::<(), ()>::Ok(());
  |                             ^
help: try adding a return type
  |
3 | fn main() -> std::result::Result<(), ()> {
  |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I see two options: 1) modify the machinery in traits/select.rs to have a way to eagerly resolve in this case or 2) modify found in-place to replace all its FreshTy to some "safe" type, probably something along the lines of std::result::Result<(), impl std::fmt::Debug>. This should only be done in the case of Result, and hope that users' implement Termination trait in non-generic structs 😬

This later suggestion would end up causing an error like the following though:

error[E0282]: type annotations needed
 --> src/test/ui/suggestions/missing_return_type.rs:3:25
  |
3 | fn main() -> Result<(), impl std::fmt::Debug>{
  |                         ^^^^^^^^^^^^^^^^^^^^ cannot infer type

Maybe suggesting replacing all the FreshTy type args with () would be enough for the intended resolution.

@estebank
Copy link
Contributor

Another pattern that would be interesting to support is

error[E0308]: mismatched types
 --> src/test/ui/suggestions/missing_return_type.rs:7:5
  |
3 | fn main() {
  |           - expected `()` because of default return type
...
7 |     Ok(Err(())?)
  |     ^^^^^^^^^^^^- help: try adding a semicolon: `;`
  |     |
  |     expected (), found enum `std::result::Result`
  |
  = note: expected type `()`
             found type `std::result::Result<_, _>`

@wirelessringo
Copy link

Ping from triage. @llogiq any updates on this?

@llogiq
Copy link
Contributor Author

llogiq commented Jul 27, 2019

I'm afraid I have a lot of code reading and understanding to do before I can implement @estebank's proposal. If you like, you can close the PR, and I'll reopen it when I have something working.

@JohnTitor
Copy link
Member

Ping from triage: @llogiq, I'll close this PR to keep things tidy. But don't worry, you can re-open this if you find a time to read and understand code to implement, we'll be happy to review it again! Thanks for taking the time to contribute.

@JohnTitor JohnTitor closed this Aug 4, 2019
@JohnTitor JohnTitor added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggest alternative return type for Options and Results
7 participants