Remove getAllAccessorDeclarations from the EmitResolver #57993
Merged
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 faster |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jakebailey
approved these changes
Mar 29, 2024
Comment on lines
+48917
to
+48922
| if (node.kind === SyntaxKind.Parameter && node.parent.kind === SyntaxKind.SetAccessor) { | ||
| const other = getAllAccessorDeclarationsForDeclaration(node.parent as SetAccessorDeclaration).getAccessor; | ||
| if (other) { | ||
| return getEffectiveReturnTypeNode(other); | ||
| } | ||
| } |
There was a problem hiding this comment.
Just to be clear, this code is a noop, right? Since we're not actually handling these and no baselines change? This is brand new code, so, checking.
There was a problem hiding this comment.
Yes, for now. My goal is to move the full "should this node be reused" logic check into the node builder, to minimize the EmitResolver's API surface. Today, the node builder never has to emit nonlocal types for get and set accessors. If one is missing an annotation, it gets the type the other one has, and if both have the same type, it prints them as a property, rather than accessors. Meanwhile, declaration emit proper obviously leaves accessor declarations as-is nowadays, and uses logic like this to pull the type node from the other accessor when one is missing an annotation. So this is slightly future-looking, in that it enables the node builder to properly reuse nodes for get/set pairs that are missing one type node, even though the declaration emitter doesn't yet ask it to do that (and you can't get an inferred type with a get/set pair without them having differing types, and thus both annotated).
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.
This reduces the coupling between the checker and the transforms a bit. We actually had a
getAllAccessorDeclarationsinutilitiesalready - the only difference being it required the container node, too (which was readily available at all callsites). Within the checker, the reverse is done - it no longer uses thegetAllAccessorDeclarationsutilitiesfunction, and now reliably uses thesymbol-based lookup method theEmitResolverpreviously exported, and more reliably looks up the type nodes from accessors. (This does not change any baselines yet, because accessors currently disqualify node reuse - another followup will change that.)