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.

Sign up for GitHub

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

Excluded the default library from rename service. #17415

Merged
merged 6 commits into from
Sep 14, 2017
Merged

Excluded the default library from rename service. #17415

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5b86055
Excluded the default library from rename service.
armanio123 Jul 25, 2017
675b6fb
Fixed failing tests and lint. Added unit tests.
armanio123 Jul 25, 2017
88262db
Changed check for default library by first checking hasNoDefaultLib, …
armanio123 Aug 15, 2017
9dd574b
Added host.useCaseSens..., memoized getDefaultLibFileName.
armanio123 Aug 16, 2017
b03fbaf
Merge branch 'master' into FixRenameInDefaultLibrary
armanio123 Sep 14, 2017
1ab67c0
Fixed sourceFiles type error
armanio123 Sep 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions src/compiler/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,12 +325,12 @@ namespace ts {
}

output += sys.newLine;
output += `${ relativeFileName }(${ firstLine + 1 },${ firstLineChar + 1 }): `;
output += `${relativeFileName}(${firstLine + 1},${firstLineChar + 1}): `;
}

const categoryColor = getCategoryFormat(diagnostic.category);
const category = DiagnosticCategory[diagnostic.category].toLowerCase();
output += `${ formatAndReset(category, categoryColor) } TS${ diagnostic.code }: ${ flattenDiagnosticMessageText(diagnostic.messageText, sys.newLine) }`;
output += `${formatAndReset(category, categoryColor)} TS${diagnostic.code}: ${flattenDiagnosticMessageText(diagnostic.messageText, sys.newLine)}`;
output += sys.newLine;
}
return output;
Expand Down Expand Up @@ -436,6 +436,8 @@ namespace ts {
host = host || createCompilerHost(options);

let skipDefaultLib = options.noLib;
const getDefaultLibraryFileName = memoize(() => host.getDefaultLibFileName(options));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

const defaultLibraryPath = host.getDefaultLibLocation ? host.getDefaultLibLocation() : getDirectoryPath(getDefaultLibraryFileName());
const programDiagnostics = createDiagnosticCollection();
const currentDirectory = host.getCurrentDirectory();
const supportedExtensions = getSupportedExtensions(options);
Expand Down Expand Up @@ -511,12 +513,11 @@ namespace ts {
// If '--lib' is not specified, include default library file according to '--target'
// otherwise, using options specified in '--lib' instead of '--target' default library file
if (!options.lib) {
processRootFile(host.getDefaultLibFileName(options), /*isDefaultLib*/ true);
processRootFile(getDefaultLibraryFileName(), /*isDefaultLib*/ true);
}
else {
const libDirectory = host.getDefaultLibLocation ? host.getDefaultLibLocation() : getDirectoryPath(host.getDefaultLibFileName(options));
forEach(options.lib, libFileName => {
processRootFile(combinePaths(libDirectory, libFileName), /*isDefaultLib*/ true);
processRootFile(combinePaths(defaultLibraryPath, libFileName), /*isDefaultLib*/ true);
});
}
}
Expand Down Expand Up @@ -555,6 +556,7 @@ namespace ts {
getFileProcessingDiagnostics: () => fileProcessingDiagnostics,
getResolvedTypeReferenceDirectives: () => resolvedTypeReferenceDirectives,
isSourceFileFromExternalLibrary,
isSourceFileDefaultLibrary,
dropDiagnosticsProducingTypeChecker,
getSourceFileFromReference,
sourceFileToPackageName,
Expand Down Expand Up @@ -975,6 +977,18 @@ namespace ts {
return sourceFilesFoundSearchingNodeModules.get(file.path);
}

function isSourceFileDefaultLibrary(file: SourceFile): boolean {
if (file.hasNoDefaultLib) {
return true;
}

if (defaultLibraryPath && defaultLibraryPath.length !== 0) {
return containsPath(defaultLibraryPath, file.path, currentDirectory, /*ignoreCase*/ !host.useCaseSensitiveFileNames());
}

return compareStrings(file.fileName, getDefaultLibraryFileName(), /*ignoreCase*/ !host.useCaseSensitiveFileNames()) === Comparison.EqualTo;
}

function getDiagnosticsProducingTypeChecker() {
return diagnosticsProducingTypeChecker || (diagnosticsProducingTypeChecker = createTypeChecker(program, /*produceDiagnostics:*/ true));
}
Expand Down Expand Up @@ -1204,7 +1218,7 @@ namespace ts {
diagnostics.push(createDiagnosticForNode(node, Diagnostics._0_can_only_be_used_in_a_ts_file, "?"));
return;
}
// falls through
// falls through
case SyntaxKind.MethodDeclaration:
case SyntaxKind.MethodSignature:
case SyntaxKind.Constructor:
Expand Down Expand Up @@ -1286,7 +1300,7 @@ namespace ts {
diagnostics.push(createDiagnosticForNodeArray(nodes, Diagnostics.type_parameter_declarations_can_only_be_used_in_a_ts_file));
return;
}
// falls through
// falls through
case SyntaxKind.VariableStatement:
// Check modifiers
if (nodes === (<ClassDeclaration | FunctionLikeDeclaration | VariableStatement>parent).modifiers) {
Expand Down Expand Up @@ -1334,8 +1348,8 @@ namespace ts {
if (isConstValid) {
continue;
}
// to report error,
// falls through
// to report error,
// falls through
case SyntaxKind.PublicKeyword:
case SyntaxKind.PrivateKeyword:
case SyntaxKind.ProtectedKeyword:
Expand Down Expand Up @@ -1551,10 +1565,10 @@ namespace ts {
}

function getSourceFileFromReferenceWorker(
fileName: string,
getSourceFile: (fileName: string) => SourceFile | undefined,
fail?: (diagnostic: DiagnosticMessage, ...argument: string[]) => void,
refFile?: SourceFile): SourceFile | undefined {
fileName: string,
getSourceFile: (fileName: string) => SourceFile | undefined,
fail?: (diagnostic: DiagnosticMessage, ...argument: string[]) => void,
refFile?: SourceFile): SourceFile | undefined {

if (hasExtension(fileName)) {
if (!options.allowNonTsExtensions && !forEach(supportedExtensions, extension => fileExtensionIs(host.getCanonicalFileName(fileName), extension))) {
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2452,6 +2452,7 @@ namespace ts {
/* @internal */ getFileProcessingDiagnostics(): DiagnosticCollection;
/* @internal */ getResolvedTypeReferenceDirectives(): Map<ResolvedTypeReferenceDirective>;
/* @internal */ isSourceFileFromExternalLibrary(file: SourceFile): boolean;
/* @internal */ isSourceFileDefaultLibrary(file: SourceFile): boolean;
// For testing purposes only.
/* @internal */ structureIsReused?: StructureIsReused;

Expand Down
44 changes: 29 additions & 15 deletions src/services/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ namespace ts {
if (!hasModifier(node, ModifierFlags.ParameterPropertyModifier)) {
break;
}
// falls through
// falls through
case SyntaxKind.VariableDeclaration:
case SyntaxKind.BindingElement: {
const decl = <VariableDeclaration>node;
Expand All @@ -682,7 +682,7 @@ namespace ts {
visit(decl.initializer);
}
}
// falls through
// falls through
case SyntaxKind.EnumMember:
case SyntaxKind.PropertyDeclaration:
case SyntaxKind.PropertySignature:
Expand Down Expand Up @@ -729,7 +729,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) { }
Copy link
Member

Choose a reason for hiding this comment

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

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

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 it is passing linter (before and after). This was done by VS Code formatting, I thought it should be pushed.


public getLineAndCharacterOfPosition(pos: number): LineAndCharacter {
return ts.getLineAndCharacterOfPosition(this, pos);
Expand Down Expand Up @@ -1130,14 +1130,14 @@ namespace ts {
const newSettings = hostCache.compilationSettings();
const shouldCreateNewSourceFiles = oldSettings &&
(oldSettings.target !== newSettings.target ||
oldSettings.module !== newSettings.module ||
oldSettings.moduleResolution !== newSettings.moduleResolution ||
oldSettings.noResolve !== newSettings.noResolve ||
oldSettings.jsx !== newSettings.jsx ||
oldSettings.allowJs !== newSettings.allowJs ||
oldSettings.disableSizeLimit !== oldSettings.disableSizeLimit ||
oldSettings.baseUrl !== newSettings.baseUrl ||
!equalOwnProperties(oldSettings.paths, newSettings.paths));
oldSettings.module !== newSettings.module ||
oldSettings.moduleResolution !== newSettings.moduleResolution ||
oldSettings.noResolve !== newSettings.noResolve ||
oldSettings.jsx !== newSettings.jsx ||
oldSettings.allowJs !== newSettings.allowJs ||
oldSettings.disableSizeLimit !== oldSettings.disableSizeLimit ||
oldSettings.baseUrl !== newSettings.baseUrl ||
!equalOwnProperties(oldSettings.paths, newSettings.paths));

// Now create a new compiler
const compilerHost: CompilerHost = {
Expand Down Expand Up @@ -1365,7 +1365,7 @@ namespace ts {
function getCompilerOptionsDiagnostics() {
synchronizeHostData();
return program.getOptionsDiagnostics(cancellationToken).concat(
program.getGlobalDiagnostics(cancellationToken));
program.getGlobalDiagnostics(cancellationToken));
}

function getCompletionsAtPosition(fileName: string, position: number): CompletionInfo {
Expand Down Expand Up @@ -1510,7 +1510,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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

let sourceFiles: SourceFile[] = [];
if (options && options.isForRename) {
for (const sourceFile of program.getSourceFiles()) {
if (!program.isSourceFileDefaultLibrary(sourceFile)) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

sourceFiles.push(sourceFile);
}
}
}
else {
sourceFiles = program.getSourceFiles();
}

return FindAllReferences.findReferencedEntries(program, cancellationToken, sourceFiles, getValidSourceFile(fileName), position, options);
}

function findReferences(fileName: string, position: number): ReferencedSymbol[] {
Expand Down Expand Up @@ -2100,7 +2114,7 @@ namespace ts {
isLiteralComputedPropertyDeclarationName(node);
}

function isObjectLiteralElement(node: Node): node is ObjectLiteralElement {
function isObjectLiteralElement(node: Node): node is ObjectLiteralElement {
switch (node.kind) {
case SyntaxKind.JsxAttribute:
case SyntaxKind.JsxSpreadAttribute:
Expand All @@ -2125,7 +2139,7 @@ namespace ts {
if (node.parent.kind === SyntaxKind.ComputedPropertyName) {
return isObjectLiteralElement(node.parent.parent) ? node.parent.parent : undefined;
}
// falls through
// falls through
case SyntaxKind.Identifier:
return isObjectLiteralElement(node.parent) &&
(node.parent.parent.kind === SyntaxKind.ObjectLiteralExpression || node.parent.parent.kind === SyntaxKind.JsxAttributes) &&
Expand Down
11 changes: 11 additions & 0 deletions tests/cases/fourslash/renameDefaultLibDontWork.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// <reference path='fourslash.ts' />

// Tests that tokens found on the default library are not renamed.
// "test" is a comment on the default library.

// @Filename: file1.ts
//// var [|test|] = "foo";
//// console.log([|test|]);

const ranges = test.ranges();
verify.renameLocations(ranges[0], { findInComments: true, ranges });