Suggest `await` in more situations where infer types are involved #91022

compiler-errors · 2021-11-18T21:32:58Z

Currently we use TyS::same_type in diagnostics that suggest adding .await to opaque future types.

This change makes the suggestion slightly more general, when we're comparing types like Result<T, E> and Result<_, _> which happens sometimes in places like match patterns or let statements with partially-elaborated types.

Question:

Is this change worthwhile? Totally fine if it doesn't make sense adding.
Should same_type_modulo_infer live in rustc_infer::infer::error_reporting or alongside the other method in rustc_middle::ty::util?
Should we generalize this change? I wanted to change all usages, but I don't want erroneous suggestions when adding .field_name...

rust-highfive · 2021-11-18T21:33:01Z

r? @jackh726

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

compiler-errors · 2021-11-18T21:48:09Z

compiler/rustc_infer/src/infer/error_reporting/mod.rs

What a strange diff. This didn't do anything except for replace same_type with same_type_modulo_infer and run the autoformatter, which caused an extra block to get added in.

If you look at it hiding whitespace changes, the diff looks more reasonable. This happens because the line went over 100 columns, so it decided to reformat the arm into a block. It's fine.

nagisa · 2021-11-19T00:42:04Z

r? @estebank

estebank · 2021-11-19T02:28:02Z

Is this change worthwhile? Totally fine if it doesn't make sense adding.

Yes.

Should same_type_modulo_infer live in rustc_infer::infer::error_reporting or alongside the other method in rustc_middle::ty::util?

That would be ideal. That would send a clear signal to anyone that reaches for it in the future for something else.

Should we generalize this change? I wanted to change all usages, but I don't want erroneous suggestions when adding .field_name...

I'm not sure I follow the question? If you could elaborate?

r=me after moving same_type_modulo_infer to error_reporting.

compiler-errors · 2021-11-19T02:36:26Z

Should we generalize this change? I wanted to change all usages, but I don't want erroneous suggestions when adding .field_name...

Sorry, I mean other usages of TyS::same_type in error_reporting, specifically the code in suggest_accessing_field_where_appropriate that suggests to add .field when it expects a type and finds a struct with a member. I found it gives a strange suggestion when you try to match a range against an int when i added this modulo-inferencing (and hence why i limited these changes just to the await suggestion).

estebank

Ah! I see what you mean. I'd like to see the strange output for Range, but I suspect you made the right call. It's preferable to not give a suggestion than giving a misleading suggestion.

estebank · 2021-11-19T17:45:55Z

compiler/rustc_infer/src/infer/error_reporting/mod.rs

I'm pretty sure there's code like this somewhere.

Yeah, this is from fn equals(Ty, Ty) -> bool.

We could possibly unify them by adding an enum Mode { None, NumInfer, Infer } or something. The complication is that this equals fn cares about the non-Ty substs in Adt, where TyS::same_ty (and the function I added) doesn't...

Thoughts?

I see. fn equals has to make sure the whole type are the same because this is used to determine whether it can be replaced with _ in the output. It's fine.

estebank · 2021-11-19T17:52:22Z

src/test/ui/async-await/suggest-missing-await.rs

I wonder, can this test be marked as // run-rustfix?

// run-rustfix seems to produce broken output.

In suggest_await_in_async_fn_return, the "add a semicolon" and "add .await" suggestions get applied out of order to produce dummy();.await

In suggest_await_in_generic_pattern (the test I added), it adds .await twice, because there are two type errors due to the Ok and Err branches.

Ah, I see. Fair enough, leave as is.

compiler-errors · 2021-11-19T21:58:38Z

I'd like to see the strange output for Range, but I suspect you made the right call.

Here's the output for that case.

diff --git a/src/test/ui/half-open-range-patterns/exclusive_range_pattern_syntax_collision.stderr b/src/test/ui/half-open-range-patterns/exclusive_range_pattern_syntax_collision.stderr
index a6f8563a047..1df7fd59f57 100644
--- a/src/test/ui/half-open-range-patterns/exclusive_range_pattern_syntax_collision.stderr
+++ b/src/test/ui/half-open-range-patterns/exclusive_range_pattern_syntax_collision.stderr
@@ -8,6 +8,10 @@ LL |         [_, 99.., _] => {},
    |
    = note: expected struct `std::ops::Range<{integer}>`
                 found type `{integer}`
