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

Class method with name get throws incorrect "invalid property name" exception #261

Closed
richarddavison opened this issue Apr 2, 2024 · 4 comments · Fixed by #258 or #289
Closed
Labels
bug Something isn't working

Comments

@richarddavison
Copy link
Contributor

When using arrow functions to define methods on a class, QJS throws exception:

SyntaxError: invalid property name
    at ./test.js:2:6

class TestClass {
  get = () => console.log("get");
}

new TestClass().get();
@richarddavison richarddavison changed the title Class method with name get throws exception. Class method with name get throws incorrect "invalid property name" exception Apr 2, 2024
@gabrielmfern
Copy link

gabrielmfern commented Apr 10, 2024

I think this might be a bigger issue here, we can't have any property — be it on a class or an object — that has a name for a reserved keyword. This doesn't seem to be the behavior of other engines, though, since Node and the browsers I've tested with don't really care.

https://github.com/bellard/quickjs/blob/3b45d155c77bbdfe9177b1e03db830d2aff0b2a8/quickjs.c#L22385-L22400

I think it should probably not check for reserved keywords like this, since the spec doesn't really mention anything on this.


Might be a bit troublesome to deal with stuff like this though
image

@chqrlie
Copy link
Collaborator

chqrlie commented Apr 10, 2024

I just tried this:

class TestClass { "get" = () => console.log("get"); }

And it works. Property names can be identifiers, numbers, strings or computed property names (expressions between [ and ]).

Now the question is should get be rejected as a syntax error? Probably not and we should fix this. We already accept keywords as class property names, but get is a special case as this syntax is supported:

class TestClass2 { get x() { console.log("get x"); return 1; }}

So we should accept get as a property name if followed by a = or ;. Same for set.

@chqrlie chqrlie added the bug Something isn't working label Apr 10, 2024
@gabrielmfern
Copy link

@chqrlie Are you also considering allowing a class method to have a name of a reserved keyword? I'm facing a compatibility issue that happens because:

class Example {
  static() { return 1234 }
}

is not supported on quickjs

@chqrlie
Copy link
Collaborator

chqrlie commented Apr 11, 2024

@gabrielmfern: yes, the fix should also allow this particular case.

chqrlie added a commit to chqrlie/quickjs that referenced this issue May 5, 2024
- accept `class P { async = 1 }}`
- accept `class P { static = 1 }}` etc.
- Fixes bellard#261
chqrlie added a commit that referenced this issue May 5, 2024
- accept `class P { async = 1 }}`
- accept `class P { static = 1 }}` etc.
- Fixes #261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants