oxc-project / backlog

backlog for collborators only
1 stars 0 forks source link

Support reading `Ancestor` siblings in a `Vec` in `Traverse` visitors #138

Open overlookmotel opened 3 months ago

overlookmotel commented 3 months ago

Broken out from oxc-project/backlog#128:

We've hit a case where the following APIs in Traverse visitors would be useful:

Minifier (#4725) needs to check when visiting a BooleanExpression whether it is true in:

Object.defineProperty(exports, 'Foo', {
  enumerable: true,
  get: function() { return Foo_1.Foo; }
});

Currently this is implemented by checking when entering a CallExpression whether it matches this pattern and setting a flag if so. It would be preferable to do this check by reading up AST in enter_boolean_literal instead.

fn enter_expression(&mut self, expr: &mut Expression<'a>, ctx: &mut TraverseCtx<'a>) {
    self.compress_boolean(expr, ctx);
}

fn compress_boolean(&mut self, expr: &mut Expression<'a>, ctx: &mut TraverseCtx<'a>) -> bool {
    let Expression::BooleanLiteral(lit) = expr else { return false; };
    if Self::bool_is_in_define_export(lit, ctx) { return false; }

    let num = self.ast.expression_numeric_literal(
        SPAN,
        if lit.value { 0.0 } else { 1.0 },
        if lit.value { "0" } else { "1" },
        NumberBase::Decimal,
    );
    *expr = self.ast.expression_unary(SPAN, UnaryOperator::LogicalNot, num);
    true
}

/// Check if `BooleanLiteral` is `true` in:
/// ```js
/// Object.defineProperty(exports, 'Foo', {
///   enumerable: true,
///   get: function() { return Foo_1.Foo; }
/// });
/// ```
fn bool_is_in_define_export(lit: &BooleanLiteral, ctx: &mut TraverseCtx<'a>) -> bool {
    // We are only interested in `true`
    if !lit.value { return false; }
    // We are only interested in this construct at top level
    if ctx.ancestors_depth() != 5 { return false; } // TODO: Is 5 correct?

    // Check `true` is in an object property `enumerable: true`
    let Ancestor::ObjectPropertyValue(prop) = ctx.parent() else { return false; };
    let PropertyKey::StaticIdentifier(key) = prop.key() else { return false; };
    if key.name != "enumerable" { return false; }

    // Check that object is param of a call expression.
    // Skip over checking parent's parent - it must be an `ObjectExpression`.
    let object_parent = ctx.ancestor(3).unwrap();
    let Ancestor::CallExpressionArguments(call_expr) = object_parent else { return false; };

    // We want to check that:
    // 1. First arg is `exports`.
    // 2. Object is 3rd arg.
    // But we can't because `Ancestor` does not give us access to `arguments`.
    // TODO(@overlookmotel): Make this possible.

    // Check callee is `Object.defineProperty`.
    // Use tighter check than `call_expr.callee.is_specific_member_access("Object", "defineProperty")`
    // because we're looking for `Object.defineProperty` specifically, not e.g. `Object['defineProperty']`.
    if let Expression::StaticMemberExpression(callee) = call_expr.callee() {
        if let Expression::Identifier(id) = &callee.object {
            if id.name == "Object" && callee.property.name == "defineProperty" {
                return true;
            }
        }
    }
    false
}

This isn't currently possible as cannot access call_expr.arguments. It would be safe to access other elements in arguments except for the current one, but this is not supported currently by Traverse's API.

Boshen commented 3 months ago

It would be preferable to do this check by reading up AST in enter_boolean_literal instead.

This sounds like a huge performance regression. It needs to check for every bool ...

overlookmotel commented 3 months ago

I'm not sure which is worse - checking every bool, or checking every function call! Either way, it's going to be cheap in majority of cases because of the ctx.ancestors_depth() check - that's a very quick check, so it'll bail out early in most cases without doing any work.