Use control flow to type CommonJS exports #42751
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows us to read our own output, plus the times when people manually write exactly the same pattern. Fixes #40555
typescript-bot
added
Author: Team
For Milestone Bug
Feels like something we could use CFA for, like how we track initializers for class properties through a constructor body. |
|
The problem with control flow is that it doesn't normally persist across aliases. It doesn't actually matter what the type of Edit: To use control flow, maybe I could create a structure to query and capture the type at the bottom of the file. Not sure how that would work either. |
|
I mean, analogously, the externally visible type of a constructor property is the one determined by CFA at the end of the conctructor, yeah? Not the union of the constructor-assigned types. |
Yeah; if we don't already have a CFA node at the end of the file (I think we should, though), you'd need to add one like we did for constructor properties. |
1. Could probably use a *lot* more tests. 2. getTypeOfAlias redoes some work from resolveAlias because it needs to not resolve the alias completely, just to its export.
sandersn
changed the title
sandersn
commented
Feb 13, 2021
src/compiler/checker.ts
Outdated
| @@ -26429,7 +26448,10 @@ namespace ts { | |||
| // accessor, or optional method. | |||
| const assignmentKind = getAssignmentTargetKind(node); | |||
| if (assignmentKind === AssignmentKind.Definite || | |||
| prop && !(prop.flags & (SymbolFlags.Variable | SymbolFlags.Property | SymbolFlags.Accessor)) && !(prop.flags & SymbolFlags.Method && propType.flags & TypeFlags.Union)) { | |||
There was a problem hiding this comment.
Only change here is to exempt isDuplicatedCommonJSExport so that it falls through to the autoType case below
src/compiler/checker.ts
Outdated
| @@ -8359,7 +8374,10 @@ namespace ts { | |||
| } | |||
|
|
|||
| function getFlowTypeOfProperty(reference: Node, prop: Symbol | undefined) { | |||
| const initialType = prop && (!isAutoTypedProperty(prop) || getEffectiveModifierFlags(prop.valueDeclaration) & ModifierFlags.Ambient) && getTypeOfPropertyInBaseClass(prop) || undefinedType; | |||
There was a problem hiding this comment.
No changes here, I just couldn't read it otherwise
|
This is a much bigger change, so
|
weswigham
reviewed
Feb 13, 2021
| function a() { } | ||
| exports.apply() | ||
| ~~~~~~~~~~~~~ | ||
| !!! error TS2722: Cannot invoke an object which is possibly 'undefined'. |
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 046bed9. You can monitor the build here. Update: The results are in! |
|
@sandersn Here they are:Comparison Report - master..42751
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
1. Update the module.exports test to match the exports ones. 2. Add a test of evolving commonjs type. 3. Add a test of assignment as last statement. (1) exposed a bug that required a better synthetic reference. (3) exposed a bug that was most easily fixed by giving source files a `endFlowNode` like functions and setting it in the binder.
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 6880763. You can monitor the build here. Update: The results are in! |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@sandersn Here they are:Comparison Report - master..42751
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Perf run: There's a lot of variance in the node 14 results, but it's both positive and negative. I don't think there's any real impact here.
|
weswigham
approved these changes
Apr 6, 2021
sandersn
deleted the
allow-redeclaring-commonjs-aliases-with-initial-undefined
branch
sandersn
added a commit
to DefinitelyTyped/DefinitelyTyped
that referenced
this pull request
Apr 8, 2021
(New changes to Typescript)[microsoft/TypeScript#42751] make it better at figuring out the type of `module.exports`. Unfortunately, it still gets confused by some bundler outputs, which is what the npm-naming rule uses to compare index.d.ts to find out if the types match the original JS well enough. This PR adds exemptions to the 7 packages that started failing after the linked Typescript PR was merged.
sandersn
added a commit
to DefinitelyTyped/DefinitelyTyped
that referenced
this pull request
Apr 8, 2021
(New changes to Typescript)[microsoft/TypeScript#42751] make it better at figuring out the type of `module.exports`. Unfortunately, it still gets confused by some bundler outputs, which is what the npm-naming rule uses to compare index.d.ts to find out if the types match the original JS well enough. This PR adds exemptions to the 7 packages that started failing after the linked Typescript PR was merged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows us to read our own output, plus the times when people manually write exactly the same pattern. It may also allow other patterns in the wild, although I need to look at the user test output to find out if that's the case.
Implementation notes
endFlowNodeproperty to source files in order to run control flow on the statements of a source file. Then I use this node to get the control flow type of an export from outside the file, when it's imported.Fixes #40555