-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Implement #[cfg]
in where
clauses
#132388
base: master
Are you sure you want to change the base?
Conversation
HIR ty lowering was modified cc @fmease Changes to the size of AST and/or HIR nodes. cc @nnethercote |
6b5f780
to
416f1eb
Compare
This comment has been minimized.
This comment has been minimized.
416f1eb
to
2aa303c
Compare
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
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.
Thanks @frank-king for the work. I haven't looked deeply into the parser code yet, I need to familiarize myself with it first.
#[cfg]
in where
clauses#[cfg]
in where
clauses
2aa303c
to
f3192a5
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
f3192a5
to
1a8208c
Compare
This comment has been minimized.
This comment has been minimized.
1a8208c
to
db4c4fd
Compare
This comment was marked as resolved.
This comment was marked as resolved.
db4c4fd
to
d8db4cf
Compare
This comment has been minimized.
This comment has been minimized.
d8db4cf
to
d0cdfab
Compare
…=<try> Refactor `where` predicates, and reserve for attributes support Refactor `WherePredicate` to `WherePredicateKind`, and reserve for attributes support in `where` predicates. This is a part of rust-lang#115590 and is split from rust-lang#132388. r? petrochenkov
c34c841
to
3411185
Compare
@rustbot ready |
(I didn't finish the full review yet.) |
This comment was marked as resolved.
This comment was marked as resolved.
3411185
to
a28ebca
Compare
This comment was marked as resolved.
This comment was marked as resolved.
a28ebca
to
26d4104
Compare
This comment was marked as resolved.
This comment was marked as resolved.
26d4104
to
8439c1f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
8439c1f
to
bf52c22
Compare
@@ -52,6 +52,7 @@ pub enum Annotatable { | |||
FieldDef(ast::FieldDef), | |||
Variant(ast::Variant), | |||
Crate(ast::Crate), | |||
WherePredicate(ast::WherePredicate), |
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.
Style nit: could you consistently put the logic for WherePredicate
immediately after the logic for Variant
s (in annotatables, ast fragments, etc)?
The new fragment is most similar to Variant
, and it's good to have things in the same order everywhere.
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.
Still need to reorder a few things in expand.rs.
span, | ||
kind: ast::WherePredicateKind::BoundPredicate(ast::WhereBoundPredicate { | ||
bound_generic_params: Default::default(), | ||
bounded_ty: ty(), |
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.
So you put a macro here to recognize placeholders, instead of adding the is_placeholder
flag like in variants.
This is a bit tricky and at least needs a documentation, but I'd probably recommend to just add the is_placeholder
flag instead.
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.
Note that you never check whether a where-predicate node is a placeholder, because this PR misses the attribute resolution/expansion logic in
- compiler\rustc_resolve\src\def_collector.rs
- compiler\rustc_resolve\src\build_reduced_graph.rs
- compiler\rustc_expand\src\placeholders.rs
Search for the .is_placeholder
there and you can also do the same things as Variant
s do.
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.
After gaining more knowledge of placeholders, I started to realize that here we need is_placeholder
because we don't have something like ast::ExprKind::MacCall
or ast::ItemKind::ItemCall
but we still need to repesent a macro call in the Variant
and WherePredicate
positions, is that correct?
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.
Is it better to add a new variant ast::WherePredicateKind::MacCall
? Then the size of ast::WherePredicate
can be smaller that adding an is_placeholder: bool
field. That also makes the codes more readable, and reserves the posibility that we can support macro calls in where clauses in the future.
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.
Is it better to add a new variant
ast::WherePredicateKind::MacCall
?
Nah, MacCall
is only for function-like macro calls, if they are not currently supported in some position, then it's better to not introduce it, and just use is_placeholder
.
compiler/rustc_passes/messages.ftl
Outdated
@@ -784,6 +784,10 @@ passes_unstable_attr_for_already_stable_feature = | |||
.item = the stability attribute annotates this item | |||
.help = consider removing the attribute | |||
passes_unsupported_attributes_in_where = | |||
attributes in `where` clauses are not supported |
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.
attributes in `where` clauses are not supported | |
most attributes are not supported in `where` clauses |
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.
"in where
clauses" in the end is a bit better grammatically.
Sorry for the long delay, I've finished the review now. |
bf52c22
to
db35ed6
Compare
Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
The job Click to see the possible cause of the failure (guessed by this bot)
|
#[derive_const(Clone)] ():, | ||
//~^ ERROR most attributes in `where` clauses are not supported | ||
//~| ERROR expected non-macro attribute, found attribute macro `derive_const` | ||
#[test_case] ():, |
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.
No need to test all the macros exhaustively, it makes the tests harder to read, just one macro would be enough.
☔ The latest upstream changes (presumably #135726) made this pull request unmergeable. Please resolve the merge conflicts. |
This PR implements #115590, which supports
#[cfg]
attributes inwhere
clauses.The biggest change is, that it adds
AttrsVec
andNodeId
to theast::WherePredicate
andHirId
to thehir::WherePredicate
.