Properly propagate ObjectFlags.NonInferrableType, clean up non-inferrable code paths #49887
Conversation
This file contains 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
typescript-bot
added
Author: Team
For Milestone Bug
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…g NonInferrableType, but breaks if it does
weswigham
reviewed
Jul 13, 2022
weswigham
reviewed
Jul 13, 2022
| @@ -17196,6 +17191,7 @@ namespace ts { | |||
| result.mapper = mapper; | |||
| result.aliasSymbol = aliasSymbol || type.aliasSymbol; | |||
| result.aliasTypeArguments = aliasSymbol ? aliasTypeArguments : instantiateTypes(type.aliasTypeArguments, mapper); | |||
| result.objectFlags |= result.aliasTypeArguments ? getPropagatingFlagsOfTypes(result.aliasTypeArguments, /*excludeKinds*/ 0) : 0; | |||
There was a problem hiding this comment.
Huh, this is interesting because this is redundant for TypeReference anonymous types (since those internally collect propagating flags from their type arguments), but required for any other anonymous object type with an alias. Go figure. I wonder if we should be propagating these object flags through conditional types, too... We already preserve them through intersections and unions. (We probably should be, since conditionals are "smart unions"...)
There was a problem hiding this comment.
Yes, I think they should. Right now I'm trying to go by hand to every createAnonymousType, createObjectType,... etc and see if there's anything missed, and it's a bit daunting because there's like 100 of them to verify, and nothing I've changed so far as actually changed any test.
I want to have something I can run while I'm debugging that'll observe each type during inference and walk down to see if there was a NonInferrableType flag that was missed, but I get stuck in stack overflows, so I don't quite know how I can do my sanity check.
There was a problem hiding this comment.
I think for this PR, I'm inclined to not try and go everywhere and add some more flag propagation without a way for me to check these; I just don't trust that I'm getting it right, and I don't have any way to observe the result.
This PR at least improves the one case I can test, and has a good amount of cleanup, so I'm happy with it (but I'll rerun the testing since I changed the structure of this quite a bit).
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at dd6bdaf. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at dd6bdaf. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at dd6bdaf. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at dd6bdaf. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
weswigham
approved these changes
Jul 13, 2022
|
@jakebailey Here they are:Comparison Report - main..refs/pull/49887/merge fp-ts2 of 4 projects failed to build with the old tsc dtslint/ts3.5/tsconfig.json
|
|
Hm, those fp-ts things seem relevant. I was hoping those would go away, but I guess not. |
|
Yeah, the actual fix in this PR breaks fp-ts's |
|
@jakebailey Here they are:
CompilerComparison Report - main..49887
System
Hosts
Scenarios
TSServerComparison Report - main..49887
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…lavent when the non-inferrable type was an any
|
Figured this out; we basically just needed to revert #26678, which worked around problems with non-inferrable types before the current non-inferrable flag and proper propagation was implemented. |
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 9e831ec. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 9e831ec. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 9e831ec. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 9e831ec. You can monitor the build here. |
weswigham
approved these changes
Jul 14, 2022
|
@jakebailey Here they are:Comparison Report - main..49887
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey |
|
All the tests look clean, phew. |
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.
Fixes #43962
The original bug required two changes to fix:
NonInferrableTypeflag needs to be propagated fromaliasTypeArgumentsto the type they're used in, but this was missed in at least one place.silentNeverTypeas its marker type to say "don't infer from this", but it only checked forsource === silentNeverType, which doesn't work when you use that type in another type. So this needs to beNonInferrableTypetoo.The rest of the changes are cleanups and documentation improvements.
nonInferrableTypeis redundant withsilentNeverType, and there are other types likeanyFunctionType,autoType, andautoArrayTypewhich are also used as marker types for this same purpose. They all can just use the flag without any changes in behavior.nonInferrableAnyTypelooks like it should beNonInferrableType, but making it have that flag breaks things. It turns out that if you just useanyType, nothing breaks.