GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Limit unsound indexed access type relations by ahejlsberg · Pull Request #27490 · microsoft/TypeScript

Conversation

@ahejlsberg

@ahejlsberg Anders Hejlsberg (ahejlsberg) commented Oct 1, 2018

Copy link
Copy Markdown
Member

Fixes #27470.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Anders Hejlsberg (ahejlsberg) added 4 commits October 1, 2018 16:20
@ahejlsberg
No unsound assignments to T[K] when T and K are both generic
c0c215f
@ahejlsberg
Accept new baselines
2801c97
@ahejlsberg
Add tests
3499462
@ahejlsberg
Accept new baselines
69cd6c0
@ahejlsberg
Anders Hejlsberg (ahejlsberg) requested review from Daniel Rosenwasser (DanielRosenwasser), Ryan Cavanaugh (RyanCavanaugh) and Wesley Wigham (weswigham) October 1, 2018 23:26
weswigham
Wesley Wigham (weswigham) approved these changes Oct 2, 2018
View reviewed changes

@weswigham Wesley Wigham (weswigham) left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

While I wholeheartedly agree with chipping away at how unsound we are here (and this looks fine in that regard), I do worry about the knock-on effects in our ecosystem (especially around, eg, react). What I've seen is that when types start getting complex (eg, generic indexes + other generic type constructors) people stop understanding them and simply (over)specify them until they work without quite understanding what shape/procedure they've described. We should probably do a diff on DT with this change to see if it's as bad as I think it might be, or if I'm overthinking it. Not to see if it's a good change, but to see if we need to identify and fix any libraries which embed a similar problem to this and accidentally or intentionally rely on it before we ship it in a full release.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
weswigham
Wesley Wigham (weswigham) reviewed Oct 2, 2018
View reviewed changes
Comment thread tests/baselines/reference/keyofAndIndexedAccess.js
function f90<T extends S2, K extends keyof S2>(x1: S2[keyof S2], x2: T[keyof S2], x3: S2[K]) {
x1 = x2;
x1 = x3;
x1 = x4;

@weswigham Wesley Wigham (weswigham) Oct 2, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Might be worth keeping all these variants (maybe in a different file or function) just to document that they're expected to fail now (and so we know if they suddenly start succeeding again).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ahejlsberg Anders Hejlsberg (ahejlsberg) Oct 2, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

They're basically covered by the new tests in keyofAndIndexedAccessErrors.ts, so I think it is fine to get rid of them.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Oct 2, 2018

Copy link
Copy Markdown
Member Author

Wesley Wigham (@weswigham) Do we have an automated way of running the DT tests? (Similar to the RWC bot.) Otherwise, what's the best way to go about it?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@weswigham

Wesley Wigham (weswigham) commented Oct 2, 2018

Copy link
Copy Markdown
Member

Anders Hejlsberg (@ahejlsberg) No, the bot doesn't have a build for baselining dt. I can probably whip one up, though I don't think it'll be able to have an automatic PR/diff viewing system like RWC does. In the absence of that, running jake runtests-parallel ru=dt on master, accepting the initial baseline snapshot, then running it again on this branch would be a way to go about it locally; but you probably want someone with a real beefy machine to do it, since they take quite a long time compared to RWC, even.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Oct 2, 2018

Copy link
Copy Markdown
Member Author

but you probably want someone with a real beefy machine to do it

Like yours? (Hint, hint)
RyanCavanaugh, weswigham, and DanielRosenwasser reacted with laugh emoji
All reactions
  • 😄 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@weswigham

Wesley Wigham (weswigham) commented Oct 2, 2018

Copy link
Copy Markdown
Member

I knew what I was getting into when I said what I said. XD
DanielRosenwasser reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@weswigham

Wesley Wigham (weswigham) commented Oct 2, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

And 4 hours later the results are in. My fears seem overblown, this change doesn't seem to affect anything on DT. For reference, my motivating issue for my worry was #27201, wherein what people are trying to do might be affected by this.

TypeScript Bot (@typescript-bot) test this just in case tho
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Oct 2, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Wesley Wigham (@weswigham), I've started to run the extended test suite on this PR at 69cd6c0. You can monitor the build here. It should now contribute to this PR's status checks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@squirly

Tyler Jones (squirly) commented Oct 3, 2018

Copy link
Copy Markdown

I just tried this in my project, and it indeed does fix #27470! 🎉
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@weswigham

Wesley Wigham (weswigham) commented Oct 3, 2018

Copy link
Copy Markdown
Member

I think rwc on this PR is fine; this PR is just missing a change in master (semicolon in a helper) which caused an RWC change. Everything looks good.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RyanCavanaugh
Ryan Cavanaugh (RyanCavanaugh) approved these changes Oct 3, 2018
View reviewed changes
@ahejlsberg
Anders Hejlsberg (ahejlsberg) merged commit 9851d6f into master Oct 3, 2018
@ahejlsberg
Anders Hejlsberg (ahejlsberg) deleted the indexedAccessConstraint branch October 3, 2018 17:58
This was referenced Nov 22, 2018
After updating to 3.x.x getting TS2536 #28647
Closed
true extends false #28654
Closed
@jack-williams Jack Williams (jack-williams) mentioned this pull request Jan 17, 2019
Constraint failing for subtype (regression) #29464
Closed
@jack-williams Jack Williams (jack-williams) mentioned this pull request Mar 7, 2019
For T extends M where M is a mapped type, T[keyof T] doesn't consider M #30217
Closed
@jack-williams Jack Williams (jack-williams) mentioned this pull request Apr 5, 2019
Improve soundness of indexed access types #30769
Merged
@gabritto Gabriela Araujo Britto (gabritto) mentioned this pull request Apr 6, 2022
Type assignable to index access of constraint of type parameter is incorrectly assignable to the index access itself #46076
Closed
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
@typescript-bot TypeScript Bot (typescript-bot) assigned Anders Hejlsberg (ahejlsberg) Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham Wesley Wigham (weswigham) weswigham approved these changes
@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) RyanCavanaugh approved these changes
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Awaiting requested review from DanielRosenwasser

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@ahejlsberg Anders Hejlsberg (ahejlsberg)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@ahejlsberg @weswigham @typescript-bot @squirly @RyanCavanaugh
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.