-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Higher-ranked trait bounds for where clauses #6921
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
Conversation
crates/hir_def/src/generics.rs
Outdated
| bound: LifetimeRef, | ||
| }, | ||
| ForLifetime { | ||
| lifetimes: Box<[LifetimeRef]>, |
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.
these should just be Names, I think; they're not references to existing lifetimes, but defining new ones
crates/hir_def/src/generics.rs
Outdated
| .map_or_else(Name::missing, |lt| Name::new_lifetime(<)); | ||
| let param = LifetimeParamData { name: name.clone() }; | ||
| // FIXME: allocating the lifetime here means it looks like this lifetime belongs to the entire item | ||
| // but it only belongs to the bounded type-param |
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.
Yeah, I'm not sure giving these lifetimes a LifetimeParamId at all is the correct approach, since they're not parameters of the item. (There's another place where new lifetime names can be defined, namely for<'x> fn() function pointer types, and I think these two should work the same.)
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.
Also, while shadowing an existing lifetime is disallowed, you can easily have the same lifetime twice on an item this way:
fn foo<T, U>() where for<'b> U: 'b, for<'b> T: 'b {}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.
Well the main reason for giving them LifetimeParamIds is that for reference search we need to be able to address them as code_model::LifetimeParams, which is backed by a LifetimeParamId. And technically they are LifetimeParams right? Just with a smaller scope. Though I can't really think of a way to solve this problem, so I guess it's better to just not support reference search for these for the time being.
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.
Yeah, I don't really know how to model this yet either, and I think supporting reference search for these is probably not super important right now.
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.
Yea I agree, will remove those parts for now then. 👍
|
r? @flodiebold |
|
bors r+ |
There is a slight problem with this which is also noted in a FIXME now but
LifetimeParametersof these ForLifetime where clauses allocate the lifetimes in the corresponding arena as if they were lifetimes of the item itself and not just the clause they belong to. I wasn't entirely sure what I could do about this but given nothing really uses lifetimes like that currently I figured it might be fine? Open to suggestions for that problem.