Loosen restriction on usages in innermost scope

amcasey · amcasey · commit bcc93f2c0865 · 2017-09-29T18:00:27.000-07:00
Now that we're smarter about where we declare the extracted local, we
can ignore usage problems in the innermost script - it'll always have
access to the same symbols as the extracted expression.
diff --git a/src/harness/unittests/extractConstants.ts b/src/harness/unittests/extractConstants.ts
@@ -91,13 +91,6 @@ namespace ts {
 //     export const y = [#|j * j|];
 // }`);
 
-        testExtractConstantFailed("extractConstant_BlockScopes_Dependencies",
-            `for (let i = 0; i < 10; i++) {
-    for (let j = 0; j < 10; j++) {
-        let x = [#|i + 1|];
-    }
-}`);
-
         testExtractConstant("extractConstant_VariableList_const",
             `const a = 1, b = [#|a + 1|];`);
 
@@ -110,10 +103,7 @@ namespace ts {
             `const /*About A*/a = 1,
     /*About B*/b = [#|a + 1|];`);
 
-        // NOTE: this test isn't normative - it just documents our sub-optimal behavior.
-        // `i` doesn't bind in the target scope (file-level), so the extraction is disallowed.
-        // TODO (17098): should probably allow extraction into the same scope
-        testExtractConstantFailed("extractConstant_BlockScopeMismatch", `
+        testExtractConstant("extractConstant_BlockScopeMismatch", `
 for (let i = 0; i < 10; i++) {
     for (let j = 0; j < 10; j++) {
         const x = [#|i + 1|];
@@ -205,7 +195,6 @@ const x = [#|2 + 1|];
 const x = [#|2 + 1|];
         `);
 
-        // NOTE: this test isn't normative - it just documents our sub-optimal behavior.
         testExtractConstant("extractConstant_PinnedCommentAndDocComment", `
 /*! Copyright */
 
@@ -217,8 +206,4 @@ const x = [#|2 + 1|];
     function testExtractConstant(caption: string, text: string) {
         testExtractSymbol(caption, text, "extractConstant", Diagnostics.Extract_constant);
     }
-
-    function testExtractConstantFailed(caption: string, text: string) {
-        testExtractSymbolFailed(caption, text, Diagnostics.Extract_constant);
-    }
 }
diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts
@@ -1349,7 +1349,10 @@ namespace ts.refactor.extractSymbol {
         for (let i = 0; i < scopes.length; i++) {
             if (!isReadonlyArray(targetRange.range)) {
                 const scopeUsages = usagesPerScope[i];
-                if (scopeUsages.usages.size > 0 || scopeUsages.typeParameterUsages.size > 0) {
+                // Special case: in the innermost scope, all usages are available.
+                // (The computed value reflects the value at the top-level of the scope, but the
+                // local will actually be declared at the same level as the extracted expression).
+                if (i > 0 && (scopeUsages.usages.size > 0 || scopeUsages.typeParameterUsages.size > 0)) {
                     constantErrorsPerScope[i].push(createDiagnosticForNode(targetRange.range, Messages.CannotAccessVariablesFromNestedScopes));
                 }
             }
diff --git a/tests/baselines/reference/extractConstant/extractConstant_BlockScopeMismatch.js b/tests/baselines/reference/extractConstant/extractConstant_BlockScopeMismatch.js
@@ -0,0 +1,18 @@
+// ==ORIGINAL==
+
+for (let i = 0; i < 10; i++) {
+    for (let j = 0; j < 10; j++) {
+        const x = i + 1;
+    }
+}
+        
+// ==SCOPE::Extract to constant in global scope==
+
+for (let i = 0; i < 10; i++) {
+    for (let j = 0; j < 10; j++) {
+        const newLocal = i + 1;
+
+        const x = /*RENAME*/newLocal;
+    }
+}
+        
diff --git a/tests/baselines/reference/extractConstant/extractConstant_BlockScopeMismatch.ts b/tests/baselines/reference/extractConstant/extractConstant_BlockScopeMismatch.ts
@@ -0,0 +1,18 @@
+// ==ORIGINAL==
+
+for (let i = 0; i < 10; i++) {
+    for (let j = 0; j < 10; j++) {
+        const x = i + 1;
+    }
+}
+        
+// ==SCOPE::Extract to constant in global scope==
+
+for (let i = 0; i < 10; i++) {
+    for (let j = 0; j < 10; j++) {
+        const newLocal = i + 1;
+
+        const x = /*RENAME*/newLocal;
+    }
+}
+        

Original file line number	Diff line number	Diff line change
`@@ -1349,7 +1349,10 @@ namespace ts.refactor.extractSymbol {`
`1349`	`1349`	`for (let i = 0; i < scopes.length; i++) {`
`1350`	`1350`	`if (!isReadonlyArray(targetRange.range)) {`
`1351`	`1351`	`const scopeUsages = usagesPerScope[i];`
`1352`		`- if (scopeUsages.usages.size > 0 \|\| scopeUsages.typeParameterUsages.size > 0) {`
	`1352`	`+ // Special case: in the innermost scope, all usages are available.`
	`1353`	`+ // (The computed value reflects the value at the top-level of the scope, but the`
	`1354`	`+ // local will actually be declared at the same level as the extracted expression).`
	`1355`	`+ if (i > 0 && (scopeUsages.usages.size > 0 \|\| scopeUsages.typeParameterUsages.size > 0)) {`
`1353`	`1356`	`constantErrorsPerScope[i].push(createDiagnosticForNode(targetRange.range, Messages.CannotAccessVariablesFromNestedScopes));`
`1354`	`1357`	`}`
`1355`	`1358`	`}`