Distribute indexed accesses when simplifying them #26281

weswigham · 2018-08-07T23:07:36Z

And use the unknownType instead of the errorType when checking for property types if there is no error node, so the types combine appropriately under intersection and union operations.

Sketch of a fix for #26274 - I mostly want to investigate this change's impact on RWC, since the changes to, eg, index signature handling in intersections (it's more correct!) could be breaky. It's also likely correct to simply distribute on initial construction, rather than waiting for simplification - but I haven't looked much into that approach yet.

Fixes #26274
Fixes #25181
Fixes #26608

weswigham · 2018-08-07T23:34:20Z

@typescript-bot test this

typescript-bot · 2018-08-07T23:34:21Z

Heya @weswigham, I've started to run the extended test suite on this PR at c881d97. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2018-08-08T01:38:53Z

Saw a pair of RWC breaks due to any as an index type not propagating (silently return the errorType had the effect of propagating it). Now T[any] is very explicitly also any. I think this is probably wrong, btw, but it's what we were doing before. It should probably also be unknown, despite how that may break some people.

@typescript-bot test this again btw

typescript-bot · 2018-08-08T01:38:55Z

Heya @weswigham, I've started to run the extended test suite on this PR at 518046c. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2018-08-08T01:40:53Z

tests/baselines/reference/varianceCallbacksAndIndexedAccesses.js

+};
+
+function f1(s: Source, t: Target) {
+    t = s;


I'm pretty sure this assignment is actually unsafe. Like, callback variance checks should prevent this from succeeding because Target's callbacks don't handle a wide enough range of things, but because we overwrite K with any for an erased comparison and WindowEventMap[any] is any it succeeds.

weswigham · 2018-08-08T18:31:19Z

Last RWC break I saw had to do with flow control - we use getConstraintAtLocation to generate the initial type, but when we went to go look it up again to see if it differs, we weren't getting the locally constrained version anymore. This meant that types which simplified (which there are now more of, thanks to these identities) were narrowed against themselves (or rather: their simplified versions), resulting in somewhat incoherent results. Now we get the constraint at the referencing location whenever we lookup the initial or declared type again.

@typescript-bot test this again, since now there should be no new breaks 🤞

typescript-bot · 2018-08-08T18:31:21Z

Heya @weswigham, I've started to run the extended test suite on this PR at 17d37ec. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2018-08-08T22:36:52Z

@typescript-bot test this now that we've accepted RWC updates so hopefully they are passing now

typescript-bot · 2018-08-08T22:36:54Z

Heya @weswigham, I've started to run the extended test suite on this PR at 17d37ec. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2018-08-08T23:45:42Z

So, I've checked: This also fixes #25181, which was also due to an issue with reasoning over an index on an intersection! This seems way cleaner than my other candidate to fix that.

weswigham · 2018-08-18T00:52:07Z

We talked about this today a bit - we think this is good, but we wanna fix the generic assignability issue one of the tests shows (T[K] being constrained to undefined to anything is assignable to it) and possibly sketch a patch to cover up the change in behavior for index signatures. (And run on DT to get a larger picture for how breaky it is)

…aint

weswigham · 2018-08-20T21:46:39Z

@typescript-bot test this again since I've fixed the constraint confusion issue

typescript-bot · 2018-08-20T21:46:41Z

Heya @weswigham, I've started to run the extended test suite on this PR at c68d37a. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2018-08-21T00:52:16Z

So I'm in the middle of running this on DT, and I've found a somewhat legitimate bug in a dt package activex-libreoffice in the same vein as #26274. They have a test of a generic createStruct function which takes a K extends keyof LibreOffice.StructNameMap. They then create a reflector over that K and attempt to call a createObject(obj: [(LibreOffice.ServicesNameMap & LibreOffice.StructNameMap)[K]]): void; using a [LibreOffice.StructNameMap[K]] for the argument type. Today this doesn't error, but on inspection you can tell that should ServicesNameMap and StructNameMap happen to overlap for some value of K, StructNameMap[K] alone does not satisfy the constraint (since it will be missing the ServicesNameMap bit).

I'm only 1/5th of the way done, so I can't say for certain just yet, but the DT changes have started off looking pretty good (given this is the only one thus far).

RyanCavanaugh · 2018-08-22T20:16:54Z

Please add repro from #26608 if it's not already covered by something representative

weswigham · 2018-08-22T20:57:11Z

@RyanCavanaugh added and confirmed for fixing #26608, too.

weswigham · 2018-08-22T20:58:53Z

@typescript-bot test this once more for a good ol' checkmark

typescript-bot · 2018-08-22T20:58:56Z

Heya @weswigham, I've started to run the extended test suite on this PR at 82b8a4f. You can monitor the build here. It should now contribute to this PR's status checks.

…redTypeRelatedTo to match the non-identity relations

weswigham · 2018-08-23T00:30:18Z

@ahejlsberg I know in person I spoke about how the one crash I found in DT while working on this was due to the recursion guard not being triggered due to simplified indexed accesses bypassing the types we guard against - rather than modify simplification further, I realized the issue was really that isIdenticalTo didn't shuffle nearly as many types through recursiveTypeRelatedTo as the paths for other relationships did, which is why the issue only occurred when identity got checked. So rather than doing the large improvement of merging simplification and constraint fetching, I've left that for a later date and simply fixed isIdenticalTo by moving most of it into an identity-only branch of structuredTypeRelatedTo.

@typescript-bot test this again now that I've fixed the redux crash from DT (there's a repro in the tests now), just in case.

typescript-bot · 2018-08-23T00:30:21Z

Heya @weswigham, I've started to run the extended test suite on this PR at 98e9b3e. You can monitor the build here. It should now contribute to this PR's status checks.

ahejlsberg · 2018-08-27T16:53:18Z

src/compiler/checker.ts

-        function getConstraintForLocation(type: Type, node: Node): Type | undefined {
+        function getConstraintForLocation(type: Type, node?: Node): Type;
+        function getConstraintForLocation(type: Type | undefined, node?: Node): Type | undefined;
+        function getConstraintForLocation(type: Type, node?: Node): Type | undefined {


Shouldn't this be node: Node | undefined, i.e. not an optional parameter? It seems pointless to make it optional since the function will never do anything if you don't pass an argument for it.

Also, when might node be undefined? Not clear to me what those changes are accomplishing.

Didn't need to be undefine'd-able at all. I probably factored the place where it may have been undefinable so it wouldn't be while I was working on it - fix't.

ahejlsberg · 2018-08-27T16:54:03Z

src/compiler/checker.ts

        function elaborateError(node: Expression | undefined, source: Type, target: Type): boolean {
            if (!node) return false;
+            if (isOrHasGenericConditional(target)) return false;


Combine this with line above and have a single if statement.

ahejlsberg · 2018-08-27T16:54:47Z

src/compiler/checker.ts

+                return type.simplified = mapType(objectType, t => getSimplifiedType(getIndexedAccessType(t, indexType)));
+            }
+            if (objectType.flags & TypeFlags.Intersection) {
+                return type.simplified = getIntersectionType(map((objectType as IntersectionType).types, t => getSimplifiedType(getIndexedAccessType(t, indexType))));


I love how much simpler this is!

sandersn · 2018-08-24T23:22:23Z

src/compiler/checker.ts

                getInitialType(<VariableDeclaration | BindingElement>node) :
-                getAssignedType(node);
+                getAssignedType(node), reference);


reference is always defined, isn't it? Then why does getConstraintForLocation change to node?: Node ?

It doesn't now. ;) I think a prior iteration needed it. It's unchanged now.

sandersn · 2018-08-24T23:24:52Z

src/compiler/checker.ts

-        function getInitialOrAssignedType(node: VariableDeclaration | BindingElement | Expression) {
-            return node.kind === SyntaxKind.VariableDeclaration || node.kind === SyntaxKind.BindingElement ?
+        function getInitialOrAssignedType(node: VariableDeclaration | BindingElement | Expression, reference: Node) {
+            return getConstraintForLocation(node.kind === SyntaxKind.VariableDeclaration || node.kind === SyntaxKind.BindingElement ?


I think this is the right thing, having poked at this during other bug fixes, but doesn't it cause a lot of type changes? Is that reflected in the baseline updates?

It is in the baselines - or rather, not doing this made the better distribution change some baselines, and then doing this changed them to the expected result.

weswigham added 3 commits August 7, 2018 14:44

unknownify accesses

1ff6007

Move to simplify to break less with fewer changes

87613bc

Accept baselines for now

c881d97

microsoft deleted a comment from typescript-bot Aug 7, 2018

Always propegate any as an index type

518046c

weswigham commented Aug 8, 2018

View reviewed changes

Fix flow control in the presence of simplifiable types

17d37ec

Add spy repro from microsoft#25181

89b9e6e

weswigham mentioned this pull request Aug 8, 2018

Add transformation to indexing intersection of homomorphic mapepd type and generic parameter #25490

Closed

weswigham requested review from ahejlsberg, sandersn and RyanCavanaugh August 8, 2018 23:46

This was referenced Aug 10, 2018

🤖 User test baselines have changed #26358

Merged

Regression with >= v3 and React, generics, defaultProps #26395

Closed

RyanCavanaugh mentioned this pull request Aug 17, 2018

Design Meeting Notes, 8/17/2018 #26518

Closed

weswigham added 2 commits August 20, 2018 13:45

Merge branch 'master' into unknownify-accesses

02bd1a1

Retain errorType when it is used as a marker for the lack of a constr…

5bf01b3

…aint

weswigham force-pushed the unknownify-accesses branch from ef9f6ee to 5bf01b3 Compare August 20, 2018 21:32

Small refinement

c68d37a

RyanCavanaugh mentioned this pull request Aug 22, 2018

Union of generic indexed types works incorrectly #26608

Closed

weswigham added 2 commits August 22, 2018 13:26

Merge branch 'master' into unknownify-accesses

bbb1164

Add new test

82b8a4f

Move most potentially recursive types from isIdenticalTo into structu…

98e9b3e

…redTypeRelatedTo to match the non-identity relations

ahejlsberg reviewed Aug 27, 2018

View reviewed changes

sandersn reviewed Aug 27, 2018

View reviewed changes

weswigham added 2 commits August 27, 2018 10:13

Fix nits

22b9a16

Doesnt need to be undefineable at all

37e5f6d

ahejlsberg approved these changes Aug 27, 2018

View reviewed changes

weswigham merged commit 0dbad04 into microsoft:master Aug 27, 2018

ahejlsberg mentioned this pull request Aug 28, 2018

Regression - able to use union containing non-indexing type as index #26714

Closed

weswigham deleted the unknownify-accesses branch August 28, 2018 19:57

ghost mentioned this pull request Sep 5, 2018

react: Comment out intersection type test DefinitelyTyped/DefinitelyTyped#28535

Merged

ghost mentioned this pull request Sep 19, 2018

No contextual type from indexed access into intersection type #27224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distribute indexed accesses when simplifying them #26281

Distribute indexed accesses when simplifying them #26281

weswigham commented Aug 7, 2018 •

edited

Loading

weswigham commented Aug 7, 2018

typescript-bot commented Aug 7, 2018 •

edited

Loading

weswigham commented Aug 8, 2018

typescript-bot commented Aug 8, 2018 •

edited

Loading

weswigham Aug 8, 2018 •

edited

Loading

weswigham commented Aug 8, 2018

typescript-bot commented Aug 8, 2018 •

edited

Loading

weswigham commented Aug 8, 2018

typescript-bot commented Aug 8, 2018 •

edited

Loading

weswigham commented Aug 8, 2018

weswigham commented Aug 18, 2018 •

edited

Loading

weswigham commented Aug 20, 2018

typescript-bot commented Aug 20, 2018 •

edited

Loading

weswigham commented Aug 21, 2018

RyanCavanaugh commented Aug 22, 2018

weswigham commented Aug 22, 2018

weswigham commented Aug 22, 2018

typescript-bot commented Aug 22, 2018 •

edited

Loading

weswigham commented Aug 23, 2018

typescript-bot commented Aug 23, 2018 •

edited

Loading

ahejlsberg Aug 27, 2018

weswigham Aug 27, 2018

ahejlsberg Aug 27, 2018

weswigham Aug 27, 2018

ahejlsberg Aug 27, 2018

sandersn Aug 24, 2018

weswigham Aug 27, 2018

sandersn Aug 24, 2018

weswigham Aug 27, 2018

Distribute indexed accesses when simplifying them #26281

Distribute indexed accesses when simplifying them #26281

Conversation

weswigham commented Aug 7, 2018 • edited Loading

weswigham commented Aug 7, 2018

typescript-bot commented Aug 7, 2018 • edited Loading

weswigham commented Aug 8, 2018

typescript-bot commented Aug 8, 2018 • edited Loading

weswigham Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

weswigham commented Aug 8, 2018

typescript-bot commented Aug 8, 2018 • edited Loading

weswigham commented Aug 8, 2018

typescript-bot commented Aug 8, 2018 • edited Loading

weswigham commented Aug 8, 2018

weswigham commented Aug 18, 2018 • edited Loading

weswigham commented Aug 20, 2018

typescript-bot commented Aug 20, 2018 • edited Loading

weswigham commented Aug 21, 2018

RyanCavanaugh commented Aug 22, 2018

weswigham commented Aug 22, 2018

weswigham commented Aug 22, 2018

typescript-bot commented Aug 22, 2018 • edited Loading

weswigham commented Aug 23, 2018

typescript-bot commented Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Aug 7, 2018 •

edited

Loading

typescript-bot commented Aug 7, 2018 •

edited

Loading

typescript-bot commented Aug 8, 2018 •

edited

Loading

weswigham Aug 8, 2018 •

edited

Loading

typescript-bot commented Aug 8, 2018 •

edited

Loading

typescript-bot commented Aug 8, 2018 •

edited

Loading

weswigham commented Aug 18, 2018 •

edited

Loading

typescript-bot commented Aug 20, 2018 •

edited

Loading

typescript-bot commented Aug 22, 2018 •

edited

Loading

typescript-bot commented Aug 23, 2018 •

edited

Loading