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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ivy: accessing undefined ViewChild properly with elvis operator in template breaks #34087

Closed
mattlewis92 opened this issue Nov 27, 2019 · 4 comments
Closed

Comments

@mattlewis92
Copy link
Contributor

@mattlewis92 mattlewis92 commented Nov 27, 2019

馃悶 bug report

Affected Package

The issue is caused by package @angular/core

Is this a regression?

Yes, works fine in v8

Description

If I add a ViewChild decorator to an undefined component in a template and try and access it using the elvis operator in a template then the expression breaks and nothing is outputted in the template.

馃敩 Minimal Reproduction

https://github.com/mattlewis92/ivy-view-child-accesssor-bug/blob/master/src/app/app.component.html#L1

Where foo is:
https://github.com/mattlewis92/ivy-view-child-accesssor-bug/blob/master/src/app/app.component.ts#L11

馃敟 Exception or Error

There is no error, it fails silently to render:
Screenshot 2019-11-27 at 15 50 48

馃實 Your Environment

Angular Version:


Angular CLI: 9.0.0-rc.3
Node: 12.10.0
OS: darwin x64
Angular: 9.0.0-rc.3
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.900.0-rc.3
@angular-devkit/build-angular     0.900.0-rc.3
@angular-devkit/build-optimizer   0.900.0-rc.3
@angular-devkit/build-webpack     0.900.0-rc.3
@angular-devkit/core              9.0.0-rc.3
@angular-devkit/schematics        9.0.0-rc.3
@ngtools/webpack                  9.0.0-rc.3
@schematics/angular               9.0.0-rc.3
@schematics/update                0.900.0-rc.3
rxjs                              6.5.3
typescript                        3.6.4
webpack                           4.41.2

Anything else relevant?
Nope

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 4, 2019

Hi @mattlewis92, thanks for reporting this issue.

I performed the investigation and found out that the code generated by Ivy Compiler in a template function looks correct (compiled locally):

  textInterpolate1('This doesn\'t work: ',(((ctx.foo == null)? null: ctx.foo.bar)? 'bar': 'bam'),'');

and it looks like parentheses are removed after further processing (presumably in CLI):

  _angular_core__WEBPACK_IMPORTED_MODULE_0__["傻傻textInterpolate1"]("This doesn't work: ", ctx.foo == null ? null : ctx.foo.bar ? "bar" : "bam", " ");

Which changes the logic and makes the code return null (and render nothing) instead of rendering 'bam' string.

@filipesilva do you know if this is an optimization step in CLI that might affect this?

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 4, 2019

FYI @alxhub performed further investigation and fixed the problem in PR #34221 馃憤
The problem was that I looked at the JIT output and the problem was present in AOT output (which is now aligned with JIT).
Thank you.

alxhub added a commit to alxhub/angular that referenced this issue 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 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 an ability in the expression AST to distinguish between
right-associative and left-associative conditional expressions. This flag is
then used to emit parentheses whenever the condition of a conditional
expression is a left-associative conditional.

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
alxhub added a commit to alxhub/angular that referenced this issue 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 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
@mattlewis92

This comment has been minimized.

Copy link
Contributor Author

@mattlewis92 mattlewis92 commented Dec 4, 2019

Amazing, thank you for looking into it so quickly! 鉂わ笍

alxhub added a commit to alxhub/angular that referenced this issue 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 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
AndrewKushnir added a commit that referenced this issue 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 issue Dec 11, 2019
鈥lar#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 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鈥檛 perform that action at this time.