Skip to content

Commit 2601bbc

Browse files
committed
Add simple tests for Extract Constant
1 parent eb1fb5c commit 2601bbc

14 files changed

+422
-13
lines changed

Jakefile.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ var harnessSources = harnessCoreSources.concat([
138138
"projectErrors.ts",
139139
"matchFiles.ts",
140140
"initializeTSConfig.ts",
141+
"extractConstants.ts",
141142
"extractMethods.ts",
142143
"printer.ts",
143144
"textChanges.ts",

src/harness/tsconfig.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@
128128
"./unittests/printer.ts",
129129
"./unittests/transform.ts",
130130
"./unittests/customTransforms.ts",
131+
"./unittests/extractConstants.ts",
131132
"./unittests/extractMethods.ts",
132133
"./unittests/textChanges.ts",
133134
"./unittests/telemetry.ts",
Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
/// <reference path="..\harness.ts" />
2+
/// <reference path="tsserverProjectSystem.ts" />
3+
4+
namespace ts {
5+
interface Range {
6+
start: number;
7+
end: number;
8+
name: string;
9+
}
10+
11+
interface Test {
12+
source: string;
13+
ranges: Map<Range>;
14+
}
15+
16+
// TODO (acasey): share
17+
function extractTest(source: string): Test {
18+
const activeRanges: Range[] = [];
19+
let text = "";
20+
let lastPos = 0;
21+
let pos = 0;
22+
const ranges = createMap<Range>();
23+
24+
while (pos < source.length) {
25+
if (source.charCodeAt(pos) === CharacterCodes.openBracket &&
26+
(source.charCodeAt(pos + 1) === CharacterCodes.hash || source.charCodeAt(pos + 1) === CharacterCodes.$)) {
27+
const saved = pos;
28+
pos += 2;
29+
const s = pos;
30+
consumeIdentifier();
31+
const e = pos;
32+
if (source.charCodeAt(pos) === CharacterCodes.bar) {
33+
pos++;
34+
text += source.substring(lastPos, saved);
35+
const name = s === e
36+
? source.charCodeAt(saved + 1) === CharacterCodes.hash ? "selection" : "extracted"
37+
: source.substring(s, e);
38+
activeRanges.push({ name, start: text.length, end: undefined });
39+
lastPos = pos;
40+
continue;
41+
}
42+
else {
43+
pos = saved;
44+
}
45+
}
46+
else if (source.charCodeAt(pos) === CharacterCodes.bar && source.charCodeAt(pos + 1) === CharacterCodes.closeBracket) {
47+
text += source.substring(lastPos, pos);
48+
activeRanges[activeRanges.length - 1].end = text.length;
49+
const range = activeRanges.pop();
50+
if (range.name in ranges) {
51+
throw new Error(`Duplicate name of range ${range.name}`);
52+
}
53+
ranges.set(range.name, range);
54+
pos += 2;
55+
lastPos = pos;
56+
continue;
57+
}
58+
pos++;
59+
}
60+
text += source.substring(lastPos, pos);
61+
62+
function consumeIdentifier() {
63+
while (isIdentifierPart(source.charCodeAt(pos), ScriptTarget.Latest)) {
64+
pos++;
65+
}
66+
}
67+
return { source: text, ranges };
68+
}
69+
70+
// TODO (acasey): share
71+
const newLineCharacter = "\n";
72+
function getRuleProvider(action?: (opts: FormatCodeSettings) => void) {
73+
const options = {
74+
indentSize: 4,
75+
tabSize: 4,
76+
newLineCharacter,
77+
convertTabsToSpaces: true,
78+
indentStyle: ts.IndentStyle.Smart,
79+
insertSpaceAfterConstructor: false,
80+
insertSpaceAfterCommaDelimiter: true,
81+
insertSpaceAfterSemicolonInForStatements: true,
82+
insertSpaceBeforeAndAfterBinaryOperators: true,
83+
insertSpaceAfterKeywordsInControlFlowStatements: true,
84+
insertSpaceAfterFunctionKeywordForAnonymousFunctions: false,
85+
insertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis: false,
86+
insertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets: false,
87+
insertSpaceAfterOpeningAndBeforeClosingNonemptyBraces: true,
88+
insertSpaceAfterOpeningAndBeforeClosingTemplateStringBraces: false,
89+
insertSpaceAfterOpeningAndBeforeClosingJsxExpressionBraces: false,
90+
insertSpaceBeforeFunctionParenthesis: false,
91+
placeOpenBraceOnNewLineForFunctions: false,
92+
placeOpenBraceOnNewLineForControlBlocks: false,
93+
};
94+
if (action) {
95+
action(options);
96+
}
97+
const rulesProvider = new formatting.RulesProvider();
98+
rulesProvider.ensureUpToDate(options);
99+
return rulesProvider;
100+
}
101+
102+
describe("extractConstants", () => {
103+
testExtractConstant("extractConstant_TopLevel",
104+
`let x = [#|1|];`);
105+
106+
testExtractConstant("extractConstant_Namespace",
107+
`namespace N {
108+
let x = [#|1|];
109+
}`);
110+
111+
testExtractConstant("extractConstant_Class",
112+
`class C {
113+
x = [#|1|];
114+
}`);
115+
116+
testExtractConstant("extractConstant_Method",
117+
`class C {
118+
M() {
119+
let x = [#|1|];
120+
}
121+
}`);
122+
123+
testExtractConstant("extractConstant_Function",
124+
`function F() {
125+
let x = [#|1|];
126+
}`);
127+
128+
testExtractConstant("extractConstant_ExpressionStatement",
129+
`[#|"hello";|]`);
130+
131+
testExtractConstant("extractConstant_ExpressionStatementExpression",
132+
`[#|"hello"|];`);
133+
134+
testExtractConstant("extractConstant_BlockScopes_NoDependencies",
135+
`for (let i = 0; i < 10; i++) {
136+
for (let j = 0; j < 10; j++) {
137+
let x = [#|1|];
138+
}
139+
}`);
140+
141+
testExtractConstant("extractConstant_ClassInsertionPosition",
142+
`class C {
143+
a = 1;
144+
b = 2;
145+
M1() { }
146+
M2() { }
147+
M3() {
148+
let x = [#|1|];
149+
}
150+
}`);
151+
152+
testExtractConstantFailed("extractConstant_Parameters",
153+
`function F() {
154+
let w = 1;
155+
let x = [#|w + 1|];
156+
}`);
157+
158+
testExtractConstantFailed("extractConstant_TypeParameters",
159+
`function F<T>(t: T) {
160+
let x = [#|t + 1|];
161+
}`);
162+
163+
testExtractConstantFailed("extractConstant_BlockScopes_Dependencies",
164+
`for (let i = 0; i < 10; i++) {
165+
for (let j = 0; j < 10; j++) {
166+
let x = [#|i + 1|];
167+
}
168+
}`);
169+
});
170+
171+
// TODO (acasey): share?
172+
function testExtractConstant(caption: string, text: string) {
173+
it(caption, () => {
174+
Harness.Baseline.runBaseline(`extractConstant/${caption}.ts`, () => {
175+
const t = extractTest(text);
176+
const selectionRange = t.ranges.get("selection");
177+
if (!selectionRange) {
178+
throw new Error(`Test ${caption} does not specify selection range`);
179+
}
180+
const f = {
181+
path: "/a.ts",
182+
content: t.source
183+
};
184+
const host = projectSystem.createServerHost([f, projectSystem.libFile]);
185+
const projectService = projectSystem.createProjectService(host);
186+
projectService.openClientFile(f.path);
187+
const program = projectService.inferredProjects[0].getLanguageService().getProgram();
188+
const sourceFile = program.getSourceFile(f.path);
189+
const context: RefactorContext = {
190+
cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } },
191+
newLineCharacter,
192+
program,
193+
file: sourceFile,
194+
startPosition: selectionRange.start,
195+
endPosition: selectionRange.end,
196+
rulesProvider: getRuleProvider()
197+
};
198+
const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end));
199+
assert.equal(rangeToExtract.errors, undefined, rangeToExtract.errors && "Range error: " + rangeToExtract.errors[0].messageText);
200+
const infos = refactor.extractSymbol.getAvailableActions(context);
201+
const actions = find(infos, info => info.description === Diagnostics.Extract_constant.message).actions;
202+
const data: string[] = [];
203+
data.push(`// ==ORIGINAL==`);
204+
data.push(sourceFile.text);
205+
for (const action of actions) {
206+
const { renameLocation, edits } = refactor.extractSymbol.getEditsForAction(context, action.name);
207+
assert.lengthOf(edits, 1);
208+
data.push(`// ==SCOPE::${action.description}==`);
209+
const newText = textChanges.applyChanges(sourceFile.text, edits[0].textChanges);
210+
const newTextWithRename = newText.slice(0, renameLocation) + "/*RENAME*/" + newText.slice(renameLocation);
211+
data.push(newTextWithRename);
212+
}
213+
return data.join(newLineCharacter);
214+
});
215+
});
216+
}
217+
218+
// TODO (acasey): share?
219+
function testExtractConstantFailed(caption: string, text: string) {
220+
it(caption, () => {
221+
const t = extractTest(text);
222+
const selectionRange = t.ranges.get("selection");
223+
if (!selectionRange) {
224+
throw new Error(`Test ${caption} does not specify selection range`);
225+
}
226+
const f = {
227+
path: "/a.ts",
228+
content: t.source
229+
};
230+
const host = projectSystem.createServerHost([f, projectSystem.libFile]);
231+
const projectService = projectSystem.createProjectService(host);
232+
projectService.openClientFile(f.path);
233+
const program = projectService.inferredProjects[0].getLanguageService().getProgram();
234+
const sourceFile = program.getSourceFile(f.path);
235+
const context: RefactorContext = {
236+
cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } },
237+
newLineCharacter,
238+
program,
239+
file: sourceFile,
240+
startPosition: selectionRange.start,
241+
endPosition: selectionRange.end,
242+
rulesProvider: getRuleProvider()
243+
};
244+
const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end));
245+
assert.isUndefined(rangeToExtract.errors, rangeToExtract.errors && "Range error: " + rangeToExtract.errors[0].messageText);
246+
const infos = refactor.extractSymbol.getAvailableActions(context);
247+
assert.isUndefined(find(infos, info => info.description === Diagnostics.Extract_constant.message));
248+
});
249+
}
250+
}

