Skip to content

Conversation

@jablko
Copy link
Contributor

@jablkojablko commented Oct 15, 2019

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

interfaceI<T>{}declarefunctionf1<T1,T2>(values: [I<T1>,I<T2>]): T1;declarefunctionf2<T1,T2>(values: readonly[I<T1>,I<T2>]): T1;declareconstvalues: [I<1>,I<number>];letone: 1;one=f1(values);// ✓ 1one=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 argumentsinferFromTypeArguments(getTypeArguments(<TypeReference>source),getTypeArguments(<TypeReference>target),getVariances((<TypeReference>source).target));}

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

@jablko
Copy link
ContributorAuthor

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

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

@jablkojablkoforce-pushed the patch-19 branch 2 times, most recently from 1a56348 to 84baa5cCompareNovember 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
ContributorAuthor

✔️ Yup, that works. Done.

@jablkojablkoforce-pushed the patch-19 branch 3 times, most recently from 9c6fe7d to 2cfb902CompareJanuary 29, 2020 01:06
@sandersnsandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
@jablkojablkoforce-pushed the patch-19 branch 5 times, most recently from c248e7f to f85c83fCompareFebruary 9, 2020 17:19
@jablkojablkoforce-pushed the patch-19 branch 5 times, most recently from 012cbda to 3f15d40CompareFebruary 13, 2020 15:42
@sandersn
Copy link
Member

@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

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

@sandersnsandersn merged commit 061338e into microsoft:masterMar 4, 2020
Copy link

@StefanXhungaStefanXhunga 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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Milestone BugPRs that fix a bug with a specific milestone

Projects

Archived in project

7 participants

@jablko@RyanCavanaugh@typescript-bot@weswigham@sandersn@ahejlsberg@StefanXhunga