+help: you might have meant to use field `start` whose type is `{integer}`
+   |
+LL |     match [5..4, 99..105, 43..44].start {
+   |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Wait... this is just wrong, isn't it? [Range<{int}>; N] doesn't have a field start, right? I think this is just a bug, so I'll file one. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b789116e3b4c72d5c06c5deaa8a55e8e

Do you still want me to replace the other usages of TyS::same_type in this file then?

r? @estebank

estebank · 2021-11-19T23:17:25Z

@bors r+

bors · 2021-11-19T23:17:26Z

📌 Commit 323a48d7b44f9b34a6cb67a6ea2105a155763119 has been approved by estebank

compiler-errors · 2021-11-20T04:46:03Z

This diff is going to conflict with #91021 which changes impl Future outputs (one of which is in the new test I added...)

@bors r-

... I don't really know how bors works but I think this will keep this from getting rolled up. Then I'll try bors again.

compiler-errors · 2021-11-20T18:01:13Z

@bors r=estebank

Fixed up the test output for the test.

(I don't know how to use bors... I think this will work)

edit: Guess not.

bors · 2021-11-20T18:01:14Z

@compiler-errors: 🔑 Insufficient privileges: Not in reviewers

compiler-errors · 2021-11-20T18:03:23Z

r? @estebank

estebank · 2021-11-20T18:07:46Z

@bors r+

bors · 2021-11-20T18:07:48Z

📌 Commit 1f625b7 has been approved by estebank

…ebank Suggest `await` in more situations where infer types are involved Currently we use `TyS::same_type` in diagnostics that suggest adding `.await` to opaque future types. This change makes the suggestion slightly more general, when we're comparing types like `Result<T, E>` and `Result<_, _>` which happens sometimes in places like `match` patterns or `let` statements with partially-elaborated types. ---- Question: 1. Is this change worthwhile? Totally fine if it doesn't make sense adding. 2. Should `same_type_modulo_infer` live in `rustc_infer::infer::error_reporting` or alongside the other method in `rustc_middle::ty::util`? 3. Should we generalize this change? I wanted to change all usages, but I don't want erroneous suggestions when adding `.field_name`...

…askrgr Rollup of 6 pull requests Successful merges: - rust-lang#89741 (Mark `Arc::from_inner` / `Rc::from_inner` as unsafe) - rust-lang#90927 (Fix float ICE) - rust-lang#90994 (Fix ICE `rust-lang#90993`: add missing call to cancel) - rust-lang#91018 (Adopt let_else in more places in rustc_mir_build) - rust-lang#91022 (Suggest `await` in more situations where infer types are involved) - rust-lang#91088 (Revert "require full validity when determining the discriminant of a value") Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned jackh726 Nov 18, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2021

compiler-errors commented Nov 18, 2021

View reviewed changes

rust-highfive assigned estebank and unassigned jackh726 Nov 19, 2021

compiler-errors force-pushed the modulo_infer branch from 0e39384 to 323a48d Compare November 19, 2021 02:44

estebank approved these changes Nov 19, 2021

View reviewed changes

compiler-errors mentioned this pull request Nov 19, 2021

Erroneous suggestion to add .field when matching arrays #91058

Closed

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 Nov 19, 2021

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 20, 2021

compiler-errors added 2 commits November 20, 2021 09:53

Suggest await on cases involving infer

33c443d

Re-bless test outputs

1f625b7

compiler-errors force-pushed the modulo_infer branch from 323a48d to 1f625b7 Compare November 20, 2021 18:00

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 20, 2021

This was referenced Nov 20, 2021

Rollup of 4 pull requests #91090

Closed

Rollup of 6 pull requests #91093

Merged

bors merged commit ec2f087 into rust-lang:master Nov 21, 2021

rustbot added this to the 1.58.0 milestone Nov 21, 2021

compiler-errors deleted the modulo_infer branch December 3, 2021 07:09

Uh oh!

Suggest await in more situations where infer types are involved #91022

Suggest await in more situations where infer types are involved #91022

Uh oh!

Conversation

compiler-errors commented Nov 18, 2021

Uh oh!

rust-highfive commented Nov 18, 2021

Uh oh!

compiler-errors Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

estebank Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

nagisa commented Nov 19, 2021

Uh oh!

estebank commented Nov 19, 2021

Uh oh!

compiler-errors commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estebank left a comment

Choose a reason for hiding this comment

Uh oh!

estebank Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

compiler-errors Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estebank Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

estebank Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

compiler-errors Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

estebank Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Nov 19, 2021

Uh oh!

estebank commented Nov 19, 2021

Uh oh!

bors commented Nov 19, 2021

Uh oh!

compiler-errors commented Nov 20, 2021

Uh oh!

compiler-errors commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Nov 20, 2021

Uh oh!

compiler-errors commented Nov 20, 2021

Uh oh!

estebank commented Nov 20, 2021

Uh oh!

bors commented Nov 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Suggest `await` in more situations where infer types are involved #91022

Suggest `await` in more situations where infer types are involved #91022

compiler-errors commented Nov 19, 2021 •

edited

Loading

compiler-errors Nov 19, 2021 •

edited

Loading

compiler-errors commented Nov 20, 2021 •

edited

Loading