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

fix(compiler): work around TypeScript bug when narrowing switch statements #52110

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Oct 9, 2023

We type check @switch blocks by generating identical TS switch statements in the TCB, however TS currently has a bug where parenthesized switch block expressions don't narrow their types. Since we use parenthesized expressions to wrap AST nodes for diagnostics, this will bug will affect all Angular-generated switch statements.

These changes work around the issue by generating if/else if/else statements that represent the switch.

Some alternatives that were considered:

  1. Moving the switch expression to a constant - this is fairly simple to implement, but it won't fully resolve the narrowing issue since the same constant will have to be used in expressions inside the different cases.
  2. Removing the outer-most parenthesis from the switch expression - this works and allows us to continue using switch statements, but because we use parenthesized expressions to map diagnostics to their template locations, I wasn't sure if it won't lead to worse template dignostics.

Fixes #52077.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release area: compiler Issues related to `ngc`, Angular's template compiler labels Oct 9, 2023
@crisbeto crisbeto added this to the v17-candidates milestone Oct 9, 2023
@crisbeto crisbeto marked this pull request as ready for review October 9, 2023 09:00
@cexbrayat
Copy link
Member

@crisbeto I wanted to bring to your attention #52107 where I was asking about the possibility of having exhaustive switches in templates. If the TCB generates if statements, will that be possible to achieve?

@JoostK
Copy link
Member

JoostK commented Oct 9, 2023

@crisbeto I wanted to bring to your attention #52107 where I was asking about the possibility of having exhaustive switches in templates. If the TCB generates if statements, will that be possible to achieve?

Yep, no problem:

if (x.type === 'foo') {}
else if (x.type === 'bar') {}
else {
  const _: never = x.type;
}

will report an error if x.type has not been narrowed to the never type. I'm not sure whether the reported error message would be clear enough though.

@cexbrayat
Copy link
Member

@JoostK Ha yes, the never trick 👍 I hope this lands one way or another, I think it would be a great feature. Thanks for the answer!

@atscott atscott added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 9, 2023
@ngbot
Copy link

ngbot bot commented Oct 9, 2023

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing
    pending status "mergeability" is pending
    pending missing required status "ci/circleci: build"
    pending 1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@atscott atscott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Oct 9, 2023
…ments

We type check `@switch` blocks by generating identical TS `switch` statements in the TCB, however TS currently has a bug where parenthesized `switch` block expressions don't narrow their types. Since we use parenthesized expressions to wrap AST nodes for diagnostics, this will bug will affect all Angular-generated `switch` statements.

These changes work around the issue by generating `if`/`else if`/`else` statements that represent the `switch`.

Some alternatives that were considered:
1. Moving the `switch` expression to a constant - this is fairly simple to implement, but it won't fully resolve the narrowing issue since the same constant will have to be used in expressions inside the different cases.
2. Removing the outer-most parenthesis from the switch expression - this works and allows us to continue using switch statements, but because we use parenthesized expressions to map diagnostics to their template locations, I wasn't sure if it won't lead to worse template dignostics.

Fixes angular#52077.
@crisbeto
Copy link
Member Author

crisbeto commented Oct 9, 2023

Reworked based on Joost's feedback.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 9, 2023
@atscott
Copy link
Contributor

atscott commented Oct 9, 2023

This PR was merged into the repository by commit 386e1e9.

@atscott atscott closed this in 386e1e9 Oct 9, 2023
crisbeto added a commit to crisbeto/angular that referenced this pull request Oct 23, 2023
…ncompatible type comparisons

In angular#52110 the compiler was changed to produce `if` statements when type checking `@switch` in order to avoid a bug in the TypeScript compiler. In order to avoid duplicate diagnostics, the main `@switch` expression was ignored in each of the `@case` comparisons. This appears to have caused a regression where comparing incompatible types wasn't being reported anymore.

These changes resolve the issue by wrapping the expression in parentheses which allows the compiler to report comparison diagnostics while ignoring diagnostics in the expression itself.

Fixes angular#52315.
crisbeto added a commit to crisbeto/angular that referenced this pull request Oct 23, 2023
…ncompatible type comparisons

In angular#52110 the compiler was changed to produce `if` statements when type checking `@switch` in order to avoid a bug in the TypeScript compiler. In order to avoid duplicate diagnostics, the main `@switch` expression was ignored in each of the `@case` comparisons. This appears to have caused a regression where comparing incompatible types wasn't being reported anymore.

These changes resolve the issue by wrapping the expression in parentheses which allows the compiler to report comparison diagnostics while ignoring diagnostics in the expression itself.

Fixes angular#52315.
dylhunn pushed a commit that referenced this pull request Oct 23, 2023
…ncompatible type comparisons (#52322)

In #52110 the compiler was changed to produce `if` statements when type checking `@switch` in order to avoid a bug in the TypeScript compiler. In order to avoid duplicate diagnostics, the main `@switch` expression was ignored in each of the `@case` comparisons. This appears to have caused a regression where comparing incompatible types wasn't being reported anymore.

These changes resolve the issue by wrapping the expression in parentheses which allows the compiler to report comparison diagnostics while ignoring diagnostics in the expression itself.

Fixes #52315.

PR Close #52322
dylhunn pushed a commit that referenced this pull request Oct 23, 2023
…ncompatible type comparisons (#52322)

In #52110 the compiler was changed to produce `if` statements when type checking `@switch` in order to avoid a bug in the TypeScript compiler. In order to avoid duplicate diagnostics, the main `@switch` expression was ignored in each of the `@case` comparisons. This appears to have caused a regression where comparing incompatible types wasn't being reported anymore.

These changes resolve the issue by wrapping the expression in parentheses which allows the compiler to report comparison diagnostics while ignoring diagnostics in the expression itself.

Fixes #52315.

PR Close #52322
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 9, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…ments (angular#52110)

We type check `@switch` blocks by generating identical TS `switch` statements in the TCB, however TS currently has a bug where parenthesized `switch` block expressions don't narrow their types. Since we use parenthesized expressions to wrap AST nodes for diagnostics, this will bug will affect all Angular-generated `switch` statements.

These changes work around the issue by generating `if`/`else if`/`else` statements that represent the `switch`.

Some alternatives that were considered:
1. Moving the `switch` expression to a constant - this is fairly simple to implement, but it won't fully resolve the narrowing issue since the same constant will have to be used in expressions inside the different cases.
2. Removing the outer-most parenthesis from the switch expression - this works and allows us to continue using switch statements, but because we use parenthesized expressions to map diagnostics to their template locations, I wasn't sure if it won't lead to worse template dignostics.

Fixes angular#52077.

PR Close angular#52110
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@switch type narrowing doesn't work
4 participants