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
Changes from 2 commits
7409a04
f2ec02b
d69f9f3
973c3ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here's a terrifying test case
If we can give an error here, we should. Or we could try to recursively do what this PR does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😱 OH NO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
|
||
==== 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'. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} |
There was a problem hiding this comment.
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 aunidirectonal
option to prevent adding amergeId
tosource
accomplishes your suggestion.There was a problem hiding this comment.
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
andmoduleAugmentation.exports
to a fresh symbol created fromcloneSymbol(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.
There was a problem hiding this comment.
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, somergeSymbol
clones it rather than mutates it, andmainModule
(the source) doesn't get mutated because it's the source. I think the only thing getting mutated here is themergedSymbols
array, which is the problem—I need to avoid callingrecordMergedSymbol
with these two symbols, because you should never be able to get frommainModule
tomoduleAugmentation
viagetMergedSymbol
.