Skip to content
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

must visit decorators before enter_node and enter_scope if have decorators field #4142

Closed
Dunqing opened this issue Jul 9, 2024 · 4 comments · Fixed by #4143 or #4147
Closed

must visit decorators before enter_node and enter_scope if have decorators field #4142

Dunqing opened this issue Jul 9, 2024 · 4 comments · Fixed by #4143 or #4147
Assignees
Labels
A-ast Area - AST C-bug Category - Bug

Comments

@Dunqing
Copy link
Member

Dunqing commented Jul 9, 2024

Please see:

fn visit_class(&mut self, class: &Class<'a>) {
// Class level decorators are transpiled as functions outside of the class taking the class
// itself as argument. They should be visited before class is entered. E.g., they inherit
// strict mode from the enclosing scope rather than from class.
for decorator in &class.decorators {

@Dunqing Dunqing added C-bug Category - Bug A-ast Area - AST labels Jul 9, 2024
@Dunqing
Copy link
Member Author

Dunqing commented Jul 9, 2024

@rzvxa cc

@rzvxa rzvxa self-assigned this Jul 9, 2024
@rzvxa
Copy link
Contributor

rzvxa commented Jul 9, 2024

We already have the #[scope(enter_before)] mechanism used on Class If you look at the source of truth you'd see it is defined like this:

/// Class Definitions
#[visited_node]
#[scope(flags(ScopeFlags::StrictMode))]
#[derive(Debug)]
#[cfg_attr(feature = "serialize", derive(Serialize, Tsify))]
#[cfg_attr(feature = "serialize", serde(rename_all = "camelCase"))]
pub struct Class<'a> {
    pub r#type: ClassType,
    #[cfg_attr(feature = "serialize", serde(flatten))]
    pub span: Span,
    pub decorators: Vec<'a, Decorator<'a>>,
    #[scope(enter_before)]
    pub id: Option<BindingIdentifier<'a>>,
    // ...
}

Which generates this:

    pub fn walk_class<'a, V: Visit<'a>>(visitor: &mut V, it: &Class<'a>) {
        let kind = AstKind::Class(visitor.alloc(it));
        visitor.enter_node(kind);
        visitor.visit_decorators(&it.decorators);
        visitor.enter_scope(ScopeFlags::StrictMode);
        if let Some(id) = &it.id {
            visitor.visit_binding_identifier(id);
        }
        // ...
        visitor.leave_node(kind);
        visitor.leave_scope();
    }

I'll add the support for a similar mechanism to mark the enter_node.

@overlookmotel
Copy link
Contributor

I understand why enter_scope should be after visiting decorators, but why also enter_node?

Boshen pushed a commit that referenced this issue Jul 10, 2024
Won't fix #4142

It is similar to #3994 but for those types that weren't relying on this order. It seems to be the right order.
technically speaking it is a breaking change but I know as a fact that it won't have a big difference on our downstream. If you want it to be chronically correct feel free to merge as a breaking change.
@Dunqing
Copy link
Member Author

Dunqing commented Jul 10, 2024

I understand why enter_scope should be after visiting decorators, but why also enter_node?

You are right. There is an incorrect behavior here. See #4156 (comment)

We shouldn't visit decorators before enter_node. That will cause we don't know the decorators from where in visit_decoratots. And may make it hard to implement the decorators plugin of the transformer (I guess)

rzvxa added a commit that referenced this issue Jul 10, 2024
Closes #4142

I can split it into 2 PRs but it seems pointless. Let me know if you guys disagree with me.
@Dunqing Dunqing closed this as completed Jul 10, 2024
Boshen pushed a commit that referenced this issue Jul 14, 2024
…ons (#4156)

related: #4142 (comment)

Although we called `enter_node(Class)`, that doesn't mean we're in the `class` scope. It causes our must to visit decorators before `enter_node`.

Let's look at this case. It causes a syntax error if we don't visit decorators before `enter_node`
```js
// This file was procedurally generated from the following sources:
// - src/decorator/decorator-call-expr-identifier-reference-yield.case
// - src/decorator/syntax/valid/cls-expr-decorators-valid-syntax.template
/*---
description: Decorator @ DecoratorCallExpression (Valid syntax for decorator on class expression)
esid: prod-ClassExpression
features: [class, decorators]
flags: [generated, noStrict]
info: |
    ClassExpression[Yield, Await] :
      DecoratorList[?Yield, ?Await]opt class BindingIdentifier[?Yield, ?Await]opt ClassTail[?Yield, ?Await]

    DecoratorList[Yield, Await] :
      DecoratorList[?Yield, ?Await]opt Decorator[?Yield, ?Await]

    Decorator[Yield, Await] :
      @ DecoratorMemberExpression[?Yield, ?Await]
      @ DecoratorParenthesizedExpression[?Yield, ?Await]
      @ DecoratorCallExpression[?Yield, ?Await]

    ...

    DecoratorCallExpression[Yield, Await] :
      DecoratorMemberExpression[?Yield, ?Await] Arguments[?Yield, ?Await]

    DecoratorMemberExpression[Yield, Await] :
      IdentifierReference[?Yield, ?Await]
      DecoratorMemberExpression[?Yield, ?Await] . IdentifierName
      DecoratorMemberExpression[?Yield, ?Await] . PrivateIdentifier

    IdentifierReference[Yield, Await] :
      [~Yield] yield
      ...

---*/
function decorator() {
  return () => {};
}
var yield = decorator;

var C = @yield() class {};

```
Errors:
```shell
  × The keyword 'yield' is reserved
    ╭─[language/statements/class/decorator/syntax/valid/decorator-call-expr-identifier-reference-yield.js:45:2]
 44 │
 45 │ @yield() class C {}
    ·  ─────
    ╰────
```

The changed code makes more sense. Only if we call `enter_scope` for class, the flags will contain `StrictMode`. Also, we can get the exact `flags` of the `scope` in the `class` at the transformer

For example:

```
class A {
   B() {
       // Before: flags is `Function`
      //  After: flags is `Function | StrictMode`
   }
}
```

The regression tests will be fixed in follow-up PRs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST C-bug Category - Bug
Projects
None yet
3 participants