Skip to content
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

Don't defer resolution of indexed access types with reducible object types for writing accesses #54689

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Jun 17, 2023

fixes #54680
fixes #54834
regression from #53098

cc @ahejlsberg

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jun 17, 2023
@@ -301,3 +301,76 @@ function getValueConcrete<K extends keyof Foo1>(
): Foo1[K] | undefined {
return o[k];
}

// repro from https://github.com/microsoft/TypeScript/issues/54680
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this regression was assessed as working as intended by @ahejlsberg here but arguably the same unsoundness can be observed in different scenarios (see here).

The new test case from #54834 doesn't manifest the same unsoundness from what I can tell though.

@sandersn sandersn added this to Not started in PR Backlog Jul 3, 2023
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Jul 19, 2023
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Maybe just put the regressions in new test files so their variables don't have to have such funky, verbose names? I'm not a huge fan of the big omni-test files we sometimes add anyway - they're a massive pain to debug.

Otherwise, I think this is probably OK, pending a sync and verification that there are no unexpected regressions in the extended test suite.

PR Backlog automation moved this from Waiting on reviewers to Waiting on author Sep 13, 2023
@Andarist
Copy link
Contributor Author

Actually, this might have to be closed. Anders first assessed that this is working as intended here and later here.

I find it problematic that a different variant of this still works though (see here) and I think that both should behave in the same way (but now one error and one doesn't). Perhaps that should be reported as a bug now - so both of them would error (?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Waiting on author
Development

Successfully merging this pull request may close these issues.

Type correlation regression Extract breaks when upgrading to TypeScript 5.1
5 participants