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(ivy): properly parenthesize ternary expressions when emitted #34221

Closed
wants to merge 1 commit into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Dec 4, 2019

Previously, ternary expressions were emitted as:

condExpr ? trueCase : falseCase

However, this causes problems when ternary operations are nested. In
particular, a template expression of the form:

a?.b ? c : d

would have compiled to:

a == null ? null : a.b ? c : d

The precedence here is incorrect:

a == null ? null : (a.b ? c : d)

when in reality the desired association is:

(a == null ? null : a.b) ? c : d

This commit adds parentheses when emitting ternary operations to preserve
the desired precedence.

Copy link
Contributor

AndrewKushnir left a comment

LGTM 👍 Thanks for the fix Alex!

@kara
kara approved these changes Dec 4, 2019
Copy link
Contributor

kara left a comment

LGTM too, though looks like one more compliance test needs to be updated (CI is unhappy)

@ngbot ngbot bot added this to the needsTriage milestone Dec 4, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/parens-conditional branch 2 times, most recently from 73cab8a to 9842512 Dec 4, 2019
Copy link
Contributor

AndrewKushnir left a comment

👍(after the latest set of changes)

Previously, ternary expressions were emitted as:

condExpr ? trueCase : falseCase

However, this causes problems when ternary operations are nested. In
particular, a template expression of the form:

a?.b ? c : d

would have compiled to:

a == null ? null : a.b ? c : d

The ternary operator is right-associative, so that expression is interpreted
as:

a == null ? null : (a.b ? c : d)

when in reality left-associativity is desired in this particular instance:

(a == null ? null : a.b) ? c : d

This commit adds a check in the expression translator to detect such
left-associative usages of ternaries and to enforce such associativity with
parentheses when necessary.

A test is also added for the template type-checking expression translator,
to ensure it correctly produces right-associative expressions for ternaries
in the user's template.

Fixes #34087
@alxhub alxhub force-pushed the alxhub:ngtsc/parens-conditional branch from 9842512 to 025bf0d Dec 4, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Dec 6, 2019

AndrewKushnir added a commit that referenced this pull request Dec 6, 2019
)

Previously, ternary expressions were emitted as:

condExpr ? trueCase : falseCase

However, this causes problems when ternary operations are nested. In
particular, a template expression of the form:

a?.b ? c : d

would have compiled to:

a == null ? null : a.b ? c : d

The ternary operator is right-associative, so that expression is interpreted
as:

a == null ? null : (a.b ? c : d)

when in reality left-associativity is desired in this particular instance:

(a == null ? null : a.b) ? c : d

This commit adds a check in the expression translator to detect such
left-associative usages of ternaries and to enforce such associativity with
parentheses when necessary.

A test is also added for the template type-checking expression translator,
to ensure it correctly produces right-associative expressions for ternaries
in the user's template.

Fixes #34087

PR Close #34221
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
…ular#34221)

Previously, ternary expressions were emitted as:

condExpr ? trueCase : falseCase

However, this causes problems when ternary operations are nested. In
particular, a template expression of the form:

a?.b ? c : d

would have compiled to:

a == null ? null : a.b ? c : d

The ternary operator is right-associative, so that expression is interpreted
as:

a == null ? null : (a.b ? c : d)

when in reality left-associativity is desired in this particular instance:

(a == null ? null : a.b) ? c : d

This commit adds a check in the expression translator to detect such
left-associative usages of ternaries and to enforce such associativity with
parentheses when necessary.

A test is also added for the template type-checking expression translator,
to ensure it correctly produces right-associative expressions for ternaries
in the user's template.

Fixes angular#34087

PR Close angular#34221
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 6, 2020

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 Jan 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.