fix(compiler): generate correct code for safe method calls #44088
Conversation
JoostK
added
target: patch
JoostK
force-pushed
the
fix-safe-method-calls
branch
from
9a23ee1
to
ff17a6a
Compare
JoostK
added
the
action: review
| <span [title]="person?.getName(config.get('title')?.enabled ?? true)"></span> | ||
| ` | ||
| }) | ||
| export class MyApp { |
There was a problem hiding this comment.
Should we have a runtime equivalent of this test as well?
There was a problem hiding this comment.
I agree. An acceptance test near here would seem appropriate: https://github.com/angular/angular/blob/master/packages/core/test/acceptance/integration_spec.ts#L2077
There was a problem hiding this comment.
Neat little fix @JoostK - Please can you add the acceptance test?
| <span [title]="person?.getName(config.get('title')?.enabled ?? true)"></span> | ||
| ` | ||
| }) | ||
| export class MyApp { |
There was a problem hiding this comment.
I agree. An acceptance test near here would seem appropriate: https://github.com/angular/angular/blob/master/packages/core/test/acceptance/integration_spec.ts#L2077
petebacondarwin
added
action: cleanup
When a safe method call such as `person?.getName()` is used, the
compiler would generate invalid code if the argument list also contained
a safe method call. For example, the following code:
```
person?.getName(config?.get('title').enabled)
```
would generate
```
let tmp;
ctx.person == null ? null : ctx.person.getName((tmp = tmp) == null ?
null : tmp.enabled)
```
Notice how the call to `config.get('title')` has completely disappeared,
with `(tmp = tmp)` having taken its place.
The issue occurred due to how the argument list would be converted
from expression AST to output AST twice. First, the outer safe method
call would first convert its arguments list. This resulted in a
temporary being allocated for `config.get('title')`, which was stored in
the internal `_resultMap`. Only after the argument list has been
converted would the outer safe method call realize that it should be
guarded by a safe access of `person`, entering the `convertSafeAccess`
procedure to convert itself. This would convert the argument list once
again, but this time the `_resultMap` would already contain the
temporary `tmp` for `config?.get('title')`. Consequently, the safe
method in the argument list would be emitted as `tmp`.
This commit fixes the issue by ensuring that nodes are only converted
once.
Closes angular#44069
JoostK
force-pushed
the
fix-safe-method-calls
branch
from
ff17a6a
to
18252d0
Compare
| const fixture = TestBed.createComponent(App); | ||
| fixture.detectChanges(/* checkNoChanges */ false); | ||
| expect(fixture.nativeElement.textContent).toContain('Hello, unknown!'); | ||
| expect(log).toEqual(['getFallbackName()']); |
There was a problem hiding this comment.
nit: I don't think that we get much out of this log. If the resulting value is correct, then we can pretty confident that the generated code is correct.
There was a problem hiding this comment.
I included it mainly to verify short-circuiting behavior, e.g. in this case the inner safe access should not have been evaluated, and to ensure that methods are not called multiple times.
JoostK
added
action: merge
|
I see that you just added the 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. |
JoostK
added
the
merge: caretaker note
|
merge-assistance: legacy-unit-test-saucelabs are almost impossible to get green. |
|
This PR was merged into the repository by commit b249e24. |
When a safe method call such as `person?.getName()` is used, the
compiler would generate invalid code if the argument list also contained
a safe method call. For example, the following code:
```
person?.getName(config?.get('title').enabled)
```
would generate
```
let tmp;
ctx.person == null ? null : ctx.person.getName((tmp = tmp) == null ?
null : tmp.enabled)
```
Notice how the call to `config.get('title')` has completely disappeared,
with `(tmp = tmp)` having taken its place.
The issue occurred due to how the argument list would be converted
from expression AST to output AST twice. First, the outer safe method
call would first convert its arguments list. This resulted in a
temporary being allocated for `config.get('title')`, which was stored in
the internal `_resultMap`. Only after the argument list has been
converted would the outer safe method call realize that it should be
guarded by a safe access of `person`, entering the `convertSafeAccess`
procedure to convert itself. This would convert the argument list once
again, but this time the `_resultMap` would already contain the
temporary `tmp` for `config?.get('title')`. Consequently, the safe
method in the argument list would be emitted as `tmp`.
This commit fixes the issue by ensuring that nodes are only converted
once.
Closes #44069
PR Close #44088
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…4088) When a safe method call such as `person?.getName()` is used, the compiler would generate invalid code if the argument list also contained a safe method call. For example, the following code: ``` person?.getName(config?.get('title').enabled) ``` would generate ``` let tmp; ctx.person == null ? null : ctx.person.getName((tmp = tmp) == null ? null : tmp.enabled) ``` Notice how the call to `config.get('title')` has completely disappeared, with `(tmp = tmp)` having taken its place. The issue occurred due to how the argument list would be converted from expression AST to output AST twice. First, the outer safe method call would first convert its arguments list. This resulted in a temporary being allocated for `config.get('title')`, which was stored in the internal `_resultMap`. Only after the argument list has been converted would the outer safe method call realize that it should be guarded by a safe access of `person`, entering the `convertSafeAccess` procedure to convert itself. This would convert the argument list once again, but this time the `_resultMap` would already contain the temporary `tmp` for `config?.get('title')`. Consequently, the safe method in the argument list would be emitted as `tmp`. This commit fixes the issue by ensuring that nodes are only converted once. Closes angular#44069 PR Close angular#44088
fix(compiler): generate correct code for safe method calls
When a safe method call such as
person?.getName()is used, thecompiler would generate invalid code if the argument list also contained
a safe method call. For example, the following code:
would generate
Notice how the call to
config.get('title')has completely disappeared,with
(tmp = tmp)having taken its place.The issue occurred due to how the argument list would be converted
from expression AST to output AST twice. First, the outer safe method
call would first convert its arguments list. This resulted in a
temporary being allocated for
config.get('title'), which was stored inthe internal
_resultMap. Only after the argument list has beenconverted would the outer safe method call realize that it should be
guarded by a safe access of
person, entering theconvertSafeAccessprocedure to convert itself. This would convert the argument list once
again, but this time the
_resultMapwould already contain thetemporary
tmpforconfig?.get('title'). Consequently, the safemethod in the argument list would be emitted as
tmp.This commit fixes the issue by ensuring that nodes are only converted
once.
Closes #44069