fix(typescript-estree): fix identifier tokens typed as Keyword
#1487
Conversation
|
Thanks for the PR, @lydell! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
is identifier token typed as `Keywordof identifier token typed as `Keyword
of identifier token typed as `Keywordof identifier token typed as Keyword
There was a problem hiding this comment.
LGTM.
Could you please also double check all of the other SyntaxKind.**Keyword types while you're at it?
Might as well make sure we don't need any more follow up PRs.
Looking at the TS source code; this is the list of keywords
(https://github.com/microsoft/TypeScript/blob/eeff036519362513cc86a95e260a281a725295f4/src/compiler/types.ts#L120-L535)
I think that these are the only ones we need to worry about:
// Contextual keywords
AbstractKeyword,
AsKeyword,
AssertsKeyword,
AnyKeyword,
AwaitKeyword,
BooleanKeyword,
ConstructorKeyword,
DeclareKeyword,
InferKeyword,
KeyOfKeyword,
NamespaceKeyword,
NeverKeyword,
ReadonlyKeyword,
RequireKeyword,
NumberKeyword,
ObjectKeyword,
StringKeyword,
SymbolKeyword,
UndefinedKeyword,
UniqueKeyword,
UnknownKeyword,
FromKeyword,
GlobalKeyword,
BigIntKeyword,
Looking at the enum, there are also these, but I think that using these as identifiers results in syntax errors:
// Reserved words
BreakKeyword,
CaseKeyword,
CatchKeyword,
ClassKeyword,
ConstKeyword,
ContinueKeyword,
DebuggerKeyword,
DefaultKeyword,
DeleteKeyword,
DoKeyword,
ElseKeyword,
EnumKeyword,
ExportKeyword,
ExtendsKeyword,
FalseKeyword,
FinallyKeyword,
ForKeyword,
FunctionKeyword,
IfKeyword,
ImportKeyword,
InKeyword,
InstanceOfKeyword,
NewKeyword,
NullKeyword,
ReturnKeyword,
SuperKeyword,
SwitchKeyword,
ThisKeyword,
ThrowKeyword,
TrueKeyword,
TryKeyword,
TypeOfKeyword,
VarKeyword,
VoidKeyword,
WhileKeyword,
WithKeyword,
// Strict mode reserved words
ImplementsKeyword,
InterfaceKeyword,
LetKeyword,
PackageKeyword,
PrivateKeyword,
ProtectedKeyword,
PublicKeyword,
StaticKeyword,
YieldKeyword,
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
in SyntaxKind there are special values that represents boundaries of node types that can be used to do range check for example i think in this case we should do something like this instead (i'm not sure tho) if ('originalKeywordKind' in token && token.originalKeywordKind) {
if (token.originalKeywordKind === SyntaxKind.NullKeyword) {
return AST_TOKEN_TYPES.Null;
} else if (
token.originalKeywordKind > SyntaxKind.LastFutureReservedWord &&
token.originalKeywordKind <= SyntaxKind.LastKeyword
) {
return AST_TOKEN_TYPES.Identifier;
}
return AST_TOKEN_TYPES.Keyword;
}this will mark 30 not reserved keywords as Identifiers if used as such |
|
This is the list of keywords which typescript allows as identifiers without throwing an error: Which suggests the range you'd want to check instead is I believe the check we want is this: if ('originalKeywordKind' in token && token.originalKeywordKind) {
if (token.originalKeywordKind === SyntaxKind.NullKeyword) {
return AST_TOKEN_TYPES.Null;
} else if (
token.originalKeywordKind >= SyntaxKind.FirstFutureReservedWord &&
token.originalKeywordKind <= SyntaxKind.LastKeyword
) {
return AST_TOKEN_TYPES.Identifier;
}
return AST_TOKEN_TYPES.Keyword;
} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
of identifier token typed as KeywordKeyword
|
Pushed an update. Hopefully I got it right. Let me know what you think. |
|
The code you've got LGTM. Now that you've got me thinking about this: const foo = { get bar() { return 1 } };
// ^^^ should this be an Identifier token or a Keyword token?Both |
|
I've raised eslint/js#428 and I'll chat with the eslint team there, and we can follow up to correct the logic in a later PR if needs be. |
As in #681 and #750, the
ofkeyword wasn't correctly handled in the parser with respect to variable names, so the token it created was typed as Keyword instead of Identifier.Fixes lydell/eslint-plugin-simple-import-sort#33