honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(compiler-cli): add `useInlining` option to type check config (#41043) 

This commit fixes the behavior when creating a type constructor for a directive when the following
conditions are met.
1. The directive has bound generic parameters.
2. Inlining is not available. (This happens for language service compiles).

Previously, we would throw an error saying 'Inlining is not supported in this environment.' The
compiler would stop type checking, and the developer could lose out on getting errors after the
compiler gives up.

This commit adds a useInlineTypeConstructors to the type check config. When set to false, we use
`any` type for bound generic parameters to avoid crashing. When set to true, we inline the type
constructor when inlining is required.

Addresses #40963

PR Close #41043
This commit is contained in:
Zach Arend 2021-03-02 14:35:32 -08:00 committed by Misko Hevery
parent c267c680d8
commit 09aefd2904
10 changed files with 254 additions and 101 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
packages/compiler-cli/src/ngtsc/core/src/compiler.ts
View File

@ -658,6 +658,8 @@ export class NgCompiler {
// is not disabled when `strictTemplates` is enabled.
const strictTemplates = !!this.options.strictTemplates;
const useInlineTypeConstructors = this.typeCheckingProgramStrategy.supportsInlineOperations;
// First select a type-checking configuration, based on whether full template type-checking is
// requested.
let typeCheckingConfig: TypeCheckingConfig;
@ -689,6 +691,7 @@ export class NgCompiler {
useContextGenericType: strictTemplates,
strictLiteralTypes: true,
enableTemplateTypeChecker: this.enableTemplateTypeChecker,
useInlineTypeConstructors,
};
} else {
typeCheckingConfig = {
@ -713,6 +716,7 @@ export class NgCompiler {
useContextGenericType: false,
strictLiteralTypes: false,
enableTemplateTypeChecker: this.enableTemplateTypeChecker,
useInlineTypeConstructors,
};
}

15
packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
View File

@ -260,6 +260,21 @@ export interface TypeCheckingConfig {
* literals are cast to `any` when declared.
*/
strictLiteralTypes: boolean;
/**
* Whether to use inline type constructors.
*
* If this is `true`, create inline type constructors when required. For example, if a type
* constructor's parameters has private types, it cannot be created normally, so we inline it in
* the directives definition file.
*
* If false, do not create inline type constructors. Fall back to using `any` type for
* constructors that normally require inlining.
*
* This option requires the environment to support inlining. If the environment does not support
* inlining, this must be set to `false`.
*/
useInlineTypeConstructors: boolean;
}

39
packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
View File

@ -179,7 +179,12 @@ export class TypeCheckContextImpl implements TypeCheckContext {
private compilerHost: Pick<ts.CompilerHost, 'getCanonicalFileName'>,
private componentMappingStrategy: ComponentToShimMappingStrategy,
private refEmitter: ReferenceEmitter, private reflector: ReflectionHost,
private host: TypeCheckingHost, private inlining: InliningMode) {}
private host: TypeCheckingHost, private inlining: InliningMode) {
if (inlining === InliningMode.Error && config.useInlineTypeConstructors) {
// We cannot use inlining for type checking since this environment does not support it.
throw new Error(`AssertionError: invalid inlining configuration.`);
}
}
/**
* A `Map` of `ts.SourceFile`s that the context has seen to the operations (additions of methods
@ -219,23 +224,21 @@ export class TypeCheckContextImpl implements TypeCheckContext {
...this.getTemplateDiagnostics(parseErrors, templateId, sourceMapping));
}
// Accumulate a list of any directives which could not have type constructors generated due to
// unsupported inlining operations.
let missingInlines: ClassDeclaration[] = [];
const boundTarget = binder.bind({template});
// Get all of the directives used in the template and record type constructors for all of them.
for (const dir of boundTarget.getUsedDirectives()) {
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
const dirNode = dirRef.node;
if (this.inlining === InliningMode.InlineOps) {
// Get all of the directives used in the template and record inline type constructors when
// required.
for (const dir of boundTarget.getUsedDirectives()) {
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
const dirNode = dirRef.node;
if (dir.isGeneric && requiresInlineTypeCtor(dirNode, this.reflector)) {
if (this.inlining === InliningMode.Error) {
missingInlines.push(dirNode);
if (!dir.isGeneric || !requiresInlineTypeCtor(dirNode, this.reflector)) {
// inlining not required
continue;
}
// Add a type constructor operation for the directive.
// Add an inline type constructor operation for the directive.
this.addInlineTypeCtor(fileData, dirNode.getSourceFile(), dirRef, {
fnName: 'ngTypeCtor',
// The constructor should have a body if the directive comes from a .ts file, but not if
@ -262,18 +265,12 @@ export class TypeCheckContextImpl implements TypeCheckContext {
// If inlining is not supported, but is required for either the TCB or one of its directive
// dependencies, then exit here with an error.
if (this.inlining === InliningMode.Error && (tcbRequiresInline || missingInlines.length > 0)) {
if (this.inlining === InliningMode.Error && tcbRequiresInline) {
// This template cannot be supported because the underlying strategy does not support inlining
// and inlining would be required.
// Record diagnostics to indicate the issues with this template.
if (tcbRequiresInline) {
shimData.oobRecorder.requiresInlineTcb(templateId, ref.node);
}
if (missingInlines.length > 0) {
shimData.oobRecorder.requiresInlineTypeConstructors(templateId, ref.node, missingInlines);
}
shimData.oobRecorder.requiresInlineTcb(templateId, ref.node);
// Checking this template would be unsupported, so don't try.
return;

2
packages/compiler-cli/src/ngtsc/typecheck/src/environment.ts
View File

@ -43,7 +43,7 @@ export class Environment {
constructor(
readonly config: TypeCheckingConfig, protected importManager: ImportManager,
private refEmitter: ReferenceEmitter, private reflector: ReflectionHost,
private refEmitter: ReferenceEmitter, readonly reflector: ReflectionHost,
protected contextFile: ts.SourceFile) {}
/**

105
packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
View File

@ -11,7 +11,7 @@ import * as ts from 'typescript';
import {Reference} from '../../imports';
import {ClassPropertyName} from '../../metadata';
import {ClassDeclaration} from '../../reflection';
import {ClassDeclaration, ReflectionHost} from '../../reflection';
import {TemplateId, TypeCheckableDirectiveMeta, TypeCheckBlockMetadata} from '../api';
import {addExpressionIdentifier, ExpressionIdentifier, markIgnoreDiagnostics} from './comments';
@ -21,7 +21,8 @@ import {Environment} from './environment';
import {astToTypescript, NULL_AS_ANY} from './expression';
import {OutOfBandDiagnosticRecorder} from './oob';
import {ExpressionSemanticVisitor} from './template_semantics';
import {tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util';
import {checkIfGenericTypesAreUnbound, tsCallMethod, tsCastToAny, tsCreateElement, tsCreateTypeQueryForCoercedInput, tsCreateVariable, tsDeclareVariable} from './ts_util';
import {requiresInlineTypeCtor} from './type_constructor';
/**
* Given a `ts.ClassDeclaration` for a component, and metadata regarding that component, compose a
@ -357,18 +358,13 @@ class TcbTextInterpolationOp extends TcbOp {
}
/**
* A `TcbOp` which constructs an instance of a directive _without_ setting any of its inputs. Inputs
* are later set in the `TcbDirectiveInputsOp`. Type checking was found to be faster when done in
* this way as opposed to `TcbDirectiveCtorOp` which is only necessary when the directive is
* generic.
*
* Executing this operation returns a reference to the directive instance variable with its inferred
* type.
* A `TcbOp` which constructs an instance of a directive. For generic directives, generic
* parameters are set to `any` type.
*/
class TcbDirectiveTypeOp extends TcbOp {
abstract class TcbDirectiveTypeOpBase extends TcbOp {
constructor(
private tcb: Context, private scope: Scope, private node: TmplAstTemplate|TmplAstElement,
private dir: TypeCheckableDirectiveMeta) {
protected tcb: Context, protected scope: Scope,
protected node: TmplAstTemplate|TmplAstElement, protected dir: TypeCheckableDirectiveMeta) {
super();
}
@ -380,9 +376,24 @@ class TcbDirectiveTypeOp extends TcbOp {
}
execute(): ts.Identifier {
const id = this.tcb.allocateId();
const dirRef = this.dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
const type = this.tcb.env.referenceType(this.dir.ref);
const rawType = this.tcb.env.referenceType(this.dir.ref);
let type: ts.TypeNode;
if (this.dir.isGeneric === false || dirRef.node.typeParameters === undefined) {
type = rawType;
} else {
if (!ts.isTypeReferenceNode(rawType)) {
throw new Error(
`Expected TypeReferenceNode when referencing the type for ${this.dir.ref.debugName}`);
}
const typeArguments = dirRef.node.typeParameters.map(
() => ts.factory.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword));
type = ts.factory.createTypeReferenceNode(rawType.typeName, typeArguments);
}
const id = this.tcb.allocateId();
addExpressionIdentifier(type, ExpressionIdentifier.DIRECTIVE);
addParseSpanInfo(type, this.node.startSourceSpan || this.node.sourceSpan);
this.scope.addStatement(tsDeclareVariable(id, type));
@ -390,6 +401,49 @@ class TcbDirectiveTypeOp extends TcbOp {
}
}
/**
* A `TcbOp` which constructs an instance of a non-generic directive _without_ setting any of its
* inputs. Inputs are later set in the `TcbDirectiveInputsOp`. Type checking was found to be
* faster when done in this way as opposed to `TcbDirectiveCtorOp` which is only necessary when the
* directive is generic.
*
* Executing this operation returns a reference to the directive instance variable with its inferred
* type.
*/
class TcbNonGenericDirectiveTypeOp extends TcbDirectiveTypeOpBase {
/**
* Creates a variable declaration for this op's directive of the argument type. Returns the id of
* the newly created variable.
*/
execute(): ts.Identifier {
const dirRef = this.dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
if (this.dir.isGeneric) {
throw new Error(`Assertion Error: expected ${dirRef.debugName} not to be generic.`);
}
return super.execute();
}
}
/**
* A `TcbOp` which constructs an instance of a generic directive with its generic parameters set
* to `any` type. This op is like `TcbDirectiveTypeOp`, except that generic parameters are set to
* `any` type. This is used for situations where we want to avoid inlining.
*
* Executing this operation returns a reference to the directive instance variable with its generic
* type parameters set to `any`.
*/
class TcbGenericDirectiveTypeWithAnyParamsOp extends TcbDirectiveTypeOpBase {
execute(): ts.Identifier {
const dirRef = this.dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
if (dirRef.node.typeParameters === undefined) {
throw new Error(`Assertion Error: expected typeParameters when creating a declaration for ${
dirRef.debugName}`);
}
return super.execute();
}
}
/**
* A `TcbOp` which creates a variable for a local ref in a template.
* The initializer for the variable is the variable expression for the directive, template, or
@ -1383,8 +1437,27 @@ class Scope {
const dirMap = new Map<TypeCheckableDirectiveMeta, number>();
for (const dir of directives) {
const directiveOp = dir.isGeneric ? new TcbDirectiveCtorOp(this.tcb, this, node, dir) :
new TcbDirectiveTypeOp(this.tcb, this, node, dir);
let directiveOp: TcbOp;
const host = this.tcb.env.reflector;
const dirRef = dir.ref as Reference<ClassDeclaration<ts.ClassDeclaration>>;
if (!dir.isGeneric) {
// The most common case is that when a directive is not generic, we use the normal
// `TcbNonDirectiveTypeOp`.
directiveOp = new TcbNonGenericDirectiveTypeOp(this.tcb, this, node, dir);
} else if (
!requiresInlineTypeCtor(dirRef.node, host) ||
this.tcb.env.config.useInlineTypeConstructors) {
// For generic directives, we use a type constructor to infer types. If a directive requires
// an inline type constructor, then inlining must be available to use the
// `TcbDirectiveCtorOp`. If not we, we fallback to using `any` – see below.
directiveOp = new TcbDirectiveCtorOp(this.tcb, this, node, dir);
} else {
// If inlining is not available, then we give up on infering the generic params, and use
// `any` type for the directive's generic parameters.
directiveOp = new TcbGenericDirectiveTypeWithAnyParamsOp(this.tcb, this, node, dir);
}
const dirIndex = this.opQueue.push(directiveOp) - 1;
dirMap.set(dir, dirIndex);

65
packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts
View File

@ -166,7 +166,7 @@ export function ngForTypeCheckTarget(): TypeCheckingTarget {
};
}
export const ALL_ENABLED_CONFIG: TypeCheckingConfig = {
export const ALL_ENABLED_CONFIG: Readonly<TypeCheckingConfig> = {
applyTemplateContextGuards: true,
checkQueries: false,
checkTemplateBodies: true,
@ -188,37 +188,55 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = {
useContextGenericType: true,
strictLiteralTypes: true,
enableTemplateTypeChecker: false,
useInlineTypeConstructors: true
};
// Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead.
export type TestDirective = Partial<Pick<
export interface TestDirective extends Partial<Pick<
TypeCheckableDirectiveMeta,
Exclude<
keyof TypeCheckableDirectiveMeta,
'ref'|'coercedInputFields'|'restrictedInputFields'|'stringLiteralInputFields'|
'undeclaredInputFields'|'inputs'|'outputs'>>>&{
selector: string, name: string, file?: AbsoluteFsPath, type: 'directive',
inputs?: {[fieldName: string]: string}, outputs?: {[fieldName: string]: string},
coercedInputFields?: string[], restrictedInputFields?: string[],
stringLiteralInputFields?: string[], undeclaredInputFields?: string[], isGeneric?: boolean;
};
'undeclaredInputFields'|'inputs'|'outputs'>>> {
selector: string;
name: string;
file?: AbsoluteFsPath;
type: 'directive';
inputs?: {[fieldName: string]: string};
outputs?: {[fieldName: string]: string};
coercedInputFields?: string[];
restrictedInputFields?: string[];
stringLiteralInputFields?: string[];
undeclaredInputFields?: string[];
isGeneric?: boolean;
code?: string;
}
export type TestPipe = {
name: string,
file?: AbsoluteFsPath, pipeName: string, type: 'pipe',
};
export interface TestPipe {
name: string;
file?: AbsoluteFsPath;
pipeName: string;
type: 'pipe';
code?: string;
}
export type TestDeclaration = TestDirective|TestPipe;
export function tcb(
template: string, declarations: TestDeclaration[] = [], config?: TypeCheckingConfig,
template: string, declarations: TestDeclaration[] = [], config?: Partial<TypeCheckingConfig>,
options?: {emitSpans?: boolean}): string {
const classes = ['Test', ...declarations.map(decl => decl.name)];
const code = classes.map(name => `export class ${name}<T extends string> {}`).join('\n');
const codeLines = [`export class Test<T extends string> {}`];
for (const decl of declarations) {
if (decl.code !== undefined) {
codeLines.push(decl.code);
} else {
codeLines.push(`export class ${decl.name}<T extends string> {}`);
}
}
const rootFilePath = absoluteFrom('/synthetic.ts');
const {program, host} = makeProgram([
{name: rootFilePath, contents: code, isRoot: true},
{name: rootFilePath, contents: codeLines.join('\n'), isRoot: true},
]);
const sf = getSourceFileOrError(program, rootFilePath);
@ -233,7 +251,7 @@ export function tcb(
const id = 'tcb' as TemplateId;
const meta: TypeCheckBlockMetadata = {id, boundTarget, pipes, schemas: []};
config = config || {
const fullConfig: TypeCheckingConfig = {
applyTemplateContextGuards: true,
checkQueries: false,
checkTypeOfInputBindings: true,
@ -253,6 +271,8 @@ export function tcb(
useContextGenericType: true,
strictLiteralTypes: true,
enableTemplateTypeChecker: false,
useInlineTypeConstructors: true,
...config
};
options = options || {
emitSpans: false,
@ -265,7 +285,7 @@ export function tcb(
const refEmmiter: ReferenceEmitter = new ReferenceEmitter(
[new LocalIdentifierStrategy(), new RelativePathStrategy(reflectionHost)]);
const env = new TypeCheckFile(fileName, config, refEmmiter, reflectionHost, host);
const env = new TypeCheckFile(fileName, fullConfig, refEmmiter, reflectionHost, host);
const ref = new Reference(clazz);
@ -373,7 +393,14 @@ export function setup(targets: TypeCheckingTarget[], overrides: {
program, checker, moduleResolver, new TypeScriptReflectionHost(checker)),
new LogicalProjectStrategy(reflectionHost, logicalFs),
]);
const fullConfig = {...ALL_ENABLED_CONFIG, ...config};
const fullConfig = {
...ALL_ENABLED_CONFIG,
useInlineTypeConstructors: overrides.inlining !== undefined ?
overrides.inlining :
ALL_ENABLED_CONFIG.useInlineTypeConstructors,
...config
};
// Map out the scope of each target component, which is needed for the ComponentScopeReader.
const scopeMap = new Map<ClassDeclaration, ScopeData>();

32
packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts
View File

@ -745,6 +745,7 @@ describe('type check blocks', () => {
useContextGenericType: true,
strictLiteralTypes: true,
enableTemplateTypeChecker: false,
useInlineTypeConstructors: true
};
describe('config.applyTemplateContextGuards', () => {
@ -1077,4 +1078,35 @@ describe('type check blocks', () => {
});
});
});
it('should use `any` type for type constructors with bound generic params ' +
'when `useInlineTypeConstructors` is `false`',
() => {
const template = `
<div dir
[inputA]='foo'
[inputB]='bar'
></div>
`;
const declarations: TestDeclaration[] = [{
code: `
interface PrivateInterface{};
export class Dir<T extends PrivateInterface, U extends string> {};
`,
type: 'directive',
name: 'Dir',
selector: '[dir]',
inputs: {
inputA: 'inputA',
inputB: 'inputB',
},
isGeneric: true
}];
const renderedTcb = tcb(template, declarations, {useInlineTypeConstructors: false});
expect(renderedTcb).toContain(`var _t1: i0.Dir<any, any> = null!;`);
expect(renderedTcb).toContain(`_t1.inputA = (((ctx).foo));`);
expect(renderedTcb).toContain(`_t1.inputB = (((ctx).bar));`);
});
});

6
packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts
View File

@ -1587,6 +1587,8 @@ function assertDomBindingSymbol(tSymbol: Symbol): asserts tSymbol is DomBindingS
}
export function setup(targets: TypeCheckingTarget[], config?: Partial<TypeCheckingConfig>) {
return baseTestSetup(
targets, {inlining: false, config: {...config, enableTemplateTypeChecker: true}});
return baseTestSetup(targets, {
inlining: false,
config: {...config, enableTemplateTypeChecker: true, useInlineTypeConstructors: false}
});
}

43
packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts
View File

@ -168,49 +168,8 @@ runInEachFileSystem(() => {
expect(diags.length).toBe(1);
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INLINE_TCB_REQUIRED));
});
it('should produce errors for components that require type constructor inlining', () => {
const fileName = absoluteFrom('/main.ts');
const dirFile = absoluteFrom('/dir.ts');
const {program, templateTypeChecker} = setup(
[
{
fileName,
source: `export class Cmp {}`,
templates: {'Cmp': '<div dir></div>'},
declarations: [{
name: 'TestDir',
selector: '[dir]',
file: dirFile,
type: 'directive',
isGeneric: true,
}]
},
{
fileName: dirFile,
source: `
// A non-exported interface used as a type bound for a generic directive causes
// an inline type constructor to be required.
interface NotExported {}
export class TestDir<T extends NotExported> {}`,
templates: {},
}
],
{inlining: false});
const sf = getSourceFileOrError(program, fileName);
const diags = templateTypeChecker.getDiagnosticsForFile(sf, OptimizeFor.WholeProgram);
expect(diags.length).toBe(1);
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.INLINE_TYPE_CTOR_REQUIRED));
// The relatedInformation of the diagnostic should point to the directive which required
// the inline type constructor.
const dirSf = getSourceFileOrError(program, dirFile);
expect(diags[0].relatedInformation).not.toBeUndefined();
expect(diags[0].relatedInformation!.length).toBe(1);
expect(diags[0].relatedInformation![0].file).not.toBeUndefined();
expect(absoluteFromSourceFile(diags[0].relatedInformation![0].file!)).toBe(dirSf.fileName);
});
});
describe('getTemplateOfComponent()', () => {
it('should provide access to a component\'s real template', () => {
const fileName = absoluteFrom('/main.ts');

44
packages/language-service/ivy/test/quick_info_spec.ts
View File

@ -479,6 +479,49 @@ describe('quick info', () => {
});
});
describe('generics', () => {
beforeEach(() => {
initMockFileSystem('Native');
env = LanguageServiceTestEnv.setup();
});
it('should get quick info for the generic input of a directive that normally requires inlining',
() => {
// When compiling normally, we would have to inline the type constructor of `GenericDir`
// because its generic type parameter references `PrivateInterface`, which is not exported.
project = env.addProject('test', {
'app.ts': `
import {Directive, Component, Input, NgModule} from '@angular/core';
interface PrivateInterface {}
@Directive({
selector: '[dir]'
})export class GenericDir <T extends PrivateInterface>{
@Input('input') input: T = null!;
}
@Component({
selector: 'some-cmp',
templateUrl: './app.html'
})export class SomeCmp{}
@NgModule({
declarations: [GenericDir, SomeCmp],
})export class AppModule{}
`,
'app.html': ``,
});
expectQuickInfo({
templateOverride: `<div dir [inp¦ut]='{value: 42}'></div>`,
expectedSpanText: 'input',
expectedDisplayString: '(property) GenericDir<any>.input: any'
});
});
});
describe('non-strict compiler options', () => {
beforeEach(() => {
initMockFileSystem('Native');
@ -512,6 +555,7 @@ describe('quick info', () => {
});
});
function expectQuickInfo(
{templateOverride, expectedSpanText, expectedDisplayString}:
{templateOverride: string, expectedSpanText: string, expectedDisplayString: string}):