Optional chaining of method calls may generate invalid instructions #44069

ties-s · 2021-11-04T21:46:23Z

Which @angular/* package(s) are the source of the bug?

forms

Is this a regression?

Yes

Description

When passing a form control value to a function that is end the end of an optional chain, the value is null.

Expected (works in v12), console:
Thu Nov 04 2021 22:42:39 GMT+0100 (CET) – Thu Nov 04 2021 22:42:39 GMT+0100 (CET)

Actual in v13:
Thu Nov 04 2021 22:42:39 GMT+0100 (CET) – null

This problem does not occur when using ! instead of ? on the form value, or when using the value outside a function that is part of an optional chain.

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

// app.component.ts
import { Component } from '@angular/core';
import { FormBuilder, Validators } from '@angular/forms';

@Component({
	selector: 'app-root',
	templateUrl: './app.component.html',
	styleUrls: ['./app.component.scss']
})
export class AppComponent {
	form = this.fb.group({
		test: [new Date(), Validators.required]
	})

	constructor(private fb: FormBuilder) {}

	public a?= {
		fn(_: any) {
			return true;
		}
	}

	test(e: any) {
		console.log(this.form.get('test')?.value, e);
	}
}

// app.component.html
<div [formGroup]="form">
	<button class="show" (click)="a?.fn(test(form.get('test')?.value))">SHOW</button>
</div>

Please provide the environment you discovered this bug in

Angular CLI: 13.0.1
Node: 16.11.1
Package Manager: npm 8.1.2
OS: darwin arm64

Angular: 13.0.0
... animations, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1300.1
@angular-devkit/build-angular   13.0.1
@angular-devkit/core            13.0.1
@angular-devkit/schematics      13.0.1
@angular/cli                    13.0.1
@schematics/angular             13.0.1
rxjs                            6.6.7
typescript                      4.4.4

Anything else?

Checked exact same sample in latest Angular 12, no problems there.

The text was updated successfully, but these errors were encountered:

ties-s · 2021-11-04T22:07:11Z

Repro: https://github.com/ties-s/ng-form-chained-repro

petebacondarwin · 2021-11-05T10:38:37Z

I think that this is indeed a bug in how the Angular expression is being converted to TS in the ivy handler.

The following expression

a?.fn(test(form.get('test')?.value))

is converted to

let tmp_b_0;
ctx.a == null ? null : ctx.a.fn(ctx.test((tmp_b_0 = tmp_b_0) == null ? null : tmp_b_0.value));

while the following expression

test(form.get('test')?.value)

is converted to

ctx.test((tmp_b_0 = ctx.form.get("test")) == null ? null : tmp_b_0.value);

Note that in the first case we have:

ctx.test((tmp_b_0 = tmp_b_0) == null ? null : tmp_b_0.value)

while in the second:

ctx.test((tmp_b_0 = ctx.form.get("test")) == null ? null : tmp_b_0.value);

So in the first case tmp_b_0 is always undefined...

ties-s · 2021-11-05T14:24:43Z

Yeah I noticed that too. So this is a bug. A pretty big one for my code base.

JoostK · 2021-11-05T17:45:54Z

I have a hunch this got broken in #42882. I'll be looking at a fix.

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 access. 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 `ctx.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

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

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

ties-s · 2021-11-09T16:14:54Z

Thank for the quick fix! Any info on when a new release will be cut?

JoostK · 2021-11-09T16:43:34Z

Typically on Wednesdays (US timezone)

angular-automatic-lock-bot · 2021-12-10T16:10:26Z

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.}

…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

JoostK mentioned this issue Nov 5, 2021

BrowserModule has already been loaded error when importing an angular library that imports BrowserModule #44015

Closed

petebacondarwin added comp: compiler P3 regression labels Nov 5, 2021

ngbot bot added this to the Backlog milestone Nov 5, 2021

JoostK self-assigned this Nov 5, 2021

JoostK changed the title ~~Missing form value in template when used as argument Angular 13~~ Optional chaining of method calls may generate invalid instructions Nov 5, 2021

JoostK mentioned this issue Nov 5, 2021

fix(compiler): generate correct code for safe method calls #44088

Closed

JoostK added the state: has PR label Nov 5, 2021

atscott closed this in b249e24 Nov 8, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 10, 2021

Dec	JAN	Feb
	31
2021	2022	2023

angular / angular Public

Optional chaining of method calls may generate invalid instructions #44069

Optional chaining of method calls may generate invalid instructions #44069

ties-s commented Nov 4, 2021 •

edited

ties-s commented Nov 4, 2021

petebacondarwin commented Nov 5, 2021

ties-s commented Nov 5, 2021

JoostK commented Nov 5, 2021

ties-s commented Nov 9, 2021

JoostK commented Nov 9, 2021

angular-automatic-lock-bot bot commented Dec 10, 2021

angular / angular Public

Optional chaining of method calls may generate invalid instructions #44069

Optional chaining of method calls may generate invalid instructions #44069

Comments

ties-s commented Nov 4, 2021 • edited

Which @angular/* package(s) are the source of the bug?

Is this a regression?

Description

Please provide a link to a minimal reproduction of the bug

Please provide the exception or error you saw

Please provide the environment you discovered this bug in

Anything else?

ties-s commented Nov 4, 2021

petebacondarwin commented Nov 5, 2021

ties-s commented Nov 5, 2021

JoostK commented Nov 5, 2021

ties-s commented Nov 9, 2021

JoostK commented Nov 9, 2021

angular-automatic-lock-bot bot commented Dec 10, 2021

ties-s commented Nov 4, 2021 •

edited