suggest `?` when method is missing on `Result<T, _>` but found on `T` #96271

compiler-errors · 2022-04-21T06:21:27Z

The wording needs help, I think.

Fixes #95729

rust-highfive · 2022-04-21T06:21:30Z

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

mejrs · 2022-04-21T20:29:48Z

Thanks! ❤️

The wording needs help, I think.

This is an mistake I see newbies make all the time (e.g. in the rust discord) and often this is their first time they encounter Result. IMO it would be better if this error (also) said something along the lines of "this is a result, you need to handle it".

compiler/rustc_typeck/src/check/method/suggest.rs

estebank · 2022-04-23T20:38:01Z

compiler/rustc_typeck/src/check/method/suggest.rs

+                        if let Some(ret_ty) = self
+                            .ret_coercion
+                            .as_ref()
+                            .map(|c| self.resolve_vars_if_possible(c.borrow().expected_ty()))
+                            && let ty::Adt(kind, _) = ret_ty.kind()
+                            && tcx.get_diagnostic_item(sym::Result) == Some(kind.did())


This is checking that the return type is something that ? can coerce to, right? If the return type isn't that, maybe we can 1) suggest changing the return type and 2) use .unwrap(). If we do 2), then we should also support this for Option. We can also build some scaffolding to suggest using if let, which could work for any ADTs...

Hmmm... I'm not sure if I want to encourage people to .unwrap, though I don't know if we have precedent for that in other suggestions. Or at least, maybe I'll suggest expect.

If we can do the more involved if let suggestion, that'd be fine by me to skip the unwrap.

If we can do the more involved if let suggestion

Are you suggesting we transform something like:

result.call();

into

if let Ok(value) = result { value.call(); }

Because if so, I'd probably want to reserve that for a follow-up PR. I need to think of heuristics for when it's meaningful to do so, because I wouldn't want to suggest it for any nested expression, but only ones that are reasonably statement-like on their own. Like not suggesting it when the bad method call is here:

let closure = || call_something(result_needs_unwrap.call(), other_expression()); // ^^^^^^^^^^^^^^^^^^^^^^^^^^

Yes, and point taken. As follow up work what we could so is check if the current ty is an enum, and prove each variant that has a single value for the type and emit a note saying "the variant Result::Ok(Ty) has method foo" (but still special case for Result and Option).

Yeah, I've been thinking of this a bit more, and I would also like to generalize some sort of message to more enums somehow in a follow-up PR.

Let me special-case it for Result and Option in this PR, along with suggesting .expect (and/or changing the return type of the function) when we don't return Result.

compiler/rustc_typeck/src/check/method/suggest.rs

src/test/ui/suggestions/result-method-probe.rs

nagisa · 2022-04-24T17:31:58Z

r? @estebank

compiler-errors · 2022-04-25T17:48:37Z

@rustbot author

bors · 2022-04-26T19:28:26Z

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

compiler/rustc_typeck/src/check/method/suggest.rs

compiler-errors · 2022-04-27T03:02:57Z

src/test/ui/suggestions/enum-method-probe.fixed

+
+fn test_result_in_plain() {	
+    let res: Result<_, ()> = Ok(Foo);
+    res.expect("REASON").get();


should this just fixup to .unwrap? I kinda prefer .expect just because it feels like better practice out of the two.

I know what you mean, expect is better style, but unwrap ends up looking cleaner... I can be convinced either way.

compiler-errors · 2022-04-27T03:03:11Z

@rustbot ready

bors · 2022-04-27T04:14:42Z

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

estebank

Need to continue reviewing, but so far this is a work of art ❤️

estebank

LGTM, but left some nitpicks.

compiler/rustc_typeck/src/check/method/suggest.rs

estebank · 2022-04-30T02:01:11Z

src/test/ui/suggestions/enum-method-probe.rs

@@ -0,0 +1,42 @@
+// run-rustfix


Let's add a check for async fn as well. I'm intrigued if they work without further changes.

bors · 2022-05-21T09:30:41Z

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

compiler-errors · 2022-05-30T21:18:40Z

@rustbot ready

estebank · 2022-06-01T00:07:03Z

compiler/rustc_typeck/src/check/method/suggest.rs

+                    );
+                }
+            }
+            // FIXME(compiler-errors): Support suggestions for other matching enum variants


This would entail looking for methods equivalent to .expect based on heuristics, right?

Maybe, or something like suggesting if let Pattern(destructure) = value { .. }. Not sure how useful that is, though.

I think it'd be worth it

estebank · 2022-06-01T00:07:18Z

@bors r+

bors · 2022-06-01T00:07:20Z

📌 Commit 2a61f0c has been approved by estebank

