feat(transformer/class-properties): transform super assignment expressions that are inside static prop initializer#7959
Conversation
github-actions
bot
added
A-ast
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Dunqing
force-pushed
the
12-17-refactor_transformer_class-properties_make_transform_instance_assignment_expression_more_generic
branch
from
79767e8 to
8a4be03
Compare
Dunqing
force-pushed
the
12-17-feat_transformer_class-properties_transform_super_assignment_expressions_that_are_inside_static_prop_initializer
branch
from
f70a0fd to
041b7f6
Compare
CodSpeed Performance ReportMerging #7959 will not alter performanceComparing Summary
|
…sions that are inside static prop initializer
Dunqing
force-pushed
the
12-17-feat_transformer_class-properties_transform_super_assignment_expressions_that_are_inside_static_prop_initializer
branch
from
041b7f6 to
a949281
Compare
| let property = &member.property; | ||
| let property = | ||
| ctx.ast.expression_string_literal(property.span, property.name.clone(), None); | ||
| let property = Expression::StringLiteral(ctx.ast.alloc(member.property.clone().into())); |
There was a problem hiding this comment.
This is shorter, but I'm not really a fan of impl From<IdentifierName> for StringLiteral.
Reason is: You're dealing with an owned type on the stack. Usually, it's better to allocate nodes into the arena as quickly as possible, and then only hold smaller types like:
Box<T>(8 bytes)- References
&Tor&mut T(8 bytes) - Enum types e.g.
Expression,Statement(16 bytes) Atom(16 bytes)
Generally, if you find yourself having to use ctx.ast.alloc, you're not in an optimal situation, because it means you have a large type on the stack.
In the original version, you only had to clone the Atom (which is essentially a Copy), whereas in this new version you have to clone the whole IdentifierName to be able to pass it to from.
In this case, it's not a big deal, because IdentifierName is only 24 bytes, but in general in my opinion it's not the best pattern.
If you wanted to, you could implement this conversion instead as a method on AstBuilder - AstBuilder:: expression_string_literal_from_identifier_name. But personally I think that'd be overkill unless we need to do that conversion in many places.
| // `object.#prop = value`, `object.#prop += value`, `object.#prop ??= value` etc | ||
| Expression::AssignmentExpression(_) => { | ||
| self.class_properties.transform_assignment_expression(expr, self.ctx); | ||
| self.class_properties.transform_super_assignment_expression(expr, self.ctx); |
There was a problem hiding this comment.
I think this is the cause of the panic in conformance. Problem is that transform_assignment_expression sometimes mutates expr so it's not an AssignmentExpression any more. So then transform_super_assignment_expression enters unreachable!().
So we need to check that it's still an AssignmentExpression:
| self.class_properties.transform_super_assignment_expression(expr, self.ctx); | |
| if matches!(expr, Expression::AssignmentExpression(_)) { | |
| self.class_properties.transform_super_assignment_expression(expr, self.ctx); | |
| } |
Having to do this check again is inefficient, but don't worry - this visitor will be removed anyway in my forthcoming refactoring, so we can change it then.
Boshen
deleted the
12-17-feat_transformer_class-properties_transform_super_assignment_expressions_that_are_inside_static_prop_initializer
branch
2 participants
No description provided.