Remove originalKeywordKind from Identifiers
#51497
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 0ce5e62. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:
CompilerComparison Report - main..51497
System
Hosts
Scenarios
TSServerComparison Report - main..51497
System
Hosts
Scenarios
StartupComparison Report - main..51497
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DanielRosenwasser
added
the
Breaking Change
DanielRosenwasser
force-pushed
the
removeOriginalKeywordKind
branch
from
0ce5e62
to
d8ba011
Compare
DanielRosenwasser
force-pushed
the
removeOriginalKeywordKind
branch
from
d8ba011
to
6ba90be
Compare
jakebailey
reviewed
Dec 14, 2022
DanielRosenwasser
force-pushed
the
removeOriginalKeywordKind
branch
from
9ae5865
to
b9a4758
Compare
|
@typescript-bot perf test this faster |
|
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at b9a4758. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:Comparison Report - main..51497
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rbuckton
requested changes
Dec 14, 2022
| @@ -1677,7 +1677,6 @@ export interface Identifier extends PrimaryExpression, Declaration, JSDocContain | |||
| * Text of identifier, but if the identifier begins with two underscores, this will begin with three. | |||
| */ | |||
| readonly escapedText: __String; | |||
| readonly originalKeywordKind?: SyntaxKind; // Original syntaxKind which get set so that we can report an error later | |||
There was a problem hiding this comment.
My only concern with this is that it is a public API breaking change. Do we expose any other way to get this information? stringToToken is currently marked /** @internal */.
If we find that there are consumers in the field that need this information, we could potentially expose it via a getter in the src/deprecatedCompat project with a @deprecated comment and the caveat that repeated access would be slower since it would be recalculated each time.
There was a problem hiding this comment.
We could try to keep it as a public but deprecated property. I do see some usage in
There was a problem hiding this comment.
Is there a best practice to add deprecated accessors to prototypes right now?
There was a problem hiding this comment.
Also https://sourcegraph.com/search?q=context:global+originalKeywordKind+-repo:microsoft/TypeScript+-file:%5C.d%5C.ts+lang:typescript+-file:src/compiler+-file:src/services&patternType=standard&sm=1 which sorts a little better; it is being used by eslint, angular, a few other big projects.
There was a problem hiding this comment.
There's no prototype to add it to, since everything inherits from Node. You would have to add it to the instance. There's an addNodeFactoryPatcher function that lets you hook into the creation of a NodeFactory to patch in a deprecation, but it doesn't affect factories that already exist (i.e., parseNodeFactory in parser.ts, the internal factory inside of the Parser namespace in parser.ts, or factory in nodeFactory.ts).
The current patching mechanism was primarily to handle patching the factory API itself, and only the one that is reachable during transformations. As a result, the current patching mechanism doesn't touch the factory used internally by the parser, which is the one you'd want most if you want to patch node instances returned by the API.
There was a problem hiding this comment.
It does seem like a win in terms of memory usage. If we really want to avoid the break, #51498 gives us both.
We should still deprecate it either way.
There was a problem hiding this comment.
Ah, I didn't notice the memory improvement.
There was a problem hiding this comment.
Actually, I misspoke. we could potentially add it to the prototype of the Identifier constructor we return from objectAllocator, although this is overridden in services so you'd need to do that in two places.
There was a problem hiding this comment.
Yeah, honestly, maybe it won't be that slow and sticking it on Identifier is enough. (I was hoping caching it somehow would help but that might be slower in general...)
There was a problem hiding this comment.
We'd have to test whether defining an accessor on Identifier actually improves memory usage. If it's a wash I'd leave originalSyntaxKind alone, or just deprecate it and remove it later.
|
@typescript-bot perf test faster |
|
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at 716e136. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:Comparison Report - main..51497
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Let's just see how a prototype property (doesn't?) affect the compiler. @typescript-bot perf test this faster |
|
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at 952715c. You can monitor the build here. Update: The results are in! |
|
I'm not sure the benchmarker is going to work when there are merge conflicts (not sure what if the merge base or something else is tested). |
|
@DanielRosenwasser Here they are:Comparison Report - main..51497
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Seems like the memory changes are a wash? Maybe better once we get a bunch more props removed too. (If we want this, probably need to stick it onto the deprecationCompat project instead so tsc isn't affected.) |
|
Closed in favor of #52170. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Experiment for #51496.