From cb6037b563e2908fd212ac5b7e82241c63762a48 Mon Sep 17 00:00:00 2001
From: Andrew Casey <andrew.casey@microsoft.com>
Date: Wed, 27 Sep 2017 10:24:52 -0700
Subject: [PATCH] Forbid extraction of constants to class scopes in JS

---
 src/harness/unittests/extractConstants.ts     |  4 +--
 src/services/refactors/extractSymbol.ts       | 30 +++++++++++--------
 .../extractConstant_Parameters.ts             | 12 ++++++++
 .../extractConstant_TypeParameters.ts         | 10 +++++++
 4 files changed, 42 insertions(+), 14 deletions(-)
 create mode 100644 tests/baselines/reference/extractConstant/extractConstant_Parameters.ts
 create mode 100644 tests/baselines/reference/extractConstant/extractConstant_TypeParameters.ts

diff --git a/src/harness/unittests/extractConstants.ts b/src/harness/unittests/extractConstants.ts
index fb776eb6af3..e4ec0932a73 100644
--- a/src/harness/unittests/extractConstants.ts
+++ b/src/harness/unittests/extractConstants.ts
@@ -51,13 +51,13 @@ namespace ts {
     }
 }`);
 
-        testExtractConstantFailed("extractConstant_Parameters",
+        testExtractConstant("extractConstant_Parameters",
             `function F() {
     let w = 1;
     let x = [#|w + 1|];
 }`);
 
-        testExtractConstantFailed("extractConstant_TypeParameters",
+        testExtractConstant("extractConstant_TypeParameters",
             `function F<T>(t: T) {
     let x = [#|t + 1|];
 }`);
diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts
index 0c3b3726312..2b7f9e0dcb8 100644
--- a/src/services/refactors/extractSymbol.ts
+++ b/src/services/refactors/extractSymbol.ts
@@ -141,6 +141,7 @@ namespace ts.refactor.extractSymbol {
         export const CannotExtractAmbientBlock = createMessage("Cannot extract code from ambient contexts");
         export const CannotAccessVariablesFromNestedScopes = createMessage("Cannot access variables from nested scopes");
         export const CannotExtractToOtherFunctionLike = createMessage("Cannot extract method to a function-like scope that is not a function");
+        export const CannotExtractToJSClass = createMessage("Cannot extract constant to a class scope in JS");
     }
 
     enum RangeFacts {
@@ -866,21 +867,17 @@ namespace ts.refactor.extractSymbol {
         const changeTracker = textChanges.ChangeTracker.fromContext(context);
 
         if (isClassLike(scope)) {
-            // always create private method in TypeScript files
+            Debug.assert(!isJS); // See CannotExtractToJSClass
             const modifiers: Modifier[] = [];
-            if (!isJS) {
-                modifiers.push(createToken(SyntaxKind.PrivateKeyword));
-            }
+            modifiers.push(createToken(SyntaxKind.PrivateKeyword));
             if (rangeFacts & RangeFacts.InStaticRegion) {
                 modifiers.push(createToken(SyntaxKind.StaticKeyword));
             }
-            if (!isJS) {
-                modifiers.push(createToken(SyntaxKind.ReadonlyKeyword));
-            }
+            modifiers.push(createToken(SyntaxKind.ReadonlyKeyword));
 
             const newVariable = createProperty(
                 /*decorators*/ undefined,
-                modifiers.length ? modifiers : undefined,
+                modifiers,
                 localNameText,
                 /*questionToken*/ undefined,
                 variableType,
@@ -1195,20 +1192,29 @@ namespace ts.refactor.extractSymbol {
         const constantErrorsPerScope: Diagnostic[][] = [];
         const visibleDeclarationsInExtractedRange: Symbol[] = [];
 
-        const expressionDiagnostics =
+        const expressionDiagnostic =
             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)
-                : [];
+                ? ((start, end) => createFileDiagnostic(sourceFile, start, end - start, Messages.ExpressionExpected))(firstOrUndefined(targetRange.range).getStart(), lastOrUndefined(targetRange.range).end)
+                : undefined;
 
         // initialize results
         for (const scope of scopes) {
             usagesPerScope.push({ usages: createMap<UsageEntry>(), typeParameterUsages: createMap<TypeParameter>(), substitutions: createMap<Expression>() });
             substitutionsPerScope.push(createMap<Expression>());
+
             functionErrorsPerScope.push(
                 isFunctionLikeDeclaration(scope) && scope.kind !== SyntaxKind.FunctionDeclaration
                     ? [createDiagnosticForNode(scope, Messages.CannotExtractToOtherFunctionLike)]
                     : []);
-            constantErrorsPerScope.push(expressionDiagnostics);
+
+            const constantErrors = [];
+            if (expressionDiagnostic) {
+                constantErrors.push(expressionDiagnostic);
+            }
+            if (isClassLike(scope) && isInJavaScriptFile(scope)) {
+                constantErrors.push(createDiagnosticForNode(scope, Messages.CannotExtractToJSClass));
+            }
+            constantErrorsPerScope.push(constantErrors);
         }
 
         const seenUsages = createMap<Usage>();
diff --git a/tests/baselines/reference/extractConstant/extractConstant_Parameters.ts b/tests/baselines/reference/extractConstant/extractConstant_Parameters.ts
new file mode 100644
index 00000000000..e6c9c474fbc
--- /dev/null
+++ b/tests/baselines/reference/extractConstant/extractConstant_Parameters.ts
@@ -0,0 +1,12 @@
+// ==ORIGINAL==
+function F() {
+    let w = 1;
+    let x = w + 1;
+}
+// ==SCOPE::Extract to constant in function 'F'==
+function F() {
+    let w = 1;
+    const newLocal = w + 1;
+
+    let x = /*RENAME*/newLocal;
+}
\ No newline at end of file
diff --git a/tests/baselines/reference/extractConstant/extractConstant_TypeParameters.ts b/tests/baselines/reference/extractConstant/extractConstant_TypeParameters.ts
new file mode 100644
index 00000000000..a323e988176
--- /dev/null
+++ b/tests/baselines/reference/extractConstant/extractConstant_TypeParameters.ts
@@ -0,0 +1,10 @@
+// ==ORIGINAL==
+function F<T>(t: T) {
+    let x = t + 1;
+}
+// ==SCOPE::Extract to constant in function 'F'==
+function F<T>(t: T) {
+    const newLocal = t + 1;
+
+    let x = /*RENAME*/newLocal;
+}
\ No newline at end of file