Skip to content

Reverse SyntaxKind query shows misleading result #18062

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

Closed
koczkatamas opened this issue Aug 26, 2017 · 4 comments
Closed

Reverse SyntaxKind query shows misleading result #18062

koczkatamas opened this issue Aug 26, 2017 · 4 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@koczkatamas
Copy link

koczkatamas commented Aug 26, 2017

TypeScript Version: 2.4.2

Code

console.log(ts.SyntaxKind[8]);

Expected behavior: NumericLiteral

Actual behavior: FirstLiteralToken

Description:

Maybe this is a duplicate of a more broad issue (which I did not find), and I am not sure if there is a workaround for this specific case, but currently I would presume that I can use SyntaxKind enum for reverse lookups of values I got from TS ast node's kind property.

As this enum describes not just unique values but also ranges (with FirstSomething and LastSomething elements), reversing the enum values is not unambiguous, and currently it gives back the less expected result (FirstLiteralToken) and not the expected one (NumericLiteral).

I think changing the order of the enum items or putting these range items to outside of the enum could solve this issue, but this would break compatibility of course.

Footnote: other tools (for example https://astexplorer.net/) also ran into this mistake and it makes harder to beginners to understand without reading TS's source code what's actually happening here. (I thought for a while that FirstLiteralToken is some kind of special token, but then I empirically found out it only shows this token type for numeric values and just then I looked into TS's source code...)

((I'll report this issue to AstExplorer too.))

image

koczkatamas added a commit to koczkatamas/astexplorer that referenced this issue Aug 26, 2017
Reverse SyntaxKind lookup does not give correct results (as described here: microsoft/TypeScript#18062) . This commit workarounds that issue (until it'll be resolved in TS compiler - if it will be ever...)
@ikatyang
Copy link
Contributor

See #3103.

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Aug 28, 2017
@RyanCavanaugh
Copy link
Member

We probably (?) should have had "first in wins" semantics for the reverse map, but it'd be a runtime breaking change (extreme risk) to change it now.

@saschanaz
Copy link
Contributor

Maybe moving FirstLiteralToken-like things to the top of the enum would help?

fkling pushed a commit to fkling/astexplorer that referenced this issue Sep 1, 2017
* Fix typescript parser's node name

Reverse SyntaxKind lookup does not give correct results (as described here: microsoft/TypeScript#18062) . This commit workarounds that issue (until it'll be resolved in TS compiler - if it will be ever...)

* Fix linting errors of commit 46cec82

* Improve workaround introduced in 46cec82

Replaces previous implementation with a (hopefully) more future-proof one in the case of TS compiler changes internal indices of SyntaxKind enum

* Move SyntaxKind workaround from parse to loadParser
@mhegazy
Copy link
Contributor

mhegazy commented Sep 12, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Sep 12, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

5 participants