From b076d7afc7598aef4ea2e38258554872ae903fb7 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 14 Oct 2022 09:42:54 +0200 Subject: [PATCH] fix(compiler-cli): implement more host directive validations as diagnostics Implements more of the runtime validations for host directives as compiler diagnostics so that they can be caught earlier. Also does some minor cleanup. --- .../annotations/common/src/diagnostics.ts | 45 +++- .../src/ngtsc/diagnostics/src/error_code.ts | 9 + .../test/ngtsc/host_directives_spec.ts | 231 +++++++++++++++++- .../test/ngtsc/template_typecheck_spec.ts | 46 ---- .../features/host_directives_feature.ts | 11 +- 5 files changed, 277 insertions(+), 65 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts index 9aa5ee6aef93d3..b41b7fbbac6bda 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts @@ -10,7 +10,7 @@ import ts from 'typescript'; import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../../diagnostics'; import {Reference} from '../../../imports'; -import {HostDirectiveMeta, MetadataReader} from '../../../metadata'; +import {DirectiveMeta, flattenInheritedDirectiveMetadata, HostDirectiveMeta, MetadataReader} from '../../../metadata'; import {describeResolvedType, DynamicValue, PartialEvaluator, ResolvedValue, traceDynamicValue} from '../../../partial_evaluator'; import {ClassDeclaration, ReflectionHost} from '../../../reflection'; import {DeclarationData, LocalModuleScopeRegistry} from '../../../scope'; @@ -161,7 +161,7 @@ export function validateHostDirectives( const diagnostics: ts.DiagnosticWithLocation[] = []; for (const current of hostDirectives) { - const hostMeta = metaReader.getDirectiveMetadata(current.directive); + const hostMeta = flattenInheritedDirectiveMetadata(metaReader, current.directive); if (hostMeta === null) { diagnostics.push(makeDiagnostic( @@ -184,11 +184,52 @@ export function validateHostDirectives( ErrorCode.HOST_DIRECTIVE_COMPONENT, current.directive.getOriginForDiagnostics(origin), `Host directive ${hostMeta.name} cannot be a component`)); } + + validateHostDirectiveMappings('input', current, hostMeta, origin, diagnostics); + validateHostDirectiveMappings('output', current, hostMeta, origin, diagnostics); } return diagnostics; } +function validateHostDirectiveMappings( + bindingType: 'input'|'output', hostDirectiveMeta: HostDirectiveMeta, meta: DirectiveMeta, + origin: ts.Expression, diagnostics: ts.DiagnosticWithLocation[]) { + const className = meta.name; + const hostDirectiveMappings = + bindingType === 'input' ? hostDirectiveMeta.inputs : hostDirectiveMeta.outputs; + const existingBindings = bindingType === 'input' ? meta.inputs : meta.outputs; + + for (const publicName in hostDirectiveMappings) { + if (hostDirectiveMappings.hasOwnProperty(publicName)) { + if (!existingBindings.hasBindingPropertyName(publicName)) { + diagnostics.push(makeDiagnostic( + ErrorCode.HOST_DIRECTIVE_UNDEFINED_BINDING, + hostDirectiveMeta.directive.getOriginForDiagnostics(origin), + `Directive ${className} does not have an ${bindingType} with a public name of ${ + publicName}.`)); + } + + const remappedPublicName = hostDirectiveMappings[publicName]; + const bindingsForPublicName = existingBindings.getByBindingPropertyName(remappedPublicName); + + if (bindingsForPublicName !== null) { + for (const binding of bindingsForPublicName) { + if (binding.bindingPropertyName !== publicName) { + diagnostics.push(makeDiagnostic( + ErrorCode.HOST_DIRECTIVE_CONFLICTING_ALIAS, + hostDirectiveMeta.directive.getOriginForDiagnostics(origin), + `Cannot alias ${bindingType} ${publicName} of host directive ${className} to ${ + remappedPublicName}, because it already has a different ${ + bindingType} with the same public name.`)); + } + } + } + } + } +} + + export function getUndecoratedClassWithAngularFeaturesDiagnostic(node: ClassDeclaration): ts.Diagnostic { return makeDiagnostic( diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index 545c3a861d7b40..c426299d80466c 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -92,6 +92,15 @@ export enum ErrorCode { */ INJECTABLE_INHERITS_INVALID_CONSTRUCTOR = 2016, + /** Raised when a host tries to alias a host directive binding that does not exist. */ + HOST_DIRECTIVE_UNDEFINED_BINDING = 2017, + + /** + * Raised when a host tries to alias a host directive + * binding to a pre-existing binding's public name. + */ + HOST_DIRECTIVE_CONFLICTING_ALIAS = 2018, + SYMBOL_NOT_EXPORTED = 3001, /** * Raised when a relationship between directives and/or pipes would cause a cyclic import to be diff --git a/packages/compiler-cli/test/ngtsc/host_directives_spec.ts b/packages/compiler-cli/test/ngtsc/host_directives_spec.ts index 9285ab02847a93..cbade7b9e2aa6a 100644 --- a/packages/compiler-cli/test/ngtsc/host_directives_spec.ts +++ b/packages/compiler-cli/test/ngtsc/host_directives_spec.ts @@ -357,8 +357,8 @@ runInEachFileSystem(() => { import {ɵɵDirectiveDeclaration} from '@angular/core'; export declare class ExternalDir { - static ɵdir: ɵɵDirectiveDeclaration; + static ɵdir: ɵɵDirectiveDeclaration; } `); @@ -434,7 +434,7 @@ runInEachFileSystem(() => { diag.messageText.messageText; } - it('should throw if a host directive is not standalone', () => { + it('should produce a diagnostic if a host directive is not standalone', () => { env.write('test.ts', ` import {Directive, Component, NgModule} from '@angular/core'; @@ -451,7 +451,7 @@ runInEachFileSystem(() => { expect(messages).toEqual(['Host directive HostDir must be standalone']); }); - it('should throw if a host directive is a component', () => { + it('should produce a diagnostic if a host directive is a component', () => { env.write('test.ts', ` import {Directive, Component, NgModule} from '@angular/core'; @@ -471,7 +471,7 @@ runInEachFileSystem(() => { expect(messages).toEqual(['Host directive HostComp cannot be a component']); }); - it('should throw if hostDirectives is not an array', () => { + it('should produce a diagnostic if hostDirectives is not an array', () => { env.write('test.ts', ` import {Component} from '@angular/core'; @@ -486,7 +486,7 @@ runInEachFileSystem(() => { expect(messages).toContain('hostDirectives must be an array'); }); - it('should throw if a host directive is not a reference', () => { + it('should produce a diagnostic if a host directive is not a reference', () => { env.write('test.ts', ` import {Component} from '@angular/core'; @@ -503,7 +503,7 @@ runInEachFileSystem(() => { expect(messages).toEqual(['Host directive must be a reference']); }); - it('should throw if a host directive is not a reference to a class', () => { + it('should produce a diagnostic if a host directive is not a reference to a class', () => { env.write('test.ts', ` import {Component} from '@angular/core'; @@ -520,7 +520,7 @@ runInEachFileSystem(() => { expect(messages).toEqual(['Host directive reference must be a class']); }); - it('should only throw host directive error once in a chain of directives', () => { + it('should only produce a diagnostic once in a chain of directives', () => { env.write('test.ts', ` import {Directive, Component, NgModule} from '@angular/core'; @@ -544,12 +544,221 @@ runInEachFileSystem(() => { export class Host {} `); - // What we're checking here is that the errors aren't produced recursively. If that were - // the case, the same error would show up more than once in the diagnostics since `HostDirB` - // is in the chain of both `Host` and `HostDirA`. + // What we're checking here is that the diagnostics aren't produced recursively. If that + // were the case, the same diagnostic would show up more than once in the diagnostics since + // `HostDirB` is in the chain of both `Host` and `HostDirA`. const messages = env.driveDiagnostics().map(extractMessage); expect(messages).toEqual(['Host directive HostDirB must be standalone']); }); + + it('should produce a diagnostic if a host directive output does not exist', () => { + env.write('test.ts', ` + import {Directive, Output, EventEmitter} from '@angular/core'; + + @Directive({standalone: true}) + class HostDir { + @Output() foo = new EventEmitter(); + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{ + directive: HostDir, + outputs: ['doesNotExist'], + }] + }) + class Dir {} + `); + + const messages = env.driveDiagnostics().map(extractMessage); + expect(messages).toEqual( + ['Directive HostDir does not have an output with a public name of doesNotExist.']); + }); + + it('should produce a diagnostic if a host directive output alias does not exist', () => { + env.write('test.ts', ` + import {Directive, Output, EventEmitter} from '@angular/core'; + + @Directive({standalone: true}) + class HostDir { + @Output('alias') foo = new EventEmitter(); + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{ + directive: HostDir, + outputs: ['foo'], + }] + }) + class Dir {} + `); + + const messages = env.driveDiagnostics().map(extractMessage); + expect(messages).toEqual( + ['Directive HostDir does not have an output with a public name of foo.']); + }); + + it('should produce a diagnostic if a host directive input does not exist', () => { + env.write('test.ts', ` + import {Directive, Input} from '@angular/core'; + + @Directive({standalone: true}) + class HostDir { + @Input() foo: any; + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{ + directive: HostDir, + inputs: ['doesNotExist'], + }] + }) + class Dir {} + `); + + const messages = env.driveDiagnostics().map(extractMessage); + expect(messages).toEqual( + ['Directive HostDir does not have an input with a public name of doesNotExist.']); + }); + + it('should produce a diagnostic if a host directive input alias does not exist', () => { + env.write('test.ts', ` + import {Directive, Input} from '@angular/core'; + + @Directive({standalone: true}) + class HostDir { + @Input('alias') foo: any; + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, inputs: ['foo']}], + }) + class Dir {} + `); + + const messages = env.driveDiagnostics().map(extractMessage); + expect(messages).toEqual( + ['Directive HostDir does not have an input with a public name of foo.']); + }); + + it('should produce a diagnostic if a host directive tries to alias to an existing input', + () => { + env.write('test.ts', ` + import {Directive, Input} from '@angular/core'; + + @Directive({selector: '[host-dir]', standalone: true}) + class HostDir { + @Input('colorAlias') color?: string; + @Input() buttonColor?: string; + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, inputs: ['colorAlias: buttonColor']}] + }) + class Dir {} + `); + + const messages = env.driveDiagnostics().map(extractMessage); + expect(messages).toEqual([ + 'Cannot alias input colorAlias of host directive HostDir to buttonColor, because it ' + + 'already has a different input with the same public name.' + ]); + }); + + it('should produce a diagnostic if a host directive tries to alias to an existing input alias', + () => { + env.write('test.ts', ` + import {Directive, Input} from '@angular/core'; + + @Directive({selector: '[host-dir]', standalone: true}) + class HostDir { + @Input('colorAlias') color?: string; + @Input('buttonColorAlias') buttonColor?: string; + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, inputs: ['colorAlias: buttonColorAlias']}] + }) + class Dir {} + `); + + const messages = env.driveDiagnostics().map(extractMessage); + expect(messages).toEqual( + ['Cannot alias input colorAlias of host directive HostDir to buttonColorAlias, ' + + 'because it already has a different input with the same public name.']); + }); + + it('should not produce a diagnostic if a host directive input aliases to the same name', + () => { + env.write('test.ts', ` + import {Directive, Input} from '@angular/core'; + + @Directive({selector: '[host-dir]', standalone: true}) + class HostDir { + @Input('color') color?: string; + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, inputs: ['color: buttonColor']}] + }) + class Dir {} + `); + + const messages = env.driveDiagnostics().map(extractMessage); + expect(messages).toEqual([]); + }); + + it('should produce a diagnostic if a host directive tries to alias to an existing output alias', + () => { + env.write('test.ts', ` + import {Directive, Output, EventEmitter} from '@angular/core'; + + @Directive({selector: '[host-dir]', standalone: true}) + class HostDir { + @Output('clickedAlias') clicked = new EventEmitter(); + @Output('tappedAlias') tapped = new EventEmitter(); + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, outputs: ['clickedAlias: tappedAlias']}] + }) + class Dir {} + `); + + const messages = env.driveDiagnostics().map(extractMessage); + expect(messages).toEqual([ + 'Cannot alias output clickedAlias of host directive HostDir ' + + 'to tappedAlias, because it already has a different output with the same public name.' + ]); + }); + + it('should not produce a diagnostic if a host directive output aliases to the same name', + () => { + env.write('test.ts', ` + import {Directive, Output, EventEmitter} from '@angular/core'; + + @Directive({selector: '[host-dir]', standalone: true}) + class HostDir { + @Output('clicked') clicked = new EventEmitter(); + } + + @Directive({ + selector: '[dir]', + hostDirectives: [{directive: HostDir, outputs: ['clicked: wasClicked']}] + }) + class Dir {} + `); + + const messages = env.driveDiagnostics().map(extractMessage); + expect(messages).toEqual([]); + }); }); }); }); diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index bdf5314a845af6..080a445ba4cc8f 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -3072,52 +3072,6 @@ suppress `Argument of type 'string' is not assignable to parameter of type 'number'.` ]); }); - - it('should check bindings to host directive inputs referring to the private name when there is a different public name', - () => { - env.write('test.ts', ` - import {Component, Directive, NgModule, Input, Output} from '@angular/core'; - - @Directive({ - standalone: true, - }) - class HostDir { - @Input('inputAlias') input: number; - @Output('outputAlias') output: string; - } - - @Directive({ - selector: '[dir]', - hostDirectives: [{directive: HostDir, inputs: ['input'], outputs: ['output']}] - }) - class Dir {} - - @Component({ - selector: 'test', - template: '
', - }) - class TestCmp { - person: { - name: string; - }; - handleStringEvent(event: string): void {} - } - - @NgModule({ - declarations: [TestCmp, Dir], - }) - class Module {} - `); - - const messages = env.driveDiagnostics().map(d => d.messageText); - - // These messages are expected to refer to the native - // typings since the inputs/outputs haven't been exposed. - expect(messages).toEqual([ - `Argument of type 'Event' is not assignable to parameter of type 'string'.`, - `Can't bind to 'input' since it isn't a known property of 'div'.` - ]); - }); }); }); }); diff --git a/packages/core/src/render3/features/host_directives_feature.ts b/packages/core/src/render3/features/host_directives_feature.ts index c9fd4a8df9f3eb..6e3e2afde86710 100644 --- a/packages/core/src/render3/features/host_directives_feature.ts +++ b/packages/core/src/render3/features/host_directives_feature.ts @@ -149,7 +149,6 @@ function patchDeclaredInputs( function validateHostDirective( hostDirectiveConfig: HostDirectiveDef, directiveDef: DirectiveDef|null, matchedDefs: DirectiveDef[]): asserts directiveDef is DirectiveDef { - // TODO(crisbeto): implement more of these checks in the compiler. const type = hostDirectiveConfig.directive; if (directiveDef === null) { @@ -186,16 +185,16 @@ function validateHostDirective( * Checks that the host directive inputs/outputs configuration is valid. * @param bindingType Kind of binding that is being validated. Used in the error message. * @param def Definition of the host directive that is being validated against. - * @param hostDirectiveDefs Host directive mapping object that shold be validated. + * @param hostDirectiveBindings Host directive mapping object that shold be validated. */ function validateMappings( bindingType: 'input'|'output', def: DirectiveDef, - hostDirectiveDefs: HostDirectiveBindingMap) { + hostDirectiveBindings: HostDirectiveBindingMap) { const className = def.type.name; const bindings: Record = bindingType === 'input' ? def.inputs : def.outputs; - for (const publicName in hostDirectiveDefs) { - if (hostDirectiveDefs.hasOwnProperty(publicName)) { + for (const publicName in hostDirectiveBindings) { + if (hostDirectiveBindings.hasOwnProperty(publicName)) { if (!bindings.hasOwnProperty(publicName)) { throw new RuntimeError( RuntimeErrorCode.HOST_DIRECTIVE_UNDEFINED_BINDING, @@ -203,7 +202,7 @@ function validateMappings( publicName}.`); } - const remappedPublicName = hostDirectiveDefs[publicName]; + const remappedPublicName = hostDirectiveBindings[publicName]; if (bindings.hasOwnProperty(remappedPublicName) && bindings[remappedPublicName] !== publicName) {