-
Couldn't load subscription status.
- 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
Retire ast::TyAliasWhereClauses. #147713
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
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.
With my comment above I meant removing any of the redundant and distracting bits of cfg-attr-issue-138010-1.rs given it's a fuzzer-produced file (extra functions, check-cfg warnings, a useless let binding, useless (): bounds, etc.).
It can be distilled to:
#![feature(lazy_type_alias)]
#![expect(incomplete_features)]
type Type
where
#[cfg(false)]
String: Copy,
= ();
fn main() {}… which just shows that it's just cfg-attr-issue-138010-2.rs.
So could you please throw out the *-1.rs or replace *-1.rs with the MCVE above and remove the *-2.rs (I don't really care about the #[cfg(true)] case but whatever).
|
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 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.
With my comment above I meant removing any of the redundant and distracting bits of cfg-attr-issue-138010-1.rs given it's a fuzzer-produced file (extra functions, check-cfg warnings, a useless let binding, useless (): bounds, etc.).
It can be distilled to:
#![feature(lazy_type_alias)]
#![expect(incomplete_features)]
type Type
where
#[cfg(false)]
String: Copy,
= ();
fn main() {}… which just shows that it's just cfg-attr-issue-138010-2.rs.
So could you please throw out the *-1.rs or replace *-1.rs with the MCVE above and remove the *-2.rs (I don't really care about the #[cfg(true)] case but whatever).
|
Thanks! Some final nits (+ optional squash), then r=me @bors rollup |
|
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. |
|
@bors r=fmease |
Rollup merge of #147713 - cjgillot:where-cfg, r=fmease Retire ast::TyAliasWhereClauses. `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
Rollup of 3 pull requests Successful merges: - rust-lang/rust#134316 (Add `String::replace_first` and `String::replace_last`) - rust-lang/rust#147713 (Retire ast::TyAliasWhereClauses.) - rust-lang/rust#148011 (Revert constification of `AsRef for Cow` due to inference failure ) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 3 pull requests Successful merges: - rust-lang/rust#134316 (Add `String::replace_first` and `String::replace_last`) - rust-lang/rust#147713 (Retire ast::TyAliasWhereClauses.) - rust-lang/rust#148011 (Revert constification of `AsRef for Cow` due to inference failure ) r? `@ghost` `@rustbot` modify labels: rollup
ast::TyAliasWhereClausesis a tentative to avoid forgetting predicates when manipulating the AST.It is incompatible with
cfgattributes on where clauses.This PR uses a regular
WhereClausefor the "second" clause.Fixes #138010
cc #138037