diff --git a/packages/compiler-cli/BUILD.bazel b/packages/compiler-cli/BUILD.bazel index 398fb53865..1ac89d3a37 100644 --- a/packages/compiler-cli/BUILD.bazel +++ b/packages/compiler-cli/BUILD.bazel @@ -27,12 +27,14 @@ ts_library( "//packages/compiler-cli/src/ngtsc/annotations", "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/entry_point", + "//packages/compiler-cli/src/ngtsc/imports", "//packages/compiler-cli/src/ngtsc/partial_evaluator", "//packages/compiler-cli/src/ngtsc/reflection", "//packages/compiler-cli/src/ngtsc/shims", "//packages/compiler-cli/src/ngtsc/switch", "//packages/compiler-cli/src/ngtsc/transform", "//packages/compiler-cli/src/ngtsc/typecheck", + "//packages/compiler-cli/src/ngtsc/util", "@ngdeps//@bazel/typescript", "@ngdeps//@types", "@ngdeps//tsickle", diff --git a/packages/compiler-cli/src/ngcc/src/analysis/module_with_providers_analyzer.ts b/packages/compiler-cli/src/ngcc/src/analysis/module_with_providers_analyzer.ts index c02636bade..5456f19543 100644 --- a/packages/compiler-cli/src/ngcc/src/analysis/module_with_providers_analyzer.ts +++ b/packages/compiler-cli/src/ngcc/src/analysis/module_with_providers_analyzer.ts @@ -62,7 +62,8 @@ export class ModuleWithProvidersAnalyzer { `The referenced NgModule in ${fn.declaration.getText()} is not a class declaration in the typings program; instead we get ${dtsNgModule.getText()}`); } // Record the usage of the internal module as it needs to become an exported symbol - this.referencesRegistry.add(new ResolvedReference(ngModule.node, fn.ngModule)); + this.referencesRegistry.add( + ngModule.node, new ResolvedReference(ngModule.node, fn.ngModule)); ngModule = {node: dtsNgModule, viaModule: null}; } diff --git a/packages/compiler-cli/src/ngcc/src/analysis/ngcc_references_registry.ts b/packages/compiler-cli/src/ngcc/src/analysis/ngcc_references_registry.ts index eaf43bf834..c0000c1ac2 100644 --- a/packages/compiler-cli/src/ngcc/src/analysis/ngcc_references_registry.ts +++ b/packages/compiler-cli/src/ngcc/src/analysis/ngcc_references_registry.ts @@ -29,7 +29,7 @@ export class NgccReferencesRegistry implements ReferencesRegistry { * Only `ResolveReference` references are stored. Other types are ignored. * @param references A collection of references to register. */ - add(...references: Reference[]): void { + add(source: ts.Declaration, ...references: Reference[]): void { references.forEach(ref => { // Only store resolved references. We are not interested in literals. if (ref instanceof ResolvedReference && hasNameIdentifier(ref.node)) { diff --git a/packages/compiler-cli/src/ngcc/src/analysis/private_declarations_analyzer.ts b/packages/compiler-cli/src/ngcc/src/analysis/private_declarations_analyzer.ts index d8b1ae02b1..bfa76cf0c6 100644 --- a/packages/compiler-cli/src/ngcc/src/analysis/private_declarations_analyzer.ts +++ b/packages/compiler-cli/src/ngcc/src/analysis/private_declarations_analyzer.ts @@ -7,10 +7,10 @@ */ import * as ts from 'typescript'; -import {ReferencesRegistry} from '../../../ngtsc/annotations'; import {Declaration} from '../../../ngtsc/reflection'; import {NgccReflectionHost} from '../host/ngcc_host'; import {hasNameIdentifier, isDefined} from '../utils'; +import {NgccReferencesRegistry} from './ngcc_references_registry'; export interface ExportInfo { identifier: string; @@ -24,7 +24,8 @@ export type PrivateDeclarationsAnalyses = ExportInfo[]; * (i.e. on an NgModule) that are not publicly exported via an entry-point. */ export class PrivateDeclarationsAnalyzer { - constructor(private host: NgccReflectionHost, private referencesRegistry: ReferencesRegistry) {} + constructor( + private host: NgccReflectionHost, private referencesRegistry: NgccReferencesRegistry) {} analyzeProgram(program: ts.Program): PrivateDeclarationsAnalyses { const rootFiles = this.getRootFiles(program); diff --git a/packages/compiler-cli/src/ngcc/test/analysis/private_declarations_analyzer_spec.ts b/packages/compiler-cli/src/ngcc/test/analysis/private_declarations_analyzer_spec.ts index 36b72f93d5..4adbf8bbaa 100644 --- a/packages/compiler-cli/src/ngcc/test/analysis/private_declarations_analyzer_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/analysis/private_declarations_analyzer_spec.ts @@ -137,15 +137,18 @@ describe('PrivateDeclarationsAnalyzer', () => { const publicComponentDeclaration = getDeclaration(program, '/src/a.js', 'PublicComponent', ts.isClassDeclaration); referencesRegistry.add( + null !, new ResolvedReference(publicComponentDeclaration, publicComponentDeclaration.name !)); const privateComponentDeclaration = getDeclaration(program, '/src/b.js', 'PrivateComponent', ts.isClassDeclaration); - referencesRegistry.add(new ResolvedReference( - privateComponentDeclaration, privateComponentDeclaration.name !)); + referencesRegistry.add( + null !, new ResolvedReference( + privateComponentDeclaration, privateComponentDeclaration.name !)); const internalComponentDeclaration = getDeclaration(program, '/src/c.js', 'InternalComponent', ts.isClassDeclaration); - referencesRegistry.add(new ResolvedReference( - internalComponentDeclaration, internalComponentDeclaration.name !)); + referencesRegistry.add( + null !, new ResolvedReference( + internalComponentDeclaration, internalComponentDeclaration.name !)); const analyses = analyzer.analyzeProgram(program); expect(analyses.length).toEqual(2); diff --git a/packages/compiler-cli/src/ngcc/test/analysis/references_registry_spec.ts b/packages/compiler-cli/src/ngcc/test/analysis/references_registry_spec.ts index ffb6a46325..46183d93ce 100644 --- a/packages/compiler-cli/src/ngcc/test/analysis/references_registry_spec.ts +++ b/packages/compiler-cli/src/ngcc/test/analysis/references_registry_spec.ts @@ -43,7 +43,7 @@ describe('NgccReferencesRegistry', () => { const registry = new NgccReferencesRegistry(host); const references = evaluator.evaluate(testArrayExpression) as Reference[]; - registry.add(...references); + registry.add(null !, ...references); const map = registry.getDeclarationMap(); expect(map.size).toEqual(2); diff --git a/packages/compiler-cli/src/ngtools_api2.ts b/packages/compiler-cli/src/ngtools_api2.ts index eef681486c..ec3a8dfed9 100644 --- a/packages/compiler-cli/src/ngtools_api2.ts +++ b/packages/compiler-cli/src/ngtools_api2.ts @@ -109,7 +109,8 @@ export interface LazyRoute { export interface Program { getTsProgram(): ts.Program; getTsOptionDiagnostics(cancellationToken?: ts.CancellationToken): ReadonlyArray; - getNgOptionDiagnostics(cancellationToken?: ts.CancellationToken): ReadonlyArray; + getNgOptionDiagnostics(cancellationToken?: ts.CancellationToken): + ReadonlyArray; getTsSyntacticDiagnostics(sourceFile?: ts.SourceFile, cancellationToken?: ts.CancellationToken): ReadonlyArray; getNgStructuralDiagnostics(cancellationToken?: ts.CancellationToken): ReadonlyArray; diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index 9cdcdcc720..2de2260e2b 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -75,7 +75,6 @@ export class NgModuleDecoratorHandler implements DecoratorHandler[] = []; if (ngModule.has('imports')) { @@ -83,7 +82,6 @@ export class NgModuleDecoratorHandler implements DecoratorHandler this._extractModuleFromModuleWithProvidersFn(ref.node)); imports = this.resolveTypeList(expr, importsMeta, 'imports'); - this.referencesRegistry.add(...imports); } let exports: Reference[] = []; if (ngModule.has('exports')) { @@ -91,14 +89,13 @@ export class NgModuleDecoratorHandler implements DecoratorHandler this._extractModuleFromModuleWithProvidersFn(ref.node)); exports = this.resolveTypeList(expr, exportsMeta, 'exports'); - this.referencesRegistry.add(...exports); + this.referencesRegistry.add(node, ...exports); } let bootstrap: Reference[] = []; if (ngModule.has('bootstrap')) { const expr = ngModule.get('bootstrap') !; const bootstrapMeta = this.evaluator.evaluate(expr); bootstrap = this.resolveTypeList(expr, bootstrapMeta, 'bootstrap'); - this.referencesRegistry.add(...bootstrap); } // Register this module's information with the SelectorScopeRegistry. This ensures that during diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/references_registry.ts b/packages/compiler-cli/src/ngtsc/annotations/src/references_registry.ts index 55a41fb98e..52731b2d6b 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/references_registry.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/references_registry.ts @@ -17,15 +17,9 @@ import {Declaration} from '../../reflection'; export interface ReferencesRegistry { /** * Register one or more references in the registry. - * Only `ResolveReference` references are stored. Other types are ignored. * @param references A collection of references to register. */ - add(...references: Reference[]): void; - /** - * Create and return a mapping for the registered resolved references. - * @returns A map of reference identifiers to reference declarations. - */ - getDeclarationMap(): Map; + add(source: ts.Declaration, ...references: Reference[]): void; } /** @@ -34,6 +28,5 @@ export interface ReferencesRegistry { * The ngcc tool implements a working version for its purposes. */ export class NoopReferencesRegistry implements ReferencesRegistry { - add(...references: Reference[]): void {} - getDeclarationMap(): Map { return new Map(); } + add(source: ts.Declaration, ...references: Reference[]): void {} } \ No newline at end of file diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/index.ts b/packages/compiler-cli/src/ngtsc/diagnostics/index.ts index c235c43985..48c91aad50 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/index.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/index.ts @@ -6,6 +6,6 @@ * found in the LICENSE file at https://angular.io/license */ -export {ErrorCode} from './src/code'; +export {ErrorCode, ngErrorCode} from './src/code'; export {FatalDiagnosticError, isFatalDiagnosticError} from './src/error'; export {replaceTsWithNgInErrors} from './src/util'; diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts index 679e7f6299..c65416334d 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts @@ -19,4 +19,13 @@ export enum ErrorCode { COMPONENT_MISSING_TEMPLATE = 2001, PIPE_MISSING_NAME = 2002, PARAM_MISSING_TOKEN = 2003, + + SYMBOL_NOT_EXPORTED = 3001, + SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002, + + CONFIG_FLAT_MODULE_NO_INDEX = 4001, +} + +export function ngErrorCode(code: ErrorCode): number { + return parseInt('-99' + code); } diff --git a/packages/compiler-cli/src/ngtsc/entry_point/BUILD.bazel b/packages/compiler-cli/src/ngtsc/entry_point/BUILD.bazel index d18b2ec729..c0c7c991bf 100644 --- a/packages/compiler-cli/src/ngtsc/entry_point/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/entry_point/BUILD.bazel @@ -10,6 +10,7 @@ ts_library( ]), module_name = "@angular/compiler-cli/src/ngtsc/entry_point", deps = [ + "//packages/compiler-cli/src/ngtsc/diagnostics", "//packages/compiler-cli/src/ngtsc/shims", "//packages/compiler-cli/src/ngtsc/util", "@ngdeps//@types/node", diff --git a/packages/compiler-cli/src/ngtsc/entry_point/index.ts b/packages/compiler-cli/src/ngtsc/entry_point/index.ts index 128dc708a0..fa4acebeac 100644 --- a/packages/compiler-cli/src/ngtsc/entry_point/index.ts +++ b/packages/compiler-cli/src/ngtsc/entry_point/index.ts @@ -7,3 +7,6 @@ */ export {FlatIndexGenerator} from './src/generator'; +export {findFlatIndexEntryPoint} from './src/logic'; +export {checkForPrivateExports} from './src/private_export_checker'; +export {ReferenceGraph} from './src/reference_graph'; diff --git a/packages/compiler-cli/src/ngtsc/entry_point/src/generator.ts b/packages/compiler-cli/src/ngtsc/entry_point/src/generator.ts index e961188eeb..cb01e6a60f 100644 --- a/packages/compiler-cli/src/ngtsc/entry_point/src/generator.ts +++ b/packages/compiler-cli/src/ngtsc/entry_point/src/generator.ts @@ -12,43 +12,18 @@ import * as path from 'path'; import * as ts from 'typescript'; import {ShimGenerator} from '../../shims'; -import {isNonDeclarationTsPath} from '../../util/src/typescript'; export class FlatIndexGenerator implements ShimGenerator { readonly flatIndexPath: string; - private constructor( - relativeFlatIndexPath: string, readonly entryPoint: string, + constructor( + readonly entryPoint: string, relativeFlatIndexPath: string, readonly moduleName: string|null) { this.flatIndexPath = path.posix.join(path.posix.dirname(entryPoint), relativeFlatIndexPath) .replace(/\.js$/, '') + '.ts'; } - static forRootFiles(flatIndexPath: string, files: ReadonlyArray, moduleName: string|null): - FlatIndexGenerator|null { - // If there's only one .ts file in the program, it's the entry. Otherwise, look for the shortest - // (in terms of characters in the filename) file that ends in /index.ts. The second behavior is - // deprecated; users should always explicitly specify a single .ts entrypoint. - const tsFiles = files.filter(file => isNonDeclarationTsPath(file)); - if (tsFiles.length === 1) { - return new FlatIndexGenerator(flatIndexPath, tsFiles[0], moduleName); - } else { - let indexFile: string|null = null; - for (const tsFile of tsFiles) { - if (tsFile.endsWith('/index.ts') && - (indexFile === null || tsFile.length <= indexFile.length)) { - indexFile = tsFile; - } - } - if (indexFile !== null) { - return new FlatIndexGenerator(flatIndexPath, indexFile, moduleName); - } else { - return null; - } - } - } - recognize(fileName: string): boolean { return fileName === this.flatIndexPath; } generate(): ts.SourceFile { diff --git a/packages/compiler-cli/src/ngtsc/entry_point/src/logic.ts b/packages/compiler-cli/src/ngtsc/entry_point/src/logic.ts new file mode 100644 index 0000000000..de233eda36 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/entry_point/src/logic.ts @@ -0,0 +1,34 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {isNonDeclarationTsPath} from '../../util/src/typescript'; + +export function findFlatIndexEntryPoint(rootFiles: ReadonlyArray): string|null { + // There are two ways for a file to be recognized as the flat module index: + // 1) if it's the only file!!!!!! + // 2) (deprecated) if it's named 'index.ts' and has the shortest path of all such files. + const tsFiles = rootFiles.filter(file => isNonDeclarationTsPath(file)); + if (tsFiles.length === 1) { + // There's only one file - this is the flat module index. + return tsFiles[0]; + } else { + // In the event there's more than one TS file, one of them can still be selected as the + // flat module index if it's named 'index.ts'. If there's more than one 'index.ts', the one + // with the shortest path wins. + // + // This behavior is DEPRECATED and only exists to support existing usages. + let indexFile: string|null = null; + for (const tsFile of tsFiles) { + if (tsFile.endsWith('/index.ts') && + (indexFile === null || tsFile.length <= indexFile.length)) { + indexFile = tsFile; + } + } + return indexFile; + } +} diff --git a/packages/compiler-cli/src/ngtsc/entry_point/src/private_export_checker.ts b/packages/compiler-cli/src/ngtsc/entry_point/src/private_export_checker.ts new file mode 100644 index 0000000000..06e04d4cd5 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/entry_point/src/private_export_checker.ts @@ -0,0 +1,147 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import * as ts from 'typescript'; + +import {ErrorCode, ngErrorCode} from '../../diagnostics'; + +import {ReferenceGraph} from './reference_graph'; + +/** + * Produce `ts.Diagnostic`s for classes that are visible from exported types (e.g. directives + * exposed by exported `NgModule`s) that are not themselves exported. + * + * This function reconciles two concepts: + * + * A class is Exported if it's exported from the main library `entryPoint` file. + * A class is Visible if, via Angular semantics, a downstream consumer can import an Exported class + * and be affected by the class in question. For example, an Exported NgModule may expose a + * directive class to its consumers. Consumers that import the NgModule may have the directive + * applied to elements in their templates. In this case, the directive is considered Visible. + * + * `checkForPrivateExports` attempts to verify that all Visible classes are Exported, and report + * `ts.Diagnostic`s for those that aren't. + * + * @param entryPoint `ts.SourceFile` of the library's entrypoint, which should export the library's + * public API. + * @param checker `ts.TypeChecker` for the current program. + * @param refGraph `ReferenceGraph` tracking the visibility of Angular types. + * @returns an array of `ts.Diagnostic`s representing errors when visible classes are not exported + * properly. + */ +export function checkForPrivateExports( + entryPoint: ts.SourceFile, checker: ts.TypeChecker, refGraph: ReferenceGraph): ts.Diagnostic[] { + const diagnostics: ts.Diagnostic[] = []; + + // Firstly, compute the exports of the entry point. These are all the Exported classes. + const topLevelExports = new Set(); + + // Do this via `ts.TypeChecker.getExportsOfModule`. + const moduleSymbol = checker.getSymbolAtLocation(entryPoint); + if (moduleSymbol === undefined) { + throw new Error(`Internal error: failed to get symbol for entrypoint`); + } + const exportedSymbols = checker.getExportsOfModule(moduleSymbol); + + // Loop through the exported symbols, de-alias if needed, and add them to `topLevelExports`. + // TODO(alxhub): use proper iteration when build.sh is removed. (#27762) + exportedSymbols.forEach(symbol => { + if (symbol.flags & ts.SymbolFlags.Alias) { + symbol = checker.getAliasedSymbol(symbol); + } + const decl = symbol.valueDeclaration; + if (decl !== undefined) { + topLevelExports.add(decl); + } + }); + + // Next, go through each exported class and expand it to the set of classes it makes Visible, + // using the `ReferenceGraph`. For each Visible class, verify that it's also Exported, and queue + // an error if it isn't. `checkedSet` ensures only one error is queued per class. + const checkedSet = new Set(); + + // Loop through each Exported class. + // TODO(alxhub): use proper iteration when the legacy build is removed. (#27762) + topLevelExports.forEach(mainExport => { + // Loop through each class made Visible by the Exported class. + refGraph.transitiveReferencesOf(mainExport).forEach(transitiveReference => { + // Skip classes which have already been checked. + if (checkedSet.has(transitiveReference)) { + return; + } + checkedSet.add(transitiveReference); + + // Verify that the Visible class is also Exported. + if (!topLevelExports.has(transitiveReference)) { + // This is an error, `mainExport` makes `transitiveReference` Visible, but + // `transitiveReference` is not Exported from the entrypoint. Construct a diagnostic to + // give to the user explaining the situation. + + const descriptor = getDescriptorOfDeclaration(transitiveReference); + const name = getNameOfDeclaration(transitiveReference); + + // Construct the path of visibility, from `mainExport` to `transitiveReference`. + let visibleVia = 'NgModule exports'; + const transitivePath = refGraph.pathFrom(mainExport, transitiveReference); + if (transitivePath !== null) { + visibleVia = transitivePath.map(seg => getNameOfDeclaration(seg)).join(' -> '); + } + + const diagnostic: ts.Diagnostic = { + category: ts.DiagnosticCategory.Error, + code: ngErrorCode(ErrorCode.SYMBOL_NOT_EXPORTED), + file: transitiveReference.getSourceFile(), ...getPosOfDeclaration(transitiveReference), + messageText: + `Unsupported private ${descriptor} ${name}. This ${descriptor} is visible to consumers via ${visibleVia}, but is not exported from the top-level library entrypoint.`, + }; + + diagnostics.push(diagnostic); + } + }); + }); + + return diagnostics; +} + +function getPosOfDeclaration(decl: ts.Declaration): {start: number, length: number} { + const node: ts.Node = getIdentifierOfDeclaration(decl) || decl; + return { + start: node.getStart(), + length: node.getEnd() + 1 - node.getStart(), + }; +} + +function getIdentifierOfDeclaration(decl: ts.Declaration): ts.Identifier|null { + if ((ts.isClassDeclaration(decl) || ts.isVariableDeclaration(decl) || + ts.isFunctionDeclaration(decl)) && + decl.name !== undefined && ts.isIdentifier(decl.name)) { + return decl.name; + } else { + return null; + } +} + +function getNameOfDeclaration(decl: ts.Declaration): string { + const id = getIdentifierOfDeclaration(decl); + return id !== null ? id.text : '(unnamed)'; +} + +function getDescriptorOfDeclaration(decl: ts.Declaration): string { + switch (decl.kind) { + case ts.SyntaxKind.ClassDeclaration: + return 'class'; + case ts.SyntaxKind.FunctionDeclaration: + return 'function'; + case ts.SyntaxKind.VariableDeclaration: + return 'variable'; + case ts.SyntaxKind.EnumDeclaration: + return 'enum'; + default: + return 'declaration'; + } +} diff --git a/packages/compiler-cli/src/ngtsc/entry_point/src/reference_graph.ts b/packages/compiler-cli/src/ngtsc/entry_point/src/reference_graph.ts new file mode 100644 index 0000000000..396e90c5e9 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/entry_point/src/reference_graph.ts @@ -0,0 +1,78 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import * as ts from 'typescript'; + +export class ReferenceGraph { + private references = new Map>(); + + add(from: T, to: T): void { + if (!this.references.has(from)) { + this.references.set(from, new Set()); + } + this.references.get(from) !.add(to); + } + + transitiveReferencesOf(target: T): Set { + const set = new Set(); + this.collectTransitiveReferences(set, target); + return set; + } + + pathFrom(source: T, target: T): T[]|null { + return this.collectPathFrom(source, target, new Set()); + } + + private collectPathFrom(source: T, target: T, seen: Set): T[]|null { + if (source === target) { + // Looking for a path from the target to itself - that path is just the target. This is the + // "base case" of the search. + return [target]; + } else if (seen.has(source)) { + // The search has already looked through this source before. + return null; + } + // Consider outgoing edges from `source`. + seen.add(source); + + if (!this.references.has(source)) { + // There are no outgoing edges from `source`. + return null; + } else { + // Look through the outgoing edges of `source`. + // TODO(alxhub): use proper iteration when the legacy build is removed. (#27762) + let candidatePath: T[]|null = null; + this.references.get(source) !.forEach(edge => { + // Early exit if a path has already been found. + if (candidatePath !== null) { + return; + } + // Look for a path from this outgoing edge to `target`. + const partialPath = this.collectPathFrom(edge, target, seen); + if (partialPath !== null) { + // A path exists from `edge` to `target`. Insert `source` at the beginning. + candidatePath = [source, ...partialPath]; + } + }); + + return candidatePath; + } + } + + private collectTransitiveReferences(set: Set, decl: T): void { + if (this.references.has(decl)) { + // TODO(alxhub): use proper iteration when the legacy build is removed. (#27762) + this.references.get(decl) !.forEach(ref => { + if (!set.has(ref)) { + set.add(ref); + this.collectTransitiveReferences(set, ref); + } + }); + } + } +} diff --git a/packages/compiler-cli/src/ngtsc/entry_point/test/BUILD.bazel b/packages/compiler-cli/src/ngtsc/entry_point/test/BUILD.bazel new file mode 100644 index 0000000000..e149fa0f33 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/entry_point/test/BUILD.bazel @@ -0,0 +1,25 @@ +package(default_visibility = ["//visibility:public"]) + +load("//tools:defaults.bzl", "jasmine_node_test", "ts_library") + +ts_library( + name = "test_lib", + testonly = True, + srcs = glob([ + "**/*.ts", + ]), + deps = [ + "//packages:types", + "//packages/compiler-cli/src/ngtsc/entry_point", + "@ngdeps//typescript", + ], +) + +jasmine_node_test( + name = "test", + bootstrap = ["angular/tools/testing/init_node_no_angular_spec.js"], + deps = [ + ":test_lib", + "//tools/testing:node_no_angular", + ], +) diff --git a/packages/compiler-cli/src/ngtsc/entry_point/test/reference_graph_spec.ts b/packages/compiler-cli/src/ngtsc/entry_point/test/reference_graph_spec.ts new file mode 100644 index 0000000000..78be9e7e6b --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/entry_point/test/reference_graph_spec.ts @@ -0,0 +1,50 @@ + +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {ReferenceGraph} from '../src/reference_graph'; + +describe('entry_point reference graph', () => { + let graph: ReferenceGraph; + + const refs = + (target: string) => { return Array.from(graph.transitiveReferencesOf(target)).sort(); }; + + beforeEach(() => { + graph = new ReferenceGraph(); + graph.add('origin', 'alpha'); + graph.add('alpha', 'beta'); + graph.add('beta', 'gamma'); + }); + + it('should track a simple chain of references', () => { + // origin -> alpha -> beta -> gamma + expect(refs('origin')).toEqual(['alpha', 'beta', 'gamma']); + expect(refs('beta')).toEqual(['gamma']); + }); + + it('should not crash on a cycle', () => { + // origin -> alpha -> beta -> gamma + // ^---------------/ + graph.add('beta', 'origin'); + expect(refs('origin')).toEqual(['alpha', 'beta', 'gamma', 'origin']); + }); + + it('should report a path between two nodes in the graph', () => { + // ,------------------------\ + // origin -> alpha -> beta -> gamma -> delta + // \----------------^ + graph.add('beta', 'delta'); + graph.add('delta', 'alpha'); + expect(graph.pathFrom('origin', 'gamma')).toEqual(['origin', 'alpha', 'beta', 'gamma']); + expect(graph.pathFrom('beta', 'alpha')).toEqual(['beta', 'delta', 'alpha']); + }); + + it('should not report a path that doesn\'t exist', + () => { expect(graph.pathFrom('gamma', 'beta')).toBeNull(); }); +}); diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 1cb8599b32..ae54925907 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -12,9 +12,11 @@ import * as ts from 'typescript'; import * as api from '../transformers/api'; import {nocollapseHack} from '../transformers/nocollapse_hack'; -import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, NoopReferencesRegistry, PipeDecoratorHandler, ResourceLoader, SelectorScopeRegistry} from './annotations'; +import {ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, NoopReferencesRegistry, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader, SelectorScopeRegistry} from './annotations'; import {BaseDefDecoratorHandler} from './annotations/src/base_def'; -import {FlatIndexGenerator} from './entry_point'; +import {ErrorCode, ngErrorCode} from './diagnostics'; +import {FlatIndexGenerator, ReferenceGraph, checkForPrivateExports, findFlatIndexEntryPoint} from './entry_point'; +import {Reference} from './imports'; import {PartialEvaluator} from './partial_evaluator'; import {TypeScriptReflectionHost} from './reflection'; import {FileResourceLoader, HostResourceLoader} from './resource_loader'; @@ -22,6 +24,7 @@ import {FactoryGenerator, FactoryInfo, GeneratedShimsHostWrapper, ShimGenerator, import {ivySwitchTransform} from './switch'; import {IvyCompilation, ivyTransformFactory} from './transform'; import {TypeCheckContext, TypeCheckProgramHost} from './typecheck'; +import {isDtsPath} from './util/src/typescript'; export class NgtscProgram implements api.Program { private tsProgram: ts.Program; @@ -35,6 +38,11 @@ export class NgtscProgram implements api.Program { private _isCore: boolean|undefined = undefined; private rootDirs: string[]; private closureCompilerEnabled: boolean; + private entryPoint: ts.SourceFile|null; + private exportReferenceGraph: ReferenceGraph|null = null; + private flatIndexGenerator: FlatIndexGenerator|null = null; + + private constructionDiagnostics: ts.Diagnostic[] = []; constructor( @@ -79,14 +87,10 @@ export class NgtscProgram implements api.Program { generators.push(summaryGenerator, factoryGenerator); } + let entryPoint: string|null = null; if (options.flatModuleOutFile !== undefined) { - const flatModuleId = options.flatModuleId || null; - const flatIndexGenerator = - FlatIndexGenerator.forRootFiles(options.flatModuleOutFile, rootNames, flatModuleId); - if (flatIndexGenerator !== null) { - generators.push(flatIndexGenerator); - rootFiles.push(flatIndexGenerator.flatIndexPath); - } else { + entryPoint = findFlatIndexEntryPoint(rootNames); + if (entryPoint === null) { // This error message talks specifically about having a single .ts file in "files". However // the actual logic is a bit more permissive. If a single file exists, that will be taken, // otherwise the highest level (shortest path) "index.ts" file will be used as the flat @@ -95,8 +99,21 @@ export class NgtscProgram implements api.Program { // // The user is not informed about the "index.ts" option as this behavior is deprecated - // an explicit entrypoint should always be specified. - throw new Error( - 'Angular compiler option "flatModuleIndex" requires one and only one .ts file in the "files" field.'); + this.constructionDiagnostics.push({ + category: ts.DiagnosticCategory.Error, + code: ngErrorCode(ErrorCode.CONFIG_FLAT_MODULE_NO_INDEX), + file: undefined, + start: undefined, + length: undefined, + messageText: + 'Angular compiler option "flatModuleOutFile" requires one and only one .ts file in the "files" field.', + }); + } else { + const flatModuleId = options.flatModuleId || null; + this.flatIndexGenerator = + new FlatIndexGenerator(entryPoint, options.flatModuleOutFile, flatModuleId); + generators.push(this.flatIndexGenerator); + rootFiles.push(this.flatIndexGenerator.flatIndexPath); } } @@ -106,6 +123,8 @@ export class NgtscProgram implements api.Program { this.tsProgram = ts.createProgram(rootFiles, options, this.host, oldProgram && oldProgram.getTsProgram()); + + this.entryPoint = entryPoint !== null ? this.tsProgram.getSourceFile(entryPoint) || null : null; } getTsProgram(): ts.Program { return this.tsProgram; } @@ -116,8 +135,8 @@ export class NgtscProgram implements api.Program { } getNgOptionDiagnostics(cancellationToken?: ts.CancellationToken| - undefined): ReadonlyArray { - return []; + undefined): ReadonlyArray { + return this.constructionDiagnostics; } getTsSyntacticDiagnostics( @@ -147,6 +166,10 @@ export class NgtscProgram implements api.Program { compilation.typeCheck(ctx); diagnostics.push(...this.compileTypeCheckProgram(ctx)); } + if (this.entryPoint !== null && this.exportReferenceGraph !== null) { + diagnostics.push(...checkForPrivateExports( + this.entryPoint, this.tsProgram.getTypeChecker(), this.exportReferenceGraph)); + } return diagnostics; } @@ -253,7 +276,17 @@ export class NgtscProgram implements api.Program { const checker = this.tsProgram.getTypeChecker(); const evaluator = new PartialEvaluator(this.reflector, checker); const scopeRegistry = new SelectorScopeRegistry(checker, this.reflector); - const referencesRegistry = new NoopReferencesRegistry(); + + // If a flat module entrypoint was specified, then track references via a `ReferenceGraph` in + // order to produce proper diagnostics for incorrectly exported directives/pipes/etc. If there + // is no flat module entrypoint then don't pay the cost of tracking references. + let referencesRegistry: ReferencesRegistry; + if (this.entryPoint !== null) { + this.exportReferenceGraph = new ReferenceGraph(); + referencesRegistry = new ReferenceGraphAdapter(this.exportReferenceGraph); + } else { + referencesRegistry = new NoopReferencesRegistry(); + } // Set up the IvyCompilation, which manages state for the Ivy transformer. const handlers = [ @@ -355,3 +388,21 @@ function isAngularCorePackage(program: ts.Program): boolean { }); }); } + +export class ReferenceGraphAdapter implements ReferencesRegistry { + constructor(private graph: ReferenceGraph) {} + + add(source: ts.Declaration, ...references: Reference[]): void { + for (const {node} of references) { + let sourceFile = node.getSourceFile(); + if (sourceFile === undefined) { + sourceFile = ts.getOriginalNode(node).getSourceFile(); + } + + // Only record local references (not references into .d.ts files). + if (sourceFile === undefined || !isDtsPath(sourceFile.fileName)) { + this.graph.add(source, node); + } + } + } +} diff --git a/packages/compiler-cli/src/transformers/api.ts b/packages/compiler-cli/src/transformers/api.ts index 419596a0c8..b0fa3e193e 100644 --- a/packages/compiler-cli/src/transformers/api.ts +++ b/packages/compiler-cli/src/transformers/api.ts @@ -307,7 +307,8 @@ export interface Program { /** * Retrieve options diagnostics for the Angular options used to create the program. */ - getNgOptionDiagnostics(cancellationToken?: ts.CancellationToken): ReadonlyArray; + getNgOptionDiagnostics(cancellationToken?: ts.CancellationToken): + ReadonlyArray; /** * Retrieve the syntax diagnostics from TypeScript. This is faster than calling diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index b5441c4894..8fe06755e3 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -6,6 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ +import * as ts from 'typescript'; + import {NgtscTestEnvironment} from './env'; const trim = (input: string): string => input.replace(/\s+/g, ' ').trim(); @@ -1252,6 +1254,155 @@ describe('ngtsc behavioral tests', () => { const dtsContents = env.getContents('flat.d.ts'); expect(dtsContents).toContain('/// '); }); + + it('should report an error when a flat module index is requested but no entrypoint can be determined', + () => { + env.tsconfig({'flatModuleOutFile': 'flat.js'}); + env.write('test.ts', 'export class Foo {}'); + env.write('test2.ts', 'export class Bar {}'); + + const errors = env.driveDiagnostics(); + expect(errors.length).toBe(1); + expect(errors[0].messageText) + .toBe( + 'Angular compiler option "flatModuleOutFile" requires one and only one .ts file in the "files" field.'); + }); + + it('should report an error when a visible directive is not exported', () => { + env.tsconfig({'flatModuleOutFile': 'flat.js'}); + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + + // The directive is not exported. + @Directive({selector: 'test'}) + class Dir {} + + // The module is, which makes the directive visible. + @NgModule({declarations: [Dir], exports: [Dir]}) + export class Module {} + `); + + const errors = env.driveDiagnostics(); + expect(errors.length).toBe(1); + expect(errors[0].messageText) + .toBe( + 'Unsupported private class Dir. This class is visible ' + + 'to consumers via Module -> Dir, but is not exported from the top-level library ' + + 'entrypoint.'); + + // Verify that the error is for the correct class. + const id = expectTokenAtPosition(errors[0].file !, errors[0].start !, ts.isIdentifier); + expect(id.text).toBe('Dir'); + expect(ts.isClassDeclaration(id.parent)).toBe(true); + }); + + it('should report an error when a deeply visible directive is not exported', () => { + env.tsconfig({'flatModuleOutFile': 'flat.js'}); + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + + // The directive is not exported. + @Directive({selector: 'test'}) + class Dir {} + + // Neither is the module which declares it - meaning the directive is not visible here. + @NgModule({declarations: [Dir], exports: [Dir]}) + class DirModule {} + + // The module is, which makes the directive visible. + @NgModule({exports: [DirModule]}) + export class Module {} + `); + + const errors = env.driveDiagnostics(); + expect(errors.length).toBe(2); + expect(errors[0].messageText) + .toBe( + 'Unsupported private class DirModule. This class is ' + + 'visible to consumers via Module -> DirModule, but is not exported from the top-level ' + + 'library entrypoint.'); + expect(errors[1].messageText) + .toBe( + 'Unsupported private class Dir. This class is visible ' + + 'to consumers via Module -> DirModule -> Dir, but is not exported from the top-level ' + + 'library entrypoint.'); + }); + + it('should report an error when a deeply visible module is not exported', () => { + env.tsconfig({'flatModuleOutFile': 'flat.js'}); + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + + // The directive is exported. + @Directive({selector: 'test'}) + export class Dir {} + + // The module which declares it is not. + @NgModule({declarations: [Dir], exports: [Dir]}) + class DirModule {} + + // The module is, which makes the module and directive visible. + @NgModule({exports: [DirModule]}) + export class Module {} + `); + + const errors = env.driveDiagnostics(); + expect(errors.length).toBe(1); + expect(errors[0].messageText) + .toBe( + 'Unsupported private class DirModule. This class is ' + + 'visible to consumers via Module -> DirModule, but is not exported from the top-level ' + + 'library entrypoint.'); + }); + + it('should not report an error when a non-exported module is imported by a visible one', () => { + env.tsconfig({'flatModuleOutFile': 'flat.js'}); + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + + // The directive is not exported. + @Directive({selector: 'test'}) + class Dir {} + + // Neither is the module which declares it. + @NgModule({declarations: [Dir], exports: [Dir]}) + class DirModule {} + + // This module is, but it doesn't re-export the module, so it doesn't make the module and + // directive visible. + @NgModule({imports: [DirModule]}) + export class Module {} + `); + + const errors = env.driveDiagnostics(); + expect(errors.length).toBe(0); + }); + + it('should not report an error when re-exporting an external symbol', () => { + env.tsconfig({'flatModuleOutFile': 'flat.js'}); + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + import {ExternalModule} from 'external'; + + // This module makes ExternalModule and ExternalDir visible. + @NgModule({exports: [ExternalModule]}) + export class Module {} + `); + env.write('node_modules/external/index.d.ts', ` + import {ɵDirectiveDefWithMeta, ɵNgModuleDefWithMeta} from '@angular/core'; + + export declare class ExternalDir { + static ngDirectiveDef: ɵDirectiveDefWithMeta; + } + + export declare class ExternalModule { + static ngModuleDef: ɵNgModuleDefWithMeta; + } + `); + + const errors = env.driveDiagnostics(); + expect(errors.length).toBe(0); + }); }); it('should execute custom transformers', () => { @@ -1282,3 +1433,11 @@ describe('ngtsc behavioral tests', () => { }); }); + +function expectTokenAtPosition( + sf: ts.SourceFile, pos: number, guard: (node: ts.Node) => node is T): T { + // getTokenAtPosition is part of TypeScript's private API. + const node = (ts as any).getTokenAtPosition(sf, pos) as ts.Node; + expect(guard(node)).toBe(true); + return node as T; +}