Parse quoted constructors as constructors, not methods

[andrewbranch]

Fixes #31020, part 2. We now properly parse class A { "constructor"() {} } as a constructor, not as a method. Part 1 was issuing an error to warn people that the behavior was changing: #31119

[ajafff]

Probably needs changes to forEachChild, transform visitor and factory APIs, too.

[ajafff]

Is the string literal allowed to contain escapes?

[rbuckton]

Yes, escapes are handled in the lexical grammar. text is the unescaped version, so it should be fine.

[ajafff]

I asked because I wasn't sure if the spec allowed escapes in the string literal. Tested it with Chrome and it recognizes it as constructor.

[andrewbranch]

Sorry, I’m not completely following—can you show me exactly what you tried with Chrome?

[ajafff]

To test whether ES6 classes allow escapes in the sting literal "constructor" I executed the following code:

new (class C {"\x63onstructor"(param) {console.log(param)}})(1)

I was just curious because "use strict" is only recognized if it does not contain escapes.

[andrewbranch]

I actually don’t see anything in the grammar that prohibits that from being a proper constructor, so maybe we should be looking at escaped text. But I could be missing something. Was looking around this area and here. I’m leaning toward assuming Node and Chrome are doing the correct thing.

[andrewbranch]

Handled escaped constructors in the latest commit. I think this is ready now.

[ajafff]

Regular constructors don't have a name property. To avoid polymorphism you could

• explicitly set it to undefined in parseConstructorDeclaration
• set it after parseConstructorDeclarationEnd (not preferred as it still introduces a new object shape)

[andrewbranch]

True; I guess we don’t need to set that property. I was setting it just because we already have the node and it would allow you to disambiguate the exact structure of the declaration if needed, but that should be covered by getChildren().

[ajafff]

Should be labeled as breaking change to the API

[rbuckton]

Why not just check the token text here and call parseConstructorDeclaration, and have parseConstructorDeclaration handle whether its a string or keyword? The public fields proposal disallows public fields with the name constructor or "constructor".

[andrewbranch]

Just because we already give a better error at the grammar checking stage for fields named constructor—it says Classes may not have a field named 'constructor'. instead of '(' expected.

[andrewbranch]

Probably needs changes to forEachChild, transform visitor and factory APIs, too.

Regular constructors don't have a name property.

I think if we want to preserve the quoted string constructor in refactors, emit, etc., the easiest thing to do is probably to keep the name property, and in that case you’re right that we need a couple more changes here.

Alternatively, if we want to drop the name property, we’d just treat quoted constructors exactly the same as non-quoted constructors, and at a glance it looks like in that case the existing getChildren(), visitors, and factories will work, just may not preserve the quotes in refactors/emit.

Curious to get @rbuckton’s opinion here.

[rbuckton]

I'm not sure how important it is to preserve whether the constructor name was quoted.

[RyanCavanaugh]

I'm not sure how important it is to preserve whether the constructor name was quoted.

I would say it's quite unimportant - it's not possibly subject to minification, which is the only thing people seem to care about in terms of preserving quotedness