fix(ivy): ngcc should only index .d.ts exports within the package (#32129)

ngcc needs to solve a unique problem when compiling typings for an
entrypoint: it must resolve a declaration within a .js file to its
representation in a .d.ts file. Since such .d.ts files can be used in deep
imports without ever being referenced from the "root" .d.ts, it's not enough
to simply match exported types to the root .d.ts. ngcc must build an index
of all .d.ts files.

Previously, this operation had a bug: it scanned all .d.ts files in the
.d.ts program, not only those within the package. Thus, if a class in the
program happened to share a name with a class exported from a dependency's
.d.ts, ngcc might accidentally modify the wrong .d.ts file, causing a
variety of issues downstream.

To fix this issue, ngcc's .d.ts scanner now limits the .d.ts files it
indexes to only those declared in the current package.

PR Close #32129
This commit is contained in:
Alex Rickabaugh 2019-08-13 17:02:44 -07:00 committed by Andrew Kushnir
parent 02bab8cf90
commit 964d72610f
6 changed files with 98 additions and 57 deletions

View File

@ -10,6 +10,7 @@ import * as ts from 'typescript';
import {AbsoluteFsPath} from '../../../src/ngtsc/file_system';
import {ClassDeclaration, ClassMember, ClassMemberKind, ClassSymbol, ConcreteDeclaration, CtorParameter, Declaration, Decorator, TypeScriptReflectionHost, isDecoratorIdentifier, reflectObjectLiteral} from '../../../src/ngtsc/reflection';
import {isWithinPackage} from '../analysis/util';
import {Logger} from '../logging/logger';
import {BundleProgram} from '../packages/bundle_program';
import {findAll, getNameText, hasNameIdentifier, isDefined, stripDollarSuffix} from '../utils';
@ -85,7 +86,8 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
protected logger: Logger, protected isCore: boolean, checker: ts.TypeChecker,
dts?: BundleProgram|null) {
super(checker);
this.dtsDeclarationMap = dts && this.computeDtsDeclarationMap(dts.path, dts.program) || null;
this.dtsDeclarationMap =
dts && this.computeDtsDeclarationMap(dts.path, dts.program, dts.package) || null;
}
/**
@ -1347,8 +1349,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
* @param dtsProgram The program containing all the typings files.
* @returns a map of class names to class declarations.
*/
protected computeDtsDeclarationMap(dtsRootFileName: AbsoluteFsPath, dtsProgram: ts.Program):
Map<string, ts.Declaration> {
protected computeDtsDeclarationMap(
dtsRootFileName: AbsoluteFsPath, dtsProgram: ts.Program,
dtsPackage: AbsoluteFsPath): Map<string, ts.Declaration> {
const dtsDeclarationMap = new Map<string, ts.Declaration>();
const checker = dtsProgram.getTypeChecker();
@ -1361,8 +1364,12 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
// Now add any additional classes that are exported from individual dts files,
// but are not publicly exported from the entry-point.
dtsProgram.getSourceFiles().forEach(
sourceFile => { collectExportedDeclarations(checker, dtsDeclarationMap, sourceFile); });
dtsProgram.getSourceFiles().forEach(sourceFile => {
if (!isWithinPackage(dtsPackage, sourceFile)) {
return;
}
collectExportedDeclarations(checker, dtsDeclarationMap, sourceFile);
});
return dtsDeclarationMap;
}

View File

@ -23,6 +23,7 @@ export interface BundleProgram {
host: ts.CompilerHost;
path: AbsoluteFsPath;
file: ts.SourceFile;
package: AbsoluteFsPath;
r3SymbolsPath: AbsoluteFsPath|null;
r3SymbolsFile: ts.SourceFile|null;
}
@ -31,7 +32,7 @@ export interface BundleProgram {
* Create a bundle program.
*/
export function makeBundleProgram(
fs: FileSystem, isCore: boolean, path: AbsoluteFsPath, r3FileName: string,
fs: FileSystem, isCore: boolean, pkg: AbsoluteFsPath, path: AbsoluteFsPath, r3FileName: string,
options: ts.CompilerOptions, host: ts.CompilerHost,
additionalFiles: AbsoluteFsPath[] = []): BundleProgram {
const r3SymbolsPath = isCore ? findR3SymbolsPath(fs, dirname(path), r3FileName) : null;
@ -48,7 +49,7 @@ export function makeBundleProgram(
const file = program.getSourceFile(path) !;
const r3SymbolsFile = r3SymbolsPath && program.getSourceFile(r3SymbolsPath) || null;
return {program, options, host, path, file, r3SymbolsPath, r3SymbolsFile};
return {program, options, host, package: pkg, path, file, r3SymbolsPath, r3SymbolsFile};
}
/**

View File

@ -57,14 +57,15 @@ export function makeEntryPointBundle(
// Create the bundle programs, as necessary.
const absFormatPath = fs.resolve(entryPoint.path, formatPath);
const typingsPath = fs.resolve(entryPoint.path, entryPoint.typings);
const src = makeBundleProgram(fs, isCore, absFormatPath, 'r3_symbols.js', options, srcHost);
const src = makeBundleProgram(
fs, isCore, entryPoint.package, absFormatPath, 'r3_symbols.js', options, srcHost);
const additionalDtsFiles = transformDts && mirrorDtsFromSrc ?
computePotentialDtsFilesFromJsFiles(fs, src.program, absFormatPath, typingsPath) :
[];
const dts = transformDts ?
makeBundleProgram(
fs, isCore, typingsPath, 'r3_symbols.d.ts', options, dtsHost, additionalDtsFiles) :
null;
const dts = transformDts ? makeBundleProgram(
fs, isCore, entryPoint.package, typingsPath, 'r3_symbols.d.ts',
options, dtsHost, additionalDtsFiles) :
null;
const isFlatCore = isCore && src.r3SymbolsFile === null;
return {entryPoint, format, rootDirs, isCore, isFlatCore, src, dts};

View File

@ -36,28 +36,31 @@ export function makeTestEntryPointBundle(
dtsRootNames?: AbsoluteFsPath[]): EntryPointBundle {
const entryPoint = makeTestEntryPoint(packageName);
const src = makeTestBundleProgram(srcRootNames[0], isCore);
const dts = dtsRootNames ? makeTestDtsBundleProgram(dtsRootNames[0], isCore) : null;
const dts =
dtsRootNames ? makeTestDtsBundleProgram(dtsRootNames[0], entryPoint.package, isCore) : null;
const isFlatCore = isCore && src.r3SymbolsFile === null;
return {entryPoint, format, rootDirs: [absoluteFrom('/')], src, dts, isCore, isFlatCore};
}
export function makeTestBundleProgram(
path: AbsoluteFsPath, isCore: boolean = false): BundleProgram {
path: AbsoluteFsPath, isCore: boolean = false,
additionalFiles?: AbsoluteFsPath[]): BundleProgram {
const fs = getFileSystem();
const entryPointPath = fs.dirname(path);
const rootDir = fs.dirname(entryPointPath);
const options: ts.CompilerOptions =
{allowJs: true, maxNodeModuleJsDepth: Infinity, checkJs: false, rootDir, rootDirs: [rootDir]};
const host = new NgccSourcesCompilerHost(fs, options, entryPointPath);
return makeBundleProgram(fs, isCore, path, 'r3_symbols.js', options, host);
return makeBundleProgram(
fs, isCore, rootDir, path, 'r3_symbols.js', options, host, additionalFiles);
}
export function makeTestDtsBundleProgram(
path: AbsoluteFsPath, isCore: boolean = false): BundleProgram {
path: AbsoluteFsPath, packagePath: AbsoluteFsPath, isCore: boolean = false): BundleProgram {
const fs = getFileSystem();
const options = {};
const host = new NgtscCompilerHost(fs, options);
return makeBundleProgram(fs, isCore, path, 'r3_symbols.d.ts', options, host);
return makeBundleProgram(fs, isCore, packagePath, path, 'r3_symbols.d.ts', options, host);
}
export function convertToDirectTsLibImport(filesystem: TestFile[]) {

View File

@ -1992,7 +1992,7 @@ exports.ExternalModule = ExternalModule;
loadTestFiles(TYPINGS_SRC_FILES);
loadTestFiles(TYPINGS_DTS_FILES);
const {program, host: compilerHost} = makeTestBundleProgram(_('/src/index.js'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'), _('/'));
const class1 =
getDeclaration(program, _('/src/class1.js'), 'Class1', ts.isVariableDeclaration);
const host =
@ -2006,7 +2006,7 @@ exports.ExternalModule = ExternalModule;
loadTestFiles(TYPINGS_SRC_FILES);
loadTestFiles(TYPINGS_DTS_FILES);
const {program, host: compilerHost} = makeTestBundleProgram(_('/src/index.js'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'), _('/'));
const mooFn =
getDeclaration(program, _('/src/func1.js'), 'mooFn', ts.isFunctionDeclaration);
const host =
@ -2019,7 +2019,7 @@ exports.ExternalModule = ExternalModule;
loadTestFiles(TYPINGS_SRC_FILES);
loadTestFiles(TYPINGS_DTS_FILES);
const {program, host: compilerHost} = makeTestBundleProgram(_('/src/index.js'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'), _('/'));
const missingClass = getDeclaration(
program, _('/src/class1.js'), 'MissingClass1', ts.isVariableDeclaration);
const host =
@ -2032,7 +2032,7 @@ exports.ExternalModule = ExternalModule;
loadTestFiles(TYPINGS_SRC_FILES);
loadTestFiles(TYPINGS_DTS_FILES);
const {program, host: compilerHost} = makeTestBundleProgram(_('/src/index.js'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'), _('/'));
const missingClass = getDeclaration(
program, _('/src/missing-class.js'), 'MissingClass2', ts.isVariableDeclaration);
const host =
@ -2046,7 +2046,7 @@ exports.ExternalModule = ExternalModule;
loadTestFiles(TYPINGS_SRC_FILES);
loadTestFiles(TYPINGS_DTS_FILES);
const {program, host: compilerHost} = makeTestBundleProgram(_('/src/index.js'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'), _('/'));
const class1 = getDeclaration(
program, _('/src/flat-file.js'), 'Class1', ts.isVariableDeclaration);
const host =
@ -2060,7 +2060,7 @@ exports.ExternalModule = ExternalModule;
loadTestFiles(TYPINGS_SRC_FILES);
loadTestFiles(TYPINGS_DTS_FILES);
const {program, host: compilerHost} = makeTestBundleProgram(_('/src/index.js'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'), _('/'));
const class3 =
getDeclaration(program, _('/src/flat-file.js'), 'Class3', ts.isVariableDeclaration);
const host =
@ -2075,7 +2075,7 @@ exports.ExternalModule = ExternalModule;
loadTestFiles(TYPINGS_SRC_FILES);
loadTestFiles(TYPINGS_DTS_FILES);
const {program, host: compilerHost} = makeTestBundleProgram(_('/src/index.js'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'), _('/'));
const internalClass = getDeclaration(
program, _('/src/internal.js'), 'InternalClass', ts.isVariableDeclaration);
const host =
@ -2090,7 +2090,7 @@ exports.ExternalModule = ExternalModule;
loadTestFiles(TYPINGS_SRC_FILES);
loadTestFiles(TYPINGS_DTS_FILES);
const {program, host: compilerHost} = makeTestBundleProgram(_('/src/index.js'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'));
const dts = makeTestDtsBundleProgram(_('/typings/index.d.ts'), _('/'));
const class2 =
getDeclaration(program, _('/src/class2.js'), 'Class2', ts.isVariableDeclaration);
const internalClass2 =

View File

@ -539,8 +539,9 @@ runInEachFileSystem(() => {
TYPINGS_SRC_FILES = [
{
name: _('/src/index.js'),
name: _('/ep/src/index.js'),
contents: `
import 'an_external_lib';
import {InternalClass} from './internal';
import * as func1 from './func1';
import * as missing from './missing-class';
@ -550,25 +551,32 @@ runInEachFileSystem(() => {
`
},
{
name: _('/src/class1.js'),
name: _('/ep/src/class1.js'),
contents: 'export class Class1 {}\nexport class MissingClass1 {}'
},
{name: _('/src/class2.js'), contents: 'export class Class2 {}'},
{name: _('/src/func1.js'), contents: 'export function mooFn() {}'}, {
name: _('/src/internal.js'),
{name: _('/ep/src/class2.js'), contents: 'export class Class2 {}'},
{name: _('/ep/src/func1.js'), contents: 'export function mooFn() {}'},
{
name: _('/ep/src/internal.js'),
contents: 'export class InternalClass {}\nexport class Class2 {}'
},
{name: _('/src/missing-class.js'), contents: 'export class MissingClass2 {}'}, {
name: _('/src/flat-file.js'),
{name: _('/ep/src/missing-class.js'), contents: 'export class MissingClass2 {}'},
{
name: _('/ep/src/flat-file.js'),
contents:
'export class Class1 {}\nexport class MissingClass1 {}\nexport class MissingClass2 {}\class Class3 {}\nexport {Class3 as xClass3};',
}
},
{
name: _('/ep/src/shadow-class.js'),
contents: 'export class ShadowClass {}',
},
];
TYPINGS_DTS_FILES = [
{
name: _('/typings/index.d.ts'),
name: _('/ep/typings/index.d.ts'),
contents: `
import '../../an_external_lib/index';
import {InternalClass} from './internal';
import {mooFn} from './func1';
export * from './class1';
@ -576,22 +584,25 @@ runInEachFileSystem(() => {
`
},
{
name: _('/typings/class1.d.ts'),
name: _('/ep/typings/class1.d.ts'),
contents: `export declare class Class1 {}\nexport declare class OtherClass {}`
},
{
name: _('/typings/class2.d.ts'),
name: _('/ep/typings/class2.d.ts'),
contents:
`export declare class Class2 {}\nexport declare interface SomeInterface {}\nexport {Class3 as xClass3} from './class3';`
},
{name: _('/typings/func1.d.ts'), contents: 'export declare function mooFn(): void;'},
{name: _('/ep/typings/func1.d.ts'), contents: 'export declare function mooFn(): void;'},
{
name: _('/typings/internal.d.ts'),
name: _('/ep/typings/internal.d.ts'),
contents: `export declare class InternalClass {}\nexport declare class Class2 {}`
},
{name: _('/typings/class3.d.ts'), contents: `export declare class Class3 {}`},
{name: _('/ep/typings/class3.d.ts'), contents: `export declare class Class3 {}`},
{name: _('/ep/typings/shadow-class.d.ts'), contents: `export declare class ShadowClass {}`},
{name: _('/an_external_lib/index.d.ts'), contents: 'export declare class ShadowClass {}'},
];
MODULE_WITH_PROVIDERS_PROGRAM = [
{
name: _('/src/index.js'),
@ -1794,12 +1805,12 @@ runInEachFileSystem(() => {
const {program} = makeTestBundleProgram(getRootFiles(TYPINGS_SRC_FILES)[0]);
const dts = makeTestBundleProgram(getRootFiles(TYPINGS_DTS_FILES)[0]);
const class1 =
getDeclaration(program, _('/src/class1.js'), 'Class1', isNamedClassDeclaration);
getDeclaration(program, _('/ep/src/class1.js'), 'Class1', isNamedClassDeclaration);
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dts);
const dtsDeclaration = host.getDtsDeclaration(class1);
expect(dtsDeclaration !.getSourceFile().fileName).toEqual(_('/typings/class1.d.ts'));
expect(dtsDeclaration !.getSourceFile().fileName).toEqual(_('/ep/typings/class1.d.ts'));
});
it('should find the dts declaration for exported functions', () => {
@ -1808,12 +1819,12 @@ runInEachFileSystem(() => {
const {program} = makeTestBundleProgram(getRootFiles(TYPINGS_SRC_FILES)[0]);
const dts = makeTestBundleProgram(getRootFiles(TYPINGS_DTS_FILES)[0]);
const mooFn =
getDeclaration(program, _('/src/func1.js'), 'mooFn', isNamedFunctionDeclaration);
getDeclaration(program, _('/ep/src/func1.js'), 'mooFn', isNamedFunctionDeclaration);
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dts);
const dtsDeclaration = host.getDtsDeclaration(mooFn);
expect(dtsDeclaration !.getSourceFile().fileName).toEqual(_('/typings/func1.d.ts'));
expect(dtsDeclaration !.getSourceFile().fileName).toEqual(_('/ep/typings/func1.d.ts'));
});
it('should return null if there is no matching class in the matching dts file', () => {
@ -1821,8 +1832,8 @@ runInEachFileSystem(() => {
loadTestFiles(TYPINGS_DTS_FILES);
const {program} = makeTestBundleProgram(getRootFiles(TYPINGS_SRC_FILES)[0]);
const dts = makeTestBundleProgram(getRootFiles(TYPINGS_DTS_FILES)[0]);
const missingClass =
getDeclaration(program, _('/src/class1.js'), 'MissingClass1', isNamedClassDeclaration);
const missingClass = getDeclaration(
program, _('/ep/src/missing-class.js'), 'MissingClass2', isNamedClassDeclaration);
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dts);
@ -1835,26 +1846,43 @@ runInEachFileSystem(() => {
const {program} = makeTestBundleProgram(getRootFiles(TYPINGS_SRC_FILES)[0]);
const dts = makeTestBundleProgram(getRootFiles(TYPINGS_DTS_FILES)[0]);
const missingClass = getDeclaration(
program, _('/src/missing-class.js'), 'MissingClass2', isNamedClassDeclaration);
program, _('/ep/src/missing-class.js'), 'MissingClass2', isNamedClassDeclaration);
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dts);
expect(host.getDtsDeclaration(missingClass)).toBe(null);
});
it('should ignore dts files outside of the entrypoint', () => {
loadTestFiles(TYPINGS_SRC_FILES);
loadTestFiles(TYPINGS_DTS_FILES);
const {program} = makeTestBundleProgram(
getRootFiles(TYPINGS_SRC_FILES)[0], false, [_('/ep/src/shadow-class.js')]);
const dts = makeTestBundleProgram(
getRootFiles(TYPINGS_DTS_FILES)[0], false, [_('/ep/typings/shadow-class.d.ts')]);
const missingClass = getDeclaration(
program, _('/ep/src/shadow-class.js'), 'ShadowClass', isNamedClassDeclaration);
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dts);
const dtsDecl = host.getDtsDeclaration(missingClass) !;
expect(dtsDecl).not.toBeNull();
expect(dtsDecl.getSourceFile().fileName).toEqual(_('/ep/typings/shadow-class.d.ts'));
});
it('should find the dts file that contains a matching class declaration, even if the source files do not match',
() => {
loadTestFiles(TYPINGS_SRC_FILES);
loadTestFiles(TYPINGS_DTS_FILES);
const {program} = makeTestBundleProgram(getRootFiles(TYPINGS_SRC_FILES)[0]);
const dts = makeTestBundleProgram(getRootFiles(TYPINGS_DTS_FILES)[0]);
const class1 =
getDeclaration(program, _('/src/flat-file.js'), 'Class1', isNamedClassDeclaration);
const class1 = getDeclaration(
program, _('/ep/src/flat-file.js'), 'Class1', isNamedClassDeclaration);
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dts);
const dtsDeclaration = host.getDtsDeclaration(class1);
expect(dtsDeclaration !.getSourceFile().fileName).toEqual(_('/typings/class1.d.ts'));
expect(dtsDeclaration !.getSourceFile().fileName).toEqual(_('/ep/typings/class1.d.ts'));
});
it('should find aliased exports', () => {
@ -1863,12 +1891,12 @@ runInEachFileSystem(() => {
const {program} = makeTestBundleProgram(getRootFiles(TYPINGS_SRC_FILES)[0]);
const dts = makeTestBundleProgram(getRootFiles(TYPINGS_DTS_FILES)[0]);
const class3 =
getDeclaration(program, _('/src/flat-file.js'), 'Class3', isNamedClassDeclaration);
getDeclaration(program, _('/ep/src/flat-file.js'), 'Class3', isNamedClassDeclaration);
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dts);
const dtsDeclaration = host.getDtsDeclaration(class3);
expect(dtsDeclaration !.getSourceFile().fileName).toEqual(_('/typings/class3.d.ts'));
expect(dtsDeclaration !.getSourceFile().fileName).toEqual(_('/ep/typings/class3.d.ts'));
});
it('should find the dts file that contains a matching class declaration, even if the class is not publicly exported',
@ -1878,12 +1906,13 @@ runInEachFileSystem(() => {
const {program} = makeTestBundleProgram(getRootFiles(TYPINGS_SRC_FILES)[0]);
const dts = makeTestBundleProgram(getRootFiles(TYPINGS_DTS_FILES)[0]);
const internalClass = getDeclaration(
program, _('/src/internal.js'), 'InternalClass', isNamedClassDeclaration);
program, _('/ep/src/internal.js'), 'InternalClass', isNamedClassDeclaration);
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dts);
const dtsDeclaration = host.getDtsDeclaration(internalClass);
expect(dtsDeclaration !.getSourceFile().fileName).toEqual(_('/typings/internal.d.ts'));
expect(dtsDeclaration !.getSourceFile().fileName)
.toEqual(_('/ep/typings/internal.d.ts'));
});
it('should prefer the publicly exported class if there are multiple classes with the same name',
@ -1893,19 +1922,19 @@ runInEachFileSystem(() => {
const {program} = makeTestBundleProgram(getRootFiles(TYPINGS_SRC_FILES)[0]);
const dts = makeTestBundleProgram(getRootFiles(TYPINGS_DTS_FILES)[0]);
const class2 =
getDeclaration(program, _('/src/class2.js'), 'Class2', isNamedClassDeclaration);
getDeclaration(program, _('/ep/src/class2.js'), 'Class2', isNamedClassDeclaration);
const internalClass2 =
getDeclaration(program, _('/src/internal.js'), 'Class2', isNamedClassDeclaration);
getDeclaration(program, _('/ep/src/internal.js'), 'Class2', isNamedClassDeclaration);
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dts);
const class2DtsDeclaration = host.getDtsDeclaration(class2);
expect(class2DtsDeclaration !.getSourceFile().fileName)
.toEqual(_('/typings/class2.d.ts'));
.toEqual(_('/ep/typings/class2.d.ts'));
const internalClass2DtsDeclaration = host.getDtsDeclaration(internalClass2);
expect(internalClass2DtsDeclaration !.getSourceFile().fileName)
.toEqual(_('/typings/class2.d.ts'));
.toEqual(_('/ep/typings/class2.d.ts'));
});
});