Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit defb02f

Browse filesBrowse files
JoostKthePunderWoman
JoostK
authored and
thePunderWoman
committed
fix(compiler-cli): handle directives that refer to a namespaced class in a type parameter bound (#43511)
The template type-checker has to emit type constructors for the directives that are used in a template, where a type constructor's declaration has to mirror the type parameter constraints as they were originally declared. Therefore, the compiler analyzes whether a type parameter constraint can be recreated, e.g. by generating imports for any type references. Some type references cannot be recreated, in which case the compiler has to fall back to a strategy where the type constructor is created inline in the original source file (which comes with a performance penalty). There used to be an issue for type references to namespaced declarations. The compiler is unable to emit such references such that an inline type constructor should be used as fallback, but this did not happen. This caused the attempt to emit the type reference to fail, as the namespaced declaration cannot be located by the reference emitters. This commit fixes the issue by using a stricter check to determine if a type parameter requires an inline type constructor. The TypeScript reflection host's `isStaticallyExported` logic was expanded to work for any declaration instead of just classes, as e.g. type declarations can also be referenced in a type parameter constraint. Closes #43383 PR Close #43511
1 parent 2690315 commit defb02f
Copy full SHA for defb02f

File tree

Expand file treeCollapse file tree

5 files changed

+93
-32
lines changed
Filter options
Expand file treeCollapse file tree

5 files changed

+93
-32
lines changed

‎packages/compiler-cli/ngcc/src/host/delegating_host.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/ngcc/src/host/delegating_host.ts
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ export class DelegatingReflectionHost implements NgccReflectionHost {
162162
return this.ngccHost.detectKnownDeclaration(decl);
163163
}
164164

165-
isStaticallyExported(clazz: ClassDeclaration): boolean {
166-
return this.ngccHost.isStaticallyExported(clazz);
165+
isStaticallyExported(decl: ts.Node): boolean {
166+
return this.ngccHost.isStaticallyExported(decl);
167167
}
168168
}

‎packages/compiler-cli/src/ngtsc/reflection/src/host.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/src/ngtsc/reflection/src/host.ts
+6-6Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -858,12 +858,12 @@ export interface ReflectionHost {
858858
getAdjacentNameOfClass(clazz: ClassDeclaration): ts.Identifier;
859859

860860
/**
861-
* Returns `true` if a class is exported from the module in which it's defined.
861+
* Returns `true` if a declaration is exported from the module in which it's defined.
862862
*
863-
* Not all mechanisms by which a class is exported can be statically detected, especially when
864-
* processing already compiled JavaScript. A `false` result does not indicate that the class is
865-
* never visible outside its module, only that it was not exported via one of the export
866-
* mechanisms that the `ReflectionHost` is capable of statically checking.
863+
* Not all mechanisms by which a declaration is exported can be statically detected, especially
864+
* when processing already compiled JavaScript. A `false` result does not indicate that the
865+
* declaration is never visible outside its module, only that it was not exported via one of the
866+
* export mechanisms that the `ReflectionHost` is capable of statically checking.
867867
*/
868-
isStaticallyExported(clazz: ClassDeclaration): boolean;
868+
isStaticallyExported(decl: ts.Node): boolean;
869869
}

‎packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts
+18-19Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -199,11 +199,11 @@ export class TypeScriptReflectionHost implements ReflectionHost {
199199
return clazz.name;
200200
}
201201

202-
isStaticallyExported(clazz: ClassDeclaration): boolean {
203-
// First check if there's an `export` modifier directly on the class declaration.
204-
let topLevel: ts.Node = clazz;
205-
if (ts.isVariableDeclaration(clazz) && ts.isVariableDeclarationList(clazz.parent)) {
206-
topLevel = clazz.parent.parent;
202+
isStaticallyExported(decl: ts.Node): boolean {
203+
// First check if there's an `export` modifier directly on the declaration.
204+
let topLevel = decl;
205+
if (ts.isVariableDeclaration(decl) && ts.isVariableDeclarationList(decl.parent)) {
206+
topLevel = decl.parent.parent;
207207
}
208208
if (topLevel.modifiers !== undefined &&
209209
topLevel.modifiers.some(modifier => modifier.kind === ts.SyntaxKind.ExportKeyword)) {
@@ -224,8 +224,8 @@ export class TypeScriptReflectionHost implements ReflectionHost {
224224
return false;
225225
}
226226

227-
const localExports = this.getLocalExportedClassesOfSourceFile(clazz.getSourceFile());
228-
return localExports.has(clazz);
227+
const localExports = this.getLocalExportedDeclarationsOfSourceFile(decl.getSourceFile());
228+
return localExports.has(decl as ts.Declaration);
229229
}
230230

231231
protected getDirectImportOfIdentifier(id: ts.Identifier): Import|null {
@@ -444,17 +444,17 @@ export class TypeScriptReflectionHost implements ReflectionHost {
444444
}
445445

446446
/**
447-
* Get the set of classes declared in `file` which are exported.
447+
* Get the set of declarations declared in `file` which are exported.
448448
*/
449-
private getLocalExportedClassesOfSourceFile(file: ts.SourceFile): Set<ClassDeclaration> {
449+
private getLocalExportedDeclarationsOfSourceFile(file: ts.SourceFile): Set<ts.Declaration> {
450450
const cacheSf: SourceFileWithCachedExports = file as SourceFileWithCachedExports;
451-
if (cacheSf[LocalExportedClasses] !== undefined) {
451+
if (cacheSf[LocalExportedDeclarations] !== undefined) {
452452
// TS does not currently narrow symbol-keyed fields, hence the non-null assert is needed.
453-
return cacheSf[LocalExportedClasses]!;
453+
return cacheSf[LocalExportedDeclarations]!;
454454
}
455455

456-
const exportSet = new Set<ClassDeclaration>();
457-
cacheSf[LocalExportedClasses] = exportSet;
456+
const exportSet = new Set<ts.Declaration>();
457+
cacheSf[LocalExportedDeclarations] = exportSet;
458458

459459
const sfSymbol = this.checker.getSymbolAtLocation(cacheSf);
460460

@@ -485,8 +485,7 @@ export class TypeScriptReflectionHost implements ReflectionHost {
485485
}
486486

487487
if (exportedSymbol.valueDeclaration !== undefined &&
488-
exportedSymbol.valueDeclaration.getSourceFile() === file &&
489-
this.isClass(exportedSymbol.valueDeclaration)) {
488+
exportedSymbol.valueDeclaration.getSourceFile() === file) {
490489
exportSet.add(exportedSymbol.valueDeclaration);
491490
}
492491
item = iter.next();
@@ -677,10 +676,10 @@ function getExportedName(decl: ts.Declaration, originalId: ts.Identifier): strin
677676
originalId.text;
678677
}
679678

680-
const LocalExportedClasses = Symbol('LocalExportedClasses');
679+
const LocalExportedDeclarations = Symbol('LocalExportedDeclarations');
681680

682681
/**
683-
* A `ts.SourceFile` expando which includes a cached `Set` of local `ClassDeclarations` that are
682+
* A `ts.SourceFile` expando which includes a cached `Set` of local `ts.Declaration`s that are
684683
* exported either directly (`export class ...`) or indirectly (via `export {...}`).
685684
*
686685
* This cache does not cause memory leaks as:
@@ -694,8 +693,8 @@ const LocalExportedClasses = Symbol('LocalExportedClasses');
694693
*/
695694
interface SourceFileWithCachedExports extends ts.SourceFile {
696695
/**
697-
* Cached `Set` of `ClassDeclaration`s which are locally declared in this file and are exported
696+
* Cached `Set` of `ts.Declaration`s which are locally declared in this file and are exported
698697
* either directly or indirectly.
699698
*/
700-
[LocalExportedClasses]?: Set<ClassDeclaration>;
699+
[LocalExportedDeclarations]?: Set<ts.Declaration>;
701700
}

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_parameter_emitter.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/src/ngtsc/typecheck/src/type_parameter_emitter.ts
+11-5Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import * as ts from 'typescript';
99

1010
import {OwningModule, Reference} from '../../imports';
11-
import {DeclarationNode, isNamedClassDeclaration, ReflectionHost} from '../../reflection';
11+
import {DeclarationNode, ReflectionHost} from '../../reflection';
1212

1313
import {canEmitType, ResolvedTypeReference, TypeEmitter} from './type_emitter';
1414

@@ -92,17 +92,23 @@ export class TypeParameterEmitter {
9292
};
9393
}
9494

95-
// If no owning module is known, the reference needs to be exported to be able to emit an import
95+
// The declaration needs to be exported as a top-level export to be able to emit an import
9696
// statement for it. If the declaration is not exported, null is returned to prevent emit.
97-
if (owningModule === null && !this.isStaticallyExported(declaration.node)) {
97+
if (!this.isTopLevelExport(declaration.node)) {
9898
return null;
9999
}
100100

101101
return new Reference(declaration.node, owningModule);
102102
}
103103

104-
private isStaticallyExported(decl: DeclarationNode): boolean {
105-
return isNamedClassDeclaration(decl) && this.reflector.isStaticallyExported(decl);
104+
private isTopLevelExport(decl: DeclarationNode): boolean {
105+
if (decl.parent === undefined || !ts.isSourceFile(decl.parent)) {
106+
// The declaration has to exist at the top-level, as the reference emitters are not capable of
107+
// generating imports to classes declared in a namespace.
108+
return false;
109+
}
110+
111+
return this.reflector.isStaticallyExported(decl);
106112
}
107113

108114
private isLocalTypeParameter(decl: DeclarationNode): boolean {

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/src/ngtsc/typecheck/test/type_parameter_emitter_spec.ts
+56Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,62 @@ runInEachFileSystem(() => {
203203
expect(emit(emitter)).toEqual('<T extends test.Internal>');
204204
});
205205

206+
it('can emit references to exported classes imported using a namespace import', () => {
207+
const additionalFiles: TestFile[] = [{
208+
name: absoluteFrom('/internal.ts'),
209+
contents: `export class Internal {}`,
210+
}];
211+
const emitter = createEmitter(
212+
`
213+
import * as ns from './internal';
214+
215+
export class TestClass<T extends ns.Internal> {}`,
216+
additionalFiles);
217+
218+
expect(emitter.canEmit()).toBe(true);
219+
expect(emit(emitter)).toEqual('<T extends test.Internal>');
220+
});
221+
222+
it('cannot emit references to local classes exported within a namespace', () => {
223+
const additionalFiles: TestFile[] = [{
224+
name: absoluteFrom('/ns.ts'),
225+
contents: `
226+
export namespace ns {
227+
export class Nested {}
228+
}
229+
`,
230+
}];
231+
const emitter = createEmitter(
232+
`
233+
import {ns} from './ns';
234+
235+
export class TestClass<T extends ns.Nested> {}`,
236+
additionalFiles);
237+
238+
expect(emitter.canEmit()).toBe(false);
239+
expect(() => emit(emitter)).toThrowError('Unable to emit an unresolved reference');
240+
});
241+
242+
it('cannot emit references to external classes exported within a namespace', () => {
243+
const additionalFiles: TestFile[] = [{
244+
name: absoluteFrom('/node_modules/ns/index.d.ts'),
245+
contents: `
246+
export namespace ns {
247+
export declare class Nested {}
248+
}
249+
`,
250+
}];
251+
const emitter = createEmitter(
252+
`
253+
import {ns} from 'ns';
254+
255+
export class TestClass<T extends ns.Nested> {}`,
256+
additionalFiles);
257+
258+
expect(emitter.canEmit()).toBe(false);
259+
expect(() => emit(emitter)).toThrowError('Unable to emit an unresolved reference');
260+
});
261+
206262
it('can emit references to interfaces', () => {
207263
const additionalFiles: TestFile[] = [{
208264
name: absoluteFrom('/node_modules/types/index.d.ts'),

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.