fix(rome_js_parser): TsPropertySignatureClassMember with initializer

[IWANABETHATGUY]

Summary

1. Part of fix(rome_js_parser): allow readonly property with initializer in ambient context #3000
2. Part of ☂️ Parser bugs and problems #2310
3. TsPropertySignatureClassMember with initializer is allowed in Typescript, see https://www.typescriptlang.org/play?#code/MYGwhgzhAEAq0G8BQ1oBMCmowCcPTzDQHsA7EAT2gBcMJroBeaAZgG4kBfIA

Test Plan

1. cargo test should pass.

[IWANABETHATGUY]

@MichaReiser , It looks that Initializer in TsPropertySignatureClassMember is valid,

here is the ast.

[MichaReiser]

Can you please list valid and invalid cases so that we can decide on where the initializer should be placed.

For example, the initializer isn't valid in combination with the definite assignment operator but this AST would now allow for a property having an initializer and a definite assignment operator.

[IWANABETHATGUY]

A 'const' initializer in an ambient context must be a string or numeric literal or literal enum reference.

[IWANABETHATGUY]

Do we really need to check this for now? If we do, we could only check for string or numeric literal, we can't check for literal enum reference

[IWANABETHATGUY]

What about adding some todos? And finish this after we have the ability to type checking?

[MichaReiser]

I don't think we have to rush this change as I would argue, that it's probably not a too common case to have an initializer in an ambient context.

Also, changing AST structures is rather involved which is why I would prefer not to rush it. What's important for now is to fully understand valid and invalid patterns and we can then start thinking about how we want to change the AST structure.

[magic-akari]

https://github.com/microsoft/TypeScript/blob/b1176ce5e8e062451d29a7e06136a949bf7fbee8/src/compiler/checker.ts#L45136-L45155

But maybe it's better to open an issue in TypeScript repo.

[magic-akari]

• Allow literal initializers of readonly properties in declaration files microsoft/TypeScript#26313
• fix(parser): allow TS declare readonly fields with initializers babel/babel#14817

[IWANABETHATGUY]

Also, this is a known issue of our parser, see #2310 (comment).

[IWANABETHATGUY]

Thanks @magic-akari for the nice research!

Valid cases in ambient context:

1. numerical literal

class T {
    declare readonly a = 3;
}

2. string literal

class T1 {
    declare readonly a = 'test';
}

class T3 {
    declare readonly a = `test`;
}

3. literal enum reference.

Invalid case in ambient context

1. TemplateStringLiteral with JsTemplateElement

class T2 {
    declare readonly a = `test${20}`;
}

any other cases are considered to be invalid.

[IWANABETHATGUY]

A few solutions I could give for now:

1. Allow TS declare readonly fields with initializers without checking, just like babel did.
2. Only checking string literal and numerical literal like Allow literal initializers of readonly properties in declaration files microsoft/TypeScript#26313 did.
   cc @MichaReiser

[MichaReiser]

Thanks @magic-akari for the nice research!

Valid cases in ambient context:

1. numerical literal

class T {

    declare readonly a = 3;

}

2. string literal

class T1 {

    declare readonly a = 'test';

}



class T3 {

    declare readonly a = `test`;

}

3. literal enum reference.

Invalid case in ambient context

1. TemplateStringLiteral with JsTemplateElement

class T2 {

    declare readonly a = `test${20}`;

}

any other cases are considered to be invalid.

Thanks for this listing. How does the syntax work in combination with the optional and definite operators?

Is it only allowed if the readonly modifier is present?

[IWANABETHATGUY]

Thanks @magic-akari for the nice research!

Valid cases in ambient context:

1. numerical literal

class T {

    declare readonly a = 3;

}

2. string literal

class T1 {

    declare readonly a = 'test';

}



class T3 {

    declare readonly a = `test`;

}

3. literal enum reference.

Invalid case in ambient context

1. TemplateStringLiteral with JsTemplateElement

class T2 {

    declare readonly a = `test${20}`;

}

any other cases are considered to be invalid.

Thanks for this listing. How does the syntax work in combination with the optional and definite operators?

Is it only allowed if the readonly modifier is present?

Yes,

Only working if there is no type annotation and with a readonly modifier

[IWANABETHATGUY]

Thanks @magic-akari for the nice research!

Valid cases in ambient context:

1. numerical literal

class T {

    declare readonly a = 3;

}

2. string literal

class T1 {

    declare readonly a = 'test';

}



class T3 {

    declare readonly a = `test`;

}

3. literal enum reference.

Invalid case in ambient context

1. TemplateStringLiteral with JsTemplateElement

class T2 {

    declare readonly a = `test${20}`;

}

any other cases are considered to be invalid.

Thanks for this listing. How does the syntax work in combination with the optional and definite operators?

Is it only allowed if the readonly modifier is present?

See the Typescript implementation https://github.com/microsoft/TypeScript/pull/26313/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R29425-R29429

[IWANABETHATGUY]

I think checking phase could left to #3000

[MichaReiser]

