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

transformer: Invalid output for stage 3 decorator #8628

Open
overlookmotel opened this issue Jan 20, 2025 · 3 comments
Open

transformer: Invalid output for stage 3 decorator #8628

overlookmotel opened this issue Jan 20, 2025 · 3 comments
Assignees
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented Jan 20, 2025

Reduced example, boiled down from failures in monitor-oxc: https://github.com/oxc-project/monitor-oxc/actions/runs/12864766675/job/35864033007

Input:

let x;

class C {
  accessor #prop = 123;

  #method() {
    this.#prop;
  }
}

Output:

import _classPrivateMethodInitSpec from "@babel/runtime/helpers/classPrivateMethodInitSpec";
import _classPrivateFieldGet from "@babel/runtime/helpers/classPrivateFieldGet2";

let x;

var _C_brand = new WeakSet();
class C {
    constructor() {
        _classPrivateMethodInitSpec(this, _C_brand);
    }
    accessor #prop = 123;
}

function _method() {
    // Next line is invalid
    _classPrivateFieldGet(, this);
}

Oxc playground

What is really weird is that if you comment out the let x line, then you get different output for _method():

function _method() {
    _classPrivateFieldGet(_C, this);
}

Oxc playground

But that's not correct either! If #prop was a normal property not an accessor, it should be _classPrivateFieldGet(_prop, this);.

The bogus output _classPrivateFieldGet(, this) is in once sense "correct" because we don't support accessor properties, so there is no _prop variable.

But why does whether there's a line let x above the class make any difference to the output? Any other statement which generates a binding also produces the empty output e.g. const x = 1;, var x;, function x() {}, class X {}, but other statements e.g. x = 1 don't.

I suspect there may be 2 different bugs at play here.

@overlookmotel overlookmotel added A-transformer Area - Transformer / Transpiler C-bug Category - Bug labels Jan 20, 2025
@overlookmotel
Copy link
Contributor Author

overlookmotel commented Jan 21, 2025

Oh I think I figured it out.

We create a dummy BoundIdentifier for the accessor with SymbolId = 0:

ClassElement::AccessorProperty(prop) => {
// TODO: Not sure what we should do here.
// Only added this to prevent panics in TS conformance tests.
if let PropertyKey::PrivateIdentifier(ident) = &prop.key {
let dummy_binding = BoundIdentifier::new(Atom::empty(), SymbolId::new(0));
private_props.insert(
ident.name,
PrivateProp::new(dummy_binding, prop.r#static, None, true),
);
}
}

The private method transform visits body of the method and replaces all references to the class name with _C. When the class is the first binding in the file, its SymbolId is 0. So it sees the reference to the dummy symbol, says "this is a reference to the class" and replaces it with _C. But if there's let x statement before the class, then the class's SymbolId is 1, so the reference doesn't match the class's SymbolId, and it doesn't replace it. Mystery solved!

So question is: What's the fix?

We could solve the monitor-oxc error easily by making the output valid syntatically (but nonsensical):

- let dummy_binding = BoundIdentifier::new(Atom::empty(), SymbolId::new(0));
+ let dummy_binding = BoundIdentifier::new(Atom::from("DUMMY"), SymbolId::new(0));

But that's not a real fix! Should we do that for now and get monitor-oxc passing, and decorators transform will deal with it properly? Or do legacy decorators not include accessors?

Other options:

  1. Emit an "accessors are not supported" error.
  2. Treat accessor #prop the same as plain #prop.

Any opinion @Dunqing?

@Boshen
Copy link
Member

Boshen commented Jan 21, 2025

Thank you for the investigation.

https://github.com/puppeteer/puppeteer/blob/89f4f3b1079874b2dd91c727bc058c1b6e09096e/packages/puppeteer-core/src/bidi/Browser.ts#L103-L104

  @bubble()
  accessor #trustedEmitter = new EventEmitter<BrowserEvents>();

This is stage 3 decorator. accessor = stage 3.

We should throw an "stage 3 decorator not supported" error right now.

@Boshen Boshen changed the title Invalid output for class properties transform transformer: Invalid output for stage 3 decorator Jan 21, 2025
@Boshen
Copy link
Member

Boshen commented Jan 21, 2025

I disabled it in oxc-project/monitor-oxc@28c3d1a

Let's come back to this after stage 3 decorator is implemented.

  • add a hard error when stage 3 decorator is encountered. @Dunqing

@Boshen Boshen closed this as not planned Won't fix, can't repro, duplicate, stale Jan 21, 2025
@Boshen Boshen reopened this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler C-bug Category - Bug
Projects
None yet
Development

No branches or pull requests

3 participants