Add simple tests for Extract Constant

Jakefile.js

@@ -138,6 +138,7 @@ var harnessSources = harnessCoreSources.concat([
     "projectErrors.ts",
     "matchFiles.ts",
     "initializeTSConfig.ts",
+    "extractConstants.ts",
     "extractMethods.ts",
     "printer.ts",
     "textChanges.ts",

src/harness/tsconfig.json

@@ -128,6 +128,7 @@
         "./unittests/printer.ts",
         "./unittests/transform.ts",
         "./unittests/customTransforms.ts",
+        "./unittests/extractConstants.ts",
         "./unittests/extractMethods.ts",
         "./unittests/textChanges.ts",
         "./unittests/telemetry.ts",

src/harness/unittests/extractConstants.ts

@@ -0,0 +1,250 @@
+/// <reference path="..\harness.ts" />
+/// <reference path="tsserverProjectSystem.ts" />
+
+namespace ts {
+    interface Range {
+        start: number;
+        end: number;
+        name: string;
+    }
+
+    interface Test {
+        source: string;
+        ranges: Map<Range>;
+    }
+
+    // TODO (acasey): share
+    function extractTest(source: string): Test {
+        const activeRanges: Range[] = [];
+        let text = "";
+        let lastPos = 0;
+        let pos = 0;
+        const ranges = createMap<Range>();
+
+        while (pos < source.length) {
+            if (source.charCodeAt(pos) === CharacterCodes.openBracket &&
+                (source.charCodeAt(pos + 1) === CharacterCodes.hash || source.charCodeAt(pos + 1) === CharacterCodes.$)) {
+                const saved = pos;
+                pos += 2;
+                const s = pos;
+                consumeIdentifier();
+                const e = pos;
+                if (source.charCodeAt(pos) === CharacterCodes.bar) {
+                    pos++;
+                    text += source.substring(lastPos, saved);
+                    const name = s === e
+                        ? source.charCodeAt(saved + 1) === CharacterCodes.hash ? "selection" : "extracted"
+                        : source.substring(s, e);
+                    activeRanges.push({ name, start: text.length, end: undefined });
+                    lastPos = pos;
+                    continue;
+                }
+                else {
+                    pos = saved;
+                }
+            }
+            else if (source.charCodeAt(pos) === CharacterCodes.bar && source.charCodeAt(pos + 1) === CharacterCodes.closeBracket) {
+                text += source.substring(lastPos, pos);
+                activeRanges[activeRanges.length - 1].end = text.length;
+                const range = activeRanges.pop();
+                if (range.name in ranges) {
+                    throw new Error(`Duplicate name of range ${range.name}`);
+                }
+                ranges.set(range.name, range);
+                pos += 2;
+                lastPos = pos;
+                continue;
+            }
+            pos++;
+        }
+        text += source.substring(lastPos, pos);
+
+        function consumeIdentifier() {
+            while (isIdentifierPart(source.charCodeAt(pos), ScriptTarget.Latest)) {
+                pos++;
+            }
+        }
+        return { source: text, ranges };
+    }
+
+    // TODO (acasey): share
+    const newLineCharacter = "\n";
+    function getRuleProvider(action?: (opts: FormatCodeSettings) => void) {
+        const options = {
+            indentSize: 4,
+            tabSize: 4,
+            newLineCharacter,
+            convertTabsToSpaces: true,
+            indentStyle: ts.IndentStyle.Smart,
+            insertSpaceAfterConstructor: false,
+            insertSpaceAfterCommaDelimiter: true,
+            insertSpaceAfterSemicolonInForStatements: true,
+            insertSpaceBeforeAndAfterBinaryOperators: true,
+            insertSpaceAfterKeywordsInControlFlowStatements: true,
+            insertSpaceAfterFunctionKeywordForAnonymousFunctions: false,
+            insertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis: false,
+            insertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets: false,
+            insertSpaceAfterOpeningAndBeforeClosingNonemptyBraces: true,
+            insertSpaceAfterOpeningAndBeforeClosingTemplateStringBraces: false,
+            insertSpaceAfterOpeningAndBeforeClosingJsxExpressionBraces: false,
+            insertSpaceBeforeFunctionParenthesis: false,
+            placeOpenBraceOnNewLineForFunctions: false,
+            placeOpenBraceOnNewLineForControlBlocks: false,
+        };
+        if (action) {
+            action(options);
+        }
+        const rulesProvider = new formatting.RulesProvider();
+        rulesProvider.ensureUpToDate(options);
+        return rulesProvider;
+    }
+
+    describe("extractConstants", () => {
+        testExtractConstant("extractConstant_TopLevel",
+            `let x = [#|1|];`);
+
+        testExtractConstant("extractConstant_Namespace",
+            `namespace N {
+    let x = [#|1|];
+}`);
+
+        testExtractConstant("extractConstant_Class",
+            `class C {
+    x = [#|1|];
+}`);
+
+        testExtractConstant("extractConstant_Method",
+            `class C {
+    M() {
+        let x = [#|1|];
+    }
+}`);
+
+        testExtractConstant("extractConstant_Function",
+            `function F() {
+    let x = [#|1|];
+}`);
+
+        testExtractConstant("extractConstant_ExpressionStatement",
+            `[#|"hello";|]`);
+
+        testExtractConstant("extractConstant_ExpressionStatementExpression",
+            `[#|"hello"|];`);
+
+        testExtractConstant("extractConstant_BlockScopes_NoDependencies",
+            `for (let i = 0; i < 10; i++) {
+    for (let j = 0; j < 10; j++) {
+        let x = [#|1|];
+    }
+}`);
+
+        testExtractConstant("extractConstant_ClassInsertionPosition",
+            `class C {
+    a = 1;
+    b = 2;
+    M1() { }
+    M2() { }
+    M3() {
+        let x = [#|1|];
+    }
+}`);
+
+        testExtractConstantFailed("extractConstant_Parameters",
+            `function F() {
+    let w = 1;
+    let x = [#|w + 1|];
+}`);
+
+        testExtractConstantFailed("extractConstant_TypeParameters",
+            `function F<T>(t: T) {
+    let x = [#|t + 1|];
+}`);
+
+        testExtractConstantFailed("extractConstant_BlockScopes_Dependencies",
+            `for (let i = 0; i < 10; i++) {
+    for (let j = 0; j < 10; j++) {
+        let x = [#|i + 1|];
+    }
+}`);
+    });
+
+    // TODO (acasey): share?
+    function testExtractConstant(caption: string, text: string) {
+        it(caption, () => {
+            Harness.Baseline.runBaseline(`extractConstant/${caption}.ts`, () => {
+                const t = extractTest(text);
+                const selectionRange = t.ranges.get("selection");
+                if (!selectionRange) {
+                    throw new Error(`Test ${caption} does not specify selection range`);
+                }
+                const f = {
+                    path: "/a.ts",
+                    content: t.source
+                };
+                const host = projectSystem.createServerHost([f, projectSystem.libFile]);
+                const projectService = projectSystem.createProjectService(host);
+                projectService.openClientFile(f.path);
+                const program = projectService.inferredProjects[0].getLanguageService().getProgram();
+                const sourceFile = program.getSourceFile(f.path);
+                const context: RefactorContext = {
+                    cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } },
+                    newLineCharacter,
+                    program,
+                    file: sourceFile,
+                    startPosition: selectionRange.start,
+                    endPosition: selectionRange.end,
+                    rulesProvider: getRuleProvider()
+                };
+                const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end));
+                assert.equal(rangeToExtract.errors, undefined, rangeToExtract.errors && "Range error: " + rangeToExtract.errors[0].messageText);
+                const infos = refactor.extractSymbol.getAvailableActions(context);
+                const actions = find(infos, info => info.description === Diagnostics.Extract_constant.message).actions;
+                const data: string[] = [];
+                data.push(`// ==ORIGINAL==`);
+                data.push(sourceFile.text);
+                for (const action of actions) {
+                    const { renameLocation, edits } = refactor.extractSymbol.getEditsForAction(context, action.name);
+                    assert.lengthOf(edits, 1);
+                    data.push(`// ==SCOPE::${action.description}==`);
+                    const newText = textChanges.applyChanges(sourceFile.text, edits[0].textChanges);
+                    const newTextWithRename = newText.slice(0, renameLocation) + "/*RENAME*/" + newText.slice(renameLocation);
+                    data.push(newTextWithRename);
+                }
+                return data.join(newLineCharacter);
+            });
+        });
+    }
+
+    // TODO (acasey): share?
+    function testExtractConstantFailed(caption: string, text: string) {
+        it(caption, () => {
+            const t = extractTest(text);
+            const selectionRange = t.ranges.get("selection");
+            if (!selectionRange) {
+                throw new Error(`Test ${caption} does not specify selection range`);
+            }
+            const f = {
+                path: "/a.ts",
+                content: t.source
+            };
+            const host = projectSystem.createServerHost([f, projectSystem.libFile]);
+            const projectService = projectSystem.createProjectService(host);
+            projectService.openClientFile(f.path);
+            const program = projectService.inferredProjects[0].getLanguageService().getProgram();
+            const sourceFile = program.getSourceFile(f.path);
+            const context: RefactorContext = {
+                cancellationToken: { throwIfCancellationRequested() { }, isCancellationRequested() { return false; } },
+                newLineCharacter,
+                program,
+                file: sourceFile,
+                startPosition: selectionRange.start,
+                endPosition: selectionRange.end,
+                rulesProvider: getRuleProvider()
+            };
+            const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end));
+            assert.isUndefined(rangeToExtract.errors, rangeToExtract.errors && "Range error: " + rangeToExtract.errors[0].messageText);
+            const infos = refactor.extractSymbol.getAvailableActions(context);
+            assert.isUndefined(find(infos, info => info.description === Diagnostics.Extract_constant.message));
+        });
+    }
+}

