Excluded the default library from rename service. #17415

armanio123 · 2017-07-25T21:27:31Z

Excludes the default library when renaming tokens.

msftclas · 2017-07-25T21:27:35Z

@armanio123,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

billti · 2017-07-25T21:35:53Z

src/services/services.ts

@@ -725,7 +725,7 @@ namespace ts {

    class SourceMapSourceObject implements SourceMapSource {
        lineMap: number[];
-        constructor (public fileName: string, public text: string, public skipTrivia?: (pos: number) => number) {}
+        constructor(public fileName: string, public text: string, public skipTrivia?: (pos: number) => number) { }


All these white-space changes are interesting. Is it passing the linter? (Was it before?)

Yes it is passing linter (before and after). This was done by VS Code formatting, I thought it should be pushed.

billti · 2017-07-25T21:37:24Z

src/compiler/program.ts

@@ -921,6 +922,10 @@ namespace ts {
            return sourceFilesFoundSearchingNodeModules.get(file.path);
        }

+        function isSourceFileDefaultLibrary(file: SourceFile): boolean {
+            return file.fileName === host.getDefaultLibFileName(options);


I thought @rbuckton was factoring the core libraries into a set of smaller files that get added. If that's the case, I'm not sure this would work (as there wouldn't be just one default lib filename).

If that the case, we might want to change this method to check for all of the different files. I'll talk to @rbuckton and get his input.

Based on what I have discussed with @rbuckton the better way of doing this it to check for GetDefaultLibDirectory. Every file that is there is going to be considered core library.

Perhaps this should be named isSourceFileReadonly or isSourceFileLockedForEdits so that we can later use the same functionality to prevent renames in declarations pulled in from node_modules or other "typeRoots" locations.

billti · 2017-07-25T21:41:28Z

src/services/services.ts

+            let sourceFiles: SourceFile[] = [];
+            if (options.isForRename) {
+                for (let sourceFile of program.getSourceFiles()) {
+                    if (!program.isSourceFileDefaultLibrary(sourceFile)) {


So previously if you tried to rename something that was defined in the core library (e.g. a JavaScript API such as Array.map, or a DOM API such as window.addEventListener), it would error because you can't rename something in the core library. With this change, will that rename now proceed, but only for all your program files (i.e. leaving you in an error state)? I'm not sure that's a better behavior.

That functionality will remain the same. That happens on the function getRenameInfo.
This piece of code specifically checks for the locations of the tokens (not the token type). It will only exclude the default library from being referenced when searching for the token to be renamed.

billti

I'd follow up with Ron on the core library not being one file piece, and Ryan on the behavior when renaming a core API now - but if they're happy, then looks good to me.

rbuckton · 2017-08-08T20:38:54Z

src/services/services.ts

@@ -1506,7 +1506,21 @@ namespace ts {

        function getReferences(fileName: string, position: number, options?: FindAllReferences.Options) {
            synchronizeHostData();
-            return FindAllReferences.findReferencedEntries(program, cancellationToken, program.getSourceFiles(), getValidSourceFile(fileName), position, options);
+
+            // Exclude default library when renaming as commonly user don't want to change that file.


Will this block renaming a symbol declared in a default library? I would think that if a symbol is declared in a default library then we shouldn't allow the rename to proceed at all, rather than renaming it in a user's source files and resulting in possible compile time errors.

There's already a feature for that. This will avoid renaming anything on the default library, like comments or strings.

rbuckton · 2017-08-08T20:41:27Z

src/compiler/program.ts

@@ -921,6 +922,10 @@ namespace ts {
            return sourceFilesFoundSearchingNodeModules.get(file.path);
        }

+        function isSourceFileDefaultLibrary(file: SourceFile): boolean {
+            return file.fileName === host.getDefaultLibFileName(options);


Perhaps this should be named isSourceFileReadonly or isSourceFileLockedForEdits so that we can later use the same functionality to prevent renames in declarations pulled in from node_modules or other "typeRoots" locations.

…second library path and third name of file.

rbuckton · 2017-08-15T02:07:02Z

src/compiler/program.ts

+                return true;
+            }
+
+            if (defaultLibraryPath !== undefined && defaultLibraryPath.length !== 0) {


This can be simplified: if (defaultLibraryPath)

rbuckton · 2017-08-15T02:08:03Z

src/compiler/program.ts

+            }
+
+            if (defaultLibraryPath !== undefined && defaultLibraryPath.length !== 0) {
+                return comparePaths(defaultLibraryPath, file.path, currentDirectory, /*ignoreCase*/ true) === Comparison.EqualTo;


Do not unconditionally pass true for ignoreCase, instead use !host.useCaseInsensitiveFileNames().

I don't think comparePaths is the one you want to call. I think you meant to call containsPath here.

rbuckton · 2017-08-15T02:08:16Z

src/compiler/program.ts

+                return comparePaths(defaultLibraryPath, file.path, currentDirectory, /*ignoreCase*/ true) === Comparison.EqualTo;
+            }
+
+            return compareStrings(file.fileName, host.getDefaultLibFileName(options), /*ignoreCase*/ true) === Comparison.EqualTo;


Do not unconditionally pass true for ignoreCase, instead use !host.useCaseInsensitiveFileNames().

rbuckton · 2017-08-15T02:10:56Z

src/compiler/program.ts

@@ -436,6 +436,7 @@ namespace ts {
        host = host || createCompilerHost(options);

        let skipDefaultLib = options.noLib;
+        const defaultLibraryPath = host.getDefaultLibLocation ? host.getDefaultLibLocation() : getDirectoryPath(host.getDefaultLibFileName(options));


There is also a libDirectory variable in this function that does the same thing. Consider combining the two and possibly storing the result of host.getDefaultLibFileName(options) in a variable rather than requery it each time.

rbuckton

Approved, with a minor comment.

rbuckton · 2017-09-14T18:33:46Z

src/compiler/program.ts

@@ -436,6 +436,8 @@ namespace ts {
        host = host || createCompilerHost(options);

        let skipDefaultLib = options.noLib;
+        const getDefaultLibraryFileName = memoize(() => host.getDefaultLibFileName(options));


Why bother with memoize and not just have const defaultLibraryFileName = host.getDefaultLibraryFileName(options)?

Depending on the options and some logic, the variable might end up not being needed. This was done for lazy evaluation.

mhegazy · 2017-09-18T20:36:53Z

I do not think this is the best solution here. the default libraries are not any special. users can replace them. and the same issue persists if some one say renamed div in a JSX tag.

A better solution is to block rename for symbols coming from a .d.ts file unless the source of the rename operation is that file. see #8227

armanio123 requested review from billti and RyanCavanaugh July 25, 2017 21:27

msftclas added the cla-not-required label Jul 25, 2017

billti reviewed Jul 25, 2017

View reviewed changes

rbuckton requested changes Aug 8, 2017

View reviewed changes

armanio123 added 2 commits August 14, 2017 17:58

Excluded the default library from rename service.

5b86055

Fixed failing tests and lint. Added unit tests.

675b6fb

armanio123 force-pushed the FixRenameInDefaultLibrary branch from 6cc5710 to 675b6fb Compare August 15, 2017 00:59

Changed check for default library by first checking hasNoDefaultLib, …

88262db

…second library path and third name of file.

rbuckton requested changes Aug 15, 2017

View reviewed changes

Added host.useCaseSens..., memoized getDefaultLibFileName.

9dd574b

rbuckton approved these changes Sep 14, 2017

View reviewed changes

armanio123 added 2 commits September 14, 2017 12:01

Merge branch 'master' into FixRenameInDefaultLibrary

b03fbaf

Fixed sourceFiles type error

1ab67c0

armanio123 merged commit 21bbdd3 into microsoft:master Sep 14, 2017

armanio123 deleted the FixRenameInDefaultLibrary branch September 14, 2017 21:04

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Excluded the default library from rename service. #17415

Excluded the default library from rename service. #17415

armanio123 commented Jul 25, 2017

msftclas commented Jul 25, 2017

billti Jul 25, 2017

armanio123 Jul 25, 2017

billti Jul 25, 2017

armanio123 Jul 25, 2017 •

edited

Loading

armanio123 Jul 29, 2017

rbuckton Aug 8, 2017

billti Jul 25, 2017

armanio123 Jul 25, 2017

billti left a comment

rbuckton Aug 8, 2017

armanio123 Aug 15, 2017

rbuckton Aug 8, 2017

rbuckton Aug 15, 2017

rbuckton Aug 15, 2017

rbuckton Aug 15, 2017

rbuckton Aug 15, 2017

rbuckton Aug 15, 2017

rbuckton left a comment

rbuckton Sep 14, 2017

armanio123 Sep 14, 2017

mhegazy commented Sep 18, 2017

Excluded the default library from rename service. #17415

Excluded the default library from rename service. #17415

Conversation

armanio123 commented Jul 25, 2017

msftclas commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanio123 Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Sep 18, 2017

armanio123 Jul 25, 2017 •

edited

Loading