Limit unsound indexed access type relations #27490
Conversation
ahejlsberg
requested review from
weswigham,
RyanCavanaugh and
DanielRosenwasser
There was a problem hiding this comment.
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.
| x1 = x2; | ||
| x1 = x3; | ||
| x1 = x4; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
They're basically covered by the new tests in keyofAndIndexedAccessErrors.ts, so I think it is fine to get rid of them.
|
@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? |
|
@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 |
Like yours? (Hint, hint) |
|
I knew what I was getting into when I said what I said. XD |
|
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 test this just in case tho |
|
Heya @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. |
|
I just tried this in my project, and it indeed does fix #27470! 🎉 |
|
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. |
Fixes #27470.