Uh oh!
There was an error while loading. Please reload this page.
Simplify mergeEmitNode - #59613
Closed
Jake Bailey (jakebailey) wants to merge 2 commits into
Closed
Conversation
Jake Bailey (jakebailey)
requested review from
Armando Aguirre (armanio123) and Ron Buckton (rbuckton)August 12, 2024 23:51
Jake Bailey (jakebailey)
commented
Aug 12, 2024
MemberAuthor
TypeScript Bot (@typescript-bot) perf test this faster |
Contributor
Comment on lines
-7521
to
-7527
| function mergeTokenSourceMapRanges(sourceRanges: (TextRange | undefined)[], destRanges: (TextRange | undefined)[]) { | ||
| if (!destRanges) destRanges = []; | ||
| for (const key in sourceRanges) { | ||
| destRanges[key] = sourceRanges[key]; | ||
| } | ||
| return destRanges; | ||
| } |
MemberAuthor
There was a problem hiding this comment.
I read this function and it sort of scared me; what if the test has more elements than the source? Then I replaced it and nothing changed, so I kept going...
Contributor
There was a problem hiding this comment.
sourceRanges and destRanges are holey arrays, so length doesn't actually matter. It probably should just be a Map<number, TextRange>, to be honest.
TypeScript Bot (typescript-bot)
commented
Aug 13, 2024
Contributor
Jake Bailey (@jakebailey) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Ron Buckton (rbuckton)
commented
Aug 13, 2024
Contributor
A lot of the complexity in |
Comment on lines
-7521
to
-7527
| function mergeTokenSourceMapRanges(sourceRanges: (TextRange | undefined)[], destRanges: (TextRange | undefined)[]) { | ||
| if (!destRanges) destRanges = []; | ||
| for (const key in sourceRanges) { | ||
| destRanges[key] = sourceRanges[key]; | ||
| } | ||
| return destRanges; | ||
| } |
Contributor
There was a problem hiding this comment.
sourceRanges and destRanges are holey arrays, so length doesn't actually matter. It probably should just be a Map<number, TextRange>, to be honest.
| if (leadingComments) { | ||
| // We use `.slice()` in case `destEmitNode.leadingComments` is pushed to later | ||
| destEmitNode.leadingComments = addRange(leadingComments.slice(), destEmitNode.leadingComments); | ||
| destEmitNode.leadingComments = leadingComments.slice(); |
Contributor
There was a problem hiding this comment.
I worry this results in a different inconsistency now. Before, we would attempt to merge the emit information for the old node into anything set on the new node prior to calling setOriginalNode. Now we will conditionally overwrite some properties but not others.
constoldNode= ...;addSyntheticLeadingComment(oldNode,SyntaxKind.SingleLineCommentTrivia,"foo");constnewNode= ...;addSyntheticLeadingComment(oldNode,SyntaxKind.SingleLineCommentTrivia,"bar");addSyntheticTrailingComment(oldNode,SyntaxKind.SingleLineCommentTrivia,"baz");setOriginalNode(newNode,oldNode);// beforegetSyntheticLeadingComments(newNode).length;// 2 (foo and bar)getSyntheticTrailingComments(newNode).length;// 1 (baz)// aftergetSyntheticLeadingComments(newNode).length;// 1 (foo)getSyntheticTrailingComments(newNode).length;// 1 (baz)After this change we end up with oldNode's leading comments but newNodes trailing comments, which seems more confusing.
MemberAuthor
There was a problem hiding this comment.
I certainly am not confident in this PR, but we do need something for #59332.
Contributor
There was a problem hiding this comment.
I originally considered using appendIfUnique to avoid duplicating but also to concatenate. Would you think that's a good fix?
Ron Buckton (rbuckton)
commented
Aug 15, 2024
Contributor
The issue that drove this PR is not so much an issue with |
Ron Buckton (rbuckton)
commented
Aug 15, 2024
Contributor
I found the actual underlying problem in #59332 in |
This file contains hidden or 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
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
Armando Aguirre (@armanio123) and I noticed in #59332 that simply copying
leadingComments/trailingCommentsand force overwriting (rather than concatenating) fixes the comment duplication problem from #59332.Reading things further, it the function already force copies over the other node's information (flags, comment ranges, etc). Out of interest, I tried removing some of the other "complicated" merging operations and found that they also worked fine when just regular slices.
Maybe this extra simplification is valid?