-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: Properly lower SelfOnly predicates
#21399
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
ChayimFriedman2
left a comment
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.
Few nits and one comment.
crates/hir-ty/src/lower.rs
Outdated
| db: &'db dyn HirDatabase, | ||
| type_alias: TypeAliasId, | ||
| ) -> (EarlyBinder<'db, Clauses<'db>>, Diagnostics) { | ||
| ) -> (EarlyBinder<'db, Clauses<'db>>, u32, Diagnostics) { |
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.
Return a struct? The role of the u32 isn't clear from the outside.
crates/hir-ty/src/lower.rs
Outdated
| self.predicates.get().map_bound(|it| &it.as_slice()[self.own_predicates_start as usize..]) | ||
| } | ||
|
|
||
| /// Returns the predicates, minus the implicit `Self: Trait` predicate for a trait. |
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.
Update this comment?
crates/hir-ty/src/lower.rs
Outdated
|
|
||
| let diagnostics = create_diagnostics(ctx.diagnostics); | ||
| let assoc_ty_bounds_start = predicates.len() as u32; | ||
| predicates.extend(assoc_ty_bounds); |
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 will mean query_own() will get the assoc type bounds from the parent as well, which isn't what we want. I think you'll need to have two assoc_ty_bounds_starts.
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.
Oh, right. I'm messing up bounds from parents here 😅
05c1226 to
b73bd07
Compare
b73bd07 to
7a48463
Compare
Fixes #19339
In the following code:
And we have some trait bounds like
Self: Foo<Assoc1 = Bar, Assoc2: Baz>, rustc considers<Self as Foo>::Assoc1 = Baras a self bound<Self as Foo>::Assoc2: Bazas a non-self boundhttps://github.com/rust-lang/rust/blob/00f49d22042dce59561b36eb075bbf5e343c23c4/compiler/rustc_hir_analysis/src/hir_ty_lowering/bounds.rs#L547-L668
As I have previously stated in some FIXME comment 😅, we and rustc has a bit different predicates lowering implementation: rustc lowers them multiple times with granular filtering options, while we lower them as little as possible.
I think our current implementation makes sense for memory usages and we don't need much granular filtering options like rustc (yet) so I just reordered those assoc item bounds at the end of the predicates and discern them by range.