fix rename cause import change wrongly #38462

ShuiRuTian · 2020-05-11T05:53:21Z

msftclas · 2020-05-11T05:53:32Z

All CLA requirements met.

ShuiRuTian · 2020-05-11T05:59:23Z

There is another choice to change src/services/getEditsForFileRename.ts rather than src/compiler/moduleSpecifiers.ts.
But I think this can prevent other issues in future.
Anyway, just tell me to change it if this is a bad choice in fact.

sheetalkamat

Code change looks correct but i would want @rbuckton to validate the vfs change.

sheetalkamat

Wait thi

sheetalkamat · 2020-05-20T23:28:24Z

src/compiler/moduleSpecifiers.ts

@@ -347,7 +347,8 @@ namespace ts.moduleSpecifiers {
        // Get a path that's relative to node_modules or the importing file's path
        // if node_modules folder is in this folder or any of its parent folders, no need to keep it.
        const pathToTopLevelNodeModules = getCanonicalFileName(moduleSpecifier.substring(0, parts.topLevelNodeModulesIndex));
-        if (!(startsWith(sourceDirectory, pathToTopLevelNodeModules) || globalTypingsCacheLocation && startsWith(getCanonicalFileName(globalTypingsCacheLocation), pathToTopLevelNodeModules))) {
+        const canonicalSourceDirectory = getCanonicalFileName(sourceDirectory);
+        if (!(startsWith(canonicalSourceDirectory, pathToTopLevelNodeModules) || globalTypingsCacheLocation && startsWith(getCanonicalFileName(globalTypingsCacheLocation), pathToTopLevelNodeModules))) {


Wait this seems incorrect. sourceDirectory is already canonical path.
https://github.com/microsoft/TypeScript/pull/38462/files#diff-aa86ac6315ba3e1d25a9a1789f7f5a78L110

Since this is the only substantive change outside of tests in this PR, is there something we are doing incorrectly when computing sourceDirectory? If not, what does this PR change other than the VFS in the test harness and adding a test?

I think we would need to diagnose why source directory is not canonical already instead.

#38462 (comment)
Would it be better to add a comment to let users know it is their responsibility to make sure sourceDirectory canonical path?

rbuckton · 2020-05-20T23:40:02Z

src/harness/vfsUtil.ts

-                    if (this._getLinks(existingNode).size > 0) throw createIOError("ENOTEMPTY");
+                    // if both old and new arguments point to the same directory, just pass. So we could rename /src/a/1 to /src/A/1 in Win.
+                    // if not and the directory pointed by the new path is not empty, throw an error.
+                    // However, in the link in the description, it says: If the old argument points to the pathname of a file that is not a directory, the new argument shall not point to the pathname of a directory. If the link named by the new argument exists, it shall be removed and old renamed to new. In this case, a link named new shall remain visible to other threads throughout the renaming operation and refer either to the file referred to by new or old before the operation began. Write access permission is required for both the directory containing old and the directory containing new.


This comment is too long to read easily in a PR. Please split this comment across multiple lines.

Remove it and the content is as follows.

In the link http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html
It says
If the old argument points to the pathname of a file that is not a directory, the new argument shall not point to the pathname of a directory. If the link named by the new argument exists, it shall be removed and old renamed to new. In this case, a link named new shall remain visible to other threads throughout the renaming operation and refer either to the file referred to by new or old before the operation began. Write access permission is required for both the directory containing old and the directory containing new.
But this function does not behave like this.
This is intended, or it is an issue need to fix? Or we just do not care it?

src/harness/vfsUtil.ts

rbuckton · 2020-05-20T23:46:47Z

src/compiler/moduleSpecifiers.ts

@@ -347,7 +347,8 @@ namespace ts.moduleSpecifiers {
        // Get a path that's relative to node_modules or the importing file's path
        // if node_modules folder is in this folder or any of its parent folders, no need to keep it.
        const pathToTopLevelNodeModules = getCanonicalFileName(moduleSpecifier.substring(0, parts.topLevelNodeModulesIndex));
-        if (!(startsWith(sourceDirectory, pathToTopLevelNodeModules) || globalTypingsCacheLocation && startsWith(getCanonicalFileName(globalTypingsCacheLocation), pathToTopLevelNodeModules))) {
+        const canonicalSourceDirectory = getCanonicalFileName(sourceDirectory);
+        if (!(startsWith(canonicalSourceDirectory, pathToTopLevelNodeModules) || globalTypingsCacheLocation && startsWith(getCanonicalFileName(globalTypingsCacheLocation), pathToTopLevelNodeModules))) {


Since this is the only substantive change outside of tests in this PR, is there something we are doing incorrectly when computing sourceDirectory? If not, what does this PR change other than the VFS in the test harness and adding a test?

Co-authored-by: Ron Buckton <[email protected]>

src/services/getEditsForFileRename.ts

ShuiRuTian · 2020-06-18T07:48:32Z

Friendly ping @rbuckton

Song Gao added 2 commits May 9, 2020 19:48

use canonical file name when resolve module

c048371

renameSync in vfs supports same folder.

d0927a8

ShuiRuTian changed the title ~~Gs/fix rename cause import change wrongly~~ fix rename cause import change wrongly May 12, 2020

sandersn assigned sheetalkamat May 20, 2020

sandersn added the For Backlog Bug PRs that fix a backlog bug label May 20, 2020

sheetalkamat requested a review from rbuckton May 20, 2020 23:24

sheetalkamat approved these changes May 20, 2020

View reviewed changes

sheetalkamat assigned rbuckton May 20, 2020

sheetalkamat requested changes May 20, 2020

View reviewed changes

rbuckton requested changes May 20, 2020

View reviewed changes

sandersn unassigned rbuckton May 21, 2020

ShuiRuTian and others added 2 commits May 22, 2020 18:19

Update src/harness/vfsUtil.ts

a806801

Co-authored-by: Ron Buckton <[email protected]>

change tss rather than compiler.

3b370c4

ShuiRuTian requested review from rbuckton and sheetalkamat May 25, 2020 04:08

remove useless comment.

61ce269

sheetalkamat reviewed May 26, 2020

View reviewed changes

src/services/getEditsForFileRename.ts Outdated Show resolved Hide resolved

Song Gao added 2 commits May 27, 2020 11:13

Merge branch 'master' into gs/fix-rename-cause-import-change-wrongly

53d5894

use fileName rather than path.

3eb8b5b

ShuiRuTian requested a review from sheetalkamat May 27, 2020 07:32

sheetalkamat approved these changes May 27, 2020

View reviewed changes

RyanCavanaugh merged commit 8293e51 into microsoft:master Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix rename cause import change wrongly #38462

fix rename cause import change wrongly #38462

ShuiRuTian commented May 11, 2020

msftclas commented May 11, 2020 •

edited

Loading

ShuiRuTian commented May 11, 2020

sheetalkamat left a comment

sheetalkamat left a comment

sheetalkamat May 20, 2020

rbuckton May 20, 2020

sheetalkamat May 21, 2020

ShuiRuTian May 22, 2020 •

edited

Loading

rbuckton May 20, 2020

ShuiRuTian May 22, 2020 •

edited

Loading

rbuckton May 20, 2020

ShuiRuTian commented Jun 18, 2020

fix rename cause import change wrongly #38462

fix rename cause import change wrongly #38462

Conversation

ShuiRuTian commented May 11, 2020

msftclas commented May 11, 2020 • edited Loading

ShuiRuTian commented May 11, 2020

sheetalkamat left a comment

Choose a reason for hiding this comment

sheetalkamat left a comment

Choose a reason for hiding this comment

sheetalkamat May 20, 2020

Choose a reason for hiding this comment

rbuckton May 20, 2020

Choose a reason for hiding this comment

sheetalkamat May 21, 2020

Choose a reason for hiding this comment

ShuiRuTian May 22, 2020 • edited Loading

Choose a reason for hiding this comment

rbuckton May 20, 2020

Choose a reason for hiding this comment

ShuiRuTian May 22, 2020 • edited Loading

Choose a reason for hiding this comment

rbuckton May 20, 2020

Choose a reason for hiding this comment

ShuiRuTian commented Jun 18, 2020

msftclas commented May 11, 2020 •

edited

Loading

ShuiRuTian May 22, 2020 •

edited

Loading

ShuiRuTian May 22, 2020 •

edited

Loading