Skip to content

Commit 2df7058

Browse files
amcaseyamcasey
authored
Merge pull request microsoft#18861 from amcasey/ConstantInsertionPosition
Improve insertion positions of extracted constants
2 parents 884c72e + 724f71c commit 2df7058

61 files changed

Lines changed: 1081 additions & 103 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎src/compiler/diagnosticMessages.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3724,7 +3724,7 @@
37243724
"code": 95003
37253725
},
37263726

3727-
"Extract to {0}": {
3727+
"Extract to {0} in {1}": {
37283728
"category": "Message",
37293729
"code": 95004
37303730
},

‎src/harness/unittests/extractConstants.ts‎

Lines changed: 132 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ namespace ts {
4040
}
4141
}`);
4242

43-
testExtractConstant("extractConstant_ClassInsertionPosition",
43+
testExtractConstant("extractConstant_ClassInsertionPosition1",
4444
`class C {
4545
a = 1;
4646
b = 2;
@@ -51,6 +51,28 @@ namespace ts {
5151
}
5252
}`);
5353

54+
testExtractConstant("extractConstant_ClassInsertionPosition2",
55+
`class C {
56+
a = 1;
57+
M1() { }
58+
b = 2;
59+
M2() { }
60+
M3() {
61+
let x = [#|1|];
62+
}
63+
}`);
64+
65+
testExtractConstant("extractConstant_ClassInsertionPosition3",
66+
`class C {
67+
M1() { }
68+
a = 1;
69+
b = 2;
70+
M2() { }
71+
M3() {
72+
let x = [#|1|];
73+
}
74+
}`);
75+
5476
testExtractConstant("extractConstant_Parameters",
5577
`function F() {
5678
let w = 1;
@@ -62,26 +84,126 @@ namespace ts {
6284
let x = [#|t + 1|];
6385
}`);
6486

65-
// TODO (acasey): handle repeated substitution
87+
// TODO (18857): handle repeated substitution
6688
// testExtractConstant("extractConstant_RepeatedSubstitution",
6789
// `namespace X {
6890
// export const j = 10;
6991
// export const y = [#|j * j|];
7092
// }`);
7193

72-
testExtractConstantFailed("extractConstant_BlockScopes_Dependencies",
73-
`for (let i = 0; i < 10; i++) {
94+
testExtractConstant("extractConstant_VariableList_const",
95+
`const a = 1, b = [#|a + 1|];`);
96+
97+
// NOTE: this test isn't normative - it just documents our sub-optimal behavior.
98+
testExtractConstant("extractConstant_VariableList_let",
99+
`let a = 1, b = [#|a + 1|];`);
100+
101+
// NOTE: this test isn't normative - it just documents our sub-optimal behavior.
102+
testExtractConstant("extractConstant_VariableList_MultipleLines",
103+
`const /*About A*/a = 1,
104+
/*About B*/b = [#|a + 1|];`);
105+
106+
testExtractConstant("extractConstant_BlockScopeMismatch", `
107+
for (let i = 0; i < 10; i++) {
74108
for (let j = 0; j < 10; j++) {
75-
let x = [#|i + 1|];
109+
const x = [#|i + 1|];
76110
}
77-
}`);
111+
}
112+
`);
113+
114+
testExtractConstant("extractConstant_StatementInsertionPosition1", `
115+
const i = 0;
116+
for (let j = 0; j < 10; j++) {
117+
const x = [#|i + 1|];
118+
}
119+
`);
120+
121+
testExtractConstant("extractConstant_StatementInsertionPosition2", `
122+
const i = 0;
123+
function F() {
124+
for (let j = 0; j < 10; j++) {
125+
const x = [#|i + 1|];
126+
}
127+
}
128+
`);
129+
130+
testExtractConstant("extractConstant_StatementInsertionPosition3", `
131+
for (let j = 0; j < 10; j++) {
132+
const x = [#|2 + 1|];
133+
}
134+
`);
135+
136+
testExtractConstant("extractConstant_StatementInsertionPosition4", `
137+
function F() {
138+
for (let j = 0; j < 10; j++) {
139+
const x = [#|2 + 1|];
140+
}
141+
}
142+
`);
143+
144+
testExtractConstant("extractConstant_StatementInsertionPosition5", `
145+
function F0() {
146+
function F1() {
147+
function F2(x = [#|2 + 1|]) {
148+
}
149+
}
150+
}
151+
`);
152+
153+
testExtractConstant("extractConstant_StatementInsertionPosition6", `
154+
class C {
155+
x = [#|2 + 1|];
156+
}
157+
`);
158+
159+
testExtractConstant("extractConstant_StatementInsertionPosition7", `
160+
const i = 0;
161+
class C {
162+
M() {
163+
for (let j = 0; j < 10; j++) {
164+
x = [#|i + 1|];
165+
}
166+
}
167+
}
168+
`);
169+
170+
testExtractConstant("extractConstant_TripleSlash", `
171+
/// <reference path="path.js"/>
172+
173+
const x = [#|2 + 1|];
174+
`);
175+
176+
testExtractConstant("extractConstant_PinnedComment", `
177+
/*! Copyright */
178+
179+
const x = [#|2 + 1|];
180+
`);
181+
182+
testExtractConstant("extractConstant_Directive", `
183+
"strict";
184+
185+
const x = [#|2 + 1|];
186+
`);
187+
188+
testExtractConstant("extractConstant_MultipleHeaders", `
189+
/*! Copyright */
190+
191+
/// <reference path="path.js"/>
192+
193+
"strict";
194+
195+
const x = [#|2 + 1|];
196+
`);
197+
198+
testExtractConstant("extractConstant_PinnedCommentAndDocComment", `
199+
/*! Copyright */
200+
201+
/* About x */
202+
const x = [#|2 + 1|];
203+
`);
78204
});
79205

80206
function testExtractConstant(caption: string, text: string) {
81207
testExtractSymbol(caption, text, "extractConstant", Diagnostics.Extract_constant);
82208
}
83-
84-
function testExtractConstantFailed(caption: string, text: string) {
85-
testExtractSymbolFailed(caption, text, Diagnostics.Extract_constant);
86-
}
87209
}

‎src/harness/unittests/extractFunctions.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,7 @@ function parsePrimaryExpression(): any {
355355
[#|function G() { }|]
356356
}`);
357357

358-
// TODO (acasey): handle repeated substitution
358+
// TODO (18857): handle repeated substitution
359359
// testExtractFunction("extractFunction_RepeatedSubstitution",
360360
// `namespace X {
361361
// export const j = 10;

‎src/harness/unittests/extractTestHelpers.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ namespace ts {
113113

114114
if (hasSyntacticDiagnostics(program)) {
115115
// Don't bother generating JS baselines for inputs that aren't valid JS.
116-
assert.equal(Extension.Js, extension);
116+
assert.equal(Extension.Js, extension, "Syntactic diagnostics found in non-JS file");
117117
return;
118118
}
119119

‎src/services/codefixes/importFixes.ts‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -407,28 +407,6 @@ namespace ts.codefix {
407407
moduleSpecifierWithoutQuotes
408408
);
409409

410-
function getSourceFileImportLocation(node: SourceFile) {
411-
// For a source file, it is possible there are detached comments we should not skip
412-
const text = node.text;
413-
let ranges = getLeadingCommentRanges(text, 0);
414-
if (!ranges) return 0;
415-
let position = 0;
416-
// However we should still skip a pinned comment at the top
417-
if (ranges.length && ranges[0].kind === SyntaxKind.MultiLineCommentTrivia && isPinnedComment(text, ranges[0])) {
418-
position = ranges[0].end + 1;
419-
ranges = ranges.slice(1);
420-
}
421-
// As well as any triple slash references
422-
for (const range of ranges) {
423-
if (range.kind === SyntaxKind.SingleLineCommentTrivia && isRecognizedTripleSlashComment(node.text, range.pos, range.end)) {
424-
position = range.end + 1;
425-
continue;
426-
}
427-
break;
428-
}
429-
return position;
430-
}
431-
432410
function getSingleQuoteStyleFromExistingImports() {
433411
const firstModuleSpecifier = forEach(sourceFile.statements, node => {
434412
if (isImportDeclaration(node) || isExportDeclaration(node)) {

0 commit comments

Comments
 (0)