src/harness/unittests/extractMethods.ts

@@ -805,7 +805,8 @@ function parsePrimaryExpression(): any {
                 };
                 const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromBounds(selectionRange.start, selectionRange.end));
                 assert.equal(rangeToExtract.errors, undefined, "expect no errors");
-                const actions = refactor.extractSymbol.getAvailableActions(context)[0].actions; // TODO (acasey): smarter index
+                const infos = refactor.extractSymbol.getAvailableActions(context);
+                const actions = find(infos, info => info.description === Diagnostics.Extract_function.message).actions;
                 const data: string[] = [];
                 data.push(`// ==ORIGINAL==`);
                 data.push(sourceFile.text);

src/services/refactors/extractSymbol.ts

@@ -262,10 +262,6 @@ namespace ts.refactor.extractSymbol {
             return { targetRange: { range: statements, facts: rangeFacts, declarations } };
         }
 
-        if (isExpressionStatement(start)) {
-            start = start.expression;
-        }
-
         // We have a single node (start)
         const errors = checkRootNode(start) || checkNode(start);
         if (errors) {
@@ -274,7 +270,7 @@ namespace ts.refactor.extractSymbol {
         return { targetRange: { range: getStatementOrExpressionRange(start), facts: rangeFacts, declarations } };
 
         function checkRootNode(node: Node): Diagnostic[] | undefined {
-            if (isIdentifier(node)) {
+            if (isIdentifier(isExpressionStatement(node) ? node.expression : node)) {
                 return [createDiagnosticForNode(node, Messages.CannotExtractIdentifier)];
             }
             return undefined;
@@ -539,8 +535,10 @@ namespace ts.refactor.extractSymbol {
         const { scopes, readsAndWrites: { target, usagesPerScope, constantErrorsPerScope } } = getPossibleExtractionsWorker(targetRange, context);
         Debug.assert(!constantErrorsPerScope[requestedChangesIndex].length, "The extraction went missing? How?");
         context.cancellationToken.throwIfCancellationRequested();
-        Debug.assert(target === targetRange.range);
-        return extractConstantInScope(target as Expression, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange.facts, context);
+        const expression = isExpression(target)
+            ? target
+            : (target.statements[0] as ExpressionStatement).expression;
+        return extractConstantInScope(expression, scopes[requestedChangesIndex], usagesPerScope[requestedChangesIndex], targetRange.facts, context);
     }
 
     interface PossibleExtraction {
@@ -1114,18 +1112,24 @@ namespace ts.refactor.extractSymbol {
             child.pos >= minPos && isFunctionLikeDeclaration(child) && !isConstructorDeclaration(child));
     }
 
-    function getNodeToInsertConstantBefore(minPos: number, scope: Scope): Node {
-        const isClassLikeScope = isClassLike(scope);
+    // TODO (acasey): need to dig into nested statements
+    function getNodeToInsertConstantBefore(maxPos: number, scope: Scope): Node {
         const children = getStatementsOrClassElements(scope);
+        Debug.assert(children.length > 0); // There must be at least one child, since we extracted from one.
+
+        const isClassLikeScope = isClassLike(scope);
         let prevChild: Statement | ClassElement | undefined = undefined;
         for (const child of children) {
-            if (child.pos >= minPos || (isClassLikeScope && !isPropertyDeclaration(child))) {
+            if (child.pos >= maxPos) {
                 break;
             }
             prevChild = child;
+            if (isClassLikeScope && !isPropertyDeclaration(child)) {
+                break;
+            }
         }
 
-        return prevChild || children[0]; // There must be one - minPos is in one.
+        return prevChild;
     }
 
     function getPropertyAssignmentsForWrites(writes: ReadonlyArray<UsageEntry>): ShorthandPropertyAssignment[] {
@@ -1192,7 +1196,7 @@ namespace ts.refactor.extractSymbol {
         const visibleDeclarationsInExtractedRange: Symbol[] = [];
 
         const expressionDiagnostics =
-            isReadonlyArray(targetRange.range)
+            isReadonlyArray(targetRange.range) && !(targetRange.range.length === 1 && isExpressionStatement(targetRange.range[0]))
                 ? ((start, end) => [createFileDiagnostic(sourceFile, start, end - start, Messages.ExpressionExpected)])(firstOrUndefined(targetRange.range).getStart(), lastOrUndefined(targetRange.range).end)
                 : [];
 

tests/baselines/reference/extractConstant/extractConstant_BlockScopes_NoDependencies.ts

@@ -0,0 +1,14 @@
+// ==ORIGINAL==
+for (let i = 0; i < 10; i++) {
+    for (let j = 0; j < 10; j++) {
+        let x = 1;
+    }
+}
+// ==SCOPE::Extract to constant in global scope==
+const newLocal = 1;
+
+for (let i = 0; i < 10; i++) {
+    for (let j = 0; j < 10; j++) {
+        let x = /*RENAME*/newLocal;
+    }
+}

tests/baselines/reference/extractConstant/extractConstant_Class.ts

@@ -0,0 +1,16 @@
+// ==ORIGINAL==
+class C {
+    x = 1;
+}
+// ==SCOPE::Extract to readonly field in class 'C'==
+class C {
+    private readonly newProperty = 1;
+
+    x = this./*RENAME*/newProperty;
+}
+// ==SCOPE::Extract to constant in global scope==
+const newLocal = 1;
+
+class C {
+    x = /*RENAME*/newLocal;
+}

tests/baselines/reference/extractConstant/extractConstant_ClassInsertionPosition.ts

@@ -0,0 +1,46 @@
+// ==ORIGINAL==
+class C {
+    a = 1;
+    b = 2;
+    M1() { }
+    M2() { }
+    M3() {
+        let x = 1;
+    }
+}
+// ==SCOPE::Extract to constant in method 'M3==
+class C {
+    a = 1;
+    b = 2;
+    M1() { }
+    M2() { }
+    M3() {
+        const newLocal = 1;
+
+        let x = /*RENAME*/newLocal;
+    }
+}
+// ==SCOPE::Extract to readonly field in class 'C'==
+class C {
+    a = 1;
+    b = 2;
+    private readonly newProperty = 1;
+
+    M1() { }
+    M2() { }
+    M3() {
+        let x = this./*RENAME*/newProperty;
+    }
+}
+// ==SCOPE::Extract to constant in global scope==
+const newLocal = 1;
+
+class C {
+    a = 1;
+    b = 2;
+    M1() { }
+    M2() { }
+    M3() {
+        let x = /*RENAME*/newLocal;
+    }
+}

tests/baselines/reference/extractConstant/extractConstant_ExpressionStatement.ts

@@ -0,0 +1,4 @@
+// ==ORIGINAL==
+"hello";
+// ==SCOPE::Extract to constant in global scope==
+const /*RENAME*/newLocal = "hello";

tests/baselines/reference/extractConstant/extractConstant_ExpressionStatementExpression.ts

@@ -0,0 +1,4 @@
+// ==ORIGINAL==
+"hello";
+// ==SCOPE::Extract to constant in global scope==
+const /*RENAME*/newLocal = "hello";

tests/baselines/reference/extractConstant/extractConstant_Function.ts

@@ -0,0 +1,16 @@
+// ==ORIGINAL==
+function F() {
+    let x = 1;
+}
+// ==SCOPE::Extract to constant in function 'F'==
+function F() {
+    const newLocal = 1;
+
+    let x = /*RENAME*/newLocal;
+}
+// ==SCOPE::Extract to constant in global scope==
+const newLocal = 1;
+
+function F() {
+    let x = /*RENAME*/newLocal;
+}

tests/baselines/reference/extractConstant/extractConstant_Method.ts

@@ -0,0 +1,30 @@
+// ==ORIGINAL==
+class C {
+    M() {
+        let x = 1;
+    }
+}
+// ==SCOPE::Extract to constant in method 'M==
+class C {
+    M() {
+        const newLocal = 1;
+
+        let x = /*RENAME*/newLocal;
+    }
+}
+// ==SCOPE::Extract to readonly field in class 'C'==
+class C {
+    private readonly newProperty = 1;
+
+    M() {
+        let x = this./*RENAME*/newProperty;
+    }
+}
+// ==SCOPE::Extract to constant in global scope==
+const newLocal = 1;
+
+class C {
+    M() {
+        let x = /*RENAME*/newLocal;
+    }
+}

tests/baselines/reference/extractConstant/extractConstant_Namespace.ts

@@ -0,0 +1,16 @@
+// ==ORIGINAL==
+namespace N {
+    let x = 1;
+}
+// ==SCOPE::Extract to constant in namespace 'N'==
+namespace N {
+    const newLocal = 1;
+
+    let x = /*RENAME*/newLocal;
+}
+// ==SCOPE::Extract to constant in global scope==
+const newLocal = 1;
+
+namespace N {
+    let x = /*RENAME*/newLocal;
+}

tests/baselines/reference/extractConstant/extractConstant_TopLevel.ts

@@ -0,0 +1,6 @@
+// ==ORIGINAL==
+let x = 1;
+// ==SCOPE::Extract to constant in global scope==
+const newLocal = 1;
+
+let x = /*RENAME*/newLocal;