…rk, r=estebank suggest `?` when method is missing on `Result<T, _>` but found on `T` The wording needs help, I think. Fixes rust-lang#95729

…askrgr Rollup of 4 pull requests Successful merges: - rust-lang#96271 (suggest `?` when method is missing on `Result<T, _>` but found on `T`) - rust-lang#97264 (Suggest `extern crate foo` when failing to resolve `use foo`) - rust-lang#97592 (rustdoc: also index impl trait and raw pointers) - rust-lang#97621 (update Miri) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned nagisa Apr 21, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2022

estebank reviewed Apr 23, 2022

View reviewed changes

compiler/rustc_typeck/src/check/method/suggest.rs Outdated Show resolved Hide resolved

estebank reviewed Apr 23, 2022

View reviewed changes

compiler/rustc_typeck/src/check/method/suggest.rs Outdated Show resolved Hide resolved

estebank reviewed Apr 23, 2022

View reviewed changes

src/test/ui/suggestions/result-method-probe.rs Outdated Show resolved Hide resolved

rust-highfive assigned estebank and unassigned nagisa Apr 24, 2022

rustbot 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 Apr 25, 2022

compiler-errors force-pushed the suggest-question-mark branch from 47328e7 to d28a216 Compare April 27, 2022 03:00

compiler-errors commented Apr 27, 2022

View reviewed changes

compiler/rustc_typeck/src/check/method/suggest.rs Show resolved Hide resolved

compiler-errors commented Apr 27, 2022

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 27, 2022

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the suggest-question-mark branch from d28a216 to 9db9177 Compare April 27, 2022 03:32

compiler-errors force-pushed the suggest-question-mark branch from 9db9177 to 90d522e Compare April 27, 2022 04:30

estebank reviewed Apr 27, 2022

View reviewed changes

estebank reviewed Apr 30, 2022

View reviewed changes

better enum suggestions

aeb765b

compiler-errors force-pushed the suggest-question-mark branch from 90d522e to a5ec87b Compare May 30, 2022 21:18

address comments

2a61f0c

compiler-errors force-pushed the suggest-question-mark branch from a5ec87b to 2a61f0c Compare May 30, 2022 21:23

estebank reviewed Jun 1, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 1, 2022

matthiaskrgr mentioned this pull request Jun 1, 2022

Rollup of 5 pull requests #97623

Closed

matthiaskrgr mentioned this pull request Jun 1, 2022

Rollup of 4 pull requests #97624

Merged

bors merged commit 7d4cf71 into rust-lang:master Jun 1, 2022

rustbot added this to the 1.63.0 milestone Jun 1, 2022

compiler-errors deleted the suggest-question-mark branch August 11, 2023 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

suggest `?` when method is missing on `Result<T, _>` but found on `T` #96271

suggest `?` when method is missing on `Result<T, _>` but found on `T` #96271

compiler-errors commented Apr 21, 2022

rust-highfive commented Apr 21, 2022

mejrs commented Apr 21, 2022

estebank Apr 23, 2022

compiler-errors Apr 23, 2022 •

edited

Loading

estebank Apr 24, 2022

compiler-errors Apr 24, 2022

estebank Apr 25, 2022

compiler-errors Apr 25, 2022

nagisa commented Apr 24, 2022

compiler-errors commented Apr 25, 2022

bors commented Apr 26, 2022

compiler-errors Apr 27, 2022

estebank Apr 27, 2022

compiler-errors commented Apr 27, 2022

This comment has been minimized.

bors commented Apr 27, 2022

estebank left a comment

estebank left a comment

estebank Apr 30, 2022

bors commented May 21, 2022

compiler-errors commented May 30, 2022

estebank Jun 1, 2022

compiler-errors Jun 1, 2022

estebank Jun 1, 2022

estebank commented Jun 1, 2022

bors commented Jun 1, 2022

suggest ? when method is missing on Result<T, _> but found on T #96271

suggest ? when method is missing on Result<T, _> but found on T #96271

Conversation

compiler-errors commented Apr 21, 2022

rust-highfive commented Apr 21, 2022

mejrs commented Apr 21, 2022

Choose a reason for hiding this comment

compiler-errors Apr 23, 2022 • 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

Choose a reason for hiding this comment

nagisa commented Apr 24, 2022

compiler-errors commented Apr 25, 2022

bors commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Apr 27, 2022

This comment has been minimized.

bors commented Apr 27, 2022

estebank left a comment

Choose a reason for hiding this comment

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 21, 2022

compiler-errors commented May 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Jun 1, 2022

bors commented Jun 1, 2022

suggest `?` when method is missing on `Result<T, _>` but found on `T` #96271

suggest `?` when method is missing on `Result<T, _>` but found on `T` #96271

compiler-errors Apr 23, 2022 •

edited

Loading