Skip to content

Commit cb6037b

Browse files
committed
Forbid extraction of constants to class scopes in JS
1 parent 697bce7 commit cb6037b

File tree

4 files changed

+42
-14
lines changed

4 files changed

+42
-14
lines changed

src/harness/unittests/extractConstants.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,13 @@ namespace ts {
5151
}
5252
}`);
5353

54-
testExtractConstantFailed("extractConstant_Parameters",
54+
testExtractConstant("extractConstant_Parameters",
5555
`function F() {
5656
let w = 1;
5757
let x = [#|w + 1|];
5858
}`);
5959

60-
testExtractConstantFailed("extractConstant_TypeParameters",
60+
testExtractConstant("extractConstant_TypeParameters",
6161
`function F<T>(t: T) {
6262
let x = [#|t + 1|];
6363
}`);

src/services/refactors/extractSymbol.ts

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ namespace ts.refactor.extractSymbol {
141141
export const CannotExtractAmbientBlock = createMessage("Cannot extract code from ambient contexts");
142142
export const CannotAccessVariablesFromNestedScopes = createMessage("Cannot access variables from nested scopes");
143143
export const CannotExtractToOtherFunctionLike = createMessage("Cannot extract method to a function-like scope that is not a function");
144+
export const CannotExtractToJSClass = createMessage("Cannot extract constant to a class scope in JS");
144145
}
145146

146147
enum RangeFacts {
@@ -866,21 +867,17 @@ namespace ts.refactor.extractSymbol {
866867
const changeTracker = textChanges.ChangeTracker.fromContext(context);
867868

868869
if (isClassLike(scope)) {
869-
// always create private method in TypeScript files
870+
Debug.assert(!isJS); // See CannotExtractToJSClass
870871
const modifiers: Modifier[] = [];
871-
if (!isJS) {
872-
modifiers.push(createToken(SyntaxKind.PrivateKeyword));
873-
}
872+
modifiers.push(createToken(SyntaxKind.PrivateKeyword));
874873
if (rangeFacts & RangeFacts.InStaticRegion) {
875874
modifiers.push(createToken(SyntaxKind.StaticKeyword));
876875
}
877-
if (!isJS) {
878-
modifiers.push(createToken(SyntaxKind.ReadonlyKeyword));
879-
}
876+
modifiers.push(createToken(SyntaxKind.ReadonlyKeyword));
880877

881878
const newVariable = createProperty(
882879
/*decorators*/ undefined,
883-
modifiers.length ? modifiers : undefined,
880+
modifiers,
884881
localNameText,
885882
/*questionToken*/ undefined,
886883
variableType,
@@ -1195,20 +1192,29 @@ namespace ts.refactor.extractSymbol {
11951192
const constantErrorsPerScope: Diagnostic[][] = [];
11961193
const visibleDeclarationsInExtractedRange: Symbol[] = [];
11971194

1198-
const expressionDiagnostics =
1195+
const expressionDiagnostic =
11991196
isReadonlyArray(targetRange.range) && !(targetRange.range.length === 1 && isExpressionStatement(targetRange.range[0]))
1200-
? ((start, end) => [createFileDiagnostic(sourceFile, start, end - start, Messages.ExpressionExpected)])(firstOrUndefined(targetRange.range).getStart(), lastOrUndefined(targetRange.range).end)
1201-
: [];
1197+
? ((start, end) => createFileDiagnostic(sourceFile, start, end - start, Messages.ExpressionExpected))(firstOrUndefined(targetRange.range).getStart(), lastOrUndefined(targetRange.range).end)
1198+
: undefined;
12021199

12031200
// initialize results
12041201
for (const scope of scopes) {
12051202
usagesPerScope.push({ usages: createMap<UsageEntry>(), typeParameterUsages: createMap<TypeParameter>(), substitutions: createMap<Expression>() });
12061203
substitutionsPerScope.push(createMap<Expression>());
1204+
12071205
functionErrorsPerScope.push(
12081206
isFunctionLikeDeclaration(scope) && scope.kind !== SyntaxKind.FunctionDeclaration
12091207
? [createDiagnosticForNode(scope, Messages.CannotExtractToOtherFunctionLike)]
12101208
: []);
1211-
constantErrorsPerScope.push(expressionDiagnostics);
1209+
1210+
const constantErrors = [];
1211+
if (expressionDiagnostic) {
1212+
constantErrors.push(expressionDiagnostic);
1213+
}
1214+
if (isClassLike(scope) && isInJavaScriptFile(scope)) {
1215+
constantErrors.push(createDiagnosticForNode(scope, Messages.CannotExtractToJSClass));
1216+
}
1217+
constantErrorsPerScope.push(constantErrors);
12121218
}
12131219

12141220
const seenUsages = createMap<Usage>();
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// ==ORIGINAL==
2+
function F() {
3+
let w = 1;
4+
let x = w + 1;
5+
}
6+
// ==SCOPE::Extract to constant in function 'F'==
7+
function F() {
8+
let w = 1;
9+
const newLocal = w + 1;
10+
11+
let x = /*RENAME*/newLocal;
12+
}
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// ==ORIGINAL==
2+
function F<T>(t: T) {
3+
let x = t + 1;
4+
}
5+
// ==SCOPE::Extract to constant in function 'F'==
6+
function F<T>(t: T) {
7+
const newLocal = t + 1;
8+
9+
let x = /*RENAME*/newLocal;
10+
}

0 commit comments

Comments
 (0)