Fix symbol merging of augmentations to pattern ambient modules #31078
Conversation
| @@ -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); | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
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.
|
|
||
| ==== tests/cases/conformance/ambient/types.ts (0 errors) ==== | ||
| declare module "*.foo" { | ||
| let everywhere: string; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@typescript-bot perf test this please and thank you |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 973c3ca. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@andrewbranch Here they are:Comparison Report - master..31078
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@sandersn the perf test is complaining about this one: +8% delta but ±5.55% variance measuring master 🤔 I can ignore this, right? |
|
Practically, yes, because it's a minor change in a component you didn't touch. @rbuckton put together those stats and can say whether that notation means it's technically statistically significant. |
Fixes #30752