honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(ivy): don't produce template diagnostics when scope is invalid (#34460) 

Previously, ngtsc would perform scope analysis (which directives/pipes are
available inside a component's template) and template type-checking of that
template as separate steps. If a component's scope was somehow invalid (e.g.
its NgModule imported something which wasn't another NgModule), the
component was treated as not having a scope. This meant that during template
type-checking, errors would be produced for any invalid expressions/usage of
other components that should have been in the scope.

This commit changes ngtsc to skip template type-checking of a component if
its scope is erroneous (as opposed to not present in the first place). Thus,
users aren't overwhelmed with diagnostic errors for the template and are
only informed of the root cause of the problem: an invalid NgModule scope.

Fixes #33849

PR Close #34460
This commit is contained in:
Alex Rickabaugh 2019-12-17 11:53:55 -08:00 committed by Kara Erickson
parent 047488c5d8
commit 498a2ffba3
6 changed files with 135 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
packages/compiler-cli/src/ngtsc/annotations/src/component.ts
View File

@ -328,6 +328,11 @@ export class ComponentDecoratorHandler implements
const scope = this.scopeReader.getScopeForComponent(node);
const selector = analysis.meta.selector;
const matcher = new SelectorMatcher<DirectiveMeta>();
if (scope === 'error') {
// Don't bother indexing components which had erroneous scopes.
return null;
}
if (scope !== null) {
for (const directive of scope.compilation.directives) {
if (directive.selector !== null) {
@ -360,6 +365,11 @@ export class ComponentDecoratorHandler implements
let schemas: SchemaMetadata[] = [];
const scope = this.scopeReader.getScopeForComponent(node);
if (scope === 'error') {
// Don't type-check components that had errors in their scopes.
return;
}
if (scope !== null) {
for (const meta of scope.compilation.directives) {
if (meta.selector !== null) {
@ -405,7 +415,7 @@ export class ComponentDecoratorHandler implements
wrapDirectivesAndPipesInClosure: false,
};
if (scope !== null) {
if (scope !== null && scope !== 'error') {
// Replace the empty components and directives from the analyze() step with a fully expanded
// scope. This is possible now because during resolve() the whole compilation unit has been
// fully analyzed.

7
packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
View File

@ -317,7 +317,7 @@ export class NgModuleDecoratorHandler implements
injectorImports: [],
};
if (scope !== null) {
if (scope !== null && scope !== 'error') {
// Using the scope information, extend the injector's imports using the modules that are
// specified as module exports.
const context = getSourceFile(node);
@ -342,7 +342,7 @@ export class NgModuleDecoratorHandler implements
return {diagnostics};
}
if (scope === null || scope.reexports === null) {
if (scope === null || scope === 'error' || scope.reexports === null) {
return {data};
} else {
return {
@ -370,9 +370,10 @@ export class NgModuleDecoratorHandler implements
for (const decl of analysis.declarations) {
if (this.scopeRegistry.getRequiresRemoteScope(decl.node)) {
const scope = this.scopeRegistry.getScopeOfModule(ts.getOriginalNode(node) as typeof node);
if (scope === null) {
if (scope === null || scope === 'error') {
continue;
}
const directives = scope.compilation.directives.map(
directive => this.refEmitter.emit(directive.ref, context));
const pipes = scope.compilation.pipes.map(pipe => this.refEmitter.emit(pipe.ref, context));

4
packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts
View File

@ -12,7 +12,7 @@ import {LocalModuleScope} from './local';
* Read information about the compilation scope of components.
*/
export interface ComponentScopeReader {
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null;
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error';
getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null;
}
@ -26,7 +26,7 @@ export interface ComponentScopeReader {
export class CompoundComponentScopeReader implements ComponentScopeReader {
constructor(private readers: ComponentScopeReader[]) {}
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error' {
for (const reader of this.readers) {
const meta = reader.getScopeForComponent(clazz);
if (meta !== null) {

38
packages/compiler-cli/src/ngtsc/scope/src/local.ts
View File

@ -109,6 +109,15 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
*/
private scopeErrors = new Map<ClassDeclaration, ts.Diagnostic[]>();
/**
* Tracks which NgModules are unreliable due to errors within their declarations.
*
* This provides a unified view of which modules have errors, across all of the different
* diagnostic categories that can be produced. Theoretically this can be inferred from the other
* properties of this class, but is tracked explicitly to simplify the logic.
*/
private taintedModules = new Set<ClassDeclaration>();
constructor(
private localReader: MetadataReader, private dependencyScopeReader: DtsModuleScopeResolver,
private refEmitter: ReferenceEmitter, private aliasingHost: AliasingHost|null) {}
@ -131,7 +140,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
registerPipeMetadata(pipe: PipeMeta): void {}
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null|'error' {
const scope = !this.declarationToModule.has(clazz) ?
null :
this.getScopeOfModule(this.declarationToModule.get(clazz) !.ngModule);
@ -158,15 +167,20 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
* `LocalModuleScope`.
*
* This method implements the logic of NgModule imports and exports. It returns the
* `LocalModuleScope` for the given NgModule if one can be produced, and `null` if no scope is
* available or the scope contains errors.
* `LocalModuleScope` for the given NgModule if one can be produced, `null` if no scope was ever
* defined, or the string `'error'` if the scope contained errors.
*/
getScopeOfModule(clazz: ClassDeclaration): LocalModuleScope|null {
getScopeOfModule(clazz: ClassDeclaration): LocalModuleScope|'error'|null {
const scope = this.moduleToRef.has(clazz) ?
this.getScopeOfModuleReference(this.moduleToRef.get(clazz) !) :
null;
// Translate undefined -> null.
return scope !== undefined ? scope : null;
// If the NgModule class is marked as tainted, consider it an error.
if (this.taintedModules.has(clazz)) {
return 'error';
}
// Translate undefined -> 'error'.
return scope !== undefined ? scope : 'error';
}
/**
@ -192,7 +206,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
const scopes: CompilationScope[] = [];
this.declarationToModule.forEach((declData, declaration) => {
const scope = this.getScopeOfModule(declData.ngModule);
if (scope !== null) {
if (scope !== null && scope !== 'error') {
scopes.push({declaration, ngModule: declData.ngModule, ...scope.compilation});
}
});
@ -220,6 +234,10 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
const duplicateDeclMap = new Map<ClassDeclaration, DeclarationData>();
const firstDeclData = this.declarationToModule.get(decl.node) !;
// Mark both modules as tainted, since their declarations are missing a component.
this.taintedModules.add(firstDeclData.ngModule);
this.taintedModules.add(ngModule);
// Being detected as a duplicate means there are two NgModules (for now) which declare this
// directive/pipe. Add both of them to the duplicate tracking map.
duplicateDeclMap.set(firstDeclData.ngModule, firstDeclData);
@ -298,6 +316,8 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
} else if (pipe !== null) {
compilationPipes.set(decl.node, {...pipe, ref: decl});
} else {
this.taintedModules.add(ngModule.ref.node);
const errorNode = decl.getOriginForDiagnostics(ngModule.rawDeclarations !);
diagnostics.push(makeDiagnostic(
ErrorCode.NGMODULE_INVALID_DECLARATION, errorNode,
@ -390,8 +410,8 @@ Either remove it from the NgModule's declarations, or add an appropriate Angular
// Save the errors for retrieval.
this.scopeErrors.set(ref.node, diagnostics);
// Return undefined to indicate the scope is invalid.
this.cache.set(ref.node, undefined);
// Mark this module as being tainted.
this.taintedModules.add(ref.node);
return undefined;
}

16
packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts
View File

@ -13,7 +13,7 @@ import {CompoundMetadataRegistry, DirectiveMeta, LocalMetadataRegistry, Metadata
import {ClassDeclaration} from '../../reflection';
import {ScopeData} from '../src/api';
import {DtsModuleScopeResolver} from '../src/dependency';
import {LocalModuleScopeRegistry} from '../src/local';
import {LocalModuleScope, LocalModuleScopeRegistry} from '../src/local';
function registerFakeRefs(registry: MetadataRegistry):
{[name: string]: Reference<ClassDeclaration>} {
@ -56,7 +56,7 @@ describe('LocalModuleScopeRegistry', () => {
rawDeclarations: null,
});
const scope = scopeRegistry.getScopeOfModule(Module.node) !;
const scope = scopeRegistry.getScopeOfModule(Module.node) as LocalModuleScope;
expect(scopeToRefs(scope.compilation)).toEqual([Dir1, Dir2, Pipe1]);
expect(scopeToRefs(scope.exported)).toEqual([Dir1, Pipe1]);
});
@ -89,7 +89,7 @@ describe('LocalModuleScopeRegistry', () => {
rawDeclarations: null,
});
const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !;
const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope;
expect(scopeToRefs(scopeA.compilation)).toEqual([DirA, DirB, DirCE]);
expect(scopeToRefs(scopeA.exported)).toEqual([]);
});
@ -114,7 +114,7 @@ describe('LocalModuleScopeRegistry', () => {
rawDeclarations: null,
});
const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !;
const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope;
expect(scopeToRefs(scopeA.compilation)).toEqual([]);
expect(scopeToRefs(scopeA.exported)).toEqual([Dir]);
});
@ -147,7 +147,7 @@ describe('LocalModuleScopeRegistry', () => {
rawDeclarations: null,
});
const scope = scopeRegistry.getScopeOfModule(ModuleA.node) !;
const scope = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope;
expect(scopeToRefs(scope.compilation)).toEqual([DirA, DirB]);
expect(scopeToRefs(scope.exported)).toEqual([DirA, DirB]);
});
@ -171,7 +171,7 @@ describe('LocalModuleScopeRegistry', () => {
rawDeclarations: null,
});
const scope = scopeRegistry.getScopeOfModule(Module.node) !;
const scope = scopeRegistry.getScopeOfModule(Module.node) as LocalModuleScope;
expect(scope.compilation.directives[0].ref.getIdentityIn(idSf)).toBe(id);
});
@ -195,7 +195,7 @@ describe('LocalModuleScopeRegistry', () => {
rawDeclarations: null,
});
const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) !;
const scopeA = scopeRegistry.getScopeOfModule(ModuleA.node) as LocalModuleScope;
expect(scopeToRefs(scopeA.exported)).toEqual([Dir]);
});
@ -219,7 +219,7 @@ describe('LocalModuleScopeRegistry', () => {
rawDeclarations: null,
});
expect(scopeRegistry.getScopeOfModule(ModuleA.node)).toBe(null);
expect(scopeRegistry.getScopeOfModule(ModuleA.node)).toBe('error');
// ModuleA should have associated diagnostics as it exports `Dir` without declaring it.
expect(scopeRegistry.getDiagnosticsOfModule(ModuleA.node)).not.toBeNull();

81
packages/compiler-cli/test/ngtsc/scope_spec.ts
View File

@ -301,6 +301,87 @@ runInEachFileSystem(() => {
expect(diagnosticToNode(error, ts.isIdentifier).text).toEqual('Dir');
});
});
it('should not produce component template type-check errors if its module is invalid', () => {
env.tsconfig({'fullTemplateTypeCheck': true});
// Set up 3 files, each of which declare an NgModule that's invalid in some way. This will
// produce a bunch of diagnostics related to the issues with the modules. Each module also
// declares a component with a template that references a <doesnt-exist> element. This test
// verifies that none of the produced diagnostics mention this nonexistent element, since
// no template type-checking should be performed for a component that's part of an invalid
// NgModule.
// This NgModule declares something which isn't a directive/pipe.
env.write('invalid-declaration.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '<doesnt-exist></doesnt-exist>',
})
export class TestCmp {}
export class NotACmp {}
@NgModule({declarations: [TestCmp, NotACmp]})
export class Module {}
`);
// This NgModule imports something which isn't an NgModule.
env.write('invalid-import.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '<doesnt-exist></doesnt-exist>',
})
export class TestCmp {}
export class NotAModule {}
@NgModule({
declarations: [TestCmp],
imports: [NotAModule],
})
export class Module {}
`);
// This NgModule imports a DepModule which itself is invalid (it declares something which
// isn't a directive/pipe).
env.write('transitive-error-in-import.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '<doesnt-exist></doesnt-exist>',
})
export class TestCmp {}
export class NotACmp {}
@NgModule({
declarations: [NotACmp],
exports: [NotACmp],
})
export class DepModule {}
@NgModule({
declarations: [TestCmp],
imports: [DepModule],
})
export class Module {}
`);
for (const diag of env.driveDiagnostics()) {
// None of the diagnostics should be related to the fact that the component uses an
// unknown element, because in all cases the component's scope was invalid.
expect(diag.messageText)
.not.toContain(
'doesnt-exist',
'Template type-checking ran for a component, when it shouldn\'t have.');
}
});
});
});