FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Don't inferFromIndexTypes() twice by jablko · Pull Request #34501 · microsoft/TypeScript · GitHub

/ TypeScript Public

Don't inferFromIndexTypes() twice - #34501

Merged
Nathan Shively-Sanders (sandersn) merged 2 commits into
microsoft:masterfrom
jablko:patch-19
Mar 4, 2020
ConversationCommits (2)ChecksFiles changed
Merged

Don't inferFromIndexTypes() twice#34501
Nathan Shively-Sanders (sandersn) merged 2 commits into
microsoft:masterfrom
jablko:patch-19

Conversation

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

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.

Jack Bates (jablko) commented Oct 15, 2019

Copy link
Copy Markdown
Contributor Author

TypeScript Bot (@typescript-bot) user test this

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

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

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.

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

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.

Open

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.

Jack Bates (jablko) force-pushed the patch-19 branch 2 times, most recently from dea9d72 to d045c4c Compare October 17, 2019 16:59
Avi Vahl (AviVahl) mentioned this pull request Oct 18, 2019
Closed
Jack Bates (jablko) mentioned this pull request Nov 12, 2019
Closed
Jack Bates (jablko) force-pushed the patch-19 branch 3 times, most recently from d37394a to 82d6ad7 Compare November 18, 2019 14:18
Jack Bates (jablko) force-pushed the patch-19 branch 2 times, most recently from 4765642 to 8af1dca Compare November 25, 2019 20:24

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.

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. ❌

Jack Bates (jablko) force-pushed the patch-19 branch 2 times, most recently from 1a56348 to 84baa5c Compare November 27, 2019 19:40

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.

Jack Bates (jablko) commented Nov 27, 2019

Copy link
Copy Markdown
Contributor Author

✔️ Yup, that works. Done.

Jack Bates (jablko) force-pushed the patch-19 branch 2 times, most recently from 09e96b8 to 6be718c Compare November 29, 2019 22:25
Jack Bates (jablko) force-pushed the patch-19 branch 3 times, most recently from 9c6fe7d to 2cfb902 Compare January 29, 2020 01:06
Nathan Shively-Sanders (sandersn) added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
Jack Bates (jablko) force-pushed the patch-19 branch 5 times, most recently from c248e7f to f85c83f Compare February 9, 2020 17:19
Jack Bates (jablko) force-pushed the patch-19 branch 5 times, most recently from 012cbda to 3f15d40 Compare February 13, 2020 15:42

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.

Wesley Wigham (weswigham) approved these changes Mar 4, 2020
View reviewed changes

Wesley Wigham (weswigham) left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Nathan Shively-Sanders (sandersn) merged commit 061338e into microsoft:master Mar 4, 2020
Grigory Streltsov (strelga) mentioned this pull request May 7, 2020
Closed
Stefan Xhunga (StefanXhunga) reviewed Oct 3, 2020
View reviewed changes

Stefan Xhunga (StefanXhunga) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Ron Buckton (rbuckton) mentioned this pull request Oct 31, 2020
Closed
github-actions Bot mentioned this pull request Aug 14, 2022
Merged
Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
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
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

Assignees

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

7 participants

Back | FazBrowse Home | New Git URL