I'm conflicted on how to best add this to our AST so that the API encodes as many constraints as possible.

• readonly: Modifiers is an array, which is why it isn't possible to encode the must have a readonly modifier
• no type annotation: One option is to add JsInitializerClause as a variant to TsAnyPropertySignatureAnnotation. This has the added benefit that it encodes the fact that the initializer is only valid if there is no type annotation (and the parser automatically marking it as an unknown node if it does have one). However, I do dislike that an initializer isn't an annotation in that sense. But comes with the downside that users of our AST may not know in which circumstances an initializer may be present.
• Adding the initializer directly to the TsPropertySignatureTypeMember has the benefit that the validation could potentially be delayed until the linter phase.

What's your preference @IWANABETHATGUY and for what reasons?

I guess, I overall dislike TypeScript design decision behind adding support of this syntax rather than using `readonly prop: "value" but that's obviously out of my control :D

[github-actions]

This PR is stale because it has been open 14 days with no activity.

[magic-akari]

Any update?

[ematipico]

@MichaReiser @IWANABETHATGUY what's the status of this PR? Is it ready for merging/approved? Or should we put it on hold?

[IWANABETHATGUY]

@MichaReiser @IWANABETHATGUY what's the status of this PR? Is it ready for merging/approved? Or should we put it on hold?

Last week I am working on the instantiation expression, I will have a look at this pr this week.

[MichaReiser]

I'm conflicted on how to best add this to our AST so that the API encodes as many constraints as possible.

* `readonly`: Modifiers is an array, which is why it isn't possible to encode the _must have a readonly_ modifier

* no type annotation: One option is to add `JsInitializerClause` as a variant to `TsAnyPropertySignatureAnnotation`. This has the added benefit that it encodes the fact that the `initializer` is only valid if there is no type annotation (and the parser automatically marking it as an unknown node if it does have one). However, I do dislike that an initializer isn't an annotation in that sense. But comes with the downside that users of our AST may not know in which circumstances an initializer may be present.

* Adding the initializer directly to the `TsPropertySignatureTypeMember` has the benefit that the validation could potentially be delayed until the linter phase.

What's your preference @IWANABETHATGUY and for what reasons?

I guess, I overall dislike TypeScript design decision behind adding support of this syntax rather than using `readonly prop: "value" but that's obviously out of my control :D

@IWANABETHATGUY what's your opinion on this?

[IWANABETHATGUY]

I prefer the third one.

[MichaReiser]

I prefer the third one.

Could you explain why?

[IWANABETHATGUY]

I prefer the third one.

Could you explain why?

Making our Ast shape more compatible with Typescript, we could validate the error either in the parser phase or analyzer phase which is more flexible.

[MichaReiser]

This new syntax is even more subtle:

• Only allowed inside classes but not interfaces
• Don't allow a type annotation

That's why I'm currently leaning towards introducing its own member type TsInitializedPropertySignatureClassMember (open to better names) that is defined as

TsInitializedPropertySignatureClassMember = 
	modifiers: TsPropertySignatureModifierList
	name: JsAnyClassMemberName
	'?'?
	initializer: JsInitializerClause
	';'?

I'm undecided if we want a specific initializer only allowing JsStringLiteralExpression and JsNumberLiteralExpressions.

@IWANABETHATGUY what do you think of a new member type that we only add to classes?

[IWANABETHATGUY]

This new syntax is even more subtle:

• Only allowed inside classes but not interfaces
• Don't allow a type annotation

That's why I'm currently leaning towards introducing its own member type TsInitializedPropertySignatureClassMember (open to better names) that is defined as

TsInitializedPropertySignatureClassMember = 
	modifiers: TsPropertySignatureModifierList
	name: JsAnyClassMemberName
	'?'?
	initializer: JsInitializerClause
	';'?

I'm undecided if we want a specific initializer only allowing JsStringLiteralExpression and JsNumberLiteralExpressions.

@IWANABETHATGUY what do you think of a new member type that we only add to classes?

I think it is alright to introduce a new type which only used for Class initializer clause, but it is not so scaled when typescript introduces more context-related initialized clause

[xunilrj]

We already have a code that checks this:

crates\rome_js_parser\src\syntax\class.rs:944

else if modifiers.has(ModifierKind::Declare) || p.state.in_ambient_context() {
            // test_err ts ts_property_initializer_ambient_context
            // declare class A { prop = "test" }
            // class B { declare prop = "test" }
            p.error(
                p.err_builder("Initializers are not allowed in ambient contexts.")
                    .primary(initializer.range(p), ""),
            );
        }

We can "fix" the example just doing:

declare class A { readonly prop = "test" }

So my suggestion is actually to use TS_PROPERTY_SIGNATURE_CLASS_MEMBER with optional initializer and error with: "Properties with initializers in ambient contexts needs to be read only."

[netlify]

✅ Deploy Preview for docs-rometools ready!

Name	Link
🔨 Latest commit	c932a1b
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/633ac71f26b8b60008604767
😎 Deploy Preview	https://deploy-preview-3003--docs-rometools.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.