Skip to content

Commit

Permalink
fix(compiler-cli): do not throw when retrieving TCB symbol for signal…
Browse files Browse the repository at this point in the history
… input with restricted access (angular#55774)

Currently when attempting to retrieve a TCB symbol for an input binding
that refers to a signal input with e.g. `protected`, while the
`honorAccessModifiersForInputBindings` flag is `false`, Angular will
throw a runtime exception because the symbol retrieval code always
expects a proper field access in the TCB.

This is not the case with `honorAccessModifiersForInputBindings =
false`, as TCB will allocate a temporary variable when ignoring the
field access. This will then trigger the runtime exception (which we
added to flag such "unexpected" cases). This commit handles it
gracefully, as it's valid TCB, but we simply cannot generate a proper
TCB symbol (yet). This is similar to `@Input` decorator inputs.

In the future we may implement logic to build up TCB symbols for
non-property access bindings, for both signal inputs or `@Input`
inputs. This commit just avoids a build exception.

Related to: angular#54324.

PR Close angular#55774
  • Loading branch information
devversion authored and atscott committed May 16, 2024
1 parent abdf453 commit 400911e
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ export class SymbolBuilder {
}

const signalInputAssignment = unwrapSignalInputWriteTAccessor(node.left);
let fieldAccessExpr: ts.PropertyAccessExpression | ts.ElementAccessExpression;
let symbolInfo: TsNodeSymbolInfo | null = null;

// Signal inputs need special treatment because they are generated with an extra keyed
Expand All @@ -424,9 +425,18 @@ export class SymbolBuilder {
// - The definition symbol of the input should be the input class member, and not the
// internal write accessor. Symbol should resolve `_t1.prop`.
if (signalInputAssignment !== null) {
// Note: If the field expression for the input binding refers to just an identifier,
// then we are handling the case of a temporary variable being used for the input field.
// This is the case with `honorAccessModifiersForInputBindings = false` and in those cases
// we cannot resolve the owning directive, similar to how we guard above with `isAccessExpression`.
if (ts.isIdentifier(signalInputAssignment.fieldExpr)) {
continue;
}

const fieldSymbol = this.getSymbolOfTsNode(signalInputAssignment.fieldExpr);
const typeSymbol = this.getSymbolOfTsNode(signalInputAssignment.typeExpr);

fieldAccessExpr = signalInputAssignment.fieldExpr;
symbolInfo =
fieldSymbol === null || typeSymbol === null
? null
Expand All @@ -436,17 +446,15 @@ export class SymbolBuilder {
tsType: typeSymbol.tsType,
};
} else {
fieldAccessExpr = node.left;
symbolInfo = this.getSymbolOfTsNode(node.left);
}

if (symbolInfo === null || symbolInfo.tsSymbol === null) {
continue;
}

const target = this.getDirectiveSymbolForAccessExpression(
signalInputAssignment?.fieldExpr ?? node.left,
consumer,
);
const target = this.getDirectiveSymbolForAccessExpression(fieldAccessExpr, consumer);
if (target === null) {
continue;
}
Expand Down Expand Up @@ -771,7 +779,7 @@ function sourceSpanEqual(a: ParseSourceSpan, b: ParseSourceSpan) {
}

function unwrapSignalInputWriteTAccessor(expr: ts.LeftHandSideExpression): null | {
fieldExpr: ts.ElementAccessExpression | ts.PropertyAccessExpression;
fieldExpr: ts.ElementAccessExpression | ts.PropertyAccessExpression | ts.Identifier;
typeExpr: ts.ElementAccessExpression;
} {
// e.g. `_t2.inputA[i2.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE]`
Expand All @@ -793,11 +801,15 @@ function unwrapSignalInputWriteTAccessor(expr: ts.LeftHandSideExpression): null
return null;
}

// Assert that the `_t2.inputA` is actually either a keyed element access, or
// property access expression. This is checked for type safety and to catch unexpected cases.
// Assert that the expression is either:
// - `_t2.inputA[ɵINPUT_SIGNAL_BRAND_WRITE_TYPE]` or (common case)
// - or `_t2['input-A'][ɵINPUT_SIGNAL_BRAND_WRITE_TYPE]` (non-identifier input field names)
// - or `_dirInput[ɵINPUT_SIGNAL_BRAND_WRITE_TYPE` (honorAccessModifiersForInputBindings=false)
// This is checked for type safety and to catch unexpected cases.
if (
!ts.isPropertyAccessExpression(expr.expression) &&
!ts.isElementAccessExpression(expr.expression)
!ts.isElementAccessExpression(expr.expression) &&
!ts.isIdentifier(expr.expression)
) {
throw new Error('Unexpected expression for signal input write type.');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1294,6 +1294,72 @@ runInEachFileSystem(() => {
).toEqual('inputB');
});

// Note that `honorAccessModifiersForInputBindings` is `false` even with `--strictTemplates`,
// so this captures a potential common scenario, assuming the input is restricted.
it('should not throw when retrieving a symbol for a signal-input with restricted access', () => {
const fileName = absoluteFrom('/main.ts');
const dirFile = absoluteFrom('/dir.ts');
const templateString = `
@if (true) {
<div dir [inputA]="'ok'"></div>
}
`;
const {program, templateTypeChecker} = setup(
[
{
fileName,
templates: {'Cmp': templateString},
declarations: [
{
name: 'TestDir',
selector: '[dir]',
file: dirFile,
type: 'directive',
restrictedInputFields: ['inputA'],
inputs: {
inputA: {
bindingPropertyName: 'inputA',
isSignal: true,
classPropertyName: 'inputA',
required: false,
transform: null,
},
},
},
],
},
{
fileName: dirFile,
source: `
import {InputSignal} from '@angular/core';
export class TestDir {
protected inputA: InputSignal<string> = null!;
}
`,
templates: {},
},
],
{honorAccessModifiersForInputBindings: false},
);
const sf = getSourceFileOrError(program, fileName);
const cmp = getClass(sf, 'Cmp');

const nodes = templateTypeChecker.getTemplate(cmp)!;
const ifNode = nodes[0] as TmplAstIfBlock;
const ifBranchNode = ifNode.branches[0];
const testElement = ifBranchNode.children[0] as TmplAstElement;

const inputAbinding = testElement.inputs[0];
const aSymbol = templateTypeChecker.getSymbolOfNode(inputAbinding, cmp);
expect(aSymbol)
.withContext(
'Symbol builder does not return symbols for restricted inputs with ' +
'`honorAccessModifiersForInputBindings = false` (same for decorator inputs)',
)
.toBe(null);
});

it('does not retrieve a symbol for an input when undeclared', () => {
const fileName = absoluteFrom('/main.ts');
const dirFile = absoluteFrom('/dir.ts');
Expand Down

0 comments on commit 400911e

Please sign in to comment.