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.

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

fix(ivy): template type checking should consider non-decorated base c… #31214

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Expand Up @@ -90,7 +90,8 @@ export class DecorationAnalyzer {
importGraph = new ImportGraph(this.moduleResolver);
cycleAnalyzer = new CycleAnalyzer(this.importGraph);
handlers: DecoratorHandler<any, any>[] = [
new BaseDefDecoratorHandler(this.reflectionHost, this.evaluator, this.isCore),
new BaseDefDecoratorHandler(
this.reflectionHost, this.evaluator, this.metaRegistry, this.isCore),
new ComponentDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullRegistry, this.fullMetaReader,
this.scopeRegistry, this.isCore, this.resourceManager, this.rootDirs,
Expand Down
13 changes: 11 additions & 2 deletions packages/compiler-cli/src/ngtsc/annotations/src/base_def.ts
Expand Up @@ -8,12 +8,14 @@

import {ConstantPool, R3BaseRefMetaData, compileBaseDefFromMetadata, makeBindingParser} from '@angular/compiler';

import {Reference} from '../../imports';
import {MetadataRegistry} from '../../metadata';
import {PartialEvaluator} from '../../partial_evaluator';
import {ClassDeclaration, ClassMember, Decorator, ReflectionHost} from '../../reflection';
import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence} from '../../transform';

import {extractHostBindings, queriesFromFields} from './directive';
import {isAngularDecorator} from './util';
import {isAngularDecorator, readBaseClass} from './util';

function containsNgTopLevelDecorator(decorators: Decorator[] | null, isCore: boolean): boolean {
if (!decorators) {
Expand All @@ -29,7 +31,7 @@ export class BaseDefDecoratorHandler implements
DecoratorHandler<R3BaseRefMetaData, R3BaseRefDecoratorDetection> {
constructor(
private reflector: ReflectionHost, private evaluator: PartialEvaluator,
private isCore: boolean) {}
private metaRegistry: MetadataRegistry, private isCore: boolean) {}

readonly precedence = HandlerPrecedence.WEAK;

Expand Down Expand Up @@ -145,6 +147,13 @@ export class BaseDefDecoratorHandler implements
metadata.host, this.evaluator, this.isCore ? undefined : '@angular/core');
}

this.metaRegistry.registerBaseMetadata({
ref: new Reference(node),
inputs: analysis.inputs || {},
outputs: analysis.outputs || {},
baseClass: readBaseClass(node, this.reflector, this.evaluator),
});

return {analysis};
}

Expand Down
12 changes: 11 additions & 1 deletion packages/compiler-cli/src/ngtsc/incremental/src/state.ts
Expand Up @@ -9,7 +9,7 @@
import * as ts from 'typescript';

import {Reference} from '../../imports';
import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata';
import {BaseMeta, DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata';
import {DependencyTracker} from '../../partial_evaluator';
import {ClassDeclaration} from '../../reflection';
import {ResourceDependencyRecorder} from '../../util/src/resource_recorder';
Expand Down Expand Up @@ -100,6 +100,15 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta
metadata.pipeMeta.set(meta.ref.node, meta);
}

getBaseMetadata(ref: Reference<ClassDeclaration>): BaseMeta|null {
const metadata = this.metadata.get(ref.node.getSourceFile()) || null;
return metadata && metadata.baseMeta.get(ref.node) || null;
}
registerBaseMetadata(meta: BaseMeta): void {
const metadata = this.ensureMetadata(meta.ref.node.getSourceFile());
metadata.baseMeta.set(meta.ref.node, meta);
}

recordResourceDependency(file: ts.SourceFile, resourcePath: string): void {
const metadata = this.ensureMetadata(file);
metadata.resourcePaths.add(resourcePath);
Expand Down Expand Up @@ -131,4 +140,5 @@ class FileMetadata {
directiveMeta = new Map<ClassDeclaration, DirectiveMeta>();
ngModuleMeta = new Map<ClassDeclaration, NgModuleMeta>();
pipeMeta = new Map<ClassDeclaration, PipeMeta>();
baseMeta = new Map<ClassDeclaration, BaseMeta>();
}
31 changes: 31 additions & 0 deletions packages/compiler-cli/src/ngtsc/metadata/src/api.ts
Expand Up @@ -70,6 +70,35 @@ export interface PipeMeta {
name: string;
}

/**
* Metadata for an undecorated class that uses Angular decorators
*/
export interface BaseMeta {
ref: Reference<ClassDeclaration>;

/**
* Set of inputs which this directive claims.
*
* Goes from property names to field names.
*/
inputs: {[property: string]: string | [string, string]};

/**
* Set of outputs which this directive claims.
*
* Goes from property names to field names.
*/
outputs: {[property: string]: string};

/**
* A `Reference` to the base class for the directive, if one was detected.
*
* A value of `'dynamic'` indicates that while the analyzer detected that this directive extends
* another type, it could not statically determine the base class.
*/
baseClass: Reference<ClassDeclaration>|'dynamic'|null;
}

/**
* Reads metadata for directives, pipes, and modules from a particular source, such as .d.ts files
* or a registry.
Expand All @@ -78,6 +107,7 @@ export interface MetadataReader {
getDirectiveMetadata(node: Reference<ClassDeclaration>): DirectiveMeta|null;
getNgModuleMetadata(node: Reference<ClassDeclaration>): NgModuleMeta|null;
getPipeMetadata(node: Reference<ClassDeclaration>): PipeMeta|null;
getBaseMetadata(node: Reference<ClassDeclaration>): BaseMeta|null;
}

/**
Expand All @@ -87,4 +117,5 @@ export interface MetadataRegistry {
registerDirectiveMetadata(meta: DirectiveMeta): void;
registerNgModuleMetadata(meta: NgModuleMeta): void;
registerPipeMetadata(meta: PipeMeta): void;
registerBaseMetadata(meta: BaseMeta): void;
}
11 changes: 10 additions & 1 deletion packages/compiler-cli/src/ngtsc/metadata/src/dts.ts
Expand Up @@ -11,7 +11,7 @@ import * as ts from 'typescript';
import {Reference} from '../../imports';
import {ClassDeclaration, ReflectionHost, isNamedClassDeclaration} from '../../reflection';

import {DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta} from './api';
import {BaseMeta, DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta} from './api';
import {extractDirectiveGuards, extractReferencesFromType, readStringArrayType, readStringMapType, readStringType} from './util';

/**
Expand Down Expand Up @@ -115,6 +115,15 @@ export class DtsMetadataReader implements MetadataReader {
const name = type.literal.text;
return {ref, name};
}

/**
* Read pipe metadata from a referenced class in a .d.ts file.
*/
getBaseMetadata(ref: Reference<ClassDeclaration>): BaseMeta|null {
// TODO(joost): Introduce BaseDefWithMeta so that the declaration
// has inputs and outputs that can be resolved here
return null;
}
}

function readBaseClass(clazz: ClassDeclaration, checker: ts.TypeChecker, reflector: ReflectionHost):
Expand Down
7 changes: 4 additions & 3 deletions packages/compiler-cli/src/ngtsc/metadata/src/inheritance.ts
Expand Up @@ -7,7 +7,7 @@
*/

import {Reference} from '../../imports';
import {DirectiveMeta, MetadataReader} from '../../metadata';
import {BaseMeta, DirectiveMeta, MetadataReader} from '../../metadata';
import {ClassDeclaration} from '../../reflection';

/**
Expand All @@ -29,11 +29,12 @@ export function flattenInheritedDirectiveMetadata(
let outputs: {[key: string]: string} = {};
let isDynamic = false;

const addMetadata = (meta: DirectiveMeta): void => {
const addMetadata = (meta: DirectiveMeta | BaseMeta): void => {
if (meta.baseClass === 'dynamic') {
isDynamic = true;
} else if (meta.baseClass !== null) {
const baseMeta = reader.getDirectiveMetadata(meta.baseClass);
const baseMeta =
reader.getDirectiveMetadata(meta.baseClass) || reader.getBaseMetadata(meta.baseClass);
if (baseMeta !== null) {
addMetadata(baseMeta);
} else {
Expand Down
13 changes: 12 additions & 1 deletion packages/compiler-cli/src/ngtsc/metadata/src/registry.ts
Expand Up @@ -9,7 +9,7 @@
import {Reference} from '../../imports';
import {ClassDeclaration} from '../../reflection';

import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from './api';
import {BaseMeta, DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from './api';

/**
* A registry of directive, pipe, and module metadata for types defined in the current compilation
Expand All @@ -19,6 +19,7 @@ export class LocalMetadataRegistry implements MetadataRegistry, MetadataReader {
private directives = new Map<ClassDeclaration, DirectiveMeta>();
private ngModules = new Map<ClassDeclaration, NgModuleMeta>();
private pipes = new Map<ClassDeclaration, PipeMeta>();
private baseClasses = new Map<ClassDeclaration, BaseMeta>();

getDirectiveMetadata(ref: Reference<ClassDeclaration>): DirectiveMeta|null {
return this.directives.has(ref.node) ? this.directives.get(ref.node) ! : null;
Expand All @@ -29,10 +30,14 @@ export class LocalMetadataRegistry implements MetadataRegistry, MetadataReader {
getPipeMetadata(ref: Reference<ClassDeclaration>): PipeMeta|null {
return this.pipes.has(ref.node) ? this.pipes.get(ref.node) ! : null;
}
getBaseMetadata(ref: Reference<ClassDeclaration>): BaseMeta|null {
return this.baseClasses.has(ref.node) ? this.baseClasses.get(ref.node) ! : null;
}

registerDirectiveMetadata(meta: DirectiveMeta): void { this.directives.set(meta.ref.node, meta); }
registerNgModuleMetadata(meta: NgModuleMeta): void { this.ngModules.set(meta.ref.node, meta); }
registerPipeMetadata(meta: PipeMeta): void { this.pipes.set(meta.ref.node, meta); }
registerBaseMetadata(meta: BaseMeta): void { this.baseClasses.set(meta.ref.node, meta); }
}

/**
Expand All @@ -58,4 +63,10 @@ export class CompoundMetadataRegistry implements MetadataRegistry {
registry.registerPipeMetadata(meta);
}
}

registerBaseMetadata(meta: BaseMeta): void {
for (const registry of this.registries) {
registry.registerBaseMetadata(meta);
}
}
}
13 changes: 12 additions & 1 deletion packages/compiler-cli/src/ngtsc/metadata/src/util.ts
Expand Up @@ -12,7 +12,7 @@ import {Reference} from '../../imports';
import {ClassDeclaration, ClassMember, ClassMemberKind, ReflectionHost, isNamedClassDeclaration, reflectTypeEntityToDeclaration} from '../../reflection';
import {nodeDebugInfo} from '../../util/src/typescript';

import {DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta, TemplateGuardMeta} from './api';
import {BaseMeta, DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta, TemplateGuardMeta} from './api';

export function extractReferencesFromType(
checker: ts.TypeChecker, def: ts.TypeNode, ngModuleImportedFrom: string | null,
Expand Down Expand Up @@ -144,6 +144,7 @@ export class CompoundMetadataReader implements MetadataReader {
}
return null;
}

getPipeMetadata(node: Reference<ClassDeclaration<ts.Declaration>>): PipeMeta|null {
for (const reader of this.readers) {
const meta = reader.getPipeMetadata(node);
Expand All @@ -153,4 +154,14 @@ export class CompoundMetadataReader implements MetadataReader {
}
return null;
}

getBaseMetadata(node: Reference<ClassDeclaration<ts.Declaration>>): BaseMeta|null {
for (const reader of this.readers) {
const meta = reader.getBaseMetadata(node);
if (meta !== null) {
return meta;
}
}
return null;
}
}
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/ngtsc/program.ts
Expand Up @@ -496,7 +496,7 @@ export class NgtscProgram implements api.Program {

// Set up the IvyCompilation, which manages state for the Ivy transformer.
const handlers = [
new BaseDefDecoratorHandler(this.reflector, evaluator, this.isCore),
new BaseDefDecoratorHandler(this.reflector, evaluator, metaRegistry, this.isCore),
new ComponentDecoratorHandler(
this.reflector, evaluator, metaRegistry, this.metaReader !, scopeRegistry, this.isCore,
this.resourceManager, this.rootDirs, this.options.preserveWhitespaces || false,
Expand Down
4 changes: 3 additions & 1 deletion packages/compiler-cli/src/ngtsc/scope/src/local.ts
Expand Up @@ -11,7 +11,7 @@ import * as ts from 'typescript';

import {ErrorCode, makeDiagnostic} from '../../diagnostics';
import {AliasGenerator, Reexport, Reference, ReferenceEmitter} from '../../imports';
import {DirectiveMeta, LocalMetadataRegistry, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata';
import {BaseMeta, DirectiveMeta, LocalMetadataRegistry, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata';
import {ClassDeclaration} from '../../reflection';
import {identifierOfNode, nodeNameForError} from '../../util/src/typescript';

Expand Down Expand Up @@ -119,6 +119,8 @@ export class LocalModuleScopeRegistry implements MetadataRegistry {

registerPipeMetadata(pipe: PipeMeta): void {}

registerBaseMetadata(base: BaseMeta): void {}

getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null {
if (!this.declarationToModule.has(clazz)) {
return null;
Expand Down
39 changes: 39 additions & 0 deletions packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
Expand Up @@ -336,4 +336,43 @@ describe('ngtsc type checking', () => {
expect(diags[1].messageText)
.toBe(`Type 'number' is not assignable to type 'boolean | undefined'.`);
});

it('should properly type-check inherited base classes without Angular decorators', () => {
env.write('test.ts', `
import {Component, Directive, Input, NgModule} from '@angular/core';

class BaseDir {
@Input() fromBase!: string;
}

@Directive({
selector: '[child]',
})
class ChildDir extends BaseDir {
@Input() fromChild!: boolean;
}

@Component({
selector: 'test',
template: '<div child [fromBase]="3" [fromChild]="4"></div>',
})
class TestCmp {}

@NgModule({
declarations: [TestCmp, ChildDir],
})
class Module {}
`);

const diags = env.driveDiagnostics();
expect(diags.length).toBe(2);

// Error from the binding to [fromBase].
expect(diags[0].messageText)
.toBe(`Type 'number' is not assignable to type 'string | undefined'.`);

// Error from the binding to [fromChild].
expect(diags[1].messageText)
.toBe(`Type 'number' is not assignable to type 'boolean | undefined'.`);
});
});