src/harness/unittests/extractMethods.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -805,7 +805,8 @@ function parsePrimaryExpression(): any {
805805
};
806806
const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end));
807807
assert.equal(rangeToExtract.errors, undefined, "expect no errors");
808-
const actions = refactor.extractSymbol.getAvailableActions(context)[0].actions; // TODO (acasey): smarter index
808+
const infos = refactor.extractSymbol.getAvailableActions(context);
809+
const actions = find(infos, info => info.description === Diagnostics.Extract_function.message).actions;
809810
const data: string[] = [];
810811
data.push(`// ==ORIGINAL==`);
811812
data.push(sourceFile.text);

src/services/refactors/extractSymbol.ts

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -262,10 +262,6 @@ namespace ts.refactor.extractSymbol {
262262
return { targetRange: { range: statements, facts: rangeFacts, declarations } };
263263
}
264264

265-
if (isExpressionStatement(start)) {
266-
start = start.expression;
267-
}
268-
269265
// We have a single node (start)
270266
const errors = checkRootNode(start) || checkNode(start);
271267
if (errors) {
@@ -274,7 +270,7 @@ namespace ts.refactor.extractSymbol {
274270
return { targetRange: { range: getStatementOrExpressionRange(start), facts: rangeFacts, declarations } };
275271

276272
function checkRootNode(node: Node): Diagnostic[] | undefined {
277-
if (isIdentifier(node)) {
273+
if (isIdentifier(isExpressionStatement(node) ? node.expression : node)) {
278274
return [createDiagnosticForNode(node, Messages.CannotExtractIdentifier)];
279275
}
280276
return undefined;
@@ -539,8 +535,10 @@ namespace ts.refactor.extractSymbol {
539535
const { scopes, readsAndWrites: { target, usagesPerScope, constantErrorsPerScope } } = getPossibleExtractionsWorker(targetRange, context);
540536
Debug.assert(!constantErrorsPerScope[requestedChangesIndex].length, "The extraction went missing? How?");
541537
context.cancellationToken.throwIfCancellationRequested();
542-
Debug.assert(target === targetRange.range);
543-
return extractConstantInScope(target as Expression, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange.facts, context);
538+
const expression = isExpression(target)
539+
? target
540+
: (target.statements[0] as ExpressionStatement).expression;
541+
return extractConstantInScope(expression, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange.facts, context);
544542
}
545543

546544
interface PossibleExtraction {
@@ -1114,18 +1112,24 @@ namespace ts.refactor.extractSymbol {
11141112
child.pos >= minPos && isFunctionLikeDeclaration(child) && !isConstructorDeclaration(child));
11151113
}
11161114

1117-
function getNodeToInsertConstantBefore(minPos: number, scope: Scope): Node {
1118-
const isClassLikeScope = isClassLike(scope);
1115+
// TODO (acasey): need to dig into nested statements
1116+
function getNodeToInsertConstantBefore(maxPos: number, scope: Scope): Node {
11191117
const children = getStatementsOrClassElements(scope);
1118+
Debug.assert(children.length > 0); // There must be at least one child, since we extracted from one.
1119+
1120+
const isClassLikeScope = isClassLike(scope);
11201121
let prevChild: Statement | ClassElement | undefined = undefined;
11211122
for (const child of children) {
1122-
if (child.pos >= minPos || (isClassLikeScope && !isPropertyDeclaration(child))) {
1123+
if (child.pos >= maxPos) {
11231124
break;
11241125
}
11251126
prevChild = child;
1127+
if (isClassLikeScope && !isPropertyDeclaration(child)) {
1128+
break;
1129+
}
11261130
}
11271131

1128-
return prevChild || children[0]; // There must be one - minPos is in one.
1132+
return prevChild;
11291133
}
11301134

11311135
function getPropertyAssignmentsForWrites(writes: ReadonlyArray<UsageEntry>): ShorthandPropertyAssignment[] {
@@ -1192,7 +1196,7 @@ namespace ts.refactor.extractSymbol {
11921196
const visibleDeclarationsInExtractedRange: Symbol[] = [];
11931197

11941198
const expressionDiagnostics =
1195-
isReadonlyArray(targetRange.range)
1199+
isReadonlyArray(targetRange.range) && !(targetRange.range.length === 1 && isExpressionStatement(targetRange.range[0]))
11961200
? ((start, end) => [createFileDiagnostic(sourceFile, start, end - start, Messages.ExpressionExpected)])(firstOrUndefined(targetRange.range).getStart(), lastOrUndefined(targetRange.range).end)
11971201
: [];
11981202

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// ==ORIGINAL==
2+
for (let i = 0; i < 10; i++) {
3+
for (let j = 0; j < 10; j++) {
4+
let x = 1;
5+
}
6+
}
7+
// ==SCOPE::Extract to constant in global scope==
8+
const newLocal = 1;
9+
10+
for (let i = 0; i < 10; i++) {
11+
for (let j = 0; j < 10; j++) {
12+
let x = /*RENAME*/newLocal;
13+
}
14+
}
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// ==ORIGINAL==
2+
class C {
3+
x = 1;
4+
}
5+
// ==SCOPE::Extract to readonly field in class 'C'==
6+
class C {
7+
private readonly newProperty = 1;
8+
9+
x = this./*RENAME*/newProperty;
10+
}
11+
// ==SCOPE::Extract to constant in global scope==
12+
const newLocal = 1;
13+
14+
class C {
15+
x = /*RENAME*/newLocal;
16+
}

0 commit comments

Comments
 (0)