Skip to content

Commit

Permalink
fix(compiler-cli): use inline type-check blocks for components outsid…
Browse files Browse the repository at this point in the history
…e `rootDir` (#46096)

An inline type-check block is required when a reference to a component class
cannot be emitted from an ngtypecheck shim file, but the logic to detect this
situation did not consider the configured `rootDir`. When a `rootDir` is
configured the reference emitter does not allow generating an import outside
this directory, which meant that a shim file wouldn't be able to reference
the component class. Consequently, type-check block generation would fail
with a fatal error that is unaccounted for, as gathering diagnostics should
be non-fallible.

This commit fixes the problem by leveraging the existing `canReferenceType`
logic of the type-checking `Environment`, instead of the rudimentary check
whether the class is exported as top-level symbol (`checkIfClassIsExported`).
Instead, `canReferenceType` pre-flights the generation of an import using the
`ReferenceEmitter` to tell exactly whether it will succeed or not; thus taking
into account the `rootDirs` constraint as well.

Fixes #44999

PR Close #46096
  • Loading branch information
JoostK authored and jessicajaniuk committed Jun 10, 2022
1 parent 8ac3513 commit b92c1a6
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 53 deletions.
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
Expand Up @@ -264,7 +264,7 @@ export class TypeCheckContextImpl implements TypeCheckContext {
});

const inliningRequirement =
requiresInlineTypeCheckBlock(ref.node, shimData.file, pipes, this.reflector);
requiresInlineTypeCheckBlock(ref, shimData.file, pipes, this.reflector);

// If inlining is not supported, but is required for either the TCB or one of its directive
// dependencies, then exit here with an error.
Expand Down
12 changes: 5 additions & 7 deletions packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts
Expand Up @@ -15,7 +15,6 @@ import {getTokenAtPosition} from '../../util/src/typescript';
import {FullTemplateMapping, SourceLocation, TemplateId, TemplateSourceMapping} from '../api';

import {hasIgnoreForDiagnosticsMarker, readSpanComment} from './comments';
import {checkIfClassIsExported} from './ts_util';
import {TypeParameterEmitter} from './type_parameter_emitter';

/**
Expand Down Expand Up @@ -73,21 +72,20 @@ export enum TcbInliningRequirement {
}

export function requiresInlineTypeCheckBlock(
node: ClassDeclaration<ts.ClassDeclaration>, env: ReferenceEmitEnvironment,
ref: Reference<ClassDeclaration<ts.ClassDeclaration>>, env: ReferenceEmitEnvironment,
usedPipes: Map<string, Reference<ClassDeclaration<ts.ClassDeclaration>>>,
reflector: ReflectionHost): TcbInliningRequirement {
// In order to qualify for a declared TCB (not inline) two conditions must be met:
// 1) the class must be exported
// 1) the class must be suitable to be referenced from `env` (e.g. it must be exported)
// 2) it must not have contextual generic type bounds
if (!checkIfClassIsExported(node)) {
if (!env.canReferenceType(ref)) {
// Condition 1 is false, the class is not exported.
return TcbInliningRequirement.MustInline;
} else if (!checkIfGenericTypeBoundsCanBeEmitted(node, reflector, env)) {
} else if (!checkIfGenericTypeBoundsCanBeEmitted(ref.node, reflector, env)) {
// Condition 2 is false, the class has constrained generic types. It should be checked with an
// inline TCB if possible, but can potentially use fallbacks to avoid inlining if not.
return TcbInliningRequirement.ShouldInlineForGenericBounds;
} else if (Array.from(usedPipes.values())
.some(pipeRef => !checkIfClassIsExported(pipeRef.node))) {
} else if (Array.from(usedPipes.values()).some(pipeRef => !env.canReferenceType(pipeRef))) {
// If one of the pipes used by the component is not exported, a non-inline TCB will not be able
// to import it, so this requires an inline TCB.
return TcbInliningRequirement.MustInline;
Expand Down
45 changes: 0 additions & 45 deletions packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts
Expand Up @@ -8,8 +8,6 @@

import ts from 'typescript';

import {ClassDeclaration} from '../../reflection';

const PARSED_TS_VERSION = parseFloat(ts.versionMajorMinor);


Expand Down Expand Up @@ -151,49 +149,6 @@ export function tsUpdateTypeParameterDeclaration(
node, /* modifiers */[], name, constraint, defaultType);
}

export function checkIfClassIsExported(node: ClassDeclaration): boolean {
// A class is exported if one of two conditions is met:
// 1) it has the 'export' modifier.
// 2) it's declared at the top level, and there is an export statement for the class.
if (node.modifiers !== undefined &&
node.modifiers.some(mod => mod.kind === ts.SyntaxKind.ExportKeyword)) {
// Condition 1 is true, the class has an 'export' keyword attached.
return true;
} else if (
node.parent !== undefined && ts.isSourceFile(node.parent) &&
checkIfFileHasExport(node.parent, node.name.text)) {
// Condition 2 is true, the class is exported via an 'export {}' statement.
return true;
}
return false;
}

function checkIfFileHasExport(sf: ts.SourceFile, name: string): boolean {
for (const stmt of sf.statements) {
if (ts.isExportDeclaration(stmt) && stmt.exportClause !== undefined &&
ts.isNamedExports(stmt.exportClause)) {
for (const element of stmt.exportClause.elements) {
if (element.propertyName === undefined && element.name.text === name) {
// The named declaration is directly exported.
return true;
} else if (element.propertyName !== undefined && element.propertyName.text == name) {
// The named declaration is exported via an alias.
return true;
}
}
}
}
return false;
}

export function checkIfGenericTypesAreUnbound(node: ClassDeclaration<ts.ClassDeclaration>):
boolean {
if (node.typeParameters === undefined) {
return true;
}
return node.typeParameters.every(param => param.constraint === undefined);
}

export function isAccessExpression(node: ts.Node): node is ts.ElementAccessExpression|
ts.PropertyAccessExpression {
return ts.isPropertyAccessExpression(node) || ts.isElementAccessExpression(node);
Expand Down
12 changes: 12 additions & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts
Expand Up @@ -688,6 +688,18 @@ class TestComponent {
[`TestComponent.html(1, 15): Type 'number' is not assignable to type 'string'.`]);
});
});

// https://github.com/angular/angular/issues/44999
it('should not fail for components outside of rootDir', () => {
// This test configures a component that is located outside the configured `rootDir`. Such
// configuration requires that an inline type-check block is used as the reference emitter does
// not allow generating imports outside `rootDir`.
const messages =
diagnose(`{{invalid}}`, `export class TestComponent {}`, [], [], {}, {rootDir: '/root'});

expect(messages).toEqual(
[`TestComponent.html(1, 3): Property 'invalid' does not exist on type 'TestComponent'.`]);
});
});

function diagnose(
Expand Down

0 comments on commit b92c1a6

Please sign in to comment.