Skip to content

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Oct 15, 2025

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 15, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 15, 2025

r? @jdonszelmann

rustbot has assigned @jdonszelmann.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@fmease fmease Oct 15, 2025

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.

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2025
@fmease fmease assigned fmease and unassigned jdonszelmann Oct 15, 2025
@jackh726
Copy link
Member

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 generics but not in after_where_clauses, that was perhaps the motivating reason for this. Them being in the same place means that nobody can forget about them anywhere.

@rustbot
Copy link
Collaborator

rustbot commented Oct 15, 2025

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rustbot rustbot added T-clippy Relevant to the Clippy team. T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue. labels Oct 15, 2025
@cjgillot
Copy link
Contributor Author

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'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.

I have mixed feelings here. I worry that this could lead to places checking the where clauses in generics but not in after_where_clauses, that was perhaps the motivating reason for this. Them being in the same place means that nobody can forget about them anywhere.

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.

@rustbot

This comment has been minimized.

@cjgillot cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 16, 2025
@jackh726
Copy link
Member

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'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.

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 generics and after_where_clauses (whereas prior to this PR, it's not an "extra" thing you have to think about)

I have mixed feelings here. I worry that this could lead to places checking the where clauses in generics but not in after_where_clauses, that was perhaps the motivating reason for this. Them being in the same place means that nobody can forget about them anywhere.

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.

Okay, good enough for me, I guess.

@rustbot
Copy link
Collaborator

rustbot commented Oct 19, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustfmt Relevant to the rustfmt team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ICE: mid > len on leading assoc ty / lazy type alias where clause with attributes

6 participants