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

Compiler should suggest expect/unwrap before the suggestion to remove the last method call #117669

Closed
3tilley opened this issue Nov 7, 2023 · 3 comments · Fixed by #117695
Closed
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@3tilley
Copy link
Contributor

3tilley commented Nov 7, 2023

Code

fn main() {
    let abs: i32 = 3i32.checked_abs();
}

Current output

Compiling playground v0.0.1 (/playground)
error[E0308]: mismatched types
 --> src/main.rs:2:20
  |
2 |     let abs: i32 = 3i32.checked_abs();
  |              ---   ^^^^^^^^^^^^^^^^^^ expected `i32`, found `Option<i32>`
  |              |
  |              expected due to this
  |
  = note: expected type `i32`
             found enum `Option<i32>`
help: try removing the method call
  |
2 -     let abs: i32 = 3i32.checked_abs();
2 +     let abs: i32 = 3i32;
  |

For more information about this error, try `rustc --explain E0308`.
error: could not compile `playground` (bin "playground") due to previous error

Desired output

Compiling playground v0.0.1 (/playground)
error[E0308]: mismatched types
 --> src/main.rs:2:20
  |
2 |     let abs: i32 = 3i32.checked_abs();
  |              ---   ^^^^^^^^^^^^^^^^^^ expected `i32`, found `Option<i32>`
  |              |
  |              expected due to this
  |
  = note: expected type `i32`
             found enum `Option<i32>`
help: consider using `Option::expect` to unwrap the `Option<i32>` value, panicking if the value is an `Option::None`
   |
2  |     let abs: i32 = 3i32.checked_abs().expect("REASON");
   |                                      +++++++++++++++++

For more information about this error, try `rustc --explain E0308`.
error: could not compile `playground` (bin "playground") due to previous error

Rationale and extra context

#105872 in Dec '22 introduced a cool new feature to suggest removing the last method call if it would alleviate a type mismatch error. This is particularly using for people wrestling with my_str.to_string() issues.

#116841 in Oct '23 introduced another useful feature to suggest to users that they might consider expecting an Option or Result type in order to solve a type mismatch. Also very cool, and conveniently by the same author @chenyukang.

Unfortunately, in cases where a method T -> Option<T> is called, the previous check takes precedence, and the compiler suggests removing that trailing method to get the code to compile. This isn't generally that useful as the expect suggestion, and I can't think of an example where it's a better one. If you can think of one though, please put it below for me to test!

#116738 surfaced this with Path.parent() but it's more easily demonstrated with ints.

I can see two ways to fix this:

  1. Run the trailing method check first, but ignore Result and Option types
  2. Reorder the checks so the option type is checked first. Note that the method check has to be high in the chain as the motive is to quickly suggest removing a call, before adding a whole load of new ones. This means the expect check has to be one of the first

I am going to work on solving it via method 2, as it feels cleaner that the method check doesn't have to know about the Option and Result check.

If anyone has strong feelings on this, please let me know!

Playground here

Other cases

No response

Anything else?

No response

@3tilley 3tilley added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 7, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 7, 2023
@3tilley
Copy link
Contributor Author

3tilley commented Nov 7, 2023

@rustbot claim

@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 7, 2023
@Noratrieb
Copy link
Member

I think this suggestion for removing the method call is way too eager, as it can easily be wrong and when it's wrong it's quite confusing. Maybe it should instead say that the type was correct before the method call (which is never wrong), nudging the user towards removing the call if they know that it's not important. Instead of strongly saying that it should be removed.
Alternatively, it could be special cased to known "unimportant" functions like to_string or as_ref.

@3tilley
Copy link
Contributor Author

3tilley commented Nov 7, 2023

I think this suggestion for removing the method call is way too eager, as it can easily be wrong and when it's wrong it's quite confusing. Maybe it should instead say that the type was correct before the method call (which is never wrong), nudging the user towards removing the call if they know that it's not important. Instead of strongly saying that it should be removed. Alternatively, it could be special cased to known "unimportant" functions like to_string or as_ref.

I think this brings up two issues:

  1. What should the method call suggestor should do, and say. There was some discussion of this in Suggest remove last method call when type coerce with expected type #105872 where it was decided to keep it generic rather than special-casing, but I don't think it's done badly at the moment. Perhaps the wording could be altered?
  2. When it should run

I'll focus on the latter here, but if you have examples where removing the method call doesn't make sense we can aggregate them and see if there is a holistic solution?

@bors bors closed this as completed in 918c17a Nov 13, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 13, 2023
Rollup merge of rust-lang#117695 - 3tilley:prioritise-unwrap-expect-over-last-method-call, r=compiler-errors

Reorder checks to make sure potential missing expect on Option/Result…

… runs before removing last method call

Fixes rust-lang#117669
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants