Skip to content

Support destructuring in for-in #54853

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

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

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Jul 1, 2023

closes #3715

Why I'm doing this:

  1. destructuring string is already supported in different contexts (TS playground) so it should also be allowed here for consistency
  2. the current es5 output is incorrect and can lead to a runtime crash if somebody ignores the fact that typechecking fails and grabs the emitted output. The current output looks like this: for (var first = (void 0)[0] in { test: 1 }) { }. As we can see, we end up with undefined[0] here

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 1, 2023
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@@ -2867,7 +2904,6 @@ export function transformES2015(context: TransformationContext): (x: SourceFile

// Disable trailing source maps for the OpenParenToken to align source map emit with the old emitter.
setEmitFlags(forStatement, EmitFlags.NoTokenTrailingSourceMaps);
setTextRange(forStatement, node);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was redundant - this call is a mutation and the same call is already done when assigning to forStatement

if (varExpr.kind === SyntaxKind.ArrayLiteralExpression || varExpr.kind === SyntaxKind.ObjectLiteralExpression) {
error(varExpr, Diagnostics.The_left_hand_side_of_a_for_in_statement_cannot_be_a_destructuring_pattern);
if (isAssignmentPattern(varExpr)) {
checkDestructuringAssignment(varExpr, getIndexTypeOrString(rightType));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this should actually be using getIndexTypeOrString(rightType). I used it because the branch below this uses that but because it does use it this code typechecks but, IMHO, it shouldn't (TS playground):

function test(obj: { a: 1; b: 2 }) {
  let key: "a" | "b";
  for (key in obj) {
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created this PR to address/discuss this: #54856

@rbuckton
Copy link
Contributor

We don't often make guarantees about the viability of the output when there are errors during the build. I'm also not entirely certain why anyone would want this? Object destructuring in for..in is somewhat pointless, though I could maybe see a justification for for (const [...codePoints] in obj), though I don't think that comes up that often, if at all.

@jakebailey
Copy link
Member

To be clear, my understanding is that this PR addresses a followup problem noted in: https://github.com/microsoft/TypeScript/pull/54801/files#r1244316943

@Andarist
Copy link
Contributor Author

The main motivation behind the PR is that:

destructuring string is already supported in different contexts (TS playground) so it should also be allowed here for consistency

So it's not exactly that I want this - I just find it weird that it's not supported here. It feels inconsistent with other parts of the checker.

We don't often make guarantees about the viability of the output when there are errors during the build.

Sure, I can certainly see that not being a strong argument - this was just added as an extra benefit of this change. On principle, I'd expect a semantically equivalent output (or smth close to that) when down-leveling syntax. The cost of this change isn't exceptionally high and I think that the added correctness/coverage/consistency is worth it.

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

Down-level destructuring in for..in statement
4 participants