Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix symbol merging of augmentations to pattern ambient modules #31078

Merged
merged 4 commits into from Apr 30, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 29 additions & 3 deletions src/compiler/checker.ts
Expand Up @@ -500,6 +500,7 @@ namespace ts {
* This is only used if there is no exact match.
*/
let patternAmbientModules: PatternAmbientModule[];
let patternAmbientModuleAugmentations: Map<Symbol> | undefined;

let globalObjectType: ObjectType;
let globalFunctionType: ObjectType;
Expand Down Expand Up @@ -890,7 +891,7 @@ namespace ts {
* Note: if target is transient, then it is mutable, and mergeSymbol with both mutate and return it.
* If target is not transient, mergeSymbol will produce a transient clone, mutate that and return it.
*/
function mergeSymbol(target: Symbol, source: Symbol): Symbol {
function mergeSymbol(target: Symbol, source: Symbol, unidirectional = false): Symbol {
if (!(target.flags & getExcludedSymbolFlags(source.flags)) ||
(source.flags | target.flags) & SymbolFlags.Assignment) {
Debug.assert(source !== target);
Expand Down Expand Up @@ -923,7 +924,9 @@ namespace ts {
if (!target.exports) target.exports = createSymbolTable();
mergeSymbolTable(target.exports, source.exports);
}
recordMergedSymbol(target, source);
if (!unidirectional) {
recordMergedSymbol(target, source);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandersn you were right that the merged symbols essentially go both ways (target and source don't share a mergeId, but target gets there by cloneSymbol(resolvedTarget) and being mutated; then source gets added here.) Introducing a unidirectonal option to prevent adding a mergeId to source accomplishes your suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I've been burned pretty badly in the past by trying to extend the architecture of symbol merging. Merging is where state touches the checker, which is otherwise stateless:
Symbols from the binder are created when loading a file and only re-created when that file is editted. But merging mutates those symbols. It's important to only do that once, and tricky to make sure that happens as checkers are re-created over and over again and run merge* each time. I'd rather not change the architecture if it's not necessary.

Can you instead investigate copying constituent symbols from mainModule.exports and moduleAugmentation.exports to a fresh symbol created from cloneSymbol(moduleAugmentation) (or maybe it's called createSymbol)? This method is more likely to break symbol sharing, so you'll need to add tests for find-all-refs and rename-symbol to make they work correctly.

The patternAmbientModuleAugmentation map is still probably needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow—moduleAgumentation (the target) isn't transient, so mergeSymbol clones it rather than mutates it, and mainModule (the source) doesn't get mutated because it's the source. I think the only thing getting mutated here is the mergedSymbols array, which is the problem—I need to avoid calling recordMergedSymbol with these two symbols, because you should never be able to get from mainModule to moduleAugmentation via getMergedSymbol.

}
}
else if (target.flags & SymbolFlags.NamespaceModule) {
error(getNameOfDeclaration(source.declarations[0]), Diagnostics.Cannot_augment_module_0_with_value_exports_because_it_resolves_to_a_non_module_entity, symbolToString(target));
Expand Down Expand Up @@ -1023,7 +1026,22 @@ namespace ts {
// obtain item referenced by 'export='
mainModule = resolveExternalModuleSymbol(mainModule);
if (mainModule.flags & SymbolFlags.Namespace) {
mainModule = mergeSymbol(mainModule, moduleAugmentation.symbol);
// If we’re merging an augmentation to a pattern ambient module, we want to
// perform the merge unidirectionally from the augmentation ('a.foo') to
// the pattern ('*.foo'), so that 'getMergedSymbol()' on a.foo gives you
// all the exports both from the pattern and from the augmentation, but
// 'getMergedSymbol()' on *.foo only gives you exports from *.foo.
if (some(patternAmbientModules, module => mainModule === module.symbol)) {
const merged = mergeSymbol(moduleAugmentation.symbol, mainModule, /*unidirectional*/ true);
if (!patternAmbientModuleAugmentations) {
patternAmbientModuleAugmentations = createMap();
}
// moduleName will be a StringLiteral since this is not `declare global`.
patternAmbientModuleAugmentations.set((moduleName as StringLiteral).text, merged);
}
else {
mergeSymbol(mainModule, moduleAugmentation.symbol);
}
}
else {
// moduleName will be a StringLiteral since this is not `declare global`.
Expand Down Expand Up @@ -2391,6 +2409,14 @@ namespace ts {
if (patternAmbientModules) {
const pattern = findBestPatternMatch(patternAmbientModules, _ => _.pattern, moduleReference);
if (pattern) {
// If the module reference matched a pattern ambient module ('*.foo') but there’s also a
// module augmentation by the specific name requested ('a.foo'), we store the merged symbol
// by the augmentation name ('a.foo'), because asking for *.foo should not give you exports
// from a.foo.
const augmentation = patternAmbientModuleAugmentations && patternAmbientModuleAugmentations.get(moduleReference);
if (augmentation) {
return getMergedSymbol(augmentation);
}
return getMergedSymbol(pattern.symbol);
}
}
Expand Down
@@ -0,0 +1,20 @@
tests/cases/conformance/ambient/testB.ts(1,22): error TS2305: Module '"*.foo"' has no exported member 'onlyInA'.


==== tests/cases/conformance/ambient/types.ts (0 errors) ====
declare module "*.foo" {
let everywhere: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's a terrifying test case

declare module "*.foo" {
  export interface OhNo { star: string }
}
declare module "a.foo" {
  export interface OhNo { a: string }
}
import { OhNo } from "b.foo"
declare let ohno: OhNo;
ohno.a // oh no

If we can give an error here, we should. Or we could try to recursively do what this PR does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱 OH NO

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Doing the "unidirectional" thing recursively fixed this immediately. It feels like I'm on the right track, so ping me if you get a chance to elaborate on your concerns about symbol mutation, and hopefully I can address them in a way that continues to work well recursively like this.

}


==== tests/cases/conformance/ambient/testA.ts (0 errors) ====
import { everywhere, onlyInA } from "a.foo";
declare module "a.foo" {
let onlyInA: number;
}

==== tests/cases/conformance/ambient/testB.ts (1 errors) ====
import { everywhere, onlyInA } from "b.foo"; // Error
~~~~~~~
!!! error TS2305: Module '"*.foo"' has no exported member 'onlyInA'.

25 changes: 25 additions & 0 deletions tests/baselines/reference/ambientDeclarationsPatterns_merging1.js
@@ -0,0 +1,25 @@
//// [tests/cases/conformance/ambient/ambientDeclarationsPatterns_merging1.ts] ////

//// [types.ts]
declare module "*.foo" {
let everywhere: string;
}


//// [testA.ts]
import { everywhere, onlyInA } from "a.foo";
declare module "a.foo" {
let onlyInA: number;
}

//// [testB.ts]
import { everywhere, onlyInA } from "b.foo"; // Error


//// [types.js]
//// [testA.js]
"use strict";
exports.__esModule = true;
//// [testB.js]
"use strict";
exports.__esModule = true;
@@ -0,0 +1,26 @@
=== tests/cases/conformance/ambient/types.ts ===
declare module "*.foo" {
>"*.foo" : Symbol("*.foo", Decl(types.ts, 0, 0))

let everywhere: string;
>everywhere : Symbol(everywhere, Decl(types.ts, 1, 5))
}


=== tests/cases/conformance/ambient/testA.ts ===
import { everywhere, onlyInA } from "a.foo";
>everywhere : Symbol(everywhere, Decl(testA.ts, 0, 8))
>onlyInA : Symbol(onlyInA, Decl(testA.ts, 0, 20))

declare module "a.foo" {
>"a.foo" : Symbol("a.foo", Decl(testA.ts, 0, 44), Decl(types.ts, 0, 0))

let onlyInA: number;
>onlyInA : Symbol(onlyInA, Decl(testA.ts, 2, 5))
}

=== tests/cases/conformance/ambient/testB.ts ===
import { everywhere, onlyInA } from "b.foo"; // Error
>everywhere : Symbol(everywhere, Decl(testB.ts, 0, 8))
>onlyInA : Symbol(onlyInA, Decl(testB.ts, 0, 20))

@@ -0,0 +1,26 @@
=== tests/cases/conformance/ambient/types.ts ===
declare module "*.foo" {
>"*.foo" : typeof import("*.foo")

let everywhere: string;
>everywhere : string
}


=== tests/cases/conformance/ambient/testA.ts ===
import { everywhere, onlyInA } from "a.foo";
>everywhere : string
>onlyInA : number

declare module "a.foo" {
>"a.foo" : typeof import("a.foo")

let onlyInA: number;
>onlyInA : number
}

=== tests/cases/conformance/ambient/testB.ts ===
import { everywhere, onlyInA } from "b.foo"; // Error
>everywhere : string
>onlyInA : any

@@ -0,0 +1,25 @@
tests/cases/conformance/ambient/testB.ts(1,22): error TS2305: Module '"*.foo"' has no exported member 'onlyInA'.
tests/cases/conformance/ambient/testB.ts(1,31): error TS2305: Module '"*.foo"' has no exported member 'alsoOnlyInA'.


==== tests/cases/conformance/ambient/types.ts (0 errors) ====
declare module "*.foo" {
let everywhere: string;
}


==== tests/cases/conformance/ambient/testA.ts (0 errors) ====
import { everywhere, onlyInA, alsoOnlyInA } from "a.foo";
declare module "a.foo" {
let onlyInA: number;
}

==== tests/cases/conformance/ambient/testB.ts (2 errors) ====
import { everywhere, onlyInA, alsoOnlyInA } from "b.foo"; // Error
~~~~~~~
!!! error TS2305: Module '"*.foo"' has no exported member 'onlyInA'.
~~~~~~~~~~~
!!! error TS2305: Module '"*.foo"' has no exported member 'alsoOnlyInA'.
declare module "a.foo" {
let alsoOnlyInA: number;
}
27 changes: 27 additions & 0 deletions tests/baselines/reference/ambientDeclarationsPatterns_merging2.js
@@ -0,0 +1,27 @@
//// [tests/cases/conformance/ambient/ambientDeclarationsPatterns_merging2.ts] ////

//// [types.ts]
declare module "*.foo" {
let everywhere: string;
}


//// [testA.ts]
import { everywhere, onlyInA, alsoOnlyInA } from "a.foo";
declare module "a.foo" {
let onlyInA: number;
}

//// [testB.ts]
import { everywhere, onlyInA, alsoOnlyInA } from "b.foo"; // Error
declare module "a.foo" {
let alsoOnlyInA: number;
}

//// [types.js]
//// [testA.js]
"use strict";
exports.__esModule = true;
//// [testB.js]
"use strict";
exports.__esModule = true;
@@ -0,0 +1,34 @@
=== tests/cases/conformance/ambient/types.ts ===
declare module "*.foo" {
>"*.foo" : Symbol("*.foo", Decl(types.ts, 0, 0))

let everywhere: string;
>everywhere : Symbol(everywhere, Decl(types.ts, 1, 5))
}


=== tests/cases/conformance/ambient/testA.ts ===
import { everywhere, onlyInA, alsoOnlyInA } from "a.foo";
>everywhere : Symbol(everywhere, Decl(testA.ts, 0, 8))
>onlyInA : Symbol(onlyInA, Decl(testA.ts, 0, 20))
>alsoOnlyInA : Symbol(alsoOnlyInA, Decl(testA.ts, 0, 29))

declare module "a.foo" {
>"a.foo" : Symbol("a.foo", Decl(testA.ts, 0, 57), Decl(types.ts, 0, 0), Decl(testB.ts, 0, 57))

let onlyInA: number;
>onlyInA : Symbol(onlyInA, Decl(testA.ts, 2, 5))
}

=== tests/cases/conformance/ambient/testB.ts ===
import { everywhere, onlyInA, alsoOnlyInA } from "b.foo"; // Error
>everywhere : Symbol(everywhere, Decl(testB.ts, 0, 8))
>onlyInA : Symbol(onlyInA, Decl(testB.ts, 0, 20))
>alsoOnlyInA : Symbol(alsoOnlyInA, Decl(testB.ts, 0, 29))

declare module "a.foo" {
>"a.foo" : Symbol("a.foo", Decl(testA.ts, 0, 57), Decl(types.ts, 0, 0), Decl(testB.ts, 0, 57))

let alsoOnlyInA: number;
>alsoOnlyInA : Symbol(alsoOnlyInA, Decl(testB.ts, 2, 5))
}
@@ -0,0 +1,34 @@
=== tests/cases/conformance/ambient/types.ts ===
declare module "*.foo" {
>"*.foo" : typeof import("*.foo")

let everywhere: string;
>everywhere : string
}


=== tests/cases/conformance/ambient/testA.ts ===
import { everywhere, onlyInA, alsoOnlyInA } from "a.foo";
>everywhere : string
>onlyInA : number
>alsoOnlyInA : number

declare module "a.foo" {
>"a.foo" : typeof import("a.foo")

let onlyInA: number;
>onlyInA : number
}

=== tests/cases/conformance/ambient/testB.ts ===
import { everywhere, onlyInA, alsoOnlyInA } from "b.foo"; // Error
>everywhere : string
>onlyInA : any
>alsoOnlyInA : any

declare module "a.foo" {
>"a.foo" : typeof import("a.foo")

let alsoOnlyInA: number;
>alsoOnlyInA : number
}
@@ -0,0 +1,14 @@
// @filename: types.ts
declare module "*.foo" {
let everywhere: string;
}


// @filename: testA.ts
import { everywhere, onlyInA } from "a.foo";
declare module "a.foo" {
let onlyInA: number;
}

// @filename: testB.ts
import { everywhere, onlyInA } from "b.foo"; // Error
@@ -0,0 +1,17 @@
// @filename: types.ts
declare module "*.foo" {
let everywhere: string;
}


// @filename: testA.ts
import { everywhere, onlyInA, alsoOnlyInA } from "a.foo";
declare module "a.foo" {
let onlyInA: number;
}

// @filename: testB.ts
import { everywhere, onlyInA, alsoOnlyInA } from "b.foo"; // Error
declare module "a.foo" {
let alsoOnlyInA: number;
}