GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Don't inferFromIndexTypes() twice by jablko · Pull Request #34501 · microsoft/TypeScript

Conversation

@jablko

@jablko Jack Bates (jablko) commented Oct 15, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Fixes #33559
Fixes #33752
Fixes #34924
Fixes #34925
Fixes #34937
Fixes #35136
Fixes #35258

interface I<T> {}
declare function f1<T1, T2>(values: [I<T1>, I<T2>]): T1;
declare function f2<T1, T2>(values: readonly [I<T1>, I<T2>]): T1;

declare const values: [I<1>, I<number>];
let one: 1;
one = f1(values); // ✓ 1
one = f2(values); // ✗ number

Currently f2(values) is number while f1(values) is 1.

There are three cases in inferFromProperties()

  1. Source and target are both tuples: infers types element-wise
  2. Target is an array: calls inferFromIndexTypes()
  3. Otherwise: iterates over properties

But then inferFromObjectTypesWorker() calls inferFromIndexTypes() again, in all cases

                if (!typesDefinitelyUnrelated(source, target)) {
                    inferFromProperties(source, target);
                    inferFromSignatures(source, target, SignatureKind.Call);
                    inferFromSignatures(source, target, SignatureKind.Construct);
                    inferFromIndexTypes(source, target);
                }

This PR moves that call from inferFromObjectTypesWorker() to the third case. Consequently

  1. inferFromIndexTypes() isn't called twice in the second (array) case
  2. it isn't called at all in the first (tuple) case, so f2(values) is 1 like f1(values)

FYI f1(values) is currently 1 unlike f2(values) because parameter and argument are the same generic type (tuple) and handled by

                if (getObjectFlags(source) & ObjectFlags.Reference && getObjectFlags(target) & ObjectFlags.Reference && (
                    (<TypeReference>source).target === (<TypeReference>target).target || isArrayType(source) && isArrayType(target)) &&
                    !((<TypeReference>source).node && (<TypeReference>target).node)) {
                    // If source and target are references to the same generic type, infer from type arguments
                    inferFromTypeArguments(getTypeArguments(<TypeReference>source), getTypeArguments(<TypeReference>target), getVariances((<TypeReference>source).target));
                }

whereas f2(values) parameter (readonly tuple) and argument (tuple) aren't.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
capaj, rktuxyn, ThomasBower, guzzur, dicotomy, Eduardogbg, BubbleBear, davishooly, Kobzol, Nividica, and AZaviruha reacted with heart emoji AnyhowStep, stephank, lambda-fairy, buu700, falkenhawk, nmchaves, tonivj5, Gaubee, rafaelpinabew, gomesalexandre, and 2 more reacted with rocket emoji
All reactions
  • ❤️ 11 reactions
  • 🚀 12 reactions
@jablko

Jack Bates (jablko) commented Oct 15, 2019

Copy link
Copy Markdown
Contributor Author

TypeScript Bot (@typescript-bot) user test this
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RyanCavanaugh

Ryan Cavanaugh (RyanCavanaugh) commented Oct 15, 2019

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) user test this
TypeScript Bot (@typescript-bot) test this
jablko reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Oct 15, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Ryan Cavanaugh (@RyanCavanaugh), I've started to run the parallelized community code test suite on this PR at e427f3f. You can monitor the build here. It should now contribute to this PR's status checks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Oct 15, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Ryan Cavanaugh (@RyanCavanaugh), I've started to run the extended test suite on this PR at e427f3f. You can monitor the build here. It should now contribute to this PR's status checks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot TypeScript Bot (typescript-bot) mentioned this pull request Oct 15, 2019
🤖 User test baselines have changed for patch-19 jablko/TypeScript#1
Open
@typescript-bot

TypeScript Bot (typescript-bot) commented Oct 15, 2019

Copy link
Copy Markdown
Contributor

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch 2 times, most recently from dea9d72 to d045c4c Compare October 17, 2019 16:59
@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) assigned Anders Hejlsberg (ahejlsberg) Oct 17, 2019
@AviVahl Avi Vahl (AviVahl) mentioned this pull request Oct 18, 2019
TS3.7: No overload matches this call #34554
Closed
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch from d045c4c to 70f90d9 Compare October 19, 2019 18:33
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch from 70f90d9 to 0d9a3bc Compare October 27, 2019 19:15
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch from 0d9a3bc to 5399ce0 Compare November 4, 2019 15:21
@jablko Jack Bates (jablko) mentioned this pull request Nov 12, 2019
3.7.2 Promise.all infers base type of interfaces rather than actual interface #34937
Closed
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch 3 times, most recently from d37394a to 82d6ad7 Compare November 18, 2019 14:18
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch 2 times, most recently from 4765642 to 8af1dca Compare November 25, 2019 20:24
@weswigham

Wesley Wigham (weswigham) commented Nov 26, 2019

Copy link
Copy Markdown
Member

Jack Bates (@jablko) rather than the other changes in checker.ts, could you just delete

                    if (isArrayType(target)) {
                        inferFromIndexTypes(source, target);
                        return;
                    }

from inferFromProperties? That way the single unconditional inferFromIndexTypes is still easy to track.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jablko

Jack Bates (jablko) commented Nov 26, 2019

Copy link
Copy Markdown
Contributor Author

Wesley Wigham (@weswigham) Thanks a lot for taking a look at this! What you describe would call inferFromIndexTypes() just once, however it would still call it in the tuple case, so I assume it wouldn't resolve the test case included in this issue: Inferring an array type instead of a tuple type.

inferFromProperties() infers a tuple type element-wise, but then inferFromObjectTypesWorker() calls inferFromIndexTypes() which re-infers an array type. ❌
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch 2 times, most recently from 1a56348 to 84baa5c Compare November 27, 2019 19:40
@weswigham

Wesley Wigham (weswigham) commented Nov 27, 2019

Copy link
Copy Markdown
Member

however it would still call it in the tuple case, so I assume it wouldn't resolve the test case included in this issue: Inferring an array type instead of a tuple type

Hm, if that's the case, can we copy the tuple check into the inferFromObjectTypesWorker instead of shifting inferFromIndexSignatures into inferFromProperties? I don't like the implicit linkage of index inference to property inference caused by the current layering.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch from 84baa5c to 730aa56 Compare November 27, 2019 20:40
@jablko

Jack Bates (jablko) commented Nov 27, 2019

Copy link
Copy Markdown
Contributor Author

✔️ Yup, that works. Done.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch 2 times, most recently from 09e96b8 to 6be718c Compare November 29, 2019 22:25
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch from 6be718c to b3f48a6 Compare December 28, 2019 17:09
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch from b3f48a6 to 19f513e Compare January 21, 2020 18:18
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch 3 times, most recently from 9c6fe7d to 2cfb902 Compare January 29, 2020 01:06
@sandersn Nathan Shively-Sanders (sandersn) added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch 5 times, most recently from c248e7f to f85c83f Compare February 9, 2020 17:19
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch 5 times, most recently from 012cbda to 3f15d40 Compare February 13, 2020 15:42
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch from 3f15d40 to 5d3d184 Compare February 21, 2020 00:06
Jack Bates (jablko) added 2 commits February 21, 2020 00:11
@jablko
Don't inferFromIndexTypes() twice
4c503a3
@jablko
Add tests
31d6f32
@jablko
Jack Bates (jablko) force-pushed the patch-19 branch from 5d3d184 to 31d6f32 Compare February 22, 2020 22:41
@sandersn

Nathan Shively-Sanders (sandersn) commented Mar 4, 2020

Copy link
Copy Markdown
Member

Wesley Wigham (@weswigham) are you happy with this change now? It's been a while, but the inference code hasn't changed much in the last few months.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
weswigham
Wesley Wigham (weswigham) approved these changes Mar 4, 2020
View reviewed changes

@weswigham Wesley Wigham (weswigham) left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Yeah, I think this is fine now (it just moves a chunk of code out of a call and into the (only) caller), I just wanted Anders Hejlsberg (@ahejlsberg) to look at it briefly before it got in, which is why he's assigned iirc.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ahejlsberg
Anders Hejlsberg (ahejlsberg) approved these changes Mar 4, 2020
View reviewed changes
@sandersn
Nathan Shively-Sanders (sandersn) merged commit 061338e into microsoft:master Mar 4, 2020
@strelga Grigory Streltsov (strelga) mentioned this pull request May 7, 2020
Better typings for Promise, like #31117 #33707
Closed
StefanXhunga
Stefan Xhunga (StefanXhunga) reviewed Oct 3, 2020
View reviewed changes

@StefanXhunga Stefan Xhunga (StefanXhunga) left a comment

Copy link
Copy Markdown

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.

Thank you for your assistance, please change the nesessery scedares!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@rbuckton Ron Buckton (rbuckton) mentioned this pull request Oct 31, 2020
Incorrect type inference in promise.all #35626
Closed
@github-actions github-actions Bot mentioned this pull request Aug 14, 2022
Remove span tag in title and unnecessary TOC in Ko microsoft/TypeScript-Website-Localizations#164
Merged
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham Wesley Wigham (weswigham) weswigham approved these changes
@ahejlsberg Anders Hejlsberg (ahejlsberg) ahejlsberg approved these changes
+1 more reviewer
@StefanXhunga Stefan Xhunga (StefanXhunga) StefanXhunga left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@ahejlsberg Anders Hejlsberg (ahejlsberg)

Labels

For Milestone Bug PRs that fix a bug with a specific milestone

Projects

PR Backlog
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3.7.2 Promise.all infers base type of interfaces rather than actual interface Promise.all not correctly inferring types. 3.7-beta regression: Promise.all wrongly adds undefined type new readonly modifiers on Promise.all break lyricist in DT

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@jablko @RyanCavanaugh @typescript-bot @weswigham @sandersn @ahejlsberg @StefanXhunga
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.