Fix self tail call return type inference in assigned anonymous functions #58124
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
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test it |
|
Hey @andrewbranch, the results of running the DT tests are ready. Everything looks the same! |
|
@andrewbranch Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
|
@andrewbranch Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@andrewbranch Here are the results of running the top 400 repos comparing Everything looks good! |
|
Hypothetically: let foo = () => { // :: () => number
if (Math.random() < 0.5)
return foo();
else
return 42;
};It's true, any reassignment, even with an entirely different function, could only narrow the return type so it's not possible to introduce unsoundness that way. It's one of those things that just feels questionable but is actually (probably) totally safe |
|
The missing error in Webpack is a good thing, showing how this is intended to work. The new error in uglify-js is an example of a reassignment of the outer function throwing this off. It’s allowed because |
|
Can you extract the uglify-js case into a test, since presumably it didn't fail in CI? I find it weird that we would want to special case const vs let here, though... |
|
The uglify-js case is added in the latest commit. We could do definite assignment analysis, but I’m not sure that’s worth it? There are other narrowing features that are predicated on const-ness, IIRC. |
|
Ah, it's the var case. I was looking for some sort of error baseline but that's the opposite of right. CI is also failing, but not sure if that's related or not. |
andrewbranch
commented
Apr 9, 2024
| return isConstantVariable(symbol) || isParameterOrMutableLocalVariable(symbol) && !isSymbolAssigned(symbol); | ||
| return isConstantVariable(symbol) | ||
| || isParameterOrMutableLocalVariable(symbol) && !isSymbolAssigned(symbol) | ||
| || !!symbol.valueDeclaration && isFunctionExpression(symbol.valueDeclaration); |
There was a problem hiding this comment.
If an identifier resolved to a function expression symbol, that function had a name, and that name cannot be reassigned, so it’s effectively constant.
|
@typescript-bot test it |
|
Starting jobs; this comment will be updated as builds start and complete.
|
|
@typescript-bot run dt |
|
@jakebailey Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos comparing Everything looks good! |
jakebailey
approved these changes
Apr 10, 2024
|
@typescript-bot pack this |
|
Hey @gabritto, 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 |
|
Is it intentional that the following example changes behavior from |
|
@gabritto yes. We weren’t quite getting it right before and we’re still not quite getting it right now, but I think it’s arguably slightly better now? Previously, we did identify the |
|
Ok, that makes sense. |
gabritto
approved these changes
Apr 10, 2024
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.
Good old
getMergedSymbol.I briefly thought through a case where the arrow function is assigned to aUser test showed one failure from this, so latest commit limits this feature to constant references (and function declarations as before).letand reassigned somewhere, but for that reassignment to change the type that can be returned by the function, it would have had to have a type annotation in the first place, so this code would never run. I think this is safe.Fixes #58100