Skip to content

Fix indexed access relation #21242

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

Merged
merged 2 commits into from
Jan 19, 2018
Merged

Fix indexed access relation #21242

merged 2 commits into from
Jan 19, 2018

Conversation

sandersn
Copy link
Member

Fixes #21185

Previously, structuredTypeRelatedTo only checked the object types of an index access type, and only if the index types were identical. Now both checks call isRelatedTo recursively. Before #17912 this was not a big problem because the preceding constraint check often erroneously used any as the constraint.

Previously, it only check the object types, and only if the index types
were identical. Now both checks call `isRelatedTo` recursively.
@sandersn
Copy link
Member Author

I tested Definitely Typed with this change, since I expect master to be more broken after #17912. I didn't see any breaks -- the only failures were a result of the test automation.

However, at least one test timed out just before the tests finished running. There has been some performance regression recently since @weswigham observed this for the RWC tests and there have been a couple of bugs about slow compilation recently.

@weswigham
Copy link
Member

This now seems to mimic what inference takes for granted, which is nice. I do wonder if we can tell if it'd be faster to check the index types or object types first, though. My intuition says that the index types are likely to be less complicated, so maybe we should compare those first; but I have no real data backing that.

@sandersn
Copy link
Member Author

@weswigham I plan to investigate performance today, although if there is any impact from this change, it’s probably a result of #17912 getting rid of the erroneous anys, which just means more checking overall. Still, I should test both orders.

@sandersn
Copy link
Member Author

The definitely typed and user tests that were timing out appear to be a result of npm slowness. I'll start looking at the bugs about compiler slowness instead.

@sandersn sandersn merged commit 4632ed6 into master Jan 19, 2018
@sandersn sandersn deleted the fix-indexed-access-relation branch January 19, 2018 21:13
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants