-
Couldn't load subscription status.
- Fork 13.9k
Fix split index calculation in check_type_alias_where_clause_location
#138037
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
This comment has been minimized.
This comment has been minimized.
895a31f to
a78592e
Compare
check_type_alias_where_clause_location
check_type_alias_where_clause_locationcheck_type_alias_where_clause_location
|
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
| before_with_attr_count: before_where_clause | ||
| .predicates | ||
| .iter() | ||
| .filter(|p| !p.attrs.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.
Filtering things which have attributes applied to them isn't really correct because it's not necessarily the case that all cfgd where clauses will be removed during macro expansion, e.g.
#![feature(where_clause_attrs, cfg_boolean_literals, lazy_type_alias)]
#![expect(incomplete_features)]
struct Foo;
trait Trait {}
impl Trait for Foo {}
type MixedWhereBounds
where
#[cfg(true)]
Foo: Trait,
= Foo
where
(): Sized;Here we have a where clause before the = that has an attribute applied to it, yet the where clause will still be present after macro expansion.
If you run the compiler on this example with your changes, and then remove the cfg, you'll get different diagnostics output even though post-expansion it'll be the same set of where clauses. That's caused by this before_with_attr_count being 1 and then when we split the where clauses into before/after you wind up splitting at 0 so both clauses end up in the "after" bucket.
Can you add both version as a test (with/without cfg(true))?
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.
I'm not really sure what the correct way to implement this is 🤔
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.
Thank you for your detailed explanation, I will add a few tests first.
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.
Hi @BoxyUwU, have you had a chance to think about this more? Or if you know of another expert in this area, we could see if they have any ideas? 🙂
a78592e to
ba719b9
Compare
|
Some tests have been added (the results are in ba719b9#diff-9daeebc4c3e4cd13b6240809d45a7c6ebb58972f6c7c032df74a9eeb53b52300), and while the error messages look good, the suggestions are a bit inaccurate. |
|
@rustbot ready |
|
☔ The latest upstream changes (presumably #140388) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I think this is still waiting on figuring out how to implement it correctly |
|
There is no clean way to implement it while keeping a single predicate list. cfg-expansion does not track which is before/after the equal sign. |
|
seems reasonable to me |
|
@stuuupidcat any updates on this? thanks |
|
Sorry, no progress yet. The proposed fix is a little complex for me to understand and implement. |
|
I'm closing this in favor of #147713. Thanks for your contribution regardless! |
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 rust-lang#138010 cc rust-lang#138037
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
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 rust-lang/rust#138010 cc rust-lang/rust#138037
Fix the calculation of the split index in
check_type_alias_where_clause_locationby ignoring before-equals-sign-clauses with attributes.Fix #138010.
cc @frank-king.