Skip to content
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

Don't inferFromIndexTypes() twice #34501

Merged
merged 2 commits into from
Mar 4, 2020
Merged

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Oct 15, 2019

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.

@jablko
Copy link
Contributor Author

jablko commented Oct 15, 2019

@typescript-bot user test this

@RyanCavanaugh
Copy link
Member

@typescript-bot user test this
@typescript-bot test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 15, 2019

Heya @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
Copy link
Collaborator

typescript-bot commented Oct 15, 2019

Heya @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.

@typescript-bot
Copy link
Collaborator

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

@weswigham
Copy link
Member

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

@jablko
Copy link
Contributor Author

jablko commented Nov 26, 2019

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

@jablko jablko force-pushed the patch-19 branch 2 times, most recently from 1a56348 to 84baa5c Compare November 27, 2019 19:40
@weswigham
Copy link
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.

@jablko
Copy link
Contributor Author

jablko commented Nov 27, 2019

✔️ Yup, that works. Done.

@jablko jablko force-pushed the patch-19 branch 2 times, most recently from 9c6fe7d to 2cfb902 Compare January 29, 2020 01:06
@sandersn sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
@jablko jablko force-pushed the patch-19 branch 5 times, most recently from c248e7f to f85c83f Compare February 9, 2020 17:19
@jablko jablko force-pushed the patch-19 branch 5 times, most recently from 012cbda to 3f15d40 Compare February 13, 2020 15:42
@sandersn
Copy link
Member

sandersn commented Mar 4, 2020

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

Copy link
Member

@weswigham weswigham left a 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 @ahejlsberg to look at it briefly before it got in, which is why he's assigned iirc.

@sandersn sandersn merged commit 061338e into microsoft:master Mar 4, 2020
Copy link

@StefanXhunga StefanXhunga left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
7 participants