Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

veera-sivarajan
Copy link
Contributor

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
Copy link
Collaborator

rustbot commented Feb 28, 2024

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

@rustbot 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 Feb 28, 2024
@nnethercote
Copy link
Contributor

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.

@rustbot rustbot assigned estebank and unassigned nnethercote Feb 28, 2024
@nnethercote
Copy link
Contributor

I will do this again:

r? @estebank

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

@rustbot
Copy link
Collaborator

rustbot commented Mar 1, 2024

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.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +154 to +155
// Don't add specialized notes on types
if !self.tcx.hir().is_impl_block(obligation.cause.body_id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@estebank estebank 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 3, 2024
@Dylan-DPC
Copy link
Member

@veera-sivarajan any updates on this? thanks

@veera-sivarajan
Copy link
Contributor Author

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

@veera-sivarajan
Copy link
Contributor Author

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
Copy link
Contributor

estebank commented May 2, 2024

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

@veera-sivarajan
Copy link
Contributor Author

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

too.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bound on iterator suggests invalid code to call iter() on the impl
5 participants