Skip to content

feat(compiler): support unary operators for more accurate type checking #37918

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

feat(compiler): support unary operators for more accurate type checking #37918

wants to merge 2 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Jul 3, 2020

Prior to this change, the unary + and - operators would be parsed as x - 0
and 0 - x respectively. The runtime semantics of these expressions are
equivalent, however they may introduce inaccurate template type checking
errors as the literal type is lost, for example:

@Component({
  template: `<button [disabled]="isAdjacent(-1)"></button>`
})
export class Example {
  isAdjacent(direction: -1 | 1): boolean { return false; }
}

would incorrectly report a type-check error:

error TS2345: Argument of type 'number' is not assignable to parameter
of type '-1 | 1'.

Additionally, the translated expression for the unary + operator would be
considered as arithmetic expression with an incompatible left-hand side:

error TS2362: The left-hand side of an arithmetic operation must be of
type 'any', 'number', 'bigint' or an enum type.

To resolve this issues, the implicit transformation should be avoided.
This commit adds a new unary AST node to represent these expressions,
allowing for more accurate type-checking.

Fixes #20845
Fixes #36178

@JoostK JoostK added type: bug/fix feature Issue that requests a new feature freq1: low severity2: inconvenient target: major This PR is targeted for the next major release area: compiler Issues related to `ngc`, Angular's template compiler labels Jul 3, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jul 3, 2020
@JoostK JoostK force-pushed the literal-unary-ops branch 2 times, most recently from 15a8618 to 07d4f45 Compare July 4, 2020 15:07
@JoostK JoostK marked this pull request as ready for review July 4, 2020 17:50
@pullapprove pullapprove bot requested a review from kyliau July 4, 2020 17:51
@JoostK JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jul 4, 2020
JoostK
JoostK commented Jul 4, 2020
View reviewed changes
@JoostK JoostK requested a review from alxhub July 4, 2020 17:52
@JoostK JoostK force-pushed the literal-unary-ops branch 2 times, most recently from 3ec804d to ba62b14 Compare July 11, 2020 16:28
@JoostK
feat(compiler): support unary operators for more accurate type checking
2babfcc 
Prior to this change, the unary + and - operators would be parsed as `x - 0`
and `0 - x` respectively. The runtime semantics of these expressions are
equivalent, however they may introduce inaccurate template type checking
errors as the literal type is lost, for example:

```ts
@component({
  template: `<button [disabled]="isAdjacent(-1)"></button>`
})
export class Example {
  isAdjacent(direction: -1 | 1): boolean { return false; }
}
```

would incorrectly report a type-check error:

> error TS2345: Argument of type 'number' is not assignable to parameter
  of type '-1 | 1'.

Additionally, the translated expression for the unary + operator would be
considered as arithmetic expression with an incompatible left-hand side:

> error TS2362: The left-hand side of an arithmetic operation must be of
  type 'any', 'number', 'bigint' or an enum type.

To resolve this issues, the implicit transformation should be avoided.
This commit adds a new unary AST node to represent these expressions,
allowing for more accurate type-checking.

Fixes angular#20845
Fixes angular#36178
@JoostK JoostK force-pushed the literal-unary-ops branch from ba62b14 to 08f643c Compare August 3, 2020 19:36
@JoostK JoostK force-pushed the literal-unary-ops branch from 08f643c to fe5e22f Compare August 3, 2020 20:02
@alxhub
Copy link
Member

alxhub commented Aug 3, 2020

Presubmit

@alxhub
Copy link
Member

alxhub commented Aug 4, 2020

Funnily enough, codelyzer is fine with this one 👍

@kyliau kyliau 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 Aug 20, 2020
@ngbot
Copy link

ngbot bot commented Aug 20, 2020

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "google3" is failing

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 master, please try rebasing to master 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.

@kyliau
Copy link
Contributor

kyliau commented Aug 20, 2020

presubmit: http://test/OCL:327567923:BASE:327567938:1597899560264:b7598ce2

@mhevery mhevery closed this in 874792d Aug 21, 2020
subratpalhar92 pushed a commit to SUBRATPALHAR-ALL-JAVASCRIPT/angular that referenced this pull request Aug 29, 2020
@JoostK @subratpalhar92
feat(compiler): support unary operators for more accurate type checki…
0a55856 
…ng (angular#37918)

Prior to this change, the unary + and - operators would be parsed as `x - 0`
and `0 - x` respectively. The runtime semantics of these expressions are
equivalent, however they may introduce inaccurate template type checking
errors as the literal type is lost, for example:

```ts
@component({
  template: `<button [disabled]="isAdjacent(-1)"></button>`
})
export class Example {
  isAdjacent(direction: -1 | 1): boolean { return false; }
}
```

would incorrectly report a type-check error:

> error TS2345: Argument of type 'number' is not assignable to parameter
  of type '-1 | 1'.

Additionally, the translated expression for the unary + operator would be
considered as arithmetic expression with an incompatible left-hand side:

> error TS2362: The left-hand side of an arithmetic operation must be of
  type 'any', 'number', 'bigint' or an enum type.

To resolve this issues, the implicit transformation should be avoided.
This commit adds a new unary AST node to represent these expressions,
allowing for more accurate type-checking.

Fixes angular#20845
Fixes angular#36178

PR Close angular#37918
subratpalhar92 added a commit to SUBRATPALHAR-ALL-JAVASCRIPT/angular that referenced this pull request Aug 31, 2020
@subratpalhar92
Revert "feat(compiler): support unary operators for more accurate typ…
52f0b16 
…e checking (angular#37918)"

This reverts commit 0a55856.
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@JoostK @profanis
feat(compiler): support unary operators for more accurate type checki…
4a85851 
…ng (angular#37918)

Prior to this change, the unary + and - operators would be parsed as `x - 0`
and `0 - x` respectively. The runtime semantics of these expressions are
equivalent, however they may introduce inaccurate template type checking
errors as the literal type is lost, for example:

```ts
@component({
  template: `<button [disabled]="isAdjacent(-1)"></button>`
})
export class Example {
  isAdjacent(direction: -1 | 1): boolean { return false; }
}
```

would incorrectly report a type-check error:

> error TS2345: Argument of type 'number' is not assignable to parameter
  of type '-1 | 1'.

Additionally, the translated expression for the unary + operator would be
considered as arithmetic expression with an incompatible left-hand side:

> error TS2362: The left-hand side of an arithmetic operation must be of
  type 'any', 'number', 'bigint' or an enum type.

To resolve this issues, the implicit transformation should be avoided.
This commit adds a new unary AST node to represent these expressions,
allowing for more accurate type-checking.

Fixes angular#20845
Fixes angular#36178

PR Close angular#37918
@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 Sep 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alxhub alxhub alxhub approved these changes

+1 more reviewer

@kyliau kyliau kyliau approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes feature Issue that requests a new feature freq1: low target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Milestone
needsTriage
4 participants
@JoostK @alxhub @kyliau @googlebot