fix53287 mergeSymbol checks if the resolved target can merge with the source
#58326
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test it |
|
Hey @iisaduan, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: karma-chai-sinon Package: bandagedbd__bdapi Package: regenerator-runtime |
|
@iisaduan Here are the results of running the user tests comparing Everything looks good! |
|
@iisaduan Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@iisaduan Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
|
@typescript-bot pack this |
|
Hey @iisaduan, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Testing results analysis: The extra errors correctly identify wrong code.
|
iisaduan
requested review from
weswigham,
andrewbranch,
sheetalkamat and
jakebailey
There was a problem hiding this comment.
The main way I can imagine this going wrong is a merge of an alias that should have happened that is now blocked. The fact that there weren't any surprises in the top400 tests is encouraging. But I'd like the other reviewers to see if they can come up with anything.
As for the likely breaks, I think we've observed those already: bad merges that no longer merge, so that half of the intended merge is missing now. Given the low frequency of breaks I think it's not a problem.
There was a problem hiding this comment.
I think this is the right approach. If we were worried about any of the specific merges that became errors, I think the thing to do would be to adjust the merge exclusion flags to allow the merge to happen (whether mediated by an alias or not), then issue errors during type checking for cases that look wrong. For example, we might decide that
declare global {
const React: typeof import("./module");
}
export {};
// module.d.ts
export = React;
export as namespace React;is fine because the global variable declaration has the same type as the value side of the UMD module declaration, which matches our rules for when two ambient variable declarations can merge without error. However, I don't think we have evidence that changing the rules is necessary right now, so I think we should go with this and adjust if we get compelling feedback.
Fixes #53287
While investigating the original issue, I found that re-exporting an alias would circumvent any checks to determine whether or not the source and the target are allowed to merge at all.
This adds a check in
mergeSymbolto check if the resolved target can merge with the source, and if they cannot merge, then we issue the same errors as if there was no reexporting.Got lots of help from @sandersn looking over the new errors 🙂