Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(compiler-cli): implement more host directive validations as diagnostics #47768

Closed
wants to merge 1 commit into from
Closed

fix(compiler-cli): implement more host directive validations as diagnostics #47768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions goldens/public-api/compiler-cli/error_code.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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
45 changes: 43 additions & 2 deletions packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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 =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you don't explicitly declare the type, for ease of reading the code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because TS can infer the type and the reader can hover over the name for more information.

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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixing something I messed up when I first wrote the test. The new diagnostics picked up on it.

{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', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed all these tests talking about "throwing an error" to be more accurate.

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([]);
});
});
});
});