Skip to content
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

Fixing range for primary edit #59369

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

navya9singh
Copy link
Member

Fixes #58936
Due to getText() and not getFullText(), new line characters were getting ignored when trying to perform a temporary file update which later made the paste at the wrong positions.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jul 19, 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.

There is another call to getText() below that is incorrectly used. It's arguable whether or not that should just reuse originalText, but I honestly am not familiar enough with the code.

But something must be wrong when copying/pasting trivia over, and the new test must not cover it.

@navya9singh
Copy link
Member Author

There is another call to getText() below that is incorrectly used. It's arguable whether or not that should just reuse originalText, but I honestly am not familiar enough with the code.

But something must be wrong when copying/pasting trivia over, and the new test must not cover it.

I haven't found a case where the next getText() would be a problem, but yes I think it's better to have that instead.
This part of the code takes the targetFile, and adds the new, 'pasted' text to it, then performs the callback function to deal with imports. In the finally block, it adds the original text to the target file which was there before the paste took place.

src/server/project.ts Outdated Show resolved Hide resolved

this.getScriptInfo(rootFile)?.editContent(0, originalText.length, updatedText);
this.updateGraph();
try {
cb(this.program!, originalProgram, (this.program?.getSourceFile(rootFile))!);
}
finally {
this.getScriptInfo(rootFile)?.editContent(0, this.program!.getSourceFile(rootFile)!.getText().length, originalText);
this.getScriptInfo(rootFile)?.editContent(0, updatedText.length, originalText);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... looking at it again, is it really safe to assume that cb doesn't itself change the contents of the file? I'm sure the way we use this function is used in our codebase, it's fine, but I just want to check. What do you think @sheetalkamat?

Copy link
Member

Choose a reason for hiding this comment

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

yes we should assume cb doesnt change the contents of file since file is never "muted" without change in script info

Copy link
Member

@DanielRosenwasser DanielRosenwasser Jul 23, 2024

Choose a reason for hiding this comment

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

(assuming you mean mutated)

What I meant was that cb could itself run editContent somehow. If that happens, the ScriptInfo object may not actually contain the same text anymore.

I'm okay with assuming that will never happen for now, I just wanted to run it by you.

Copy link
Member

Choose a reason for hiding this comment

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

The alternative is just to replace the text contents with the script info's own text contents. It should all be available anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I did mean mutated ☺️ Currently editing script info from callback is not possible as it’s just trying to calculate import fixes.. when that changes we can revisit this ..

@navya9singh navya9singh merged commit b04c8a0 into microsoft:main Jul 23, 2024
32 checks passed
navya9singh added a commit to navya9singh/TypeScript that referenced this pull request Aug 7, 2024
navya9singh added a commit to navya9singh/TypeScript that referenced this pull request Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

getPasteEdits returns wrong range for primary edit
4 participants