-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
alias-relate: add fast reject optimization #124852
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
this unfortunately does not fix the hang in rayon. closing while we're trying other approaches |
if self.fast_reject_unnameable_rigid_term(param_env, lhs, rhs) | ||
&& self.fast_reject_unnameable_rigid_term(param_env, rhs, lhs) |
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.
damn 🤔 if only there was a reason why this fast path didn't fix the hang in rayon
this literally never triggers 🙃 it needs to be ||
if there's |
5908e13
to
97b546f
Compare
☔ The latest upstream changes (presumably #125324) made this pull request unmergeable. Please resolve the merge conflicts. |
97b546f
to
ea50666
Compare
ea50666
to
ac9bcd8
Compare
return false; | ||
} | ||
|
||
let mut referenced_placeholders = |
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.
Why IgnoreAliases::Yes
?
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.
Feels like this fast reject could be quickly side-stepped w/ something like Mirror<!A> alias-relate Mirror<!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.
Update: It occurred to me last night while I was sleeping that we ignore aliases here b/c e.g. Alias<!A>
could normalize to sth not mentioning !A
.
|
||
let mut referenced_placeholders = | ||
self.collect_placeholders_in_term(rigid_term, IgnoreAliases::Yes); | ||
for clause in param_env.caller_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.
Would be nice to explain in detail what this is searching for. Specifically, we disqualify any of our rigid term's placeholders if they show up in projection goals, since that may assist in normalizing.
e.g. if we have
rigid = W<!A>
alias = Alias<!B>
param-env = [Alias<!B> normalizes-to W<!A>, ...]
then we exclude !A
from our rigid placeholder list, right?
I think this may also be worth mentioning that this doesn't really care about how the placeholders show up in the term of the projection predicate, since this is really just a quick heuristic.
} | ||
} | ||
|
||
if referenced_placeholders.is_empty() { |
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.
Probably a somewhat redundant note, but maybe just say "empty set is a subset of everything, so no need to compute placeholders of alias"
} | ||
|
||
let alias_placeholders = self.collect_placeholders_in_term(alias, IgnoreAliases::No); | ||
// If the rigid term references a placeholder not mentioned by the alias, |
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.
// If the rigid term references a placeholder not mentioned by the alias, | |
// If the rigid term references a placeholder not mentioned by the alias, and isn't potentially reachable by a projection-predicate, |
or something, to explain the way that the projection predicate search above influences this search
My only outstanding question was answered by myself. I think the only tweaks I want are more thorough comments bc this code is subtle. r=me afterwards |
@rustbot author |
☔ The latest upstream changes (presumably #126614) made this pull request unmergeable. Please resolve the merge conflicts. |
@lcnr |
may be necessary to compile rayon rust-lang/trait-system-refactor-initiative#109
r? @compiler-errors