-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add isolatedModules error for ambiguous imports referenced in decorator metadata #42915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
724550d
Add isolatedModules error for ambiguous imports referenced in decorat…
andrewbranch 4da4da1
Improve test and accept baselines
andrewbranch 33664a2
Error only for es2015+
andrewbranch 0f4aaac
Merge branch 'main' into bug/42890
andrewbranch a8ee539
Add namespace import to error message as workaround
andrewbranch 00004a7
Add codefix
andrewbranch 4119080
Merge branch 'main' into bug/42890
andrewbranch cb66b00
Fix merge fallout
andrewbranch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
70 changes: 70 additions & 0 deletions
70
src/services/codefixes/fixUnreferenceableDecoratorMetadata.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /* @internal */ | ||
| namespace ts.codefix { | ||
| const fixId = "fixUnreferenceableDecoratorMetadata"; | ||
| 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]; | ||
| registerCodeFix({ | ||
| errorCodes, | ||
| getCodeActions: context => { | ||
| const importDeclaration = getImportDeclaration(context.sourceFile, context.program, context.span.start); | ||
| if (!importDeclaration) return; | ||
|
|
||
| const namespaceChanges = textChanges.ChangeTracker.with(context, t => importDeclaration.kind === SyntaxKind.ImportSpecifier && doNamespaceImportChange(t, context.sourceFile, importDeclaration, context.program)); | ||
| const typeOnlyChanges = textChanges.ChangeTracker.with(context, t => doTypeOnlyImportChange(t, context.sourceFile, importDeclaration, context.program)); | ||
| let actions: CodeFixAction[] | undefined; | ||
| if (namespaceChanges.length) { | ||
| actions = append(actions, createCodeFixActionWithoutFixAll(fixId, namespaceChanges, Diagnostics.Convert_named_imports_to_namespace_import)); | ||
| } | ||
| if (typeOnlyChanges.length) { | ||
| actions = append(actions, createCodeFixActionWithoutFixAll(fixId, typeOnlyChanges, Diagnostics.Convert_to_type_only_import)); | ||
| } | ||
| return actions; | ||
| }, | ||
| fixIds: [fixId], | ||
| }); | ||
|
|
||
| function getImportDeclaration(sourceFile: SourceFile, program: Program, start: number): ImportClause | ImportSpecifier | ImportEqualsDeclaration | undefined { | ||
| const identifier = tryCast(getTokenAtPosition(sourceFile, start), isIdentifier); | ||
| if (!identifier || identifier.parent.kind !== SyntaxKind.TypeReference) return; | ||
|
|
||
| const checker = program.getTypeChecker(); | ||
| const symbol = checker.getSymbolAtLocation(identifier); | ||
| return find(symbol?.declarations || emptyArray, or(isImportClause, isImportSpecifier, isImportEqualsDeclaration) as (n: Node) => n is ImportClause | ImportSpecifier | ImportEqualsDeclaration); | ||
| } | ||
|
|
||
| // Converts the import declaration of the offending import to a type-only import, | ||
| // only if it can be done without affecting other imported names. If the conversion | ||
| // cannot be done cleanly, we could offer to *extract* the offending import to a | ||
| // new type-only import declaration, but honestly I doubt anyone will ever use this | ||
| // codefix at all, so it's probably not worth the lines of code. | ||
| function doTypeOnlyImportChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, importDeclaration: ImportClause | ImportSpecifier | ImportEqualsDeclaration, program: Program) { | ||
| if (importDeclaration.kind === SyntaxKind.ImportEqualsDeclaration) { | ||
| changes.insertModifierBefore(sourceFile, SyntaxKind.TypeKeyword, importDeclaration.name); | ||
| return; | ||
| } | ||
|
|
||
| const importClause = importDeclaration.kind === SyntaxKind.ImportClause ? importDeclaration : importDeclaration.parent.parent; | ||
| if (importClause.name && importClause.namedBindings) { | ||
| // Cannot convert an import with a default import and named bindings to type-only | ||
| // (it's a grammar error). | ||
| return; | ||
| } | ||
|
|
||
| const checker = program.getTypeChecker(); | ||
| const importsValue = !!forEachImportClauseDeclaration(importClause, decl => { | ||
| if (skipAlias(decl.symbol, checker).flags & SymbolFlags.Value) return true; | ||
| }); | ||
|
|
||
| if (importsValue) { | ||
| // Assume that if someone wrote a non-type-only import that includes some values, | ||
| // they intend to use those values in value positions, even if they haven't yet. | ||
| // Don't convert it to type-only. | ||
| return; | ||
| } | ||
|
|
||
| changes.insertModifierBefore(sourceFile, SyntaxKind.TypeKeyword, importClause); | ||
| } | ||
|
|
||
| function doNamespaceImportChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, importDeclaration: ImportSpecifier, program: Program) { | ||
| refactor.doChangeNamedToNamespaceOrDefault(sourceFile, program, changes, importDeclaration.parent); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
108 changes: 108 additions & 0 deletions
108
tests/baselines/reference/emitDecoratorMetadata_isolatedModules(module=commonjs).js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| //// [tests/cases/compiler/emitDecoratorMetadata_isolatedModules.ts] //// | ||
|
|
||
| //// [type1.ts] | ||
| interface T1 {} | ||
| export type { T1 } | ||
|
|
||
| //// [type2.ts] | ||
| export interface T2 {} | ||
|
|
||
| //// [class3.ts] | ||
| export class C3 {} | ||
|
|
||
| //// [index.ts] | ||
| import { T1 } from "./type1"; | ||
| import * as t1 from "./type1"; | ||
| import type { T2 } from "./type2"; | ||
| import { C3 } from "./class3"; | ||
| declare var EventListener: any; | ||
|
|
||
| class HelloWorld { | ||
| @EventListener('1') | ||
| handleEvent1(event: T1) {} // Error | ||
|
|
||
| @EventListener('2') | ||
| handleEvent2(event: T2) {} // Ok | ||
|
|
||
| @EventListener('1') | ||
| p1!: T1; // Error | ||
|
|
||
| @EventListener('1') | ||
| p1_ns!: t1.T1; // Ok | ||
|
|
||
| @EventListener('2') | ||
| p2!: T2; // Ok | ||
|
|
||
| @EventListener('3') | ||
| handleEvent3(event: C3): T1 { return undefined! } // Ok, Error | ||
| } | ||
|
|
||
|
|
||
| //// [type1.js] | ||
| "use strict"; | ||
| exports.__esModule = true; | ||
| //// [type2.js] | ||
| "use strict"; | ||
| exports.__esModule = true; | ||
| //// [class3.js] | ||
| "use strict"; | ||
| exports.__esModule = true; | ||
| exports.C3 = void 0; | ||
| var C3 = /** @class */ (function () { | ||
| function C3() { | ||
| } | ||
| return C3; | ||
| }()); | ||
| exports.C3 = C3; | ||
| //// [index.js] | ||
| "use strict"; | ||
| var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) { | ||
| var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d; | ||
| if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc); | ||
| 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; | ||
| return c > 3 && r && Object.defineProperty(target, key, r), r; | ||
| }; | ||
| var __metadata = (this && this.__metadata) || function (k, v) { | ||
| if (typeof Reflect === "object" && typeof Reflect.metadata === "function") return Reflect.metadata(k, v); | ||
| }; | ||
| exports.__esModule = true; | ||
| var t1 = require("./type1"); | ||
| var class3_1 = require("./class3"); | ||
| var HelloWorld = /** @class */ (function () { | ||
| function HelloWorld() { | ||
| } | ||
| HelloWorld.prototype.handleEvent1 = function (event) { }; // Error | ||
| HelloWorld.prototype.handleEvent2 = function (event) { }; // Ok | ||
| HelloWorld.prototype.handleEvent3 = function (event) { return undefined; }; // Ok, Error | ||
| __decorate([ | ||
| EventListener('1'), | ||
| __metadata("design:type", Function), | ||
| __metadata("design:paramtypes", [Object]), | ||
| __metadata("design:returntype", void 0) | ||
| ], HelloWorld.prototype, "handleEvent1"); | ||
| __decorate([ | ||
| EventListener('2'), | ||
| __metadata("design:type", Function), | ||
| __metadata("design:paramtypes", [Object]), | ||
| __metadata("design:returntype", void 0) | ||
| ], HelloWorld.prototype, "handleEvent2"); | ||
| __decorate([ | ||
| EventListener('1'), | ||
| __metadata("design:type", Object) | ||
| ], HelloWorld.prototype, "p1"); | ||
| __decorate([ | ||
| EventListener('1'), | ||
| __metadata("design:type", Object) | ||
| ], HelloWorld.prototype, "p1_ns"); | ||
| __decorate([ | ||
| EventListener('2'), | ||
| __metadata("design:type", Object) | ||
| ], HelloWorld.prototype, "p2"); | ||
| __decorate([ | ||
| EventListener('3'), | ||
| __metadata("design:type", Function), | ||
| __metadata("design:paramtypes", [class3_1.C3]), | ||
| __metadata("design:returntype", Object) | ||
| ], HelloWorld.prototype, "handleEvent3"); | ||
| return HelloWorld; | ||
| }()); |
83 changes: 83 additions & 0 deletions
83
tests/baselines/reference/emitDecoratorMetadata_isolatedModules(module=commonjs).symbols
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| === tests/cases/compiler/type1.ts === | ||
| interface T1 {} | ||
| >T1 : Symbol(T1, Decl(type1.ts, 0, 0)) | ||
|
|
||
| export type { T1 } | ||
| >T1 : Symbol(T1, Decl(type1.ts, 1, 13)) | ||
|
|
||
| === tests/cases/compiler/type2.ts === | ||
| export interface T2 {} | ||
| >T2 : Symbol(T2, Decl(type2.ts, 0, 0)) | ||
|
|
||
| === tests/cases/compiler/class3.ts === | ||
| export class C3 {} | ||
| >C3 : Symbol(C3, Decl(class3.ts, 0, 0)) | ||
|
|
||
| === tests/cases/compiler/index.ts === | ||
| import { T1 } from "./type1"; | ||
| >T1 : Symbol(T1, Decl(index.ts, 0, 8)) | ||
|
|
||
| import * as t1 from "./type1"; | ||
| >t1 : Symbol(t1, Decl(index.ts, 1, 6)) | ||
|
|
||
| import type { T2 } from "./type2"; | ||
| >T2 : Symbol(T2, Decl(index.ts, 2, 13)) | ||
|
|
||
| import { C3 } from "./class3"; | ||
| >C3 : Symbol(C3, Decl(index.ts, 3, 8)) | ||
|
|
||
| declare var EventListener: any; | ||
| >EventListener : Symbol(EventListener, Decl(index.ts, 4, 11)) | ||
|
|
||
| class HelloWorld { | ||
| >HelloWorld : Symbol(HelloWorld, Decl(index.ts, 4, 31)) | ||
|
|
||
| @EventListener('1') | ||
| >EventListener : Symbol(EventListener, Decl(index.ts, 4, 11)) | ||
|
|
||
| handleEvent1(event: T1) {} // Error | ||
| >handleEvent1 : Symbol(HelloWorld.handleEvent1, Decl(index.ts, 6, 18)) | ||
| >event : Symbol(event, Decl(index.ts, 8, 15)) | ||
| >T1 : Symbol(T1, Decl(index.ts, 0, 8)) | ||
|
|
||
| @EventListener('2') | ||
| >EventListener : Symbol(EventListener, Decl(index.ts, 4, 11)) | ||
|
|
||
| handleEvent2(event: T2) {} // Ok | ||
| >handleEvent2 : Symbol(HelloWorld.handleEvent2, Decl(index.ts, 8, 28)) | ||
| >event : Symbol(event, Decl(index.ts, 11, 15)) | ||
| >T2 : Symbol(T2, Decl(index.ts, 2, 13)) | ||
|
|
||
| @EventListener('1') | ||
| >EventListener : Symbol(EventListener, Decl(index.ts, 4, 11)) | ||
|
|
||
| p1!: T1; // Error | ||
| >p1 : Symbol(HelloWorld.p1, Decl(index.ts, 11, 28)) | ||
| >T1 : Symbol(T1, Decl(index.ts, 0, 8)) | ||
|
|
||
| @EventListener('1') | ||
| >EventListener : Symbol(EventListener, Decl(index.ts, 4, 11)) | ||
|
|
||
| p1_ns!: t1.T1; // Ok | ||
| >p1_ns : Symbol(HelloWorld.p1_ns, Decl(index.ts, 14, 10)) | ||
| >t1 : Symbol(t1, Decl(index.ts, 1, 6)) | ||
| >T1 : Symbol(t1.T1, Decl(type1.ts, 1, 13)) | ||
|
|
||
| @EventListener('2') | ||
| >EventListener : Symbol(EventListener, Decl(index.ts, 4, 11)) | ||
|
|
||
| p2!: T2; // Ok | ||
| >p2 : Symbol(HelloWorld.p2, Decl(index.ts, 17, 16)) | ||
| >T2 : Symbol(T2, Decl(index.ts, 2, 13)) | ||
|
|
||
| @EventListener('3') | ||
| >EventListener : Symbol(EventListener, Decl(index.ts, 4, 11)) | ||
|
|
||
| handleEvent3(event: C3): T1 { return undefined! } // Ok, Error | ||
| >handleEvent3 : Symbol(HelloWorld.handleEvent3, Decl(index.ts, 20, 10)) | ||
| >event : Symbol(event, Decl(index.ts, 23, 15)) | ||
| >C3 : Symbol(C3, Decl(index.ts, 3, 8)) | ||
| >T1 : Symbol(T1, Decl(index.ts, 0, 8)) | ||
| >undefined : Symbol(undefined) | ||
| } | ||
|
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line seems extraneous