refactor(minifier): make Ctx take &mut TraverseCtx

[Boshen]

No description provided.

[Boshen]

• feat(minifier)!: add Treeshake options #11786
• refactor(minifier): make Ctx take &mut TraverseCtx #11771 👈 (View in Graphite)
• feat(traverse)!: introduce TraverseCtx<'a, State> #11770
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

[codspeed-hq]

CodSpeed Instrumentation Performance Report

Merging #11771 will degrade performances by 13.01%

_{Comparing 06-17-refactor_minifier_make_ctx_take_mut_traversectx_ (d5a8f18) with 06-17-feat_traverse_introduce_traversectx_a_state_ (8ef1be2)}

Summary

❌ 1 regressions
✅ 37 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	mangler[cal.com.tsx]	3.1 ms	3.5 ms	-13.01%

[overlookmotel]

Ctx is a wrapper around &mut TraverseCtx, so it's unfortunate to have to pass around &Ctx and &mut Ctx - these are double-references & &mut TraverseCtx / &mut &mut TraverseCtx.

A couple of possible alternatives:

Remove Ctx

Would it be viable to remove the Ctx wrapper and implement Ctx's methods as an extension trait instead? Then you pass around &mut TraverseCtx<MinifierState>, instead of passing Ctx / &Ctx / &mut Ctx.

We could implement IsGlobalReference on TraverseCtx in oxc_traverse crate.

impl<'_, State> IsGlobalReference for TraverseCtx<State> {
    fn is_global_reference(&self, ident: &IdentifierReference<'_>) -> Option<bool> {
        Some(ident.is_global_reference(self.scoping()))
    }
}

MayHaveSideEffectsContext and ConstantEvaluationCtx are more tricky, but I think could make it work one of 2 ways:

1. In minifier

struct MinifierEvalContext<'a, 's> {
    scoping: &'s Scoping,
    ast: AstBuilder<'a>,
}

impl IsGlobalReference for MinifierEvalContext<'_, '_> {
    fn is_global_reference(&self, ident: &IdentifierReference<'_>) -> Option<bool> {
        Some(ident.is_global_reference(self.scoping))
    }
}

impl MayHaveSideEffectsContext for MinifierEvalContext<'_> {
    fn respect_annotations(&self) -> bool {
        true
    }
    // ... etc ...
}

impl<'a> ConstantEvaluationCtx<'a> for MinifierEvalContext<'a, '_> {
    fn ast(&self) -> AstBuilder<'a> {
        self.ast
    }
}

impl<'a> ExtensionTrait for TraverseCtx<'a, MinifierState> {
    fn eval_ctx(&self) -> MinifierEvalContext<'a, '_> {
        MinifierEvalContext { scoping: self.scoping(), ast: self.ast() }
    }
}

Then when you need to pass an &impl MayHaveSideEffectsContext or &impl ConstantEvaluationCtx to a method, you pass it &ctx.eval_ctx().

2. In oxc_traverse

// oxc_traverse
pub trait StateEvalContext {
    fn respect_annotations(&self) -> bool;
    // ... etc ...
}

impl<'a, State> MayHaveSideEffectsContext for TraverseCtx<'a, State>
    where State: StateEvalContext
{
    fn respect_annotations(&self) -> bool {
        self.state.respect_annotations()
    }
    // ... etc ...
}

impl<'a, State> ConstantEvaluationCtx<'a> for TraverseCtx<'a, State>
    where State: StateEvalContext
{
    fn ast(&self) -> AstBuilder<'a> {
        self.ast
    }
    // ... etc ...
}

// oxc_minifier
impl StateEvalContext for MinifierState {
    fn respect_annotations(&self) -> bool {
        true
    }
    // ... etc ...
}

Now TraverseCtx<MinifierState> implements MayHaveSideEffectsContext and ConstantEvaluationCtx. Ctx wrapper is no longer necessary.

Personally I think this is best solution.

borrow_mut method

Alternatively, could add a borrow_mut method to Ctx which doesn't consume the Ctx and generates a new one:

impl<'a, 'b> Ctx<'a, 'b> {
    #[inline(always)] // Because it's a no-op at runtime
    pub fn borrow_mut<'borrow>(&'borrow mut self) -> Ctx<'a, 'borrow> {
        Ctx(self.0)
    }
}

That'd solve the double-reference problem. Ctx stays, and all Ctx's methods take self, same as now. Unfortunately, it'd introduce the need for .borrow_mut() calls in a lot of places.

[graphite-app]

Merge activity

• Jun 18, 6:41 AM UTC: Boshen added this pull request to the Graphite merge queue.
• Jun 18, 6:49 AM UTC: Merged by the Graphite merge queue.