Note that the caller chooses a type for type param #122195

jieyouxu · 2024-03-08T15:06:51Z

error[E0308]: mismatched types
  --> $DIR/return-impl-trait.rs:23:5
   |
LL | fn other_bounds<T>() -> T
   |                 -       -
   |                 |       |
   |                 |       expected `T` because of return type
   |                 |       help: consider using an impl return type: `impl Trait`
   |                 expected this type parameter
...
LL |     ()
   |     ^^ expected type parameter `T`, found `()`
   |
   = note: expected type parameter `T`
                   found unit type `()`
   = note: the caller chooses the type of T which can be different from ()

Tried to see if "expected this type parameter" can be replaced, but that goes all the way to rustc_infer so seems not worth the effort and can affect other diagnostics.

Revives #112088 and #104755.

rustbot · 2024-03-08T15:06:58Z

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

fmease · 2024-03-08T15:42:29Z

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

@@ -1010,6 +1010,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
            format!("impl {all_bounds_str}"),
            Applicability::MaybeIncorrect,
        );
+
+        err.note(format!(
+            "the caller chooses the type of {} which can be different from {}",


Suggested change

"the caller chooses the type of {} which can be different from {}",

"the caller chooses a type for `{}` which can be different from `{}`",

Backticks & “type for” over “type of”. The former is more precise semantically: T stands for a type and can't be of a type, it's a type itself. Cf. the statement i32 is the type of 0i32.

Make sense, changed the wording to this.

jieyouxu · 2024-03-08T16:32:55Z

You still need to bless the other tests/ui tests :)

(I think these are literally the only two tests that test this diagnostic)

fmease

Thanks! One last thing, could you please squash the commits into one? Then r=me

fmease · 2024-03-08T19:07:01Z

@bors rollup

fmease · 2024-03-08T19:08:10Z

@bors delegate+

bors · 2024-03-08T19:08:13Z

✌️ @jieyouxu, you can now approve this pull request!

If @fmease told you to "r=me" after making some further change, please make that change, then do @bors r=@fmease

fmease · 2024-03-08T19:12:33Z

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

@@ -889,7 +889,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                            self.dcx(),
                            errors::ExpectedReturnTypeLabel::Other { span: hir_ty.span, expected },
                        );
-                        self.try_suggest_return_impl_trait(err, expected, ty, fn_id);
+                        self.try_suggest_return_impl_trait(err, expected, found, fn_id);


Actually I wonder if we should add this note in more cases, not just in cases where we suggest RPITs. For example here:

fn f<T>() -> (T,) { (0,) }

wait how do u undo a r= lol (if you want me to add it in more places in this PR)

Okay, let's do this!

jieyouxu · 2024-03-08T19:12:39Z

@bors r=@fmease

bors · 2024-03-08T19:12:41Z

📌 Commit a29ac16 has been approved by fmease

It is now in the queue for this repository.

fmease · 2024-03-08T19:16:36Z

Like this:
@bors r-
:)

jieyouxu · 2024-03-09T21:40:33Z

Changes since last review:

I forgor rustc_hir_typeck already uses translatable diagnostics, so made the note into a subdiagnostic instead.
Added the note for cases like fn f<T>() -> (T,) { (0,) } as well.

@rustbot ready

fmease · 2024-03-16T21:13:18Z

tests/ui/typeck/return-ty-mismatch-note.rs

I wonder if we should put this test into tests/ui/return/, too 🤔

Moved the test into tests/ui/return/.

fmease · 2024-03-16T21:16:50Z

tests/ui/const-generics/issues/issue-67945-1.full.stderr

@@ -11,6 +11,7 @@ LL |         let x: S = MaybeUninit::uninit();
   |
   = note: expected type parameter `S`
                       found union `MaybeUninit<_>`
+   = note: the caller chooses a type for `S` which can be different from `MaybeUninit<_>`


“caller” isn't accurate here, there's no function. While we could say something like “user of Bug” I'm not really sure we want to emit this note at all. See my other review comment.

Moved the emission so it no longer notes for arbitrary expression coercion.

fmease · 2024-03-16T21:23:01Z

compiler/rustc_hir_typeck/src/expr.rs

+            self.demand_coerce_diag(expr, ty, expected, expected_ty_expr, AllowTwoPhase::No);
+
+        if let Some(diag) = diag {
+            self.note_caller_chooses_ty_for_ty_param(diag, expected, ty);


I don't think this is the right place since check_expr_coercible_to_type does not only get run on return expressions but almost anywhere where coercions can happen. See my comment over at file issue-67945-1.full.stderr.

I think you should move the err.subdiagnostic(NoteCallerChoosesTyForTyParam { … }) out of try_suggest_return_impl_trait and into the hir::FnRetTy::Return branch if that makes sense.

Moved the suggestion into the hir::FnRetTy::Return branch.

jieyouxu · 2024-03-16T22:15:26Z

Changes since last review:

Moved test into tests/ui/return/.
Moved emission of note to hir::FnRetTy::Return branch to avoid emitting note on arbitrary coercions.

fmease

Thanks!

jieyouxu · 2024-03-16T22:32:43Z

(One sec I forgot to bless some tests)
EDIT: Blessed them now.

fmease · 2024-03-16T22:53:56Z

tests/ui/type/type-params-in-different-spaces-3.stderr

@@ -14,6 +14,7 @@ LL |         u
              found type parameter `X`
   = note: a type parameter was expected, but a different one was found; you might be missing a type parameter or trait bound
   = note: for more information, visit https://doc.rust-lang.org/book/ch10-02-traits.html#traits-as-parameters
+   = note: the caller chooses a type for `Self` which can be different from `X`


Ah, this one is interesting. The note is technically speaking correct but I don't know if it helps the user. Remember that we add the note “the caller chooses …” because the user might not understand that fn f<T>() -> T doesn't mean “can return anything the callee / the author of the function wants to return”. What do you think? Do you think it's useful?

This note doesn't seem very helpful in this instance, no. This particular case probably needs a separate note suggesting that X might not satisfy X: Sized but Self: Sized or something to that effect. Not entirely sure what help to give here though.

Let's not block this PR on this (given the long history of this diagnostic change with several failed PRs), we should probably not emit the note if the param is kw::SelfUpper but that can be done in a follow up.

Sorry for the delay, I was sitting at my desk wondering why we're not suggesting impl-Trait on fn f<T>() -> (T,) { (0,) } etc. (so, fn f() -> (impl Sized,) { (0,) } which works perfectly). I should probably open an issue for that. If we end up making try_suggest_return_impl_trait smarter at some point we can then probably also move the note back into impl-Trait since it does some other checks, too, which we don't do for note_caller_chooses_ty_for_ty_param.

suggesting that X might not satisfy X: Sized but Self: Sized or something to that effect

Nah, that's not relevant here. The Sized bounds only exist to allow the method to return self.

…Trait

rustbot · 2024-03-16T23:21:06Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

fmease · 2024-03-22T16:23:14Z

Let's merge this. This should probably be further tweaked over time (like maybe making the heuristics stricter) but I don't want to block this.

@bors r+ rollup

bors · 2024-03-22T16:23:16Z

📌 Commit cacdf92 has been approved by fmease

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#114009 (compiler: allow transmute of ZST arrays with generics) - rust-lang#122195 (Note that the caller chooses a type for type param) - rust-lang#122651 (Suggest `_` for missing generic arguments in turbofish) - rust-lang#122784 (Add `tag_for_variant` query) - rust-lang#122839 (Split out `PredicatePolarity` from `ImplPolarity`) - rust-lang#122873 (Merge my contributor emails into one using mailmap) - rust-lang#122885 (Adjust better spastorino membership to triagebot's adhoc_groups) - rust-lang#122888 (add a couple more tests) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122195 - jieyouxu:impl-return-note, r=fmease Note that the caller chooses a type for type param ``` error[E0308]: mismatched types --> $DIR/return-impl-trait.rs:23:5 | LL | fn other_bounds<T>() -> T | - - | | | | | expected `T` because of return type | | help: consider using an impl return type: `impl Trait` | expected this type parameter ... LL | () | ^^ expected type parameter `T`, found `()` | = note: expected type parameter `T` found unit type `()` = note: the caller chooses the type of T which can be different from () ``` Tried to see if "expected this type parameter" can be replaced, but that goes all the way to `rustc_infer` so seems not worth the effort and can affect other diagnostics. Revives rust-lang#112088 and rust-lang#104755.

Note that the caller chooses a type for type param ``` error[E0308]: mismatched types --> $DIR/return-impl-trait.rs:23:5 | LL | fn other_bounds<T>() -> T | - - | | | | | expected `T` because of return type | | help: consider using an impl return type: `impl Trait` | expected this type parameter ... LL | () | ^^ expected type parameter `T`, found `()` | = note: expected type parameter `T` found unit type `()` = note: the caller chooses the type of T which can be different from () ``` Tried to see if "expected this type parameter" can be replaced, but that goes all the way to `rustc_infer` so seems not worth the effort and can affect other diagnostics. Revives rust-lang#112088 and rust-lang#104755.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#114009 (compiler: allow transmute of ZST arrays with generics) - rust-lang#122195 (Note that the caller chooses a type for type param) - rust-lang#122651 (Suggest `_` for missing generic arguments in turbofish) - rust-lang#122784 (Add `tag_for_variant` query) - rust-lang#122839 (Split out `PredicatePolarity` from `ImplPolarity`) - rust-lang#122873 (Merge my contributor emails into one using mailmap) - rust-lang#122885 (Adjust better spastorino membership to triagebot's adhoc_groups) - rust-lang#122888 (add a couple more tests) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned fmease Mar 8, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 8, 2024

jieyouxu force-pushed the impl-return-note branch from 5cdde1c to 99d6193 Compare March 8, 2024 15:12

This comment has been minimized.

Sign in to view

jieyouxu force-pushed the impl-return-note branch from 99d6193 to 7852895 Compare March 8, 2024 15:41

fmease requested changes Mar 8, 2024

View reviewed changes

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 Mar 8, 2024

This comment has been minimized.

Sign in to view

fmease approved these changes Mar 8, 2024

View reviewed changes

jieyouxu force-pushed the impl-return-note branch from 27774a0 to a29ac16 Compare March 8, 2024 19:12

fmease reviewed Mar 8, 2024

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

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 Mar 8, 2024

jieyouxu force-pushed the impl-return-note branch 2 times, most recently from 0f368e3 to eb7bbe8 Compare March 9, 2024 21:38

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 Mar 9, 2024

jieyouxu changed the title ~~Note that type param is chosen by caller when suggesting return impl Trait~~ Note that the caller chooses a type for type param Mar 9, 2024

fmease requested changes Mar 16, 2024

View reviewed changes

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 Mar 16, 2024

jieyouxu force-pushed the impl-return-note branch from eb7bbe8 to 09c1667 Compare March 16, 2024 22:11

This comment has been minimized.

Sign in to view

fmease approved these changes Mar 16, 2024

View reviewed changes

jieyouxu force-pushed the impl-return-note branch from 09c1667 to 61d5d33 Compare March 16, 2024 22:45

fmease reviewed Mar 16, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Note that type param is chosen by caller when suggesting return impl …

cacdf92

…Trait

jieyouxu force-pushed the impl-return-note branch from 61d5d33 to cacdf92 Compare March 16, 2024 23:21

fmease 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 Mar 21, 2024

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 Mar 22, 2024

matthiaskrgr mentioned this pull request Mar 22, 2024

Rollup of 8 pull requests #122900

Merged

bors merged commit aa184c5 into rust-lang:master Mar 23, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Mar 23, 2024

jieyouxu deleted the impl-return-note branch March 23, 2024 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Note that the caller chooses a type for type param #122195

Note that the caller chooses a type for type param #122195

jieyouxu commented Mar 8, 2024 •

edited

rustbot commented Mar 8, 2024

This comment has been minimized.

fmease Mar 8, 2024 •

edited

jieyouxu Mar 8, 2024

This comment has been minimized.

jieyouxu commented Mar 8, 2024

fmease left a comment

fmease commented Mar 8, 2024 •

edited

fmease commented Mar 8, 2024

bors commented Mar 8, 2024

fmease Mar 8, 2024 •

edited

jieyouxu Mar 8, 2024 •

edited

fmease Mar 8, 2024

jieyouxu commented Mar 8, 2024

bors commented Mar 8, 2024

fmease commented Mar 8, 2024

jieyouxu commented Mar 9, 2024 •

edited

fmease Mar 16, 2024

jieyouxu Mar 16, 2024

fmease Mar 16, 2024

jieyouxu Mar 16, 2024 •

edited

fmease Mar 16, 2024 •

edited

jieyouxu Mar 16, 2024

jieyouxu commented Mar 16, 2024

This comment has been minimized.

fmease left a comment

jieyouxu commented Mar 16, 2024 •

edited

fmease Mar 16, 2024

jieyouxu Mar 16, 2024 •

edited

fmease Mar 22, 2024

This comment has been minimized.

rustbot commented Mar 16, 2024

fmease commented Mar 22, 2024 •

edited

bors commented Mar 22, 2024

	"the caller chooses the type of {} which can be different from {}",
	"the caller chooses a type for `{}` which can be different from `{}`",

Note that the caller chooses a type for type param #122195

Note that the caller chooses a type for type param #122195

Conversation

jieyouxu commented Mar 8, 2024 • edited

rustbot commented Mar 8, 2024

This comment has been minimized.

fmease Mar 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

jieyouxu commented Mar 8, 2024

fmease left a comment

Choose a reason for hiding this comment

fmease commented Mar 8, 2024 • edited

fmease commented Mar 8, 2024

bors commented Mar 8, 2024

fmease Mar 8, 2024 • edited

Choose a reason for hiding this comment

jieyouxu Mar 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented Mar 8, 2024

bors commented Mar 8, 2024

fmease commented Mar 8, 2024

jieyouxu commented Mar 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu Mar 16, 2024 • edited

Choose a reason for hiding this comment

fmease Mar 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented Mar 16, 2024

This comment has been minimized.

fmease left a comment

Choose a reason for hiding this comment

jieyouxu commented Mar 16, 2024 • edited

Choose a reason for hiding this comment

jieyouxu Mar 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Mar 16, 2024

fmease commented Mar 22, 2024 • edited

bors commented Mar 22, 2024

jieyouxu commented Mar 8, 2024 •

edited

fmease Mar 8, 2024 •

edited

fmease commented Mar 8, 2024 •

edited

fmease Mar 8, 2024 •

edited

jieyouxu Mar 8, 2024 •

edited

jieyouxu commented Mar 9, 2024 •

edited

jieyouxu Mar 16, 2024 •

edited

fmease Mar 16, 2024 •

edited

jieyouxu commented Mar 16, 2024 •

edited

jieyouxu Mar 16, 2024 •

edited

fmease commented Mar 22, 2024 •

edited