Skip to content

Commit 96bcf4f

Browse files
pmvaldthePunderWoman
pmvald
authored and
thePunderWoman
committed
fix(compiler-cli): forbid custom/duplicate decorator when option forbidOrphanComponents is set (#54139)
The deps tracker which is responsible to track orphan components does not work for classes mutated by custom decorator. Some work needed to make this happen (tracked in b/320536434). As a result, with option `forbidOrphanComponents` being true the deps tracker will falsely report any component as orphan if it or its NgModule have custom/duplicate decorators. So it is unsafe to use this option in the presence of custom/duplicate decorator and we disable it until it is made compatible. Note that applying custom/duplicate decorators to `@Injectable` classes is ok since these classes never make it into the deps tracker. So we excempt them. PR Close #54139
1 parent a592904 commit 96bcf4f

4 files changed

Lines changed: 109 additions & 30 deletions

File tree

‎packages/compiler-cli/src/ngtsc/core/src/compiler.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1181,7 +1181,7 @@ export class NgCompiler {
11811181
const traitCompiler = new TraitCompiler(
11821182
handlers, reflector, this.delegatingPerfRecorder, this.incrementalCompilation,
11831183
this.options.compileNonExportedClasses !== false, compilationMode, dtsTransforms,
1184-
semanticDepGraphUpdater, this.adapter, isCore);
1184+
semanticDepGraphUpdater, this.adapter, isCore, !!this.options.forbidOrphanComponents);
11851185

11861186
// Template type-checking may use the `ProgramDriver` to produce new `ts.Program`(s). If this
11871187
// happens, they need to be tracked by the `NgCompiler`.

‎packages/compiler-cli/src/ngtsc/transform/src/compilation.ts‎

Lines changed: 35 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -97,16 +97,13 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
9797

9898
constructor(
9999
private handlers: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>[],
100-
private reflector: ReflectionHost,
101-
private perf: PerfRecorder,
100+
private reflector: ReflectionHost, private perf: PerfRecorder,
102101
private incrementalBuild: IncrementalBuild<ClassRecord, unknown>,
103-
private compileNonExportedClasses: boolean,
104-
private compilationMode: CompilationMode,
102+
private compileNonExportedClasses: boolean, private compilationMode: CompilationMode,
105103
private dtsTransforms: DtsTransformRegistry,
106104
private semanticDepGraphUpdater: SemanticDepGraphUpdater|null,
107-
private sourceFileTypeIdentifier: SourceFileTypeIdentifier,
108-
private readonly isCore: boolean,
109-
) {
105+
private sourceFileTypeIdentifier: SourceFileTypeIdentifier, private readonly isCore: boolean,
106+
private readonly forbidOrphanComponents: boolean) {
110107
for (const handler of handlers) {
111108
this.handlersByName.set(handler.name, handler);
112109
}
@@ -260,11 +257,13 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
260257
let record: ClassRecord|null = this.recordFor(clazz);
261258
let foundTraits: PendingTrait<unknown, unknown, SemanticSymbol|null, unknown>[] = [];
262259

263-
// A set to track the detected decorators in local compilation mode. An error will be issued if
260+
// A set to track the detected decorators in some cases. An error will be issued if
264261
// undetected decorators (= either non-Angular decorators or Angular duplicate decorators) are
265-
// found.
262+
// found in these cases.
266263
const detectedDecorators =
267-
this.compilationMode === CompilationMode.LOCAL ? new Set<Decorator>() : null;
264+
(this.compilationMode === CompilationMode.LOCAL || this.forbidOrphanComponents) ?
265+
new Set<Decorator>() :
266+
null;
268267

269268
for (const handler of this.handlers) {
270269
const result = handler.detect(clazz, decorators);
@@ -343,30 +342,38 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
343342
}
344343
}
345344

346-
// Local compilation uses the `DepsTracker` utility, which at the moment cannot track classes
345+
// Local compilation and the logic for detecting orphan components both use the `DepsTracker` utility, which at the moment cannot track classes
347346
// mutated by custom/duplicate decorators. So we forbid custom/duplicate decorators on classes
348347
// used by deps tracker, i.e., Component, Directive, etc (basically everything except
349348
// Injectable)
350349
// TODO(b/320536434) Support custom/duplicate decorators for the DepsTracker utility.
351350
if (decorators !== null && detectedDecorators !== null &&
352351
detectedDecorators.size < decorators.length && record !== null &&
353-
record.metaDiagnostics === null &&
354-
hasDepsTrackerAffectingScopeDecorator(detectedDecorators, this.isCore)) {
355-
// Custom or duplicate decorators found in local compilation mode for a class which has
356-
// Angular decorators other than `@Injectable`! This is not supported yet. But will eventually
357-
// do (b/320536434). For now a temporary error is thrown.
358-
record.metaDiagnostics =
359-
decorators.filter(decorator => !detectedDecorators.has(decorator))
360-
.map(
361-
decorator => ({
362-
category: ts.DiagnosticCategory.Error,
363-
code: Number('-99' + ErrorCode.DECORATOR_UNEXPECTED),
364-
file: getSourceFile(clazz),
365-
start: decorator.node.getStart(),
366-
length: decorator.node.getWidth(),
367-
messageText:
368-
'In local compilation mode, Angular does not support custom decorators or duplicate Angular decorators (except for `@Injectable` classes). Ensure all class decorators are from Angular and each decorator is used at most once for each class.',
369-
}));
352+
record.metaDiagnostics === null && hasDepsTrackerAffectingScopeDecorator(detectedDecorators, this.isCore)) {
353+
// Custom or duplicate decorators found for a class which has
354+
// Angular decorators other than `@Injectable`! This is not supported yet in some cases. But
355+
// will eventually do (b/320536434). For now a temporary error is thrown.
356+
357+
let messageText: string;
358+
if (this.compilationMode === CompilationMode.LOCAL) {
359+
messageText =
360+
'In local compilation mode, Angular does not support custom decorators or duplicate Angular decorators (except for `@Injectable` classes). Ensure all class decorators are from Angular and each decorator is used at most once for each class.';
361+
} else if (this.forbidOrphanComponents) {
362+
messageText =
363+
'When the Angular compiler option "forbidOrphanComponents" is set, Angular does not support custom decorators or duplicate Angular decorators (except for `@Injectable` classes). Ensure all class decorators are from Angular and each decorator is used at most once for each class.';
364+
} else {
365+
throw new Error('Impossible state!');
366+
}
367+
368+
record.metaDiagnostics = decorators.filter(decorator => !detectedDecorators.has(decorator))
369+
.map(decorator => ({
370+
category: ts.DiagnosticCategory.Error,
371+
code: Number('-99' + ErrorCode.DECORATOR_UNEXPECTED),
372+
file: getSourceFile(clazz),
373+
start: decorator.node.getStart(),
374+
length: decorator.node.getWidth(),
375+
messageText,
376+
}));
370377
record.traits = foundTraits = [];
371378
}
372379

‎packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ runInEachFileSystem(() => {
4141
const compiler = new TraitCompiler(
4242
handlers, reflectionHost, NOOP_PERF_RECORDER, NOOP_INCREMENTAL_BUILD, true,
4343
compilationMode, new DtsTransformRegistry(), null, fakeSfTypeIdentifier,
44-
/** isCore */ false);
44+
/** isCore */ false, /** forbidOrphanComponents */ false);
4545
const sourceFile = program.getSourceFile(filename)!;
4646

4747
return {compiler, sourceFile, program, filename: _('/' + filename)};

‎packages/compiler-cli/test/ngtsc/ngtsc_spec.ts‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,78 @@ function allTests(os: string) {
552552
})]);
553553
});
554554

555+
it('should error for custom decorators when forbidOrphanComponents is true', () => {
556+
env.tsconfig({
557+
forbidOrphanComponents: true,
558+
});
559+
env.write('test.ts', `
560+
import {Component} from '@angular/core';
561+
562+
function customDecorator<T extends new (...args: any[]) => {}>(original: T) {
563+
return class extends original {
564+
someProp = 'default';
565+
};
566+
}
567+
568+
@Component({template: ''})
569+
@customDecorator
570+
class MyComp {}
571+
`);
572+
573+
const diagnostics = env.driveDiagnostics();
574+
575+
expect(diagnostics).toEqual([jasmine.objectContaining({
576+
messageText: jasmine.stringMatching(
577+
/When the Angular compiler option "forbidOrphanComponents" is set, Angular does not support custom decorators or duplicate Angular decorators/),
578+
})]);
579+
});
580+
581+
it('should error for duplicate Component decorator when forbidOrphanComponents is true', () => {
582+
env.tsconfig({
583+
forbidOrphanComponents: true,
584+
});
585+
env.write('test.ts', `
586+
import {Component} from '@angular/core';
587+
588+
@Component({template: '1'})
589+
@Component({template: '2'})
590+
class MyComp {}
591+
`);
592+
593+
const diagnostics = env.driveDiagnostics();
594+
595+
expect(diagnostics).toEqual([jasmine.objectContaining({
596+
messageText: jasmine.stringMatching(
597+
/When the Angular compiler option "forbidOrphanComponents" is set, Angular does not support custom decorators or duplicate Angular decorators/),
598+
})]);
599+
});
600+
601+
it('should not error for duplicate @Injectable decorators when forbidOrphanComponents is true',
602+
() => {
603+
env.tsconfig({
604+
forbidOrphanComponents: true,
605+
});
606+
env.write('test.ts', `
607+
import {Injectable} from '@angular/core';
608+
609+
class SomeServiceImpl {
610+
getMessage() {
611+
return 'Hi!';
612+
}
613+
}
614+
615+
@Injectable({providedIn: 'root', useExisting: SomeServiceImpl})
616+
@Injectable({providedIn: 'root'})
617+
export abstract class SomeService {
618+
abstract getMessage(): string;
619+
}
620+
`);
621+
622+
const diagnostics = env.driveDiagnostics();
623+
624+
expect(diagnostics).toEqual([]);
625+
});
626+
555627
// This test triggers the Tsickle compiler which asserts that the file-paths
556628
// are valid for the real OS. When on non-Windows systems it doesn't like paths
557629
// that start with `C:`.

0 commit comments

Comments
 (0)