-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Fix anon const def-creation when macros are involved take 2 #130337
Conversation
self.is_const_arg_trivial_macro_expansion(&*constant.value, Some(&constant)) | ||
{ | ||
self.pending_anon_const_info = Some(pending_anon); | ||
return self.visit_macro_invoc(macro_invoc); |
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.
This change to call visit_macro_invoc
instead of walk_anon_const
is what fixes #130321. Everything else in this PR is changes made to track if we've already unwrapped a brace or not (and related code deduplication since it would be pretty messy otherwise)
@@ -1199,14 +1199,15 @@ impl Expr { | |||
} | |||
} | |||
|
|||
pub fn maybe_unwrap_block(&self) -> &Expr { | |||
/// Returns an expression with (when possible) *one* outter brace removed | |||
pub fn maybe_unwrap_block(&self) -> (bool, &Expr) { |
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.
Wondering if it would be clearer to return Option<&Expr>
(also matching the function's name)? Then it could be used as expr.maybe_unwrap_block().unwrap_or(expr)
e.g. Maybe that'd be worse, but it just came to mind.
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.
Yeah I think this is probably just worse for every caller unfortunately because this is not the nicest name/signature combo 😅
tests/ui/const-generics/early/trivial-const-arg-macro-braced-expansion.rs
Show resolved
Hide resolved
90ff601
to
248dfe7
Compare
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.
r=me with these comments addressed. Thanks!
tests/ui/const-generics/early/trivial-const-arg-macro-braced-expansion.rs
Show resolved
Hide resolved
248dfe7
to
781ec11
Compare
@bors r=camelid rollup=never |
☀️ Test successful - checks-actions |
Finished benchmarking commit (55043f0): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary -0.7%, secondary -1.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 0.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 769.615s -> 769.437s (-0.02%) |
Fixes #130321
There were two cases that #129137 did not handle correctly:
Foo<{ bar!() }>
in whichbar!()
expands toN
, we would visit the anon const and then visit the{ bar() }
expression instead of visiting the macro call. This meant that we would build a def for the anon const as{ bar!() }
is not a trivial const argument asbar!()
is not a path.Foo<{ bar!() }>
is whichbar!()
expands to{ qux!() }
in whichqux!()
expands toN
, it should not be considered a trivial const argument as{{ N }}
has two pairs of braces. If we only looked atqux
's expansion it would look like a trivial const argument even though it is not. We have to track whether we have "unwrapped" a brace already when recursing into the expansions ofbar
/qux
/any macror? @camelid