Don't Add Specialized Notes to Error Messages Pointing at a Type

[veera-sivarajan]

Fixes #121398

Previously, the on_unimplemented mechanism was adding specialized notes by blindly matching on the type. This resulted in specialized notes for a span pointing to String.

This PR fixes it by checking if the expected obligation is enclosed in a impl block; assuming that calls like .iter() can only happen inside a function/method block. While this satisfies all the test cases, I'm not sure if there is a better way to check if a span is pointing to an expression or type.

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
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

[nnethercote]

I see you added the tests in the first commit and then made the change in the second commit. This is a good way to do things for PRs like this, because it makes it easy for the reviewer to see the effect of the change on the error messages. Except you didn't add the .stderr files in the first commit, whoops.

Everything else seems fine to me but I'm going to say r? @estebank because I don't know the answer to this question:

I'm not sure if there is a better way to check if a span is pointing to an expression or type.

[nnethercote]

I will do this again:

r? @estebank

because the bors queue is still showing me as the assignee.

[rustbot]

Could not assign reviewer from: estebank.
User(s) estebank are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

[estebank]

Interestingly, #121826 also happens to address the specific case seen in the test here and in the original report, but almost tangentially because of changing the reporting to looking at the root obligation, which in these cases is IntoIterator instead of the more commonly used Iterator. I think we need both changes (for people that would write trait T: IntoIterator { .. }).

I still think that it'd be better to provide more filtering information rather than less.

[estebank]

Instead of flat out removing the information from rustc_on_unimplemented, wouldn't it be better to add the information that this obligation is coming from an impl block so that it can be specially targeted for custom notes too? I'm thinking of things like "implementing this trait requires you to implement this other one because foo". That would require changing the rustc_on_unimplemented annotation itself.

[veera-sivarajan]

Thank you for the comments.

Based on the suggestions, the error would roughly look like this?

error[E0277]: `Vec<T>` does not implement the trait `Iterator`
  --> test.rs:12:17
   |
12 | impl<T> Bar for Vec<T> {}
   |                 ^^^^^^ Implementing this trait for `Vec<T>` requires you to implement trait `Iterator` for `Vec<T>`
   |
   = help: consider implementing the trait `Iterator` for `Vec<T>`
note: required by a bound in `Bar`
  --> test.rs:10:12
   |
10 | trait Bar: Iterator {}
   |            ^^^^^^^^ required by this bound in `Bar`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0277`.

[estebank]

Yes, that's exactly the right amount of context needed for this.

[Dylan-DPC]

@veera-sivarajan any updates on this? thanks

[veera-sivarajan]

Working on it. Will push my updates in a couple of days.

[veera-sivarajan]

My implementation of is_impl_block matches on assoicated types too. Is there a better way to check that a LocalDefId is in an impl signature and not inside the impl block?

https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/How.20to.20Check.20if.20Type.20is.20in.20Impl.20Signature/near/436368673

[estebank]

@veera-sivarajan wouldn't tcx.def_kind(def_id) work for you here?

[veera-sivarajan]

No, it doesn't. It returns Impl { of_trait: true } for cases like this

rust/tests/ui/associated-types/hr-associated-type-bound-1.rs

Line 12 in 561b5de

type U = str;

too.

Will try to find some other way to check for types in impl signature.