Skip to content

Commit 6221d70

Browse files
weswighamweswigham
authored
Fix import addition location (microsoft#17327)
* Add test with bug * Fix for import placement * Consolidate comment recognition functions into utilities * Add another test with all 3 kinds * Recognize path directives as part of triple slash directives * Also handle no-default-lib triple-slash comments * Test for all the triple-slash kinds * Keep import-placement logic in the quickfix, since its not really a node start; accept new baselines * Work in not-ES6, use a real no-lib comment * Remove no default lib triple slash comment, it disables checking and thereby quick fixes * Copy regex rather than have a regex copy
1 parent 445cc8d commit 6221d70

9 files changed

Lines changed: 118 additions & 19 deletions

‎src/compiler/comments.ts‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -415,17 +415,7 @@ namespace ts {
415415
* @return true if the comment is a triple-slash comment else false
416416
*/
417417
function isTripleSlashComment(commentPos: number, commentEnd: number) {
418-
// Verify this is /// comment, but do the regexp match only when we first can find /// in the comment text
419-
// so that we don't end up computing comment string and doing match for all // comments
420-
if (currentText.charCodeAt(commentPos + 1) === CharacterCodes.slash &&
421-
commentPos + 2 < commentEnd &&
422-
currentText.charCodeAt(commentPos + 2) === CharacterCodes.slash) {
423-
const textSubStr = currentText.substring(commentPos, commentEnd);
424-
return textSubStr.match(fullTripleSlashReferencePathRegEx) ||
425-
textSubStr.match(fullTripleSlashAMDReferencePathRegEx) ?
426-
true : false;
427-
}
428-
return false;
418+
return isRecognizedTripleSlashComment(currentText, commentPos, commentEnd);
429419
}
430420
}
431421
}

‎src/compiler/utilities.ts‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,32 @@ namespace ts {
262262
return !nodeIsMissing(node);
263263
}
264264

265+
/**
266+
* Determine if the given comment is a triple-slash
267+
*
268+
* @return true if the comment is a triple-slash comment else false
269+
*/
270+
export function isRecognizedTripleSlashComment(text: string, commentPos: number, commentEnd: number) {
271+
// Verify this is /// comment, but do the regexp match only when we first can find /// in the comment text
272+
// so that we don't end up computing comment string and doing match for all // comments
273+
if (text.charCodeAt(commentPos + 1) === CharacterCodes.slash &&
274+
commentPos + 2 < commentEnd &&
275+
text.charCodeAt(commentPos + 2) === CharacterCodes.slash) {
276+
const textSubStr = text.substring(commentPos, commentEnd);
277+
return textSubStr.match(fullTripleSlashReferencePathRegEx) ||
278+
textSubStr.match(fullTripleSlashAMDReferencePathRegEx) ||
279+
textSubStr.match(fullTripleSlashReferenceTypeReferenceDirectiveRegEx) ||
280+
textSubStr.match(defaultLibReferenceRegEx) ?
281+
true : false;
282+
}
283+
return false;
284+
}
285+
286+
export function isPinnedComment(text: string, comment: CommentRange) {
287+
return text.charCodeAt(comment.pos + 1) === CharacterCodes.asterisk &&
288+
text.charCodeAt(comment.pos + 2) === CharacterCodes.exclamation;
289+
}
290+
265291
export function getTokenPosOfNode(node: Node, sourceFile?: SourceFileLike, includeJsDoc?: boolean): number {
266292
// With nodes that have no width (i.e. 'Missing' nodes), we actually *don't*
267293
// want to skip trivia because this will launch us forward to the next token.
@@ -660,6 +686,7 @@ namespace ts {
660686
export let fullTripleSlashReferencePathRegEx = /^(\/\/\/\s*<reference\s+path\s*=\s*)('|")(.+?)\2.*?\/>/;
661687
export let fullTripleSlashReferenceTypeReferenceDirectiveRegEx = /^(\/\/\/\s*<reference\s+types\s*=\s*)('|")(.+?)\2.*?\/>/;
662688
export let fullTripleSlashAMDReferencePathRegEx = /^(\/\/\/\s*<amd-dependency\s+path\s*=\s*)('|")(.+?)\2.*?\/>/;
689+
export let defaultLibReferenceRegEx = /^(\/\/\/\s*<reference\s+no-default-lib\s*=\s*)('|")(.+?)\2\s*\/>/;
663690

664691
export function isPartOfTypeNode(node: Node): boolean {
665692
if (SyntaxKind.FirstTypeNode <= node.kind && node.kind <= SyntaxKind.LastTypeNode) {
@@ -1846,7 +1873,7 @@ namespace ts {
18461873

18471874
export function getFileReferenceFromReferencePath(comment: string, commentRange: CommentRange): ReferencePathMatchResult {
18481875
const simpleReferenceRegEx = /^\/\/\/\s*<reference\s+/gim;
1849-
const isNoDefaultLibRegEx = /^(\/\/\/\s*<reference\s+no-default-lib\s*=\s*)('|")(.+?)\2\s*\/>/gim;
1876+
const isNoDefaultLibRegEx = new RegExp(defaultLibReferenceRegEx.source, "gim");
18501877
if (simpleReferenceRegEx.test(comment)) {
18511878
if (isNoDefaultLibRegEx.test(comment)) {
18521879
return { isNoDefaultLib: true };
@@ -2823,7 +2850,7 @@ namespace ts {
28232850
//
28242851
// var x = 10;
28252852
if (node.pos === 0) {
2826-
leadingComments = filter(getLeadingCommentRanges(text, node.pos), isPinnedComment);
2853+
leadingComments = filter(getLeadingCommentRanges(text, node.pos), isPinnedCommentLocal);
28272854
}
28282855
}
28292856
else {
@@ -2869,9 +2896,8 @@ namespace ts {
28692896

28702897
return currentDetachedCommentInfo;
28712898

2872-
function isPinnedComment(comment: CommentRange) {
2873-
return text.charCodeAt(comment.pos + 1) === CharacterCodes.asterisk &&
2874-
text.charCodeAt(comment.pos + 2) === CharacterCodes.exclamation;
2899+
function isPinnedCommentLocal(comment: CommentRange) {
2900+
return isPinnedComment(text, comment);
28752901
}
28762902

28772903
}

‎src/services/codefixes/importFixes.ts‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -396,7 +396,7 @@ namespace ts.codefix {
396396
: createImportClause(/*name*/ undefined, createNamedImports([createImportSpecifier(/*propertyName*/ undefined, createIdentifier(symbolName))]));
397397
const importDecl = createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, importClause, createLiteral(moduleSpecifierWithoutQuotes));
398398
if (!lastImportDeclaration) {
399-
changeTracker.insertNodeAt(sourceFile, sourceFile.getStart(), importDecl, { suffix: `${context.newLineCharacter}${context.newLineCharacter}` });
399+
changeTracker.insertNodeAt(sourceFile, getSourceFileImportLocation(sourceFile), importDecl, { suffix: `${context.newLineCharacter}${context.newLineCharacter}` });
400400
}
401401
else {
402402
changeTracker.insertNodeAfter(sourceFile, lastImportDeclaration, importDecl, { suffix: context.newLineCharacter });
@@ -413,6 +413,28 @@ namespace ts.codefix {
413413
moduleSpecifierWithoutQuotes
414414
);
415415

416+
function getSourceFileImportLocation(node: SourceFile) {
417+
// For a source file, it is possible there are detached comments we should not skip
418+
const text = node.text;
419+
let ranges = getLeadingCommentRanges(text, 0);
420+
if (!ranges) return 0;
421+
let position = 0;
422+
// However we should still skip a pinned comment at the top
423+
if (ranges.length && ranges[0].kind === SyntaxKind.MultiLineCommentTrivia && isPinnedComment(text, ranges[0])) {
424+
position = ranges[0].end + 1;
425+
ranges = ranges.slice(1);
426+
}
427+
// As well as any triple slash references
428+
for (const range of ranges) {
429+
if (range.kind === SyntaxKind.SingleLineCommentTrivia && isRecognizedTripleSlashComment(node.text, range.pos, range.end)) {
430+
position = range.end + 1;
431+
continue;
432+
}
433+
break;
434+
}
435+
return position;
436+
}
437+
416438
function getModuleSpecifierForNewImport() {
417439
const fileName = sourceFile.fileName;
418440
const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().fileName;

‎tests/baselines/reference/1.0lib-noErrors.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1158,3 +1158,4 @@ MERCHANTABLITY OR NON-INFRINGEMENT.
11581158
See the Apache Version 2.0 License for specific language governing permissions
11591159
and limitations under the License.
11601160
***************************************************************************** */
1161+
/// <reference no-default-lib="true"/>

‎tests/baselines/reference/typeReferenceDirectives1.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ interface A {
1010
}
1111

1212
//// [app.js]
13+
/// <reference types="lib"/>
1314

1415

1516
//// [app.d.ts]

‎tests/baselines/reference/typeReferenceDirectives3.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ interface A {
1616
}
1717

1818
//// [app.js]
19+
/// <reference types="lib"/>
1920
/// <reference path="ref.d.ts" />
2021

2122

‎tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/// <reference path="fourslash.ts" />
22

3-
////[|/*
3+
////[|/*!
44
//// * I'm a license or something
55
//// */
66
////f1/*0*/();|]
@@ -12,7 +12,7 @@
1212
//// }
1313

1414
verify.importFixAtPosition([
15-
`/*
15+
`/*!
1616
* I'm a license or something
1717
*/
1818
import { f1 } from "ambient-module";

‎tests/cases/fourslash/importNameCodeFixNewImportFileAllComments.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
//// [|/*!
4+
//// * This is a license or something
5+
//// */
6+
//// /// <reference types="node" />
7+
//// /// <reference path="./a.ts" />
8+
//// /// <amd-dependency path="./b.ts" />
9+
//// /**
10+
//// * This is a comment intended to be attached to this interface
11+
//// */
12+
//// export interface SomeInterface {
13+
//// }
14+
//// f1/*0*/();|]
15+
16+
// @Filename: module.ts
17+
//// export function f1() {}
18+
//// export var v1 = 5;
19+
20+
verify.importFixAtPosition([
21+
`/*!
22+
* This is a license or something
23+
*/
24+
/// <reference types="node" />
25+
/// <reference path="./a.ts" />
26+
/// <amd-dependency path="./b.ts" />
27+
import { f1 } from "./module";
28+
29+
/**
30+
* This is a comment intended to be attached to this interface
31+
*/
32+
export interface SomeInterface {
33+
}
34+
f1();`
35+
]);

‎tests/cases/fourslash/importNameCodeFixNewImportFileDetachedComments.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
//// [|/**
4+
//// * This is a comment intended to be attached to this interface
5+
//// */
6+
//// export interface SomeInterface {
7+
//// }
8+
//// f1/*0*/();|]
9+
10+
// @Filename: module.ts
11+
//// export function f1() {}
12+
//// export var v1 = 5;
13+
14+
verify.importFixAtPosition([
15+
`import { f1 } from "./module";
16+
17+
/**
18+
* This is a comment intended to be attached to this interface
19+
*/
20+
export interface SomeInterface {
21+
}
22+
f1();`
23+
]);

0 commit comments

Comments
 (0)