Skip to content

Commit df1faa0

Browse files
andrewbranchandrewbranch
authored
Add isolatedModules error for ambiguous imports referenced in decorator metadata (microsoft#42915)
* Add isolatedModules error for ambiguous imports referenced in decorator metadata * Improve test and accept baselines * Error only for es2015+ * Add namespace import to error message as workaround * Add codefix * Fix merge fallout
1 parent b996287 commit df1faa0

20 files changed

+1175
-14
lines changed

‎src/compiler/checker.ts‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36430,21 +36430,32 @@ namespace ts {
3643036430
* marked as referenced to prevent import elision.
3643136431
*/
3643236432
function markTypeNodeAsReferenced(node: TypeNode) {
36433-
markEntityNameOrEntityExpressionAsReference(node && getEntityNameFromTypeNode(node));
36433+
markEntityNameOrEntityExpressionAsReference(node && getEntityNameFromTypeNode(node), /*forDecoratorMetadata*/ false);
3643436434
}
3643536435

36436-
function markEntityNameOrEntityExpressionAsReference(typeName: EntityNameOrEntityNameExpression | undefined) {
36436+
function markEntityNameOrEntityExpressionAsReference(typeName: EntityNameOrEntityNameExpression | undefined, forDecoratorMetadata: boolean) {
3643736437
if (!typeName) return;
3643836438

3643936439
const rootName = getFirstIdentifier(typeName);
3644036440
const meaning = (typeName.kind === SyntaxKind.Identifier ? SymbolFlags.Type : SymbolFlags.Namespace) | SymbolFlags.Alias;
3644136441
const rootSymbol = resolveName(rootName, rootName.escapedText, meaning, /*nameNotFoundMessage*/ undefined, /*nameArg*/ undefined, /*isReference*/ true);
36442-
if (rootSymbol
36443-
&& rootSymbol.flags & SymbolFlags.Alias
36444-
&& symbolIsValue(rootSymbol)
36445-
&& !isConstEnumOrConstEnumOnlyModule(resolveAlias(rootSymbol))
36446-
&& !getTypeOnlyAliasDeclaration(rootSymbol)) {
36447-
markAliasSymbolAsReferenced(rootSymbol);
36442+
if (rootSymbol && rootSymbol.flags & SymbolFlags.Alias) {
36443+
if (symbolIsValue(rootSymbol)
36444+
&& !isConstEnumOrConstEnumOnlyModule(resolveAlias(rootSymbol))
36445+
&& !getTypeOnlyAliasDeclaration(rootSymbol)) {
36446+
markAliasSymbolAsReferenced(rootSymbol);
36447+
}
36448+
else if (forDecoratorMetadata
36449+
&& compilerOptions.isolatedModules
36450+
&& getEmitModuleKind(compilerOptions) >= ModuleKind.ES2015
36451+
&& !symbolIsValue(rootSymbol)
36452+
&& !some(rootSymbol.declarations, isTypeOnlyImportOrExportDeclaration)) {
36453+
const diag = error(typeName, Diagnostics.A_type_referenced_in_a_decorated_signature_must_be_imported_with_import_type_or_a_namespace_import_when_isolatedModules_and_emitDecoratorMetadata_are_enabled);
36454+
const aliasDeclaration = find(rootSymbol.declarations || emptyArray, isAliasSymbolDeclaration);
36455+
if (aliasDeclaration) {
36456+
addRelatedInfo(diag, createDiagnosticForNode(aliasDeclaration, Diagnostics._0_was_imported_here, idText(rootName)));
36457+
}
36458+
}
3644836459
}
3644936460
}
3645036461

@@ -36458,7 +36469,7 @@ namespace ts {
3645836469
function markDecoratorMedataDataTypeNodeAsReferenced(node: TypeNode | undefined): void {
3645936470
const entityName = getEntityNameForDecoratorMetadata(node);
3646036471
if (entityName && isEntityName(entityName)) {
36461-
markEntityNameOrEntityExpressionAsReference(entityName);
36472+
markEntityNameOrEntityExpressionAsReference(entityName, /*forDecoratorMetadata*/ true);
3646236473
}
3646336474
}
3646436475

‎src/compiler/diagnosticMessages.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -879,6 +879,10 @@
879879
"category": "Error",
880880
"code": 1271
881881
},
882+
"A type referenced in a decorated signature must be imported with 'import type' or a namespace import when 'isolatedModules' and 'emitDecoratorMetadata' are enabled.": {
883+
"category": "Error",
884+
"code": 1272
885+
},
882886

883887
"'with' statements are not allowed in an async function block.": {
884888
"category": "Error",
@@ -7174,6 +7178,7 @@
71747178
"code": 95173
71757179
},
71767180

7181+
71777182
"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
71787183
"category": "Error",
71797184
"code": 18004

‎src/services/codefixes/fixUnreferenceableDecoratorMetadata.ts‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/* @internal */
2+
namespace ts.codefix {
3+
const fixId = "fixUnreferenceableDecoratorMetadata";
4+
const errorCodes = [Diagnostics.A_type_referenced_in_a_decorated_signature_must_be_imported_with_import_type_or_a_namespace_import_when_isolatedModules_and_emitDecoratorMetadata_are_enabled.code];
5+
registerCodeFix({
6+
errorCodes,
7+
getCodeActions: context => {
8+
const importDeclaration = getImportDeclaration(context.sourceFile, context.program, context.span.start);
9+
if (!importDeclaration) return;
10+
11+
const namespaceChanges = textChanges.ChangeTracker.with(context, t => importDeclaration.kind === SyntaxKind.ImportSpecifier && doNamespaceImportChange(t, context.sourceFile, importDeclaration, context.program));
12+
const typeOnlyChanges = textChanges.ChangeTracker.with(context, t => doTypeOnlyImportChange(t, context.sourceFile, importDeclaration, context.program));
13+
let actions: CodeFixAction[] | undefined;
14+
if (namespaceChanges.length) {
15+
actions = append(actions, createCodeFixActionWithoutFixAll(fixId, namespaceChanges, Diagnostics.Convert_named_imports_to_namespace_import));
16+
}
17+
if (typeOnlyChanges.length) {
18+
actions = append(actions, createCodeFixActionWithoutFixAll(fixId, typeOnlyChanges, Diagnostics.Convert_to_type_only_import));
19+
}
20+
return actions;
21+
},
22+
fixIds: [fixId],
23+
});
24+
25+
function getImportDeclaration(sourceFile: SourceFile, program: Program, start: number): ImportClause | ImportSpecifier | ImportEqualsDeclaration | undefined {
26+
const identifier = tryCast(getTokenAtPosition(sourceFile, start), isIdentifier);
27+
if (!identifier || identifier.parent.kind !== SyntaxKind.TypeReference) return;
28+
29+
const checker = program.getTypeChecker();
30+
const symbol = checker.getSymbolAtLocation(identifier);
31+
return find(symbol?.declarations || emptyArray, or(isImportClause, isImportSpecifier, isImportEqualsDeclaration) as (n: Node) => n is ImportClause | ImportSpecifier | ImportEqualsDeclaration);
32+
}
33+
34+
// Converts the import declaration of the offending import to a type-only import,
35+
// only if it can be done without affecting other imported names. If the conversion
36+
// cannot be done cleanly, we could offer to *extract* the offending import to a
37+
// new type-only import declaration, but honestly I doubt anyone will ever use this
38+
// codefix at all, so it's probably not worth the lines of code.
39+
function doTypeOnlyImportChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, importDeclaration: ImportClause | ImportSpecifier | ImportEqualsDeclaration, program: Program) {
40+
if (importDeclaration.kind === SyntaxKind.ImportEqualsDeclaration) {
41+
changes.insertModifierBefore(sourceFile, SyntaxKind.TypeKeyword, importDeclaration.name);
42+
return;
43+
}
44+
45+
const importClause = importDeclaration.kind === SyntaxKind.ImportClause ? importDeclaration : importDeclaration.parent.parent;
46+
if (importClause.name && importClause.namedBindings) {
47+
// Cannot convert an import with a default import and named bindings to type-only
48+
// (it's a grammar error).
49+
return;
50+
}
51+
52+
const checker = program.getTypeChecker();
53+
const importsValue = !!forEachImportClauseDeclaration(importClause, decl => {
54+
if (skipAlias(decl.symbol, checker).flags & SymbolFlags.Value) return true;
55+
});
56+
57+
if (importsValue) {
58+
// Assume that if someone wrote a non-type-only import that includes some values,
59+
// they intend to use those values in value positions, even if they haven't yet.
60+
// Don't convert it to type-only.
61+
return;
62+
}
63+
64+
changes.insertModifierBefore(sourceFile, SyntaxKind.TypeKeyword, importClause);
65+
}
66+
67+
function doNamespaceImportChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, importDeclaration: ImportSpecifier, program: Program) {
68+
refactor.doChangeNamedToNamespaceOrDefault(sourceFile, program, changes, importDeclaration.parent);
69+
}
70+
}

‎src/services/refactors/convertImport.ts‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,22 +79,25 @@ namespace ts.refactor {
7979
if (importClause.namedBindings.kind === SyntaxKind.NamespaceImport) {
8080
return { convertTo: ImportKind.Named, import: importClause.namedBindings };
8181
}
82-
const compilerOptions = context.program.getCompilerOptions();
83-
const shouldUseDefault = getAllowSyntheticDefaultImports(compilerOptions)
84-
&& isExportEqualsModule(importClause.parent.moduleSpecifier, context.program.getTypeChecker());
82+
const shouldUseDefault = getShouldUseDefault(context.program, importClause);
8583

8684
return shouldUseDefault
8785
? { convertTo: ImportKind.Default, import: importClause.namedBindings }
8886
: { convertTo: ImportKind.Namespace, import: importClause.namedBindings };
8987
}
9088

89+
function getShouldUseDefault(program: Program, importClause: ImportClause) {
90+
return getAllowSyntheticDefaultImports(program.getCompilerOptions())
91+
&& isExportEqualsModule(importClause.parent.moduleSpecifier, program.getTypeChecker());
92+
}
93+
9194
function doChange(sourceFile: SourceFile, program: Program, changes: textChanges.ChangeTracker, info: ImportConversionInfo): void {
9295
const checker = program.getTypeChecker();
9396
if (info.convertTo === ImportKind.Named) {
9497
doChangeNamespaceToNamed(sourceFile, checker, changes, info.import, getAllowSyntheticDefaultImports(program.getCompilerOptions()));
9598
}
9699
else {
97-
doChangeNamedToNamespaceOrDefault(sourceFile, checker, changes, info.import, info.convertTo === ImportKind.Default);
100+
doChangeNamedToNamespaceOrDefault(sourceFile, program, changes, info.import, info.convertTo === ImportKind.Default);
98101
}
99102
}
100103

@@ -153,7 +156,8 @@ namespace ts.refactor {
153156
return isPropertyAccessExpression(propertyAccessOrQualifiedName) ? propertyAccessOrQualifiedName.expression : propertyAccessOrQualifiedName.left;
154157
}
155158

156-
function doChangeNamedToNamespaceOrDefault(sourceFile: SourceFile, checker: TypeChecker, changes: textChanges.ChangeTracker, toConvert: NamedImports, shouldUseDefault: boolean) {
159+
export function doChangeNamedToNamespaceOrDefault(sourceFile: SourceFile, program: Program, changes: textChanges.ChangeTracker, toConvert: NamedImports, shouldUseDefault = getShouldUseDefault(program, toConvert.parent)): void {
160+
const checker = program.getTypeChecker();
157161
const importDecl = toConvert.parent.parent;
158162
const { moduleSpecifier } = importDecl;
159163

‎src/services/tsconfig.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
"codefixes/fixForgottenThisPropertyAccess.ts",
8989
"codefixes/fixInvalidJsxCharacters.ts",
9090
"codefixes/fixUnmatchedParameter.ts",
91+
"codefixes/fixUnreferenceableDecoratorMetadata.ts",
9192
"codefixes/fixUnusedIdentifier.ts",
9293
"codefixes/fixUnreachableCode.ts",
9394
"codefixes/fixUnusedLabel.ts",

‎tests/baselines/reference/emitDecoratorMetadata_isolatedModules(module=commonjs).js‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
//// [tests/cases/compiler/emitDecoratorMetadata_isolatedModules.ts] ////
2+
3+
//// [type1.ts]
4+
interface T1 {}
5+
export type { T1 }
6+
7+
//// [type2.ts]
8+
export interface T2 {}
9+
10+
//// [class3.ts]
11+
export class C3 {}
12+
13+
//// [index.ts]
14+
import { T1 } from "./type1";
15+
import * as t1 from "./type1";
16+
import type { T2 } from "./type2";
17+
import { C3 } from "./class3";
18+
declare var EventListener: any;
19+
20+
class HelloWorld {
21+
@EventListener('1')
22+
handleEvent1(event: T1) {} // Error
23+
24+
@EventListener('2')
25+
handleEvent2(event: T2) {} // Ok
26+
27+
@EventListener('1')
28+
p1!: T1; // Error
29+
30+
@EventListener('1')
31+
p1_ns!: t1.T1; // Ok
32+
33+
@EventListener('2')
34+
p2!: T2; // Ok
35+
36+
@EventListener('3')
37+
handleEvent3(event: C3): T1 { return undefined! } // Ok, Error
38+
}
39+
40+
41+
//// [type1.js]
42+
"use strict";
43+
exports.__esModule = true;
44+
//// [type2.js]
45+
"use strict";
46+
exports.__esModule = true;
47+
//// [class3.js]
48+
"use strict";
49+
exports.__esModule = true;
50+
exports.C3 = void 0;
51+
var C3 = /** @class */ (function () {
52+
function C3() {
53+
}
54+
return C3;
55+
}());
56+
exports.C3 = C3;
57+
//// [index.js]
58+
"use strict";
59+
var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
60+
var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
61+
if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
62+
else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
63+
return c > 3 && r && Object.defineProperty(target, key, r), r;
64+
};
65+
var __metadata = (this && this.__metadata) || function (k, v) {
66+
if (typeof Reflect === "object" && typeof Reflect.metadata === "function") return Reflect.metadata(k, v);
67+
};
68+
exports.__esModule = true;
69+
var t1 = require("./type1");
70+
var class3_1 = require("./class3");
71+
var HelloWorld = /** @class */ (function () {
72+
function HelloWorld() {
73+
}
74+
HelloWorld.prototype.handleEvent1 = function (event) { }; // Error
75+
HelloWorld.prototype.handleEvent2 = function (event) { }; // Ok
76+
HelloWorld.prototype.handleEvent3 = function (event) { return undefined; }; // Ok, Error
77+
__decorate([
78+
EventListener('1'),
79+
__metadata("design:type", Function),
80+
__metadata("design:paramtypes", [Object]),
81+
__metadata("design:returntype", void 0)
82+
], HelloWorld.prototype, "handleEvent1");
83+
__decorate([
84+
EventListener('2'),
85+
__metadata("design:type", Function),
86+
__metadata("design:paramtypes", [Object]),
87+
__metadata("design:returntype", void 0)
88+
], HelloWorld.prototype, "handleEvent2");
89+
__decorate([
90+
EventListener('1'),
91+
__metadata("design:type", Object)
92+
], HelloWorld.prototype, "p1");
93+
__decorate([
94+
EventListener('1'),
95+
__metadata("design:type", Object)
96+
], HelloWorld.prototype, "p1_ns");
97+
__decorate([
98+
EventListener('2'),
99+
__metadata("design:type", Object)
100+
], HelloWorld.prototype, "p2");
101+
__decorate([
102+
EventListener('3'),
103+
__metadata("design:type", Function),
104+
__metadata("design:paramtypes", [class3_1.C3]),
105+
__metadata("design:returntype", Object)
106+
], HelloWorld.prototype, "handleEvent3");
107+
return HelloWorld;
108+
}());

‎tests/baselines/reference/emitDecoratorMetadata_isolatedModules(module=commonjs).symbols‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
=== tests/cases/compiler/type1.ts ===
2+
interface T1 {}
3+
>T1 : Symbol(T1, Decl(type1.ts, 0, 0))
4+
5+
export type { T1 }
6+
>T1 : Symbol(T1, Decl(type1.ts, 1, 13))
7+
8+
=== tests/cases/compiler/type2.ts ===
9+
export interface T2 {}
10+
>T2 : Symbol(T2, Decl(type2.ts, 0, 0))
11+
12+
=== tests/cases/compiler/class3.ts ===
13+
export class C3 {}
14+
>C3 : Symbol(C3, Decl(class3.ts, 0, 0))
15+
16+
=== tests/cases/compiler/index.ts ===
17+
import { T1 } from "./type1";
18+
>T1 : Symbol(T1, Decl(index.ts, 0, 8))
19+
20+
import * as t1 from "./type1";
21+
>t1 : Symbol(t1, Decl(index.ts, 1, 6))
22+
23+
import type { T2 } from "./type2";
24+
>T2 : Symbol(T2, Decl(index.ts, 2, 13))
25+
26+
import { C3 } from "./class3";
27+
>C3 : Symbol(C3, Decl(index.ts, 3, 8))
28+
29+
declare var EventListener: any;
30+
>EventListener : Symbol(EventListener, Decl(index.ts, 4, 11))
31+
32+
class HelloWorld {
33+
>HelloWorld : Symbol(HelloWorld, Decl(index.ts, 4, 31))
34+
35+
@EventListener('1')
36+
>EventListener : Symbol(EventListener, Decl(index.ts, 4, 11))
37+
38+
handleEvent1(event: T1) {} // Error
39+
>handleEvent1 : Symbol(HelloWorld.handleEvent1, Decl(index.ts, 6, 18))
40+
>event : Symbol(event, Decl(index.ts, 8, 15))
41+
>T1 : Symbol(T1, Decl(index.ts, 0, 8))
42+
43+
@EventListener('2')
44+
>EventListener : Symbol(EventListener, Decl(index.ts, 4, 11))
45+
46+
handleEvent2(event: T2) {} // Ok
47+
>handleEvent2 : Symbol(HelloWorld.handleEvent2, Decl(index.ts, 8, 28))
48+
>event : Symbol(event, Decl(index.ts, 11, 15))
49+
>T2 : Symbol(T2, Decl(index.ts, 2, 13))
50+
51+
@EventListener('1')
52+
>EventListener : Symbol(EventListener, Decl(index.ts, 4, 11))
53+
54+
p1!: T1; // Error
55+
>p1 : Symbol(HelloWorld.p1, Decl(index.ts, 11, 28))
56+
>T1 : Symbol(T1, Decl(index.ts, 0, 8))
57+
58+
@EventListener('1')
59+
>EventListener : Symbol(EventListener, Decl(index.ts, 4, 11))
60+
61+
p1_ns!: t1.T1; // Ok
62+
>p1_ns : Symbol(HelloWorld.p1_ns, Decl(index.ts, 14, 10))
63+
>t1 : Symbol(t1, Decl(index.ts, 1, 6))
64+
>T1 : Symbol(t1.T1, Decl(type1.ts, 1, 13))
65+
66+
@EventListener('2')
67+
>EventListener : Symbol(EventListener, Decl(index.ts, 4, 11))
68+
69+
p2!: T2; // Ok
70+
>p2 : Symbol(HelloWorld.p2, Decl(index.ts, 17, 16))
71+
>T2 : Symbol(T2, Decl(index.ts, 2, 13))
72+
73+
@EventListener('3')
74+
>EventListener : Symbol(EventListener, Decl(index.ts, 4, 11))
75+
76+
handleEvent3(event: C3): T1 { return undefined! } // Ok, Error
77+
>handleEvent3 : Symbol(HelloWorld.handleEvent3, Decl(index.ts, 20, 10))
78+
>event : Symbol(event, Decl(index.ts, 23, 15))
79+
>C3 : Symbol(C3, Decl(index.ts, 3, 8))
80+
>T1 : Symbol(T1, Decl(index.ts, 0, 8))
81+
>undefined : Symbol(undefined)
82+
}
83+

0 commit comments

Comments
 (0)