Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixing delay caused in vscode due to pasteEdits #59542

Merged
merged 21 commits into from
Aug 20, 2024

Conversation

navya9singh
Copy link
Member

@navya9singh navya9singh commented Aug 7, 2024

This pr fixes the delay in vscode. The delay happened due to iterating over the entire target file and going through each node to see if it needed to be resolved. This is fixed by going over only the pasted range of text and then stepping through each child node to see if it needs to be resolved

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 7, 2024
Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create some tests around the specific questions I mentioned if they don't already exist.

Also, I think you need a few more tests for when the paste replacement text items are (compared to their target ranges):

  1. consistently equal in size
  2. consistently smaller in size
  3. consistently larger in size
  4. vary by growing in size
  5. vary by shrinking in size

src/services/pasteEdits.ts Outdated Show resolved Hide resolved
const updatedRanges: TextRange[] = [];
let offset = 0;
pasteLocations.forEach((location, i) => {
const deletionNeeded = location.pos === location.end ? 0 : location.end - location.pos;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If location.pos === location.end, you'll end up with 0 on the second branch - so this is more about the length of the paste selection, right?

I'd recommend getting rid of the unnecessary branch and rewriting this to something like

const selectedTextLength = location.end - location.pos;

or

const pasteSelectionLength = location.end - location.pos;

or

const oldTextLength = location.end - location.pos;

src/services/pasteEdits.ts Outdated Show resolved Hide resolved
src/services/pasteEdits.ts Outdated Show resolved Hide resolved
});

updatedRanges.forEach(range => {
const enclosingNode = findAncestor(getTokenAtPosition(context.sourceFile, range.pos), ancestorNode => rangeContainsPosition(ancestorNode, range.end));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: I almost wanted to suggest rangeContainsRange but I guess this just works because you've found the token at the start position, but you're trying to find the parent that also contains the end.

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking a lot better! I left some sugestions. I'd look over the other tests to see if they need similar feedback, but there's a cat on my right arm.

src/services/pasteEdits.ts Outdated Show resolved Hide resolved
Comment on lines 41 to 46
pastedText: [ `const t = 1 + juice + p;`,`function avacado() { return sauce; }`,
`fig + kiwi`,
`function k() {
const cherry = 3 + tomato + cucumber;
}`
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pastedText: [ `const t = 1 + juice + p;`,`function avacado() { return sauce; }`,
`fig + kiwi`,
`function k() {
const cherry = 3 + tomato + cucumber;
}`
],
pastedText: [
`const t = 1 + juice + p;`,`function avacado() { return sauce; }`,
`fig + kiwi`,
`function k() {
const cherry = 3 + tomato + cucumber;
}`
],

Comment on lines 118 to 125
pasteLocations.forEach((location, i) => {
const oldTextLength = location.end - location.pos;
const textToBePasted = actualPastedText ? actualPastedText[0] : pastedText[i];
const startPos = location.pos + offset;
const endPos = startPos + textToBePasted.length;
updatedRanges.push({ pos: startPos, end: endPos });
offset += textToBePasted.length - oldTextLength;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it that this need to be separate pass and not inline where we are iterating over nodes in the the updated range?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the updatedFile, we don't know the exact range of where the pasted text exists, so doing this pass helps us get the right location of the pasted text. Then, it becomes easy to just go over the updated ranges and check for unresolved nodes. Is that what you were asking about?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isnt it same as :

let offset = 0;
            pasteLocations.forEach((location, i) => {
                const oldTextLength = location.end - location.pos;
                const textToBePasted = actualPastedText ? actualPastedText[0] : pastedText[i];
                const startPos = location.pos + offset;
                const endPos = startPos + textToBePasted.length;
                const range = { pos: startPos, end: endPos } satisfies TextRange;
                offset += textToBePasted.length - oldTextLength;
                const enclosingNode = findAncestor(
                    getTokenAtPosition(context.sourceFile, range.pos),
                    ancestorNode => rangeContainsRange(ancestorNode, range),
                );
                if (!enclosingNode) return;
                forEachChild(enclosingNode, function importUnresolvedIdentifiers(node) {
                    if (
                        isIdentifier(node) && rangeContainsPosition(
                            range,
                            node.getStart(updatedFile),
                        ) && !originalProgram?.getTypeChecker().resolveName(
                            node.text,
                            node,
                            SymbolFlags.All,
                            /*excludeGlobals*/ false,
                        )
                    ) {
                        return importAdder.addImportForUnresolvedIdentifier(
                            context,
                            node,
                            /*useAutoImportProvider*/ true,
                        );
                    }
                    node.forEachChild(importUnresolvedIdentifiers);
                });
            });

@navya9singh navya9singh marked this pull request as ready for review August 14, 2024 16:42
src/services/pasteEdits.ts Show resolved Hide resolved
const textToBePasted = actualPastedText ? actualPastedText[0] : pastedText[i];
const startPos = location.pos + offset;
const endPos = startPos + textToBePasted.length;
const range = { pos: startPos, end: endPos } satisfies TextRange;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const range = { pos: startPos, end: endPos } satisfies TextRange;
const range: TextRange = { pos: startPos, end: endPos };

src/services/pasteEdits.ts Show resolved Hide resolved
if (
isIdentifier(node) && rangeContainsPosition(
range,
node.getStart(updatedFile),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to run node.getStart()? Can you just use node.pos?

Copy link
Member Author

@navya9singh navya9singh Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, node.pos gives the position with the whitespaces. So, for example in pasteEdits_multiplePastesConsistentlyLargerInSize.ts for the 3rd paste location (const test = [|1 + 2|] + 3; ) , node.pos will give the position of the whitespace before 1, but the updated range of the pasted text starts from 1. As node.pos will not lie in this range, it will skip checking for the pasted text here. node.Start() avoids this by giving the exact position of 1.

src/services/pasteEdits.ts Outdated Show resolved Hide resolved
src/services/pasteEdits.ts Outdated Show resolved Hide resolved
`function k() {
const cherry = 3 + tomato + cucumber;
}`
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
],
],

let offset = 0;
pasteLocations.forEach((location, i) => {
const oldTextLength = location.end - location.pos;
const textToBePasted = actualPastedText ? actualPastedText[0] : pastedText[i];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ever possible that the count of paste locations exceeds the number of items in pastedText?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done at top of the function to make sure its always right

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in that case all pastedText is joined together with a newline character and that is pasted at all locations. Line 64 does this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha - while you're here, can you fix that so it uses the appropriate newline character instead of \n?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, actualPastedText doesn't have to be an array if you only ever use it for actualPastedText[0]. Just use actualPastedText ?? pastedText[i].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean using getNewLineOrDefaultFromHost() for the appropriate newline character?

Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from daniel's code formatting comments, looks good.

forEachChild(enclosingNode, function importUnresolvedIdentifiers(node) {
const isImportCandidate = isIdentifier(node) &&
rangeContainsPosition(range, node.getStart(updatedFile)) &&
!originalProgram?.getTypeChecker().resolveName(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn’t this use the updated program? For example, if I have a single paste of

const b = 0;
console.log(b);

won’t this try to resolve b against the original text which didn’t have a declaration for b, so we’ll try to auto-import it unnecessarily?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didnt see this before, but originalProgram's typechecker usage is sketchy as well.. The reuse of sourceFile and parent pointer changing makes it non-viable to use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test that fails when using originalProgram?

pastedText: [
`const b = 1;
console.log(b);`],
pasteLocations: [range[0]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things

  1. Make this test.ranges() like everywhere else.
  2. In the future, if you have an array, use the plural version (ranges instead of range).

@DanielRosenwasser DanielRosenwasser merged commit 2192336 into microsoft:main Aug 20, 2024
30 checks passed
@DanielRosenwasser
Copy link
Member

@typescript-bot cherry-pick this to release-5.6

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 20, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
cherry-pick this to release-5.6 ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

Hey, @DanielRosenwasser! I've created #59695 for you.

DanielRosenwasser pushed a commit that referenced this pull request Aug 20, 2024
…e-5.6 (#59695)

Co-authored-by: navya9singh <108360753+navya9singh@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants