fix(ivy): properly parenthesize ternary expressions when emitted (#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 #34087 PR Close #34221
This commit is contained in:
parent
f72de51a6f
commit
718d7fe5fe
|
@ -292,8 +292,36 @@ class ExpressionTranslatorVisitor implements ExpressionVisitor, StatementVisitor
|
||||||
}
|
}
|
||||||
|
|
||||||
visitConditionalExpr(ast: ConditionalExpr, context: Context): ts.ConditionalExpression {
|
visitConditionalExpr(ast: ConditionalExpr, context: Context): ts.ConditionalExpression {
|
||||||
|
let cond: ts.Expression = ast.condition.visitExpression(this, context);
|
||||||
|
|
||||||
|
// Ordinarily the ternary operator is right-associative. The following are equivalent:
|
||||||
|
// `a ? b : c ? d : e` => `a ? b : (c ? d : e)`
|
||||||
|
//
|
||||||
|
// However, occasionally Angular needs to produce a left-associative conditional, such as in
|
||||||
|
// the case of a null-safe navigation production: `{{a?.b ? c : d}}`. This template produces
|
||||||
|
// a ternary of the form:
|
||||||
|
// `a == null ? null : rest of expression`
|
||||||
|
// If the rest of the expression is also a ternary though, this would produce the form:
|
||||||
|
// `a == null ? null : a.b ? c : d`
|
||||||
|
// which, if left as right-associative, would be incorrectly associated as:
|
||||||
|
// `a == null ? null : (a.b ? c : d)`
|
||||||
|
//
|
||||||
|
// In such cases, the left-associativity needs to be enforced with parentheses:
|
||||||
|
// `(a == null ? null : a.b) ? c : d`
|
||||||
|
//
|
||||||
|
// Such parentheses could always be included in the condition (guaranteeing correct behavior) in
|
||||||
|
// all cases, but this has a code size cost. Instead, parentheses are added only when a
|
||||||
|
// conditional expression is directly used as the condition of another.
|
||||||
|
//
|
||||||
|
// TODO(alxhub): investigate better logic for precendence of conditional operators
|
||||||
|
if (ast.condition instanceof ConditionalExpr) {
|
||||||
|
// The condition of this ternary needs to be wrapped in parentheses to maintain
|
||||||
|
// left-associativity.
|
||||||
|
cond = ts.createParen(cond);
|
||||||
|
}
|
||||||
|
|
||||||
return ts.createConditional(
|
return ts.createConditional(
|
||||||
ast.condition.visitExpression(this, context), ast.trueCase.visitExpression(this, context),
|
cond, ast.trueCase.visitExpression(this, context),
|
||||||
ast.falseCase !.visitExpression(this, context));
|
ast.falseCase !.visitExpression(this, context));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -35,6 +35,11 @@ describe('type check blocks', () => {
|
||||||
expect(tcb(TEMPLATE)).toContain('((ctx).a)[(ctx).b];');
|
expect(tcb(TEMPLATE)).toContain('((ctx).a)[(ctx).b];');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should handle nested ternary expressions', () => {
|
||||||
|
const TEMPLATE = `{{a ? b : c ? d : e}}`;
|
||||||
|
expect(tcb(TEMPLATE)).toContain('((ctx).a ? (ctx).b : ((ctx).c ? (ctx).d : (ctx).e))');
|
||||||
|
});
|
||||||
|
|
||||||
it('should handle attribute values for directive inputs', () => {
|
it('should handle attribute values for directive inputs', () => {
|
||||||
const TEMPLATE = `<div dir inputA="value"></div>`;
|
const TEMPLATE = `<div dir inputA="value"></div>`;
|
||||||
const DIRECTIVES: TestDeclaration[] = [{
|
const DIRECTIVES: TestDeclaration[] = [{
|
||||||
|
|
|
@ -755,4 +755,29 @@ describe('compiler compliance: template', () => {
|
||||||
|
|
||||||
expectEmit(result.source, template, 'Incorrect template');
|
expectEmit(result.source, template, 'Incorrect template');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should safely nest ternary operations', () => {
|
||||||
|
const files = {
|
||||||
|
app: {
|
||||||
|
'spec.ts': `
|
||||||
|
import {Component, NgModule} from '@angular/core';
|
||||||
|
|
||||||
|
@Component({
|
||||||
|
selector: 'my-component',
|
||||||
|
template: \`
|
||||||
|
{{a?.b ? 1 : 2 }}\`
|
||||||
|
})
|
||||||
|
export class MyComponent {}
|
||||||
|
|
||||||
|
@NgModule({declarations: [MyComponent]})
|
||||||
|
export class MyModule {}
|
||||||
|
`
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
const template = `i0.ɵɵtextInterpolate1(" ", (ctx.a == null ? null : ctx.a.b) ? 1 : 2, "")`;
|
||||||
|
|
||||||
|
const result = compile(files, angularFiles);
|
||||||
|
expectEmit(result.source, template, 'Incorrect template');
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
Loading…
Reference in New Issue