Fix generic type substitution in impl trait with assoc type #11107
Conversation
| subst.clone_subtree().clone_for_update().syntax(), | ||
| ); | ||
| } else { | ||
| ted::replace( |
There was a problem hiding this comment.
Even without this else branch, all tests pass. I am wondering in which scenarios is this code executed and if that's what is desired.
There was a problem hiding this comment.
The only case where I can see this happening is in Patterns using the path I believe, so PathPat, or the path in TupleStructPat etc, might be good to check that.
There was a problem hiding this comment.
I couldn't come up with a code snippet that would exercise this branch. It would need to:
- use a generic type parameter, because this part of code deals with
hir::PathResolution::TypeParam(tp)substitution - must not use associated type/constant, because this case is handled by
if let Some(parent) = ast::Path::cast(parent.clone())above - must be in a pattern or path expression
Therefore, as far as I can tell, the path here will always be a type (having PathType as parent) or will have an associated type/constant (having Path as parent). And so this else branch seems unreachable to me and can be removed.
Nevertheless, thanks to playing with path patterns and expressions, I discovered that handling associated constants was missing in my fix. It works now (a710b87).
There was a problem hiding this comment.
Can't hurt leaving the branch either so I think keeping it is fine, either way works.
PR lgtm then, thanks!
bors d+
| if let hir::ModuleDef::Trait(_) = def { | ||
| if matches!(path.segment()?.kind()?, ast::PathSegmentKind::Type { .. }) { | ||
| // `speculative_resolve` resolves segments like `<T as | ||
| // Trait>` into `Trait`, but just the trait name should | ||
| // not be used as the replacement of the original | ||
| // segment. | ||
| return None; | ||
| } | ||
| } |
There was a problem hiding this comment.
This feels hacky. But without this exception, the following thing happens:
pub trait Behavior<T: Types> {
fn reproduce(&self, foo: <T as Types>::Foo);
}
pub struct Impl;
impl Behavior<u32> for Impl { $0 }
// -->
impl Behavior<u32> for Impl {
fn reproduce(&self, foo: Types::Foo) {
${0:todo!()}
}
}
|
✌️ pnevyk can now approve this pull request. To approve and merge a pull request, simply reply with |
|
bors r+ |
Fixes #11045
The path transform now detects if a type parameter that is being substituted has an associated type. In that case it is necessary (or safe in general case) to fully qualify the substitution with a trait which the associated type belongs to.
This PR also fixes the previous wrong behavior of the substitution that could create an invalid tree
PATH_TYPE -> PATH_TYPE -> ....