-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Error on mapped type w/properties #46346
Conversation
1. Error on properties of type literals with computed properties whose name is a binary expression with `in`, because that's a good sign of a mapped type. 2. Parse following properties on mapped types, and error on them. 3. Stop checking computed property names in (1) to avoid producing errors based on misinterpreting mapped type syntax as an expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also fix #18299 ? Can you add a test that uses an interface
declaration if so?
@@ -1707,6 +1707,8 @@ namespace ts { | |||
readonly nameType?: TypeNode; | |||
readonly questionToken?: QuestionToken | PlusToken | MinusToken; | |||
readonly type?: TypeNode; | |||
/** Used only to produce grammar errors */ | |||
readonly members?: NodeArray<TypeElement>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any /* @internal */
node members? I assume not. Would make forEachChild
act strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I've seen so far in our codebase, other node properties that are there for error reporting only just have a comment saying so 😕
It now fixes #18299 too. |
!!! error TS7061: A mapped type may not declare properties or methods. | ||
} | ||
const E = class { | ||
[P in 'a' | 'b']: any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the reason this one gets a different error is that the computed property name is parsed as (P in 'a') | 'b'
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the old error; I wanted to show that the new error isn't smart enough to displace the old because it only looks at the top binary expression.
Separately, it's annoying that type parsing precedence is different from value parsing precedence for the same characters.
* Error on mapped types with properties 1. Error on properties of type literals with computed properties whose name is a binary expression with `in`, because that's a good sign of a mapped type. 2. Parse following properties on mapped types, and error on them. 3. Stop checking computed property names in (1) to avoid producing errors based on misinterpreting mapped type syntax as an expression. * add comment in types.ts * Update API again * Check interfaces and classes too * Add missed check in updateMappedTypeNode
in
, because that's a good sign of a mapped type.Fixes #45089
Fixes #18299
Open questions
members
parameter tocreateMappedTypeNode
because the parser needs to use it. Nobody else should though! I'm not sure how best to indicate this (or if it needs to be indicated).