diagnostics: add note when param-env shadows global impl#150424
diagnostics: add note when param-env shadows global impl#150424xonx4l wants to merge 9 commits intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
| let all_impls = | ||
| impls.blanket_impls().iter().chain(impls.non_blanket_impls().values().flatten()); | ||
|
|
||
| for &impl_def_id in all_impls { |
There was a problem hiding this comment.
use compute_applicable_impls_for_diagnostics instead of all_impls here
There was a problem hiding this comment.
and specialization_graph::assoc_def to get the DefId of the assoc item
| "the associated type `{}` is defined as `{}` in the implementation, \ | ||
| but the generic bound `{}` hides this definition", |
There was a problem hiding this comment.
cc @rust-lang/types unsure about this note. I think it's certainly useful. Ideally we'd link to some reference section or github issue explaining impl shadowing.
I think it's easiest to write a special github issue for this quickly explaining why this happens, what they can do about it, and how this may change in the future
There was a problem hiding this comment.
Im not sure a github issue is the best place but also I don't know where would be better... If something reference-like was t-types controlled then certainly there 🤔 github issue is probably good enough :)
There was a problem hiding this comment.
should I open a issue then ?
There was a problem hiding this comment.
going to do so myself as I have strong opinions on the way we frame this
There was a problem hiding this comment.
opened #152409. It took me a while to get to this 😅 sorry
can you refer to this issue in the note (and also make sure its wording matches the wording used in that issue)
r=me after
This comment has been minimized.
This comment has been minimized.
xonx4l
force-pushed
the
suggest_param_env_shadowing
branch
from
22a4a6b to
f616235
Compare
|
@lcnr I made the suggested changes and CI is also green now . |
|
@lcnr is anything more required other than squashing commits?? |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: lcnr <rust@lcnr.de>
xonx4l
force-pushed
the
suggest_param_env_shadowing
branch
from
408b4a3 to
071d0cd
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
| }; | ||
|
|
||
| let impl_item_def_id = leaf_def.item.def_id; | ||
| let impl_assoc_ty = tcx.type_of(impl_item_def_id).instantiate(tcx, impl_substs); |
There was a problem hiding this comment.
this currently incorrectly emits the note for the following I think
trait Trait {
type Assoc;
}
impl<T> Trait for T {
type Assoc = T;
}
fn foo<T: Trait>(x: T::Assoc) -> u32 { x }This also doesn't handle GATs and the following probably ICEs
trait Trait {
type Assoc<T>;
}
impl<T> Trait for T {
type Assoc<U> = U;
}
fn foo<T: Trait>(x: T::Assoc<T>) -> u32 { x }There was a problem hiding this comment.
to handle this, consider the relevant code in fn translate_args
goal.predicate.alias.args.rebase_onto(cx, impl_trait_ref.def_id.into(), impl_args)for this, you actually do have to equate the alias trait_ref with the impl trait_ref (and you can unwrap this equate as it should never fail)
There was a problem hiding this comment.
I've made changes to the logic to handle GATs as suggested. Thanks for the direction.
There was a problem hiding this comment.
please add the aboe snippets as tests
There was a problem hiding this comment.
Done. Added the required tests.
| @@ -0,0 +1,13 @@ | |||
| trait Trait { | |||
There was a problem hiding this comment.
can you add a comment to each test explaining what it is testing?
There was a problem hiding this comment.
Added all the comments for the tests.
5 participants
This PR adds a diagnostics note when param-env shadows global impl as discussed in #149910
It adds a note explaining that the definition is hidden by the generic bound.
r?lcnr