Skip to content

Commit

Permalink
fix(compiler-cli): implement more host directive validations as diagn…
Browse files Browse the repository at this point in the history
…ostics (#47768)

Implements more of the runtime validations for host directives as compiler diagnostics so that they can be caught earlier. Also does some minor cleanup.

PR Close #47768
  • Loading branch information
crisbeto authored and dylhunn committed Oct 17, 2022
1 parent d366664 commit 309b2cd
Show file tree
Hide file tree
Showing 6 changed files with 279 additions and 65 deletions.
2 changes: 2 additions & 0 deletions goldens/public-api/compiler-cli/error_code.md
Expand Up @@ -38,8 +38,10 @@ export enum ErrorCode {
DUPLICATE_VARIABLE_DECLARATION = 8006,
HOST_BINDING_PARSE_ERROR = 5001,
HOST_DIRECTIVE_COMPONENT = 2015,
HOST_DIRECTIVE_CONFLICTING_ALIAS = 2018,
HOST_DIRECTIVE_INVALID = 2013,
HOST_DIRECTIVE_NOT_STANDALONE = 2014,
HOST_DIRECTIVE_UNDEFINED_BINDING = 2017,
IMPORT_CYCLE_DETECTED = 3003,
IMPORT_GENERATION_FAILURE = 3004,
INJECTABLE_DUPLICATE_PROV = 9001,
Expand Down
Expand Up @@ -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';
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand Down
9 changes: 9 additions & 0 deletions packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
Expand Up @@ -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
Expand Down
231 changes: 220 additions & 11 deletions packages/compiler-cli/test/ngtsc/host_directives_spec.ts
Expand Up @@ -357,8 +357,8 @@ runInEachFileSystem(() => {
import {ɵɵDirectiveDeclaration} from '@angular/core';
export declare class ExternalDir {
static ɵdir: ɵɵDirectiveDeclaration<ExternalDir, '[test]', never, never,
{input: "input"}, {output: "output"}, never, true, never>;
static ɵdir: ɵɵDirectiveDeclaration<ExternalDir, '[test]', never,
{input: "input"}, {output: "output"}, never, never, true, never>;
}
`);

Expand Down Expand Up @@ -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';
Expand All @@ -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';
Expand All @@ -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';
Expand All @@ -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';
Expand All @@ -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';
Expand All @@ -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';
Expand All @@ -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([]);
});
});
});
});

0 comments on commit 309b2cd

Please sign in to comment.