-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Fixed array expression spreads into variadic const calls #52845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed array expression spreads into variadic const calls #52845
Conversation
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
@@ -12,4 +12,4 @@ f2(1); | |||
f2(1, 2, 3); | |||
f2(1, 2, 3, 4, 5); | |||
f2(1, 2, 3, 4, 5, 6, 7); | |||
f2(...[1], 2, 3, 4, 5, 6); | |||
f2(1, 2, 3, 4, 5, ...[6, 7]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this since the previous version became OK with this PR - spreading a fixed array expression into a call is now treated just as if positional arguments would be provided (thanks to getEffectiveCallArguments
creating synthetic expressions)
@@ -29665,9 +29665,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
const elementFlags: ElementFlags[] = []; | |||
pushCachedContextualType(node); | |||
const inDestructuringPattern = isAssignmentTarget(node); | |||
const inConstContext = isConstContext(node); | |||
const isSpreadIntoCallOrNew = isSpreadElement(node.parent) && isCallOrNewExpression(node.parent.parent); | |||
const inConstContext = isSpreadIntoCallOrNew || isConstContext(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initially, I wanted to adjust isConstContext
- but spread arguments never have contextual types + asking for a contextual type of an argument calls getEffectiveCallArguments
and we are within this call already, so it was creating an infinite loop
I was looking into other ways to check for the const context for such spread expressions but I couldn't figure out anything since I can't even check the signature (it's still resolvingSignature
at this point).
@Andarist could you create a bug to track this as well? The problem itself seems pretty straightforward, but it would help us track bugs better. |
@typescript-bot test this |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 4550763. 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 4550763. You can monitor the build here. |
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 4550763. 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 4550763. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 4550763. You can monitor the build here. |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 4550763. You can monitor the build here. Update: The results are in! |
Hey @jakebailey, 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 |
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
Hey @jakebailey, the results of running the DT tests are ready. Branch only errors:Package: lodash
Package: inquirer
Package: zen-observable
Package: inquirer/v8
|
I have yet to look at the DT changes, but, that VS Code change actually seems correct given the definition of IAction? |
That inquirer change seems unrelated. @gabritto |
The lodash change seems like a actual improvement; I think it's now correctly applying the tuple to the overloads and now one of the arguments is actually typed properly as though it were called with two args. The zen-observable change seems positive too. |
Ignoring the spurious |
Nice, added the |
I don't think you need to; I've seen that exact failure on another PR, so it's probably a glitch in the diff. |
@jakebailey Here they are:
CompilerComparison Report - main..52845
System
Hosts
Scenarios
TSServerComparison Report - main..52845
System
Hosts
Scenarios
StartupComparison Report - main..52845
System
Hosts
Scenarios
Developer Information: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, LGTM; these all feel like good changes. But, will need to update DT to reflect the improvement.
I noticed the issue while working on another PR here:
TS playground
fixes #53149