-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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): generate correct code for safe method calls #44088
Conversation
9a23ee1
to
ff17a6a
Compare
<span [title]="person?.getName(config.get('title')?.enabled ?? true)"></span> | ||
` | ||
}) | ||
export class MyApp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a runtime equivalent of this test as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 theconvertSafeAccess
procedure to convert itself. This would convert the argument list once
again, but this time the
_resultMap
would already contain thetemporary
tmp
forconfig?.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