-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Point to type parameter definition when not finding variant, method and associated item #98298
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
b6e51f0
to
0fc385b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some general nits that I would appreciate if you fix up. The changes look fine though, and I like the diagnostic improvement.
9dcc5d5
to
4a82fbb
Compare
This comment has been minimized.
This comment has been minimized.
4a82fbb
to
82e4e6d
Compare
r? @compiler-errors, if that's OK. |
let generics = self.tcx.generics_of( | ||
tcx.hir() | ||
.body_owner_def_id(hir::BodyId { hir_id: self.body_id }) | ||
.to_def_id(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I overcomplicated this. Instead of tcx.hir().body_owner_def_id(..)
, let's just do self.body_id.owner.to_def_id()
-- I've seen this being used elsewhere in typeck.
); | ||
let type_param = generics.type_param(param_type, self.tcx); | ||
Some((self.tcx.def_span(type_param.def_id), "for this")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a def-id, do you think it's useful to use self.def_kind(def_id).descr()
so we can say "for this type parameter" or "for this enum" below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is useful. I will implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I am beginning to think it might be redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, why so? I don't know why this is more clear (current):
LL | struct Foo {
| --- method `clone` not found for this
Than this (with the change):
LL | struct Foo {
| --- method `clone` not found for this struct
The "this" feels incomplete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at least we should be saying "for this type parameter" in this case, even if we don't add the def_kind(def_id).descr()
in the ADT case below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I will implement it 👍
After the review, I will squash my commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, and I'm glad to see the code got simpler. I think "here" in the original message was vague.
Please squash then r=me
@bors delegate+ |
✌️ @TaKO8Ki can now approve this pull request |
…ssoc type use `def_ident_span` , `body_owner_def_id` instead of `in_progress_typeck_results`, `guess_head_span` use `body_id.owner` directly add description to label
d7e4caa
to
402dceb
Compare
@bors r=compiler-errors |
📌 Commit 402dceb has been approved by |
…ion, r=compiler-errors Point to type parameter definition when not finding variant, method and associated item fixes rust-lang#77391
…ion, r=compiler-errors Point to type parameter definition when not finding variant, method and associated item fixes rust-lang#77391
…askrgr Rollup of 9 pull requests Successful merges: - rust-lang#96412 (Windows: Iterative `remove_dir_all`) - rust-lang#98126 (Mitigate MMIO stale data vulnerability) - rust-lang#98149 (Set relocation_model to Pic on emscripten target) - rust-lang#98194 (Leak pthread_{mutex,rwlock}_t if it's dropped while locked.) - rust-lang#98298 (Point to type parameter definition when not finding variant, method and associated item) - rust-lang#98311 (Reverse folder hierarchy) - rust-lang#98401 (Add tracking issues to `--extern` option docs.) - rust-lang#98429 (Use correct substs in enum discriminant cast) - rust-lang#98431 (Suggest defining variable as mutable on `&mut _` type mismatch in pats) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
fixes #77391