From e10b3e22acf30d6a980082ae4d5b17afd9c6c507 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 2 Oct 2020 12:12:52 -0700 Subject: [PATCH] refactor(compiler): Add ngModule to directive symbol (#39099) This is needed so that the Language Service can provide the module name in the quick info for a directive/component. To accomplish this, the compiler's `LocalModuleScope` is provided to the `TemplateTypeCheckerImpl`. This will also allow the `TemplateTypeChecker` to provide more completions in the future, giving it a way to determine all the directives/pipes/etc. available to a template. PR Close #39099 --- .../src/ngtsc/core/src/compiler.ts | 5 +-- .../src/ngtsc/typecheck/BUILD.bazel | 1 + .../src/ngtsc/typecheck/api/symbols.ts | 4 +++ .../src/ngtsc/typecheck/src/checker.ts | 7 +++-- .../typecheck/src/template_symbol_builder.ts | 31 ++++++++++++++++--- .../src/ngtsc/typecheck/test/BUILD.bazel | 1 + .../src/ngtsc/typecheck/test/test_utils.ts | 28 +++++++++++++++-- ...ecker__get_symbol_of_template_node_spec.ts | 13 ++++++++ 8 files changed, 79 insertions(+), 11 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index a867e72613..1fc88bb792 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -21,7 +21,7 @@ import {CompoundMetadataReader, CompoundMetadataRegistry, DtsMetadataReader, Inj import {ModuleWithProvidersScanner} from '../../modulewithproviders'; import {PartialEvaluator} from '../../partial_evaluator'; import {NOOP_PERF_RECORDER, PerfRecorder} from '../../perf'; -import {TypeScriptReflectionHost} from '../../reflection'; +import {ClassDeclaration, TypeScriptReflectionHost} from '../../reflection'; import {AdapterResourceLoader} from '../../resource'; import {entryPointKeyFor, NgModuleRouteAnalyzer} from '../../routing'; import {ComponentScopeReader, LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../scope'; @@ -780,7 +780,8 @@ export class NgCompiler { const templateTypeChecker = new TemplateTypeCheckerImpl( this.tsProgram, this.typeCheckingProgramStrategy, traitCompiler, - this.getTypeCheckingConfig(), refEmitter, reflector, this.adapter, this.incrementalDriver); + this.getTypeCheckingConfig(), refEmitter, reflector, this.adapter, this.incrementalDriver, + scopeRegistry); return { isCore, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel index b9c2e93c72..f1553346ed 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/BUILD.bazel @@ -16,6 +16,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/incremental:api", "//packages/compiler-cli/src/ngtsc/metadata", "//packages/compiler-cli/src/ngtsc/reflection", + "//packages/compiler-cli/src/ngtsc/scope", "//packages/compiler-cli/src/ngtsc/shims", "//packages/compiler-cli/src/ngtsc/shims:api", "//packages/compiler-cli/src/ngtsc/translator", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/symbols.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/symbols.ts index b74d6520d3..58428c7ae6 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/symbols.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/symbols.ts @@ -10,6 +10,7 @@ import {TmplAstElement, TmplAstReference, TmplAstTemplate, TmplAstVariable} from import * as ts from 'typescript'; import {AbsoluteFsPath} from '../../file_system'; +import {ClassDeclaration} from '../../reflection'; export enum SymbolKind { Input, @@ -238,6 +239,9 @@ export interface DirectiveSymbol { /** `true` if this `DirectiveSymbol` is for a @Component. */ isComponent: boolean; + + /** The `NgModule` that this directive is declared in or `null` if it could not be determined. */ + ngModule: ClassDeclaration|null; } /** diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts index 5ef92a8a60..b2b2343ae1 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/checker.ts @@ -13,6 +13,7 @@ import {absoluteFromSourceFile, AbsoluteFsPath, getSourceFileOrError} from '../. import {ReferenceEmitter} from '../../imports'; import {IncrementalBuild} from '../../incremental/api'; import {ReflectionHost} from '../../reflection'; +import {ComponentScopeReader} from '../../scope'; import {isShim} from '../../shims'; import {getSourceFileOrNull} from '../../util/src/typescript'; import {OptimizeFor, ProgramTypeCheckAdapter, Symbol, TemplateId, TemplateTypeChecker, TypeCheckingConfig, TypeCheckingProgramStrategy, UpdateMode} from '../api'; @@ -38,7 +39,8 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { private typeCheckAdapter: ProgramTypeCheckAdapter, private config: TypeCheckingConfig, private refEmitter: ReferenceEmitter, private reflector: ReflectionHost, private compilerHost: Pick, - private priorBuild: IncrementalBuild) {} + private priorBuild: IncrementalBuild, + private readonly componentScopeReader: ComponentScopeReader) {} resetOverrides(): void { for (const fileRecord of this.state.values()) { @@ -372,7 +374,8 @@ export class TemplateTypeCheckerImpl implements TemplateTypeChecker { return null; } - return new SymbolBuilder(typeChecker, shimPath, tcb, data).getSymbol(node); + return new SymbolBuilder(typeChecker, shimPath, tcb, data, this.componentScopeReader) + .getSymbol(node); } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts index d9bb5429d9..20f28b58a8 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts @@ -10,6 +10,8 @@ import {AST, ASTWithSource, BindingPipe, MethodCall, PropertyWrite, SafeMethodCa import * as ts from 'typescript'; import {AbsoluteFsPath} from '../../file_system'; +import {ClassDeclaration} from '../../reflection'; +import {ComponentScopeReader} from '../../scope'; import {isAssignment} from '../../util/src/typescript'; import {DirectiveSymbol, DomBindingSymbol, ElementSymbol, ExpressionSymbol, InputBindingSymbol, OutputBindingSymbol, ReferenceSymbol, Symbol, SymbolKind, TemplateSymbol, TsNodeSymbolInfo, TypeCheckableDirectiveMeta, VariableSymbol} from '../api'; @@ -24,8 +26,12 @@ import {TcbDirectiveOutputsOp} from './type_check_block'; */ export class SymbolBuilder { constructor( - private readonly typeChecker: ts.TypeChecker, private readonly shimPath: AbsoluteFsPath, - private readonly typeCheckBlock: ts.Node, private readonly templateData: TemplateData) {} + private readonly typeChecker: ts.TypeChecker, + private readonly shimPath: AbsoluteFsPath, + private readonly typeCheckBlock: ts.Node, + private readonly templateData: TemplateData, + private readonly componentScopeReader: ComponentScopeReader, + ) {} getSymbol(node: TmplAstTemplate|TmplAstElement): TemplateSymbol|ElementSymbol|null; getSymbol(node: TmplAstReference|TmplAstVariable): ReferenceSymbol|VariableSymbol|null; @@ -99,14 +105,16 @@ export class SymbolBuilder { .map(node => { const symbol = this.getSymbolOfTsNode(node.parent); if (symbol === null || symbol.tsSymbol === null || - symbol.tsSymbol.declarations.length === 0) { + symbol.tsSymbol.valueDeclaration === undefined || + !ts.isClassDeclaration(symbol.tsSymbol.valueDeclaration)) { return null; } - const meta = this.getDirectiveMeta(element, symbol.tsSymbol.declarations[0]); + const meta = this.getDirectiveMeta(element, symbol.tsSymbol.valueDeclaration); if (meta === null) { return null; } + const ngModule = this.getDirectiveModule(symbol.tsSymbol.valueDeclaration); const selector = meta.selector ?? null; const isComponent = meta.isComponent ?? null; const directiveSymbol: DirectiveSymbol = { @@ -114,6 +122,7 @@ export class SymbolBuilder { tsSymbol: symbol.tsSymbol, selector, isComponent, + ngModule, kind: SymbolKind.Directive }; return directiveSymbol; @@ -132,6 +141,14 @@ export class SymbolBuilder { return directives.find(m => m.ref.node === directiveDeclaration) ?? null; } + private getDirectiveModule(declaration: ts.ClassDeclaration): ClassDeclaration|null { + const scope = this.componentScopeReader.getScopeForComponent(declaration as ClassDeclaration); + if (scope === null || scope === 'error') { + return null; + } + return scope.ngModule; + } + private getSymbolOfBoundEvent(eventBinding: TmplAstBoundEvent): OutputBindingSymbol|null { // Outputs are a `ts.CallExpression` that look like one of the two: // * _outputHelper(_t1["outputField"]).subscribe(handler); @@ -239,10 +256,13 @@ export class SymbolBuilder { } const symbol = this.getSymbolOfTsNode(declaration); - if (symbol === null || symbol.tsSymbol === null) { + if (symbol === null || symbol.tsSymbol === null || + symbol.tsSymbol.valueDeclaration === undefined || + !ts.isClassDeclaration(symbol.tsSymbol.valueDeclaration)) { return null; } + const ngModule = this.getDirectiveModule(symbol.tsSymbol.valueDeclaration); return { kind: SymbolKind.Directive, tsSymbol: symbol.tsSymbol, @@ -250,6 +270,7 @@ export class SymbolBuilder { shimLocation: symbol.shimLocation, isComponent, selector, + ngModule, }; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel index 523f14ae0d..958d0c02fb 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/BUILD.bazel @@ -18,6 +18,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/incremental", "//packages/compiler-cli/src/ngtsc/metadata", "//packages/compiler-cli/src/ngtsc/reflection", + "//packages/compiler-cli/src/ngtsc/scope", "//packages/compiler-cli/src/ngtsc/shims", "//packages/compiler-cli/src/ngtsc/testing", "//packages/compiler-cli/src/ngtsc/typecheck", diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index 217dd6024c..a8eb8cf6dc 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -11,10 +11,11 @@ import * as ts from 'typescript'; import {absoluteFrom, AbsoluteFsPath, getSourceFileOrError, LogicalFileSystem} from '../../file_system'; import {TestFile} from '../../file_system/testing'; -import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, Reference, ReferenceEmitter} from '../../imports'; +import {AbsoluteModuleStrategy, LocalIdentifierStrategy, LogicalProjectStrategy, ModuleResolver, Reexport, Reference, ReferenceEmitter} from '../../imports'; import {NOOP_INCREMENTAL_BUILD} from '../../incremental'; import {ClassPropertyMapping} from '../../metadata'; import {ClassDeclaration, isNamedClassDeclaration, TypeScriptReflectionHost} from '../../reflection'; +import {ComponentScopeReader, ScopeData} from '../../scope'; import {makeProgram} from '../../testing'; import {getRootDirs} from '../../util/src/typescript'; import {ProgramTypeCheckAdapter, TemplateTypeChecker, TypeCheckContext} from '../api'; @@ -404,9 +405,32 @@ export function setup(targets: TypeCheckingTarget[], overrides: { (programStrategy as any).supportsInlineOperations = overrides.inlining; } + const fakeScopeReader = { + getRequiresRemoteScope() { + return null; + }, + // If there is a module with [className] + 'Module' in the same source file, returns + // `LocalModuleScope` with the ngModule class and empty arrays for everything else. + getScopeForComponent(clazz: ClassDeclaration) { + try { + const ngModule = getClass(clazz.getSourceFile(), `${clazz.name.getText()}Module`); + const stubScopeData = {directives: [], ngModules: [], pipes: []}; + return { + ngModule, + compilation: stubScopeData, + reexports: [], + schemas: [], + exported: stubScopeData + }; + } catch (e) { + return null; + } + } + }; + const templateTypeChecker = new TemplateTypeCheckerImpl( program, programStrategy, checkAdapter, fullConfig, emitter, reflectionHost, host, - NOOP_INCREMENTAL_BUILD); + NOOP_INCREMENTAL_BUILD, fakeScopeReader); return { templateTypeChecker, program, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts index 5c7eb20e4f..1deb4dc9a9 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts @@ -1304,7 +1304,11 @@ runInEachFileSystem(() => { fileName: dirFile, source: ` export class TestDir {} + // Allow the fake ComponentScopeReader to return a module for TestDir + export class TestDirModule {} export class TestDir2 {} + // Allow the fake ComponentScopeReader to return a module for TestDir2 + export class TestDir2Module {} export class TestDirAllDivs {} `, templates: {}, @@ -1327,6 +1331,15 @@ runInEachFileSystem(() => { const expectedSelectors = ['[dir]', '[dir2]', 'div'].sort(); const actualSelectors = symbol.directives.map(dir => dir.selector).sort(); expect(actualSelectors).toEqual(expectedSelectors); + + // Testing this fully requires an integration test with a real `NgCompiler` (like in the + // Language Service, which uses the ngModule name for quick info). However, this path does + // assert that we are able to handle when the scope reader returns `null` or a class from + // the fake implementation. + const expectedModules = new Set([null, 'TestDirModule', 'TestDir2Module']); + const actualModules = + new Set(symbol.directives.map(dir => dir.ngModule?.name.getText() ?? null)); + expect(actualModules).toEqual(expectedModules); }); });