perf(es/compat): Merge exponentation_operator
#11310
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
kdy1
changed the title
kdy1
force-pushed
the
kdy1/exponentation-operator
branch
from
946f74a to
3f8225b
Compare
Pull Request Review: perf(es/compat): Merge exponentation operatorThank you for working on this performance optimization! I've reviewed the PR and have several concerns about the implementation. Critical Issues1. Empty Implementation - No Transformation LogicThe new #[derive(Default)]
struct ExponentiationOperatorPass {}
impl VisitMutHook<TraverseCtx> for ExponentiationOperatorPass {}This is missing the actual transformation logic that converts exponentiation operators. The existing implementation in
Impact: Without this logic, the exponentiation operator will not be transformed at all, breaking compatibility for ES2016 targets. 2. Typo in Field NameIn options.env.es2016.exponentation_operator = true;The field is named 3. Missing Import RemovalIn Code Quality & Best Practices4. Missing DocumentationFollowing CLAUDE.md guideline #7 ("Write documentation for your code"), the new module and struct lack documentation explaining:
The existing implementation has excellent documentation starting at line 9 of 5. Missing Unit TestsFollowing CLAUDE.md guideline #5 ("Write unit tests for your code"), there are no tests for the new implementation. The existing implementation includes comprehensive tests starting at line 153 of Performance ConsiderationsThe PR title mentions this is a performance optimization to "merge" the exponentiation operator pass. However:
Recommendations
Security & Other ConcernsNo security issues identified. The transformation itself is straightforward AST manipulation. Summary: This PR appears to be a work-in-progress. The structure is good, but the implementation is incomplete. Please address the empty transformation logic before merging. |
Code Review - PR #11310: perf(es/compat): Merge exponentation operatorSummaryThis PR is migrating the exponentiation operator transformation from a separate pass to being merged into the unified single-pass transformer architecture. This is part of a larger effort to improve compilation performance by reducing the number of AST traversals. Critical Issues1. Typo in naming: 'exponentation' instead of 'exponentiation'Severity: High Throughout the PR, there's a consistent typo:
Location:
Recommendation: Rename all instances to use the correct spelling 2. Empty implementation - No actual transformation logicSeverity: Critical 🚨 The new #[derive(Default)]
struct ExponentiationOperatorPass {}
impl VisitMutHook<TraverseCtx> for ExponentiationOperatorPass {}Issue: The implementation has no methods that actually transform the exponentiation operator. The existing transformation logic from Expected implementation: Should include hooks like
Location: Performance Considerations3. Performance improvement is the goal but implementation is incompleteStatus: In Progress The PR title mentions "perf" improvement by merging this operator into the single-pass compiler. However, since the transformation logic hasn't been implemented yet:
Test Coverage4. No tests includedSeverity: High According to CLAUDE.md guideline #5: "Write unit tests for your code." The original implementation in
Recommendation: Port these tests to the new architecture or ensure integration tests verify the transformation still works. Code Quality5. Documentation missingSeverity: Medium According to CLAUDE.md guideline #7: "Write documentation for your code." The new
The original implementation had good documentation: /// `@babel/plugin-transform-exponentiation-operator`
///
/// # Example
/// ## In
/// ```js
/// let x = 10 ** 2;
/// ```
/// ## Out
/// ```js
/// let x = Math.pow(10, 2);
/// ```
Architecture & Design6. Integration with preset-env is correctStatus: Good ✅ The change in
This follows the same pattern as the related PR #11309 for 7. OptionalHook usage is appropriateStatus: Good ✅ Using Security ConcernsNo security issues identified. The transformation deals with syntax conversion only. RecommendationsBefore merging:
Missing implementation details to port:
Related ContextThis PR is part of a stack (depends on #11309) working toward merging all ES compatibility transforms into a unified single-pass compiler for better performance. The approach is sound, but this particular PR appears to be incomplete and may have been accidentally pushed before implementation was finished. Overall Assessment: The architectural direction is correct and follows established patterns in the codebase, but the implementation is incomplete. The most critical issue is that the transformation logic is missing, which will break ES2016 compatibility for code using the exponentiation operator. |
Code Review for PR #11310: perf(es/compat): Merge exponentation operatorThank you for this performance optimization! This PR aims to merge the exponentiation operator transformation into the single-pass compiler. However, I've identified several critical issues that need to be addressed before this can be merged. 🔴 Critical Issues1. Empty Implementation - No Transformation LogicThe new impl VisitMutHook<TraverseCtx> for ExponentiationOperatorPass {}Issue: This implementation doesn't actually transform exponentiation operators (
Impact: This PR will completely break exponentiation transformation, causing ES2016+ code to remain untransformed for older targets. Fix Required: The transformation logic from 2. Typo in Field Namepub exponentation_operator: bool, // Should be: exponentiation_operatorLocation:
Issue: "exponentation" should be "exponentiation" (missing 'i'). This typo appears consistently across the codebase and in file names ( Fix Required: Rename to
|
CodSpeed Performance ReportMerging #11310 will degrade performances by 6.16%Comparing Summary
Benchmarks breakdown
Footnotes |
Binary Sizes
Commit: c779486 |
kdy1
force-pushed
the
kdy1/exponentation-operator
branch
from
3f8225b to
1252b09
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the ES2016 exponentiation operator transformation to use an optional hook pattern, enabling conditional compilation based on configuration. The changes consolidate the transformation logic into a new module structure that can be selectively enabled.
- Introduces an
Es2016Optionsstruct with a configurable flag for the exponentiation operator transform - Replaces the previous
Es2016Passimplementation with anOptionalHookthat conditionally applies the transformation - Creates a new module
exponentation_operator(note: misspelled) with a dedicated pass for the exponentiation operator
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/swc_ecma_transformer/src/es2016/mod.rs | Refactored to use OptionalHook pattern with Es2016Options for conditional exponentiation operator transformation |
| crates/swc_ecma_transformer/src/es2016/exponentation_operator.rs | New module containing dedicated ExponentiationOperatorPass (contains spelling errors) |
| crates/swc_ecma_preset_env/src/lib.rs | Updated to set exponentation_operator flag based on Feature::ExponentiationOperator compatibility check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,17 +1,19 @@ | |||
| mod exponentation_operator; | |||
There was a problem hiding this comment.
Misspelled module name. Should be "exponentiation_operator" instead of "exponentation_operator" to match the standard spelling and the rest of the codebase (e.g., Feature::ExponentiationOperator, exponentiation() function).
| pub fn hook(options: Es2016Options) -> impl VisitMutHook<TraverseCtx> { | ||
| Es2016Pass { options } | ||
| pub struct Es2016Options { | ||
| pub exponentation_operator: bool, |
There was a problem hiding this comment.
Misspelled field name. Should be "exponentiation_operator" instead of "exponentation_operator" to match the standard spelling and the rest of the codebase.
| OptionalHook(if options.exponentation_operator { | ||
| Some(self::exponentation_operator::hook()) |
There was a problem hiding this comment.
Misspelled identifiers. Should be "exponentiation_operator" instead of "exponentation_operator" to match the standard spelling and the rest of the codebase.
| use swc_ecma_hooks::VisitMutHook; | ||
|
|
||
| use crate::TraverseCtx; | ||
|
|
||
| pub fn hook() -> impl VisitMutHook<TraverseCtx> { | ||
| ExponentiationOperatorPass::default() | ||
| } | ||
|
|
||
| #[derive(Default)] | ||
| struct ExponentiationOperatorPass {} | ||
|
|
||
| impl VisitMutHook<TraverseCtx> for ExponentiationOperatorPass {} |
There was a problem hiding this comment.
Misspelled filename. This file should be named "exponentiation_operator.rs" instead of "exponentation_operator.rs" to match the standard spelling and the rest of the codebase.
Pull Request Review: perf(es/compat): Merge exponentation operatorThank you for working on this performance optimization! I've reviewed PR #11310 and identified several critical issues that need to be addressed before merging. 🔴 Critical Issues1. Empty Implementation - No Transformation LogicLocation: The new #[derive(Default)]
struct ExponentiationOperatorPass {}
impl VisitMutHook<TraverseCtx> for ExponentiationOperatorPass {}Problem: This is missing all the actual transformation logic. The existing implementation in
Impact: Without this logic, the exponentiation operator will not be transformed at all when using the new transformer, breaking ES2016 compatibility for affected targets. Recommendation: Port the full transformation logic from 2. Typo: "exponentation" should be "exponentiation"Locations:
Problem: Consistent misspelling throughout the codebase. The correct spelling is "exponentiation" (with an 'i'). Recommendation: Rename all occurrences to use the correct spelling. 3. Failing CI TestThe test suite Recommendation: Investigate and fix the failing test before merging. 📋 Code Quality & Best Practices✅ Positive Aspects:
|
kdy1
changed the title
exponentation_operator
Code Review for PR #11310SummaryThis PR aims to merge the exponentiation operator transformation into the new architecture. However, there is a critical issue: the new implementation is incomplete. 🚨 Critical Issues1. Empty Implementation (crates/swc_ecma_transformer/src/es2016/exponentiation_operator.rs:12)The new impl VisitMutHook<TraverseCtx> for ExponentiationOperatorPass {}Problem: This does nothing. The original implementation in
Impact: All exponentiation operator transformations will silently fail. Tests will likely fail. Fix needed: The transformation logic needs to be implemented. At minimum, you need: impl VisitMutHook<TraverseCtx> for ExponentiationOperatorPass {
fn enter_expr(&mut self, node: &mut Expr, _ctx: &mut TraverseCtx) {
// Handle BinExpr with op!("\*\*")
// Handle AssignExpr with op!("\*\*=")
}
}
|
kdy1
force-pushed
the
kdy1/exponentation-operator
branch
from
28a97ff to
267c144
Compare
Pull Request Review: Merge
|
Code Review for PR #11310: perf(es/compat): Merge
|
Pull Request ReviewOverviewThis PR refactors the exponentiation operator transformation by moving it from into a new unified transformer architecture (). The implementation shows good architectural improvements but has several critical issues that need to be addressed. Critical Issues1. Incorrect Side-Effect Handling (Major Bug)Location: The current implementation duplicates property accesses and function calls, causing side effects to execute multiple times: // Input
array[++i] **= 2
// Current (incorrect) output - evaluates ++i multiple times
array[++i] = Math.pow(array[++i], 2)
// Expected output - evaluates ++i only once
var _ref = ++i;
array[_ref] = Math.pow(array[_ref], 2)Impact: This is a critical correctness bug. The test snapshots show this is intentional, but it's semantically incorrect according to the ES2016 spec. The old implementation in the diff appears to have attempted to handle this with temporary variables. Evidence from test snapshots:
Recommendation: Implement proper memoization using temporary variables for:
The old implementation had code for this (lines using 2. Unsafe Pointer UsageLocation: let address = stmt as *const Stmt as usize;Issue: Using raw pointer addresses as HashMap keys is inherently unsafe and unreliable:
Recommendation: Use a more robust addressing scheme:
Code Quality Issues3. Incomplete ImplementationLocation: // For simplicity, handle the basic case without temp variables
// A more complete implementation would add temp variables to avoid side effectsThis comment acknowledges the side-effect problem but doesn't address it. This is insufficient for a production compiler. 4. Dead CodeLocation: The entire
Question: Is this infrastructure intended for future use, or was it meant to be used in this PR but got left incomplete? 5. Inconsistent DUMMY_SP UsageThroughout 6. Missing Test for Side EffectsThe ignored test reader.x **= 2;
expect(counters).toBe(1); // Should only evaluate 'reader' onceThis test being ignored is a red flag that the implementation doesn't handle this correctly. Positive Aspects
Performance ConsiderationsThe refactoring aligns with the project goal (per CLAUDE.md: "Write performant code"). However:
RecommendationsMust Fix (Blocking)
Should Fix
Nice to Have
Security & SafetyNo security vulnerabilities identified, but the pointer casting is unsafe and could lead to undefined behavior. ConclusionThis PR makes good architectural progress but has a critical correctness bug in side-effect handling that makes it unsuitable for merging as-is. The exponentiation operator implementation needs to properly memoize expressions to avoid evaluating them multiple times. The test snapshots appear to codify incorrect behavior - they should be updated once the implementation is fixed to use temporary variables appropriately. Recommendation: Request changes to address the side-effect handling before merging. |
Pull Request ReviewThis PR refactors the exponentiation operator transformation by merging it into a new unified transformer architecture. Here's my review: ✅ Strengths
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Code Review for PR #11310: Merge
|
Description:
BREAKING CHANGE:
Related issue (if exists):