From 5143bb851ac39e066f16552773a8539db1b41709 Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Thu, 28 Nov 2024 01:20:33 +0000 Subject: [PATCH] fix(traverse)!: remove unsound APIs (#7514) It's essential to `oxc_traverse`'s safety scheme that the user cannot create a `TraverseAncestry`, because they could then substitute it for the one stored in `TraverseCtx`, and cause a buffer underrun when an ancestor gets popped off stack which should never be empty - but it is because user has sneakily swapped it for another one. Not being able to create a `TraverseAncestry` also requires that user cannot obtain an owned `TraverseCtx` either, because you can obtain an owned `TraverseAncestry` from an owned `TraverseCtx`. Therefore, it's unsound for `TraverseCtx::new` to be public. However, it is useful in minifier to be able to re-use the same `TraverseCtx` over and over, which requires having an owned `TraverseCtx`. To support this use case, introduce `ReusableTraverseCtx`. It is an opaque wrapper around `TraverseCtx`, which prevents accessing the `TraverseCtx` inside it. It's safe for user to own a `ReusableTraverseCtx`, because there's nothing they can do with it except for using it to traverse via `traverse_mut_with_ctx`, which ensures the safety invariants are upheld. At some point, we'll hopefully be able to reduce the number of passes in the minifier, and so remove the need for `ReusableTraverseCtx`.But in the meantime, this keeps `Traverse`'s API safe from unsound abuse. Note: Strictly speaking, there is still room to abuse the API and produce UB by initiating a 2nd traversal of a different AST in an `Traverse` visitor, and then `mem::swap` the 2 x `&mut TraverseCtx`s. But this is a completely bizarre thing to do, and would basically require you to write malicious code specifically designed to cause UB, so it's not a real risk in practice. We'd need branded lifetimes to close that hole too. So this PR doesn't 100% ensure safety in a formal sense, but it at least makes it very hard to trigger UB *by accident*, which was the risk before. --- .../collapse_variable_declarations.rs | 6 +- .../src/ast_passes/exploit_assigns.rs | 6 +- crates/oxc_minifier/src/ast_passes/mod.rs | 12 +-- .../src/ast_passes/peephole_fold_constants.rs | 6 +- .../peephole_minimize_conditions.rs | 6 +- .../ast_passes/peephole_remove_dead_code.rs | 6 +- .../peephole_replace_known_methods.rs | 6 +- .../peephole_substitute_alternate_syntax.rs | 6 +- .../src/ast_passes/remove_syntax.rs | 6 +- .../src/ast_passes/statement_fusion.rs | 6 +- crates/oxc_minifier/src/compressor.rs | 6 +- crates/oxc_minifier/src/tester.rs | 4 +- .../src/es2022/class_static_block.rs | 93 ++++++++++--------- crates/oxc_traverse/src/context/ancestry.rs | 3 +- crates/oxc_traverse/src/context/mod.rs | 21 +++-- crates/oxc_traverse/src/context/reusable.rs | 56 +++++++++++ crates/oxc_traverse/src/context/scoping.rs | 9 +- crates/oxc_traverse/src/lib.rs | 29 ++++-- 18 files changed, 181 insertions(+), 106 deletions(-) create mode 100644 crates/oxc_traverse/src/context/reusable.rs diff --git a/crates/oxc_minifier/src/ast_passes/collapse_variable_declarations.rs b/crates/oxc_minifier/src/ast_passes/collapse_variable_declarations.rs index dd074e7a4c5bd..a33dd08bcc246 100644 --- a/crates/oxc_minifier/src/ast_passes/collapse_variable_declarations.rs +++ b/crates/oxc_minifier/src/ast_passes/collapse_variable_declarations.rs @@ -1,6 +1,6 @@ use oxc_allocator::Vec; use oxc_ast::ast::*; -use oxc_traverse::{Traverse, TraverseCtx}; +use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx}; use crate::CompressorPass; @@ -17,9 +17,9 @@ impl<'a> CompressorPass<'a> for CollapseVariableDeclarations { self.changed } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { self.changed = false; - oxc_traverse::walk_program(self, program, ctx); + traverse_mut_with_ctx(self, program, ctx); } } diff --git a/crates/oxc_minifier/src/ast_passes/exploit_assigns.rs b/crates/oxc_minifier/src/ast_passes/exploit_assigns.rs index 50558bd8cc4a8..06d51fff3821f 100644 --- a/crates/oxc_minifier/src/ast_passes/exploit_assigns.rs +++ b/crates/oxc_minifier/src/ast_passes/exploit_assigns.rs @@ -1,5 +1,5 @@ use oxc_ast::ast::*; -use oxc_traverse::{Traverse, TraverseCtx}; +use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse}; use crate::CompressorPass; @@ -15,9 +15,9 @@ impl<'a> CompressorPass<'a> for ExploitAssigns { self.changed } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { self.changed = false; - oxc_traverse::walk_program(self, program, ctx); + traverse_mut_with_ctx(self, program, ctx); } } diff --git a/crates/oxc_minifier/src/ast_passes/mod.rs b/crates/oxc_minifier/src/ast_passes/mod.rs index 8073a0b41d701..7d303aec7974b 100644 --- a/crates/oxc_minifier/src/ast_passes/mod.rs +++ b/crates/oxc_minifier/src/ast_passes/mod.rs @@ -20,12 +20,12 @@ pub use statement_fusion::StatementFusion; use oxc_allocator::Vec; use oxc_ast::ast::*; -use oxc_traverse::{Traverse, TraverseCtx}; +use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx}; pub trait CompressorPass<'a>: Traverse<'a> { fn changed(&self) -> bool; - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>); + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>); } // See `latePeepholeOptimizations` @@ -61,9 +61,9 @@ impl<'a> CompressorPass<'a> for EarlyPass { self.changed } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { self.changed = false; - oxc_traverse::walk_program(self, program, ctx); + traverse_mut_with_ctx(self, program, ctx); self.changed = self.x0_statement_fusion.changed() || self.x1_peephole_remove_dead_code.changed() || self.x2_peephole_minimize_conditions.changed() @@ -167,9 +167,9 @@ impl<'a> CompressorPass<'a> for LatePass { self.changed } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { self.changed = false; - oxc_traverse::walk_program(self, program, ctx); + traverse_mut_with_ctx(self, program, ctx); self.changed = self.x0_exploit_assigns.changed() || self.x0_exploit_assigns.changed() || self.x1_collapse_variable_declarations.changed() diff --git a/crates/oxc_minifier/src/ast_passes/peephole_fold_constants.rs b/crates/oxc_minifier/src/ast_passes/peephole_fold_constants.rs index 2a547f25d4d79..de5114922f958 100644 --- a/crates/oxc_minifier/src/ast_passes/peephole_fold_constants.rs +++ b/crates/oxc_minifier/src/ast_passes/peephole_fold_constants.rs @@ -8,7 +8,7 @@ use oxc_syntax::{ number::{NumberBase, ToJsString}, operator::{BinaryOperator, LogicalOperator}, }; -use oxc_traverse::{Ancestor, Traverse, TraverseCtx}; +use oxc_traverse::{traverse_mut_with_ctx, Ancestor, ReusableTraverseCtx, Traverse, TraverseCtx}; use crate::{node_util::Ctx, CompressorPass}; @@ -24,9 +24,9 @@ impl<'a> CompressorPass<'a> for PeepholeFoldConstants { self.changed } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { self.changed = false; - oxc_traverse::walk_program(self, program, ctx); + traverse_mut_with_ctx(self, program, ctx); } } diff --git a/crates/oxc_minifier/src/ast_passes/peephole_minimize_conditions.rs b/crates/oxc_minifier/src/ast_passes/peephole_minimize_conditions.rs index 1892f523042a3..61028193e2f25 100644 --- a/crates/oxc_minifier/src/ast_passes/peephole_minimize_conditions.rs +++ b/crates/oxc_minifier/src/ast_passes/peephole_minimize_conditions.rs @@ -1,7 +1,7 @@ use oxc_ast::ast::*; use oxc_ecmascript::ToBoolean; use oxc_span::SPAN; -use oxc_traverse::{Traverse, TraverseCtx}; +use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx}; use crate::CompressorPass; @@ -21,9 +21,9 @@ impl<'a> CompressorPass<'a> for PeepholeMinimizeConditions { self.changed } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { self.changed = false; - oxc_traverse::walk_program(self, program, ctx); + traverse_mut_with_ctx(self, program, ctx); } } diff --git a/crates/oxc_minifier/src/ast_passes/peephole_remove_dead_code.rs b/crates/oxc_minifier/src/ast_passes/peephole_remove_dead_code.rs index 822e45fbec6f8..90e039e154382 100644 --- a/crates/oxc_minifier/src/ast_passes/peephole_remove_dead_code.rs +++ b/crates/oxc_minifier/src/ast_passes/peephole_remove_dead_code.rs @@ -2,7 +2,7 @@ use oxc_allocator::Vec; use oxc_ast::{ast::*, Visit}; use oxc_ecmascript::constant_evaluation::{ConstantEvaluation, IsLiteralValue}; use oxc_span::SPAN; -use oxc_traverse::{Ancestor, Traverse, TraverseCtx}; +use oxc_traverse::{traverse_mut_with_ctx, Ancestor, ReusableTraverseCtx, Traverse, TraverseCtx}; use crate::node_util::Ctx; use crate::{keep_var::KeepVar, CompressorPass}; @@ -22,9 +22,9 @@ impl<'a> CompressorPass<'a> for PeepholeRemoveDeadCode { self.changed } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { self.changed = false; - oxc_traverse::walk_program(self, program, ctx); + traverse_mut_with_ctx(self, program, ctx); } } diff --git a/crates/oxc_minifier/src/ast_passes/peephole_replace_known_methods.rs b/crates/oxc_minifier/src/ast_passes/peephole_replace_known_methods.rs index 9a433e29556a9..66a1595ca8da5 100644 --- a/crates/oxc_minifier/src/ast_passes/peephole_replace_known_methods.rs +++ b/crates/oxc_minifier/src/ast_passes/peephole_replace_known_methods.rs @@ -4,7 +4,7 @@ use oxc_ecmascript::{ constant_evaluation::ConstantEvaluation, StringCharAt, StringCharCodeAt, StringIndexOf, StringLastIndexOf, StringSubstring, }; -use oxc_traverse::{Traverse, TraverseCtx}; +use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx}; use crate::{node_util::Ctx, CompressorPass}; @@ -19,9 +19,9 @@ impl<'a> CompressorPass<'a> for PeepholeReplaceKnownMethods { self.changed } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { self.changed = false; - oxc_traverse::walk_program(self, program, ctx); + traverse_mut_with_ctx(self, program, ctx); } } diff --git a/crates/oxc_minifier/src/ast_passes/peephole_substitute_alternate_syntax.rs b/crates/oxc_minifier/src/ast_passes/peephole_substitute_alternate_syntax.rs index a774af3c3d96e..91ada095c7dc5 100644 --- a/crates/oxc_minifier/src/ast_passes/peephole_substitute_alternate_syntax.rs +++ b/crates/oxc_minifier/src/ast_passes/peephole_substitute_alternate_syntax.rs @@ -7,7 +7,7 @@ use oxc_syntax::{ number::NumberBase, operator::{BinaryOperator, UnaryOperator}, }; -use oxc_traverse::{Ancestor, Traverse, TraverseCtx}; +use oxc_traverse::{traverse_mut_with_ctx, Ancestor, ReusableTraverseCtx, Traverse, TraverseCtx}; use crate::{node_util::Ctx, CompressorPass}; @@ -32,9 +32,9 @@ impl<'a> CompressorPass<'a> for PeepholeSubstituteAlternateSyntax { self.changed } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { self.changed = false; - oxc_traverse::walk_program(self, program, ctx); + traverse_mut_with_ctx(self, program, ctx); } } diff --git a/crates/oxc_minifier/src/ast_passes/remove_syntax.rs b/crates/oxc_minifier/src/ast_passes/remove_syntax.rs index 49ca3a74523c8..9de2b974bb685 100644 --- a/crates/oxc_minifier/src/ast_passes/remove_syntax.rs +++ b/crates/oxc_minifier/src/ast_passes/remove_syntax.rs @@ -1,7 +1,7 @@ use oxc_allocator::Vec; use oxc_ast::ast::*; use oxc_span::GetSpan; -use oxc_traverse::{Traverse, TraverseCtx}; +use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx}; use crate::{CompressOptions, CompressorPass}; @@ -19,8 +19,8 @@ impl<'a> CompressorPass<'a> for RemoveSyntax { false } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { - oxc_traverse::walk_program(self, program, ctx); + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { + traverse_mut_with_ctx(self, program, ctx); } } diff --git a/crates/oxc_minifier/src/ast_passes/statement_fusion.rs b/crates/oxc_minifier/src/ast_passes/statement_fusion.rs index 3b3e497639838..95ddc736ebea8 100644 --- a/crates/oxc_minifier/src/ast_passes/statement_fusion.rs +++ b/crates/oxc_minifier/src/ast_passes/statement_fusion.rs @@ -2,7 +2,7 @@ use oxc_allocator::Vec; use oxc_ast::ast::*; use oxc_ecmascript::side_effects::MayHaveSideEffects; use oxc_span::SPAN; -use oxc_traverse::{Traverse, TraverseCtx}; +use oxc_traverse::{traverse_mut_with_ctx, ReusableTraverseCtx, Traverse, TraverseCtx}; use crate::CompressorPass; @@ -20,9 +20,9 @@ impl<'a> CompressorPass<'a> for StatementFusion { self.changed } - fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn build(&mut self, program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { self.changed = false; - oxc_traverse::walk_program(self, program, ctx); + traverse_mut_with_ctx(self, program, ctx); } } diff --git a/crates/oxc_minifier/src/compressor.rs b/crates/oxc_minifier/src/compressor.rs index 8270821ce5059..b465b48fbce2c 100644 --- a/crates/oxc_minifier/src/compressor.rs +++ b/crates/oxc_minifier/src/compressor.rs @@ -1,7 +1,7 @@ use oxc_allocator::Allocator; use oxc_ast::ast::*; use oxc_semantic::{ScopeTree, SemanticBuilder, SymbolTable}; -use oxc_traverse::TraverseCtx; +use oxc_traverse::ReusableTraverseCtx; use crate::{ ast_passes::{ @@ -33,7 +33,7 @@ impl<'a> Compressor<'a> { scopes: ScopeTree, program: &mut Program<'a>, ) { - let mut ctx = TraverseCtx::new(scopes, symbols, self.allocator); + let mut ctx = ReusableTraverseCtx::new(scopes, symbols, self.allocator); RemoveSyntax::new(self.options).build(program, &mut ctx); if self.options.dead_code_elimination { @@ -62,7 +62,7 @@ impl<'a> Compressor<'a> { LatePass::new().build(program, &mut ctx); } - fn dead_code_elimination(program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>) { + fn dead_code_elimination(program: &mut Program<'a>, ctx: &mut ReusableTraverseCtx<'a>) { PeepholeFoldConstants::new().build(program, ctx); PeepholeMinimizeConditions::new().build(program, ctx); PeepholeRemoveDeadCode::new().build(program, ctx); diff --git a/crates/oxc_minifier/src/tester.rs b/crates/oxc_minifier/src/tester.rs index 393f199fa7854..e0bcd85d5d800 100644 --- a/crates/oxc_minifier/src/tester.rs +++ b/crates/oxc_minifier/src/tester.rs @@ -3,7 +3,7 @@ use oxc_codegen::{CodeGenerator, CodegenOptions}; use oxc_parser::Parser; use oxc_semantic::SemanticBuilder; use oxc_span::SourceType; -use oxc_traverse::TraverseCtx; +use oxc_traverse::ReusableTraverseCtx; use crate::{ast_passes::CompressorPass, ast_passes::RemoveSyntax, CompressOptions}; @@ -40,7 +40,7 @@ fn run<'a, P: CompressorPass<'a>>( if let Some(pass) = pass { let (symbols, scopes) = SemanticBuilder::new().build(&program).semantic.into_symbol_table_and_scope_tree(); - let mut ctx = TraverseCtx::new(scopes, symbols, allocator); + let mut ctx = ReusableTraverseCtx::new(scopes, symbols, allocator); RemoveSyntax::new(CompressOptions::all_false()).build(&mut program, &mut ctx); pass.build(&mut program, &mut ctx); } diff --git a/crates/oxc_transformer/src/es2022/class_static_block.rs b/crates/oxc_transformer/src/es2022/class_static_block.rs index a29a5a8dc1f54..e6879ee41814f 100644 --- a/crates/oxc_transformer/src/es2022/class_static_block.rs +++ b/crates/oxc_transformer/src/es2022/class_static_block.rs @@ -281,7 +281,7 @@ impl<'a> Keys<'a> { mod test { use oxc_allocator::Allocator; use oxc_semantic::{ScopeTree, SymbolTable}; - use oxc_traverse::TraverseCtx; + use oxc_traverse::ReusableTraverseCtx; use super::Keys; @@ -290,7 +290,10 @@ mod test { let allocator = Allocator::default(); let scopes = ScopeTree::default(); let symbols = SymbolTable::default(); - let mut $ctx = TraverseCtx::new(scopes, symbols, &allocator); + let ctx = ReusableTraverseCtx::new(scopes, symbols, &allocator); + // SAFETY: Macro user only gets a `&mut TraverseCtx`, which cannot be abused + let mut ctx = unsafe { ctx.unwrap() }; + let $ctx = &mut ctx; }; } @@ -300,18 +303,18 @@ mod test { let mut keys = Keys::default(); - assert_eq!(keys.get_unique(&mut ctx), "_"); - assert_eq!(keys.get_unique(&mut ctx), "_2"); - assert_eq!(keys.get_unique(&mut ctx), "_3"); - assert_eq!(keys.get_unique(&mut ctx), "_4"); - assert_eq!(keys.get_unique(&mut ctx), "_5"); - assert_eq!(keys.get_unique(&mut ctx), "_6"); - assert_eq!(keys.get_unique(&mut ctx), "_7"); - assert_eq!(keys.get_unique(&mut ctx), "_8"); - assert_eq!(keys.get_unique(&mut ctx), "_9"); - assert_eq!(keys.get_unique(&mut ctx), "_10"); - assert_eq!(keys.get_unique(&mut ctx), "_11"); - assert_eq!(keys.get_unique(&mut ctx), "_12"); + assert_eq!(keys.get_unique(ctx), "_"); + assert_eq!(keys.get_unique(ctx), "_2"); + assert_eq!(keys.get_unique(ctx), "_3"); + assert_eq!(keys.get_unique(ctx), "_4"); + assert_eq!(keys.get_unique(ctx), "_5"); + assert_eq!(keys.get_unique(ctx), "_6"); + assert_eq!(keys.get_unique(ctx), "_7"); + assert_eq!(keys.get_unique(ctx), "_8"); + assert_eq!(keys.get_unique(ctx), "_9"); + assert_eq!(keys.get_unique(ctx), "_10"); + assert_eq!(keys.get_unique(ctx), "_11"); + assert_eq!(keys.get_unique(ctx), "_12"); } #[test] @@ -328,9 +331,9 @@ mod test { keys.reserve("_foo"); keys.reserve("_2foo"); - assert_eq!(keys.get_unique(&mut ctx), "_"); - assert_eq!(keys.get_unique(&mut ctx), "_2"); - assert_eq!(keys.get_unique(&mut ctx), "_3"); + assert_eq!(keys.get_unique(ctx), "_"); + assert_eq!(keys.get_unique(ctx), "_2"); + assert_eq!(keys.get_unique(ctx), "_3"); } #[test] @@ -340,9 +343,9 @@ mod test { let mut keys = Keys::default(); keys.reserve("_"); - assert_eq!(keys.get_unique(&mut ctx), "_2"); - assert_eq!(keys.get_unique(&mut ctx), "_3"); - assert_eq!(keys.get_unique(&mut ctx), "_4"); + assert_eq!(keys.get_unique(ctx), "_2"); + assert_eq!(keys.get_unique(ctx), "_3"); + assert_eq!(keys.get_unique(ctx), "_4"); } #[test] @@ -354,15 +357,15 @@ mod test { keys.reserve("_4"); keys.reserve("_11"); - assert_eq!(keys.get_unique(&mut ctx), "_"); - assert_eq!(keys.get_unique(&mut ctx), "_3"); - assert_eq!(keys.get_unique(&mut ctx), "_5"); - assert_eq!(keys.get_unique(&mut ctx), "_6"); - assert_eq!(keys.get_unique(&mut ctx), "_7"); - assert_eq!(keys.get_unique(&mut ctx), "_8"); - assert_eq!(keys.get_unique(&mut ctx), "_9"); - assert_eq!(keys.get_unique(&mut ctx), "_10"); - assert_eq!(keys.get_unique(&mut ctx), "_12"); + assert_eq!(keys.get_unique(ctx), "_"); + assert_eq!(keys.get_unique(ctx), "_3"); + assert_eq!(keys.get_unique(ctx), "_5"); + assert_eq!(keys.get_unique(ctx), "_6"); + assert_eq!(keys.get_unique(ctx), "_7"); + assert_eq!(keys.get_unique(ctx), "_8"); + assert_eq!(keys.get_unique(ctx), "_9"); + assert_eq!(keys.get_unique(ctx), "_10"); + assert_eq!(keys.get_unique(ctx), "_12"); } #[test] @@ -375,16 +378,16 @@ mod test { keys.reserve("_12"); keys.reserve("_13"); - assert_eq!(keys.get_unique(&mut ctx), "_"); - assert_eq!(keys.get_unique(&mut ctx), "_2"); - assert_eq!(keys.get_unique(&mut ctx), "_3"); - assert_eq!(keys.get_unique(&mut ctx), "_6"); - assert_eq!(keys.get_unique(&mut ctx), "_7"); - assert_eq!(keys.get_unique(&mut ctx), "_8"); - assert_eq!(keys.get_unique(&mut ctx), "_9"); - assert_eq!(keys.get_unique(&mut ctx), "_10"); - assert_eq!(keys.get_unique(&mut ctx), "_11"); - assert_eq!(keys.get_unique(&mut ctx), "_14"); + assert_eq!(keys.get_unique(ctx), "_"); + assert_eq!(keys.get_unique(ctx), "_2"); + assert_eq!(keys.get_unique(ctx), "_3"); + assert_eq!(keys.get_unique(ctx), "_6"); + assert_eq!(keys.get_unique(ctx), "_7"); + assert_eq!(keys.get_unique(ctx), "_8"); + assert_eq!(keys.get_unique(ctx), "_9"); + assert_eq!(keys.get_unique(ctx), "_10"); + assert_eq!(keys.get_unique(ctx), "_11"); + assert_eq!(keys.get_unique(ctx), "_14"); } #[test] @@ -396,9 +399,9 @@ mod test { keys.reserve("_4"); keys.reserve("_"); - assert_eq!(keys.get_unique(&mut ctx), "_3"); - assert_eq!(keys.get_unique(&mut ctx), "_5"); - assert_eq!(keys.get_unique(&mut ctx), "_6"); + assert_eq!(keys.get_unique(ctx), "_3"); + assert_eq!(keys.get_unique(ctx), "_5"); + assert_eq!(keys.get_unique(ctx), "_6"); } #[test] @@ -410,8 +413,8 @@ mod test { keys.reserve("_4"); keys.reserve("_"); - assert_eq!(keys.get_unique(&mut ctx), "_2"); - assert_eq!(keys.get_unique(&mut ctx), "_3"); - assert_eq!(keys.get_unique(&mut ctx), "_6"); + assert_eq!(keys.get_unique(ctx), "_2"); + assert_eq!(keys.get_unique(ctx), "_3"); + assert_eq!(keys.get_unique(ctx), "_6"); } } diff --git a/crates/oxc_traverse/src/context/ancestry.rs b/crates/oxc_traverse/src/context/ancestry.rs index d3523beaac728..45866540fe675 100644 --- a/crates/oxc_traverse/src/context/ancestry.rs +++ b/crates/oxc_traverse/src/context/ancestry.rs @@ -35,7 +35,8 @@ const INITIAL_STACK_CAPACITY: usize = 64; // 64 entries = 1 KiB /// 1. `TraverseAncestry`'s `stack` field is private. /// 2. Public methods of `TraverseAncestry` provide no means for mutating `stack`. /// 3. Visitors receive a `&mut TraverseCtx`, but cannot overwrite its `ancestry` field because they: -/// a. cannot create a new `TraverseAncestry` - `TraverseAncestry::new` is private. +/// a. cannot create a new `TraverseAncestry` +/// - `TraverseAncestry::new` and `TraverseCtx::new` are private. /// b. cannot obtain an owned `TraverseAncestry` from a `&TraverseAncestry` /// - `TraverseAncestry` is not `Clone`. pub struct TraverseAncestry<'a> { diff --git a/crates/oxc_traverse/src/context/mod.rs b/crates/oxc_traverse/src/context/mod.rs index fb9b22fdbb87c..18b5ed65a3d10 100644 --- a/crates/oxc_traverse/src/context/mod.rs +++ b/crates/oxc_traverse/src/context/mod.rs @@ -19,11 +19,13 @@ use crate::{ mod ancestry; mod bound_identifier; mod maybe_bound_identifier; +mod reusable; mod scoping; use ancestry::PopToken; pub use ancestry::TraverseAncestry; pub use bound_identifier::BoundIdentifier; pub use maybe_bound_identifier::MaybeBoundIdentifier; +pub use reusable::ReusableTraverseCtx; pub use scoping::TraverseScoping; /// Traverse context. @@ -119,14 +121,6 @@ pub struct TraverseCtx<'a> { // Public methods impl<'a> TraverseCtx<'a> { - /// Create new traversal context. - pub fn new(scopes: ScopeTree, symbols: SymbolTable, allocator: &'a Allocator) -> Self { - let ancestry = TraverseAncestry::new(); - let scoping = TraverseScoping::new(scopes, symbols); - let ast = AstBuilder::new(allocator); - Self { ancestry, scoping, ast } - } - /// Allocate a node in the arena. /// /// Returns a [`Box`]. @@ -601,6 +595,17 @@ impl<'a> TraverseCtx<'a> { // Methods used internally within crate impl<'a> TraverseCtx<'a> { + /// Create new traversal context. + /// + /// # SAFETY + /// This function must not be public to maintain soundness of [`TraverseAncestry`]. + pub(crate) fn new(scopes: ScopeTree, symbols: SymbolTable, allocator: &'a Allocator) -> Self { + let ancestry = TraverseAncestry::new(); + let scoping = TraverseScoping::new(scopes, symbols); + let ast = AstBuilder::new(allocator); + Self { ancestry, scoping, ast } + } + /// Shortcut for `self.ancestry.push_stack`, to make `walk_*` methods less verbose. /// /// # SAFETY diff --git a/crates/oxc_traverse/src/context/reusable.rs b/crates/oxc_traverse/src/context/reusable.rs new file mode 100644 index 0000000000000..180321d3a7379 --- /dev/null +++ b/crates/oxc_traverse/src/context/reusable.rs @@ -0,0 +1,56 @@ +use oxc_allocator::Allocator; +use oxc_semantic::{ScopeTree, SymbolTable}; + +use super::TraverseCtx; + +/// Wrapper around [`TraverseCtx`], allowing its reuse. +/// +/// We cannot expose ability to obtain an owned [`TraverseCtx`], as it's then possible to circumvent +/// the safety invariants of [`TraverseAncestry`]. +/// +/// This wrapper type can safely be passed to user code as only ways it can be used are to: +/// +/// * Call `traverse_mut_with_ctx`, which maintains safety invariants. +/// * Unwrap it to [`SymbolTable`] and [`ScopeTree`], which discards the sensitive [`TraverseAncestry`] +/// in the process. +/// +/// [`TraverseAncestry`]: super::TraverseAncestry +#[repr(transparent)] +pub struct ReusableTraverseCtx<'a>(TraverseCtx<'a>); + +// Public methods +impl<'a> ReusableTraverseCtx<'a> { + /// Create new [`ReusableTraverseCtx`]. + pub fn new(scopes: ScopeTree, symbols: SymbolTable, allocator: &'a Allocator) -> Self { + Self(TraverseCtx::new(scopes, symbols, allocator)) + } + + /// Consume [`ReusableTraverseCtx`] and return [`SymbolTable`] and [`ScopeTree`]. + pub fn into_symbol_table_and_scope_tree(self) -> (SymbolTable, ScopeTree) { + self.0.scoping.into_symbol_table_and_scope_tree() + } + + /// Unwrap [`TraverseCtx`] in a [`ReusableTraverseCtx`]. + /// + /// Only for use in tests. Allows circumventing the safety invariants of [`TraverseAncestry`]. + /// + /// # SAFETY + /// Caller must ensure [`TraverseCtx`] returned by this method is not used unsoundly. + /// See [`TraverseAncestry`] for details of the invariants. + /// + /// [`TraverseAncestry`]: super::TraverseAncestry + #[inline] + #[expect(clippy::missing_safety_doc)] + pub unsafe fn unwrap(self) -> TraverseCtx<'a> { + self.0 + } +} + +// Internal methods +impl<'a> ReusableTraverseCtx<'a> { + /// Mutably borrow [`TraverseCtx`] from a [`ReusableTraverseCtx`]. + #[inline] // because this function is a no-op at run time + pub(crate) fn get_mut(&mut self) -> &mut TraverseCtx<'a> { + &mut self.0 + } +} diff --git a/crates/oxc_traverse/src/context/scoping.rs b/crates/oxc_traverse/src/context/scoping.rs index d391acc3d8fce..61bc4c00b98c6 100644 --- a/crates/oxc_traverse/src/context/scoping.rs +++ b/crates/oxc_traverse/src/context/scoping.rs @@ -31,10 +31,6 @@ pub struct TraverseScoping { // Public methods impl TraverseScoping { - pub fn into_symbol_table_and_scope_tree(self) -> (SymbolTable, ScopeTree) { - (self.symbols, self.scopes) - } - /// Get current scope ID #[inline] pub fn current_scope_id(&self) -> ScopeId { @@ -408,6 +404,11 @@ impl TraverseScoping { } } + /// Consume [`TraverseScoping`] and return [`SymbolTable`] and [`ScopeTree`]. + pub(super) fn into_symbol_table_and_scope_tree(self) -> (SymbolTable, ScopeTree) { + (self.symbols, self.scopes) + } + /// Set current scope ID #[inline] pub(crate) fn set_current_scope_id(&mut self, scope_id: ScopeId) { diff --git a/crates/oxc_traverse/src/lib.rs b/crates/oxc_traverse/src/lib.rs index b5fc9d4ce9650..3f2cd7714a0de 100644 --- a/crates/oxc_traverse/src/lib.rs +++ b/crates/oxc_traverse/src/lib.rs @@ -60,6 +60,8 @@ //! scheme could very easily be derailed entirely by a single mistake, so in my opinion, it's unwise //! to edit by hand. +use std::ptr; + use oxc_allocator::Allocator; use oxc_ast::ast::Program; use oxc_semantic::{ScopeTree, SymbolTable}; @@ -67,7 +69,8 @@ use oxc_semantic::{ScopeTree, SymbolTable}; pub mod ast_operations; mod context; pub use context::{ - BoundIdentifier, MaybeBoundIdentifier, TraverseAncestry, TraverseCtx, TraverseScoping, + BoundIdentifier, MaybeBoundIdentifier, ReusableTraverseCtx, TraverseAncestry, TraverseCtx, + TraverseScoping, }; mod generated { @@ -77,7 +80,7 @@ mod generated { pub(super) mod walk; } pub use generated::{ancestor, ancestor::Ancestor, traverse::Traverse}; -use generated::{scopes_collector, walk}; +use generated::{scopes_collector, walk::walk_program}; mod compile_fail_tests; @@ -144,7 +147,6 @@ mod compile_fail_tests; /// } /// } /// ``` - pub fn traverse_mut<'a, Tr: Traverse<'a>>( traverser: &mut Tr, allocator: &'a Allocator, @@ -152,17 +154,24 @@ pub fn traverse_mut<'a, Tr: Traverse<'a>>( symbols: SymbolTable, scopes: ScopeTree, ) -> (SymbolTable, ScopeTree) { - let mut ctx = TraverseCtx::new(scopes, symbols, allocator); - walk_program(traverser, program, &mut ctx); - debug_assert!(ctx.ancestors_depth() == 1); - ctx.scoping.into_symbol_table_and_scope_tree() + let mut ctx = ReusableTraverseCtx::new(scopes, symbols, allocator); + traverse_mut_with_ctx(traverser, program, &mut ctx); + ctx.into_symbol_table_and_scope_tree() } -pub fn walk_program<'a, Tr: Traverse<'a>>( +/// Traverse AST with a [`Traverse`] impl, reusing an existing [`ReusableTraverseCtx`]. +/// +/// [`ReusableTraverseCtx`] is specific to a single AST. It will likely cause malfunction if +/// `traverse_mut_with_ctx` is called with a [`Program`] and [`ReusableTraverseCtx`] which do not match. +/// +/// See [`traverse_mut`] for more details of how traversal works. +pub fn traverse_mut_with_ctx<'a, Tr: Traverse<'a>>( traverser: &mut Tr, program: &mut Program<'a>, - ctx: &mut TraverseCtx<'a>, + ctx: &mut ReusableTraverseCtx<'a>, ) { + let ctx = ctx.get_mut(); // SAFETY: Walk functions are constructed to avoid unsoundness - unsafe { walk::walk_program(traverser, std::ptr::from_mut(program), ctx) }; + unsafe { walk_program(traverser, ptr::from_mut(program), ctx) }; + debug_assert!(ctx.ancestors_depth() == 1); }