-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Meta] Port single-pass compiler from oxc #11292
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
base: main
Are you sure you want to change the base?
Conversation
|
Code Review: PR #11292 - Port oxc single-pass transform using DevBirdOverviewThis PR adds 29,331 lines of new code porting oxc's single-pass transform to SWC. This is a massive change that introduces an entirely new transformer implementation in the Positive Aspects
Critical Concerns1. Test Coverage (CRITICAL)
2. Incomplete Implementation (HIGH)Several features are explicitly marked as incomplete or TODO:
3. Unsafe Code Usage (MEDIUM)
4. Code Formatting (MEDIUM)
5. Large File Sizes (LOW-MEDIUM)
6. CI Check Failures
Code Quality IssuesDocumentation✅ Good: Extensive module-level documentation with examples Best Practices✅ Follows CLAUDE.md performance guidelines Performance✅ Arena allocation usage Security
Specific Recommendations
Questions for Author
VerdictNEEDS WORK - Cannot recommend merging in current state due to:
Suggested Path Forward:
The code quality itself appears good, but the lack of tests for 29K lines of new code is a blocker. Per CLAUDE.md: "Write unit tests for your code." |
Binary Sizes
Commit: d4659a1 |
CodSpeed Performance ReportMerging #11292 will not alter performanceComparing Summary
|
8d4fc97 to
30ab33c
Compare
Pull Request Review: Port oxc single-pass transform using DevBirdOverviewThis PR adds 29,331 lines of new code to port the oxc single-pass transformer. This is a massive addition that introduces a new transformer implementation under ✅ Positive Aspects1. Code Documentation
2. Code Organization
3. Safety & Error Handling
|
|
|
PR Review: Port oxc single-pass transform using DevBirdOverviewThis PR adds ~30,000 lines of code porting the oxc transformer to SWC's VisitMutHook architecture. The PR includes two main components:
Critical Issues🔴 1. Architecture Mismatch - Major Design FlawThe PR description states this is "for VisitMutHook-based architecture," but the Evidence from use oxc_allocator::{Allocator, TakeIn, Vec as ArenaVec};
use oxc_ast::{AstBuilder, ast::*};
use oxc_traverse::{Traverse, traverse_mut};This code imports from oxc_ast and oxc_traverse, which are external dependencies NOT listed in the Cargo.toml. The entire
Impact: The 95 files in 🔴 2. Missing DependenciesThe
Running 🟡 3. Test Coverage - InsufficientPer CLAUDE.md rule #5: "Write unit tests for your code" Current test coverage:
Recommendation: Before adding 30K lines of transform logic, prove the foundation works with:
🟡 4. Documentation - IncompleteWhile the foundation code has excellent documentation, key gaps exist: Missing:
Good:
Detailed Code ReviewFoundation Code Quality (src/) ✅The 3 core files are well-written and follow best practices: context.rs - Excellent
transformer.rs - Well-designed
lib.rs - Clear
Potential Bugs & Issues1. Context State Management (context.rs:123-143)The self.in_loop = self
.ancestor_stack
.iter()
.any(|a| matches!(a, AncestorNode::Loop));Performance concern: O(n) scan on every pop. For deeply nested code, this could be inefficient. Consider maintaining a counter instead: loop_depth: usize // increment on push, decrement on pop
in_loop() -> bool { self.loop_depth > 0 }2. UID Generation Not Collision-Safe (context.rs:80-83)pub fn generate_uid(&mut self, prefix: &str) -> String {
self.uid_counter += 1;
format!("_{}_{}", prefix, self.uid_counter)
}If user code already has variables like
3. Memory Leak in identifier_metadata (context.rs:38)The
Performance ConsiderationsPositive:
Concerns:
SecurityNo security concerns identified in the foundation code. The transformer doesn't:
CLAUDE.md Compliance
RecommendationsBefore Merging:
VerdictThe foundation code (src/) is excellent and ready to merge. However, the PR should either: Option A (Recommended): Remove the Option B: Actually port the oxc transforms to use the VisitMutHook architecture before merging (significant additional work). The current state with 95 non-compiling files is not mergeable. The PR description's statement "I made a mistake in the previous attempt" suggests awareness of issues, but they haven't been fully addressed. Summary:
Rating: 3/10 as-is (due to dead code), 8/10 for just the foundation Happy to discuss any of these points! The core design is solid and I'm excited to see this architecture develop. |
3bc5042 to
c0806fd
Compare
Pull Request Review: Port single-pass compiler from oxcOverviewThis is a substantial PR adding ~27,600 lines of new code to port oxc's single-pass transformer architecture to SWC. The implementation includes transformers for ES2015-ES2026 features, TypeScript, JSX, decorators, and various plugins. Code Quality & Best Practices ✅Strengths:
Areas for Improvement:
Potential Bugs & Issues
|
Pull Request Review: Port single-pass compiler from oxcOverviewThis is a substantial PR adding ~27,600 lines of code across 88 files, porting a single-pass transformer/transpiler architecture from the oxc project. The code introduces a new directory structure under Positive ObservationsArchitecture & Design✅ Well-structured module organization - Clear separation by ECMAScript version (es2015-es2026), features (jsx, typescript, decorator), and common utilities Code Quality✅ Comprehensive option handling - Strong Babel compatibility with detailed option structures Areas of Concern1. Test Coverage
|
Description:
I made a mistake in the previous attempt