Note that the caller chooses a type for type param by jieyouxu · Pull Request #1...

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

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

This comment has been minimized.

Outdated

		@@ -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 {}",

Member

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.

jieyouxu reacted with thumbs up emoji

Contributor

Author

Make sense, changed the wording to this.

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

This comment has been minimized.

Contributor

Author

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

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

fmease reacted with thumbs up emoji

Member

fmease

left a comment

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

jieyouxu reacted with thumbs up emoji

Member

fmease

commented

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

•

edited

@bors rollup

Member

@bors delegate+

Contributor

✌️ @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

		@@ -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);

Member

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,)
}

Contributor

Author

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

fmease reacted with laugh emoji

Okay, let's do this!

Contributor

Author

Contributor

📌 Commit a29ac16 has been approved by fmease

It is now in the queue for this repository.

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

Member

Like this:
@bors r-
:)

jieyouxu reacted with thumbs up emoji

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

Contributor

Author

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 reacted with thumbs up emoji

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

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

Outdated

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

jieyouxu reacted with thumbs up emoji

Contributor

Author

Moved the test into tests/ui/return/.

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

Outdated

		@@ -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.

jieyouxu reacted with thumbs up emoji

Contributor

Author

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

compiler/rustc_hir_typeck/src/expr.rs

Outdated

		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);

Member

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.

jieyouxu reacted with thumbs up emoji

Contributor

Author

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

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

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

Contributor

Author

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.

This comment has been minimized.

Member

fmease

left a comment

Thanks!

Contributor

Author

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

		@@ -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?

Contributor

Author

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.

jieyouxu reacted with thumbs up emoji

This comment has been minimized.

Collaborator

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

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

Member

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

jieyouxu reacted with thumbs up emoji

Contributor

📌 Commit cacdf92 has been approved by fmease

It is now in the queue for this repository.

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