Skip to content

Commit 243b94c

Browse files
crisbetothePunderWoman
crisbeto
authored and
thePunderWoman
committed
refactor(compiler-cli): fix regression in two-way bindings to inputs with different getter/setter types (angular#54252)
In a previous commit the TCB was changed to cast the assignment to an input in order to widen its type to allow `WritableSignal`. This ended up breaking existing inputs whose setter has a wider type than its getter. These changes switch to unwrapping the value on the binding side. PR Close angular#54252
1 parent 551c579 commit 243b94c

13 files changed

Lines changed: 120 additions & 97 deletions

File tree

‎goldens/public-api/core/errors.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,10 +123,10 @@ export const enum RuntimeErrorCode {
123123
// (undocumented)
124124
REQUIRED_INPUT_NO_VALUE = -950,
125125
// (undocumented)
126-
REQUIRED_QUERY_NO_VALUE = -951,
127-
// (undocumented)
128126
REQUIRED_MODEL_NO_VALUE = -952,
129127
// (undocumented)
128+
REQUIRED_QUERY_NO_VALUE = -951,
129+
// (undocumented)
130130
RUNTIME_DEPS_INVALID_IMPORTED_TYPE = 1000,
131131
// (undocumented)
132132
RUNTIME_DEPS_ORPHAN_COMPONENT = 1001,

‎packages/compiler-cli/src/ngtsc/testing/fake_core/index.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,10 @@ export interface WritableSignal<T> extends Signal<T> {
132132
asReadonly(): Signal<T>;
133133
}
134134

135+
export function ɵunwrapWritableSignal<T>(value: T|WritableSignal<T>): T {
136+
return null!;
137+
}
138+
135139
export function signal<T>(initialValue: T): WritableSignal<T> {
136140
return null!;
137141
}

‎packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts‎

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -350,31 +350,15 @@ export class SymbolBuilder {
350350
return host !== null ? {kind: SymbolKind.DomBinding, host} : null;
351351
}
352352

353-
const isTwoWayBinding =
354-
binding instanceof TmplAstBoundAttribute && binding.type === BindingType.TwoWay;
355353
const nodes = findAllMatchingNodes(
356354
this.typeCheckBlock, {withSpan: binding.sourceSpan, filter: isAssignment});
357355
const bindings: BindingSymbol[] = [];
358356
for (const node of nodes) {
359-
let assignment: ts.PropertyAccessExpression|ts.ElementAccessExpression|null = null;
360-
361-
if (isAccessExpression(node.left)) {
362-
// One-way bindings usually are in the form of `dir.input = expression`.
363-
assignment = node.left;
364-
} else if (
365-
isTwoWayBinding && ts.isParenthesizedExpression(node.left) &&
366-
ts.isAsExpression(node.left.expression) &&
367-
isAccessExpression(node.left.expression.expression)) {
368-
// The property side of two-way bindings is in the
369-
// form of `(dir.input as someType) = expression`.
370-
assignment = node.left.expression.expression;
371-
}
372-
373-
if (assignment === null) {
357+
if (!isAccessExpression(node.left)) {
374358
continue;
375359
}
376360

377-
const signalInputAssignment = unwrapSignalInputWriteTAccessor(assignment);
361+
const signalInputAssignment = unwrapSignalInputWriteTAccessor(node.left);
378362
let symbolInfo: TsNodeSymbolInfo|null = null;
379363

380364
// Signal inputs need special treatment because they are generated with an extra keyed
@@ -392,15 +376,15 @@ export class SymbolBuilder {
392376
tsType: typeSymbol.tsType,
393377
};
394378
} else {
395-
symbolInfo = this.getSymbolOfTsNode(assignment);
379+
symbolInfo = this.getSymbolOfTsNode(node.left);
396380
}
397381

398382
if (symbolInfo === null || symbolInfo.tsSymbol === null) {
399383
continue;
400384
}
401385

402386
const target = this.getDirectiveSymbolForAccessExpression(
403-
signalInputAssignment?.fieldExpr ?? assignment, consumer);
387+
signalInputAssignment?.fieldExpr ?? node.left, consumer);
404388
if (target === null) {
405389
continue;
406390
}

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts‎

Lines changed: 28 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -642,7 +642,7 @@ class TcbDirectiveCtorOp extends TcbOp {
642642
attr.attribute instanceof TmplAstTextAttribute) {
643643
continue;
644644
}
645-
for (const {fieldName} of attr.inputs) {
645+
for (const {fieldName, isTwoWayBinding} of attr.inputs) {
646646
// Skip the field if an attribute has already been bound to it; we can't have a duplicate
647647
// key in the type constructor call.
648648
if (genericInputs.has(fieldName)) {
@@ -656,6 +656,7 @@ class TcbDirectiveCtorOp extends TcbOp {
656656
field: fieldName,
657657
expression,
658658
sourceSpan: attr.attribute.sourceSpan,
659+
isTwoWayBinding,
659660
});
660661
}
661662
}
@@ -809,13 +810,15 @@ class TcbDirectiveInputsOp extends TcbOp {
809810
target = ts.factory.createElementAccessExpression(target, inputSignalBrandWriteSymbol);
810811
}
811812

812-
if (isTwoWayBinding) {
813-
target = this.getTwoWayBindingTarget(target);
814-
}
815-
816813
if (attr.attribute.keySpan !== undefined) {
817814
addParseSpanInfo(target, attr.attribute.keySpan);
818815
}
816+
817+
// Two-way bindings accept `T | WritableSignal<T>` so we have to unwrap the value.
818+
if (isTwoWayBinding) {
819+
assignment = unwrapWritableSignal(assignment, this.tcb);
820+
}
821+
819822
// Finally the assignment is extended by assigning it into the target expression.
820823
assignment =
821824
ts.factory.createBinaryExpression(target, ts.SyntaxKind.EqualsToken, assignment);
@@ -850,35 +853,6 @@ class TcbDirectiveInputsOp extends TcbOp {
850853
this.tcb.id, this.node, this.dir.name, this.dir.isComponent, missing);
851854
}
852855
}
853-
854-
private getTwoWayBindingTarget(target: ts.LeftHandSideExpression): ts.LeftHandSideExpression {
855-
// Two-way bindings to inputs allow both the input's defined type and a `WritableSignal`
856-
// of that type. For example `[(value)]="val"` where `@Input() value: number | string`
857-
// allows `val` to be `number | string | WritableSignal<number | string>`. We generate the
858-
// following expressions to expand the type:
859-
// ```
860-
// var captureType = dir.value;
861-
// (dir.value as typeof captureType | WritableSignal<typeof captureType>) = expression;
862-
// ```
863-
// Some notes:
864-
// - We wrap the assignment, insted of for example declaring a variable and assigning to it,
865-
// because keeping the assignment makes it easier to do lookups in the language service.
866-
// - The `captureType` variable can be inline, but for signal input expressions it can be
867-
// long so we use it make the code a bit neater. It also saves us some code that would
868-
// have to convert property/element access expressions into type query nodes.
869-
const captureType = this.tcb.allocateId();
870-
const captureTypeVar = tsCreateVariable(captureType, target);
871-
markIgnoreDiagnostics(captureTypeVar);
872-
this.scope.addStatement(captureTypeVar);
873-
874-
const typeQuery = ts.factory.createTypeQueryNode(captureType);
875-
const writableSignalRef = this.tcb.env.referenceExternalType(
876-
R3Identifiers.WritableSignal.moduleName, R3Identifiers.WritableSignal.name,
877-
[new ExpressionType(new TypeofExpr(new WrappedNodeExpr(captureType)))]);
878-
879-
return ts.factory.createParenthesizedExpression(ts.factory.createAsExpression(
880-
target, ts.factory.createUnionTypeNode([typeQuery, writableSignalRef])));
881-
}
882856
}
883857

884858
/**
@@ -2523,7 +2497,12 @@ function tcbCallTypeCtor(
25232497

25242498
if (input.type === 'binding') {
25252499
// For bound inputs, the property is assigned the binding expression.
2526-
const expr = widenBinding(input.expression, tcb);
2500+
let expr = widenBinding(input.expression, tcb);
2501+
2502+
if (input.isTwoWayBinding) {
2503+
expr = unwrapWritableSignal(expr, tcb);
2504+
}
2505+
25272506
const assignment =
25282507
ts.factory.createPropertyAssignment(propertyName, wrapForDiagnostics(expr));
25292508
addParseSpanInfo(assignment, input.sourceSpan);
@@ -2623,6 +2602,15 @@ function widenBinding(expr: ts.Expression, tcb: Context): ts.Expression {
26232602
}
26242603
}
26252604

2605+
/**
2606+
* Wraps an expression in an `unwrapSignal` call which extracts the signal's value.
2607+
*/
2608+
function unwrapWritableSignal(expression: ts.Expression, tcb: Context): ts.CallExpression {
2609+
const unwrapRef = tcb.env.referenceExternalSymbol(
2610+
R3Identifiers.unwrapWritableSignal.moduleName, R3Identifiers.unwrapWritableSignal.name);
2611+
return ts.factory.createCallExpression(unwrapRef, undefined, [expression]);
2612+
}
2613+
26262614
/**
26272615
* An input binding that corresponds with a field of a directive.
26282616
*/
@@ -2643,6 +2631,11 @@ interface TcbDirectiveBoundInput {
26432631
* The source span of the full attribute binding.
26442632
*/
26452633
sourceSpan: ParseSourceSpan;
2634+
2635+
/**
2636+
* Whether the binding is part of a two-way binding.
2637+
*/
2638+
isTwoWayBinding: boolean;
26462639
}
26472640

26482641
/**

‎packages/compiler-cli/src/ngtsc/typecheck/test/output_function_diagnostics.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ runInEachFileSystem(() => {
3737
template: `<div dir [(value)]="bla">`,
3838
component: `bla = true;`,
3939
expected: [
40-
`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string | WritableSignal<string>'.`,
40+
`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`,
4141
],
4242
},
4343
{

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -659,9 +659,23 @@ describe('type check blocks', () => {
659659
}];
660660
const block = tcb(TEMPLATE, DIRECTIVES);
661661
expect(block).toContain('var _t1 = null! as i0.TwoWay;');
662-
expect(block).toContain('var _t2 = _t1.input;');
663-
expect(block).toContain(
664-
'(_t1.input as typeof _t2 | i1.WritableSignal<typeof _t2>) = (((this).value));');
662+
expect(block).toContain('_t1.input = i1.ɵunwrapWritableSignal((((this).value)));');
663+
});
664+
665+
it('should handle a two-way binding to an input/output pair of a generic directive', () => {
666+
const TEMPLATE = `<div twoWay [(input)]="value"></div>`;
667+
const DIRECTIVES: TestDeclaration[] = [{
668+
type: 'directive',
669+
name: 'TwoWay',
670+
selector: '[twoWay]',
671+
inputs: {input: 'input'},
672+
outputs: {inputChange: 'inputChange'},
673+
isGeneric: true,
674+
}];
675+
const block = tcb(TEMPLATE, DIRECTIVES);
676+
expect(block).toContain('const _ctor1: <T extends string = any>(init: Pick<i0.TwoWay<T>, "input">) => i0.TwoWay<T> = null!');
677+
expect(block).toContain('var _t1 = _ctor1({ "input": (i1.ɵunwrapWritableSignal(((this).value))) });');
678+
expect(block).toContain('_t1.input = i1.ɵunwrapWritableSignal((((this).value)));');
665679
});
666680

667681
it('should handle a two-way binding to a model()', () => {
@@ -683,9 +697,7 @@ describe('type check blocks', () => {
683697
}];
684698
const block = tcb(TEMPLATE, DIRECTIVES);
685699
expect(block).toContain('var _t1 = null! as i0.TwoWay;');
686-
expect(block).toContain('var _t2 = _t1.input[i1.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE];');
687-
expect(block).toContain(
688-
'(_t1.input[i1.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE] as typeof _t2 | i1.WritableSignal<typeof _t2>) = (((this).value));');
700+
expect(block).toContain('_t1.input[i1.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE] = i1.ɵunwrapWritableSignal((((this).value)));');
689701
});
690702

691703
it('should handle a two-way binding to an input with a transform', () => {
@@ -715,9 +727,7 @@ describe('type check blocks', () => {
715727
}];
716728
const block = tcb(TEMPLATE, DIRECTIVES);
717729
expect(block).toContain('var _t1 = null! as boolean | string;');
718-
expect(block).toContain('var _t2 = _t1;');
719-
expect(block).toContain(
720-
'(_t1 as typeof _t2 | i1.WritableSignal<typeof _t2>) = (((this).value));');
730+
expect(block).toContain('_t1 = i1.ɵunwrapWritableSignal((((this).value)));');
721731
});
722732

723733
describe('experimental DOM checking via lib.dom.d.ts', () => {

‎packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,10 @@ export function angularCoreDts(): TestFile {
177177
178178
export type Signal<T> = (() => T);
179179
180+
export function ɵunwrapWritableSignal<T>(value: T|WritableSignal<T>): T {
181+
return null!;
182+
}
183+
180184
export interface ModelOptions {
181185
alias?: string;
182186
}

‎packages/compiler-cli/test/ngtsc/authoring_spec.ts‎

Lines changed: 10 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -222,8 +222,7 @@ runInEachFileSystem(() => {
222222

223223
const diags = env.driveDiagnostics();
224224
expect(diags.length).toBe(1);
225-
expect(diags[0].messageText)
226-
.toBe(`Type 'number' is not assignable to type 'string | WritableSignal<string>'.`);
225+
expect(diags[0].messageText).toBe(`Type 'number' is not assignable to type 'string'.`);
227226
});
228227

229228
describe('type checking', () => {
@@ -552,8 +551,7 @@ runInEachFileSystem(() => {
552551

553552
const diags = env.driveDiagnostics();
554553
expect(diags.length).toBe(1);
555-
expect(diags[0].messageText)
556-
.toBe(`Type 'boolean' is not assignable to type 'number | WritableSignal<number>'.`);
554+
expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`);
557555
});
558556

559557
it('should check a signal value bound to a model input via a two-way binding', () => {
@@ -580,10 +578,7 @@ runInEachFileSystem(() => {
580578

581579
const diags = env.driveDiagnostics();
582580
expect(diags.length).toBe(1);
583-
expect(diags[0].messageText).toEqual(jasmine.objectContaining({
584-
messageText:
585-
`Type 'WritableSignal<boolean>' is not assignable to type 'number | WritableSignal<number>'.`
586-
}));
581+
expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`);
587582
});
588583

589584
it('should check two-way binding of a signal to a decorator-based input/output pair', () => {
@@ -611,10 +606,7 @@ runInEachFileSystem(() => {
611606

612607
const diags = env.driveDiagnostics();
613608
expect(diags.length).toBe(1);
614-
expect(diags[0].messageText).toEqual(jasmine.objectContaining({
615-
messageText:
616-
`Type 'WritableSignal<boolean>' is not assignable to type 'number | WritableSignal<number>'.`
617-
}));
609+
expect(diags[0].messageText).toBe(`Type 'boolean' is not assignable to type 'number'.`);
618610
});
619611

620612
it('should not allow a non-writable signal to be assigned to a model', () => {
@@ -641,10 +633,8 @@ runInEachFileSystem(() => {
641633

642634
const diags = env.driveDiagnostics();
643635
expect(diags.length).toBe(1);
644-
expect(diags[0].messageText).toEqual(jasmine.objectContaining({
645-
messageText:
646-
`Type 'InputSignal<number>' is not assignable to type 'number | WritableSignal<number>'.`
647-
}));
636+
expect(diags[0].messageText)
637+
.toBe(`Type 'InputSignal<number>' is not assignable to type 'number'.`);
648638
});
649639

650640
it('should allow a model signal to be bound to another model signal', () => {
@@ -752,10 +742,8 @@ runInEachFileSystem(() => {
752742

753743
const diags = env.driveDiagnostics();
754744
expect(diags.length).toBe(1);
755-
expect(diags[0].messageText).toEqual(jasmine.objectContaining({
756-
messageText:
757-
`Type '{ id: number; }' is not assignable to type '{ id: string; } | WritableSignal<{ id: string; }>'.`
758-
}));
745+
expect(diags[0].messageText)
746+
.toBe(`Type '{ id: number; }' is not assignable to type '{ id: string; }'.`);
759747
});
760748

761749
it('should check generic two-way model binding with a signal value', () => {
@@ -782,10 +770,8 @@ runInEachFileSystem(() => {
782770

783771
const diags = env.driveDiagnostics();
784772
expect(diags.length).toBe(1);
785-
expect(diags[0].messageText).toEqual(jasmine.objectContaining({
786-
messageText:
787-
`Type 'WritableSignal<{ id: number; }>' is not assignable to type '{ id: string; } | WritableSignal<{ id: string; }>'.`
788-
}));
773+
expect(diags[0].messageText)
774+
.toEqual(`Type '{ id: number; }' is not assignable to type '{ id: string; }'.`);
789775
});
790776

791777
it('should report unwrapped signals assigned to a model in a one-way binding', () => {

0 commit comments

Comments
 (0)