-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Retire ast::TyAliasWhereClauses. #147713
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
base: master
Are you sure you want to change the base?
Retire ast::TyAliasWhereClauses. #147713
Conversation
rustbot has assigned @jdonszelmann. Use |
This comment has been minimized.
This comment has been minimized.
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.
Could you use the more minimal reproducers, #138010 (comment) and/or #138010 (comment) which are less obfuscated. Moreover, #138010 (comment) wouldn't require any extra unstable features.
IIRC adding the purpose of adding this type was never to avoid allocating vecs, but rather to maintain the two separate Span locations and the predicates derived for them, and also unify the handling of the where clauses (with the idea that we may remove the ability to remove the before where clauses at some point, I also don't remember if there was some specific motivating reason for this or if it was just a general decision). I'm a little surprised we don't regress any test outputs, but I guess all the info is still there, but now less well-defined. I have mixed feelings here. I worry that this could lead to places checking the where clauses in |
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
I'm not sure about the "less well-defined". In the current state, we have the information about the "before" span twice. Here we have everything once.
This is mitigated by lowering. Most of the places where we manipulate predicates are on HIR, which does merge the lists. On the other hand, AST needs to be as close to the concrete syntax as possible for macro expansion. |
This comment has been minimized.
This comment has been minimized.
Yes, "less well-defined" is perhaps not the right way to say this. What I mostly trying to say is that I could have imagined that it would have been easy (and could be easy in the future) to miss a case where we need to do something with both the where clauses in
Okay, good enough for me, I guess. |
This PR was rebased onto a different master 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. |
ast::TyAliasWhereClauses
is a tentative to avoid forgetting predicates when manipulating the AST.It is incompatible with
cfg
attributes on where clauses.This PR uses a regular
WhereClause
for the "second" clause.Fixes #138010
cc #138037