Skip to content

Commit b478dfb

Browse files
devversiondylhunn
devversion
authored and
dylhunn
committed
fix(compiler-cli): report errors when initializer APIs are used on private fields (#55070)
This commit ensures that the new APIs like `input`, `model`, `output`, or signal-based queries are not accidentally used on fields that have a problematic visibility/access level that won't work. For example, queries defined using a private identifier (e.g. `#bla`) will not be accessible by the Angular runtime and therefore _dont_ work. This commit ensures: - `input` is only declared via public and protected fields. - `output` is only declared via public and protected fields. - `model` is only declared via public and protected fields. - signal queries are only declared via public, protected and TS private fields (`private` works, while `#bla` does not). Fixes #54863. PR Close #55070
1 parent 75d1cae commit b478dfb

File tree

14 files changed

+420
-95
lines changed

14 files changed

+420
-95
lines changed

‎packages/compiler-cli/src/ngtsc/annotations/directive/src/input_function.ts‎

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,9 @@ import ts from 'typescript';
1010

1111
import {ImportedSymbolsTracker} from '../../../imports';
1212
import {InputMapping} from '../../../metadata';
13-
import {ClassMember, ReflectionHost} from '../../../reflection';
13+
import {ClassMember, ClassMemberAccessLevel, ReflectionHost} from '../../../reflection';
1414

15+
import {validateAccessOfInitializerApiMember} from './initializer_function_access';
1516
import {tryParseInitializerApi} from './initializer_functions';
1617
import {parseAndValidateInputAndOutputOptions} from './input_output_parse_options';
1718

@@ -20,18 +21,34 @@ import {parseAndValidateInputAndOutputOptions} from './input_output_parse_option
2021
* input mapping if possible.
2122
*/
2223
export function tryParseSignalInputMapping(
23-
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
24+
member: Pick<ClassMember, 'name'|'value'|'accessLevel'>, reflector: ReflectionHost,
2425
importTracker: ImportedSymbolsTracker): InputMapping|null {
25-
const signalInput = member.value === null ?
26-
null :
27-
tryParseInitializerApi(
28-
[{functionName: 'input', owningModule: '@angular/core'}], member.value, reflector,
29-
importTracker);
26+
if (member.value === null) {
27+
return null;
28+
}
3029

30+
const signalInput = tryParseInitializerApi(
31+
[{
32+
functionName: 'input',
33+
owningModule: '@angular/core',
34+
// Inputs are accessed from parents, via the `property` instruction.
35+
// Conceptually, the fields need to be publicly readable, but in practice,
36+
// accessing `protected` or `private` members works at runtime, so we can allow
37+
// cases where the input is intentionally not part of the public API, programmatically.
38+
// Note: `private` is omitted intentionally as this would be a conceptual confusion point.
39+
allowedAccessLevels: [
40+
ClassMemberAccessLevel.PublicWritable,
41+
ClassMemberAccessLevel.PublicReadonly,
42+
ClassMemberAccessLevel.Protected,
43+
],
44+
}],
45+
member.value, reflector, importTracker);
3146
if (signalInput === null) {
3247
return null;
3348
}
3449

50+
validateAccessOfInitializerApiMember(signalInput, member);
51+
3552
const optionsNode = (signalInput.isRequired ? signalInput.call.arguments[0] :
3653
signalInput.call.arguments[1]) as ts.Expression |
3754
undefined;

‎packages/compiler-cli/src/ngtsc/annotations/directive/src/model_function.ts‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,27 +10,43 @@ import ts from 'typescript';
1010

1111
import {ImportedSymbolsTracker} from '../../../imports';
1212
import {ModelMapping} from '../../../metadata';
13-
import {ClassMember, ReflectionHost} from '../../../reflection';
13+
import {ClassMember, ClassMemberAccessLevel, ReflectionHost} from '../../../reflection';
1414

15+
import {validateAccessOfInitializerApiMember} from './initializer_function_access';
1516
import {tryParseInitializerApi} from './initializer_functions';
1617
import {parseAndValidateInputAndOutputOptions} from './input_output_parse_options';
1718

1819
/**
1920
* Attempts to parse a model class member. Returns the parsed model mapping if possible.
2021
*/
2122
export function tryParseSignalModelMapping(
22-
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
23+
member: Pick<ClassMember, 'name'|'value'|'accessLevel'>, reflector: ReflectionHost,
2324
importTracker: ImportedSymbolsTracker): ModelMapping|null {
24-
const model = member.value === null ?
25-
null :
26-
tryParseInitializerApi(
27-
[{functionName: 'model', owningModule: '@angular/core'}], member.value, reflector,
28-
importTracker);
25+
if (member.value === null) {
26+
return null;
27+
}
2928

29+
const model = tryParseInitializerApi(
30+
[{
31+
functionName: 'model',
32+
owningModule: '@angular/core',
33+
// Inputs are accessed from parents, via the `property` instruction.
34+
// Conceptually, the fields need to be publicly readable, but in practice,
35+
// accessing `protected` or `private` members works at runtime, so we can allow
36+
// cases where the input is intentionally not part of the public API, programmatically.
37+
allowedAccessLevels: [
38+
ClassMemberAccessLevel.PublicWritable,
39+
ClassMemberAccessLevel.PublicReadonly,
40+
ClassMemberAccessLevel.Protected,
41+
],
42+
}],
43+
member.value, reflector, importTracker);
3044
if (model === null) {
3145
return null;
3246
}
3347

48+
validateAccessOfInitializerApiMember(model, member);
49+
3450
const optionsNode =
3551
(model.isRequired ? model.call.arguments[0] : model.call.arguments[1]) as ts.Expression |
3652
undefined;

‎packages/compiler-cli/src/ngtsc/annotations/directive/src/output_function.ts‎

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,27 +11,50 @@ import ts from 'typescript';
1111
import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
1212
import {ImportedSymbolsTracker} from '../../../imports';
1313
import {InputOrOutput} from '../../../metadata';
14-
import {ClassMember, ReflectionHost} from '../../../reflection';
14+
import {ClassMember, ClassMemberAccessLevel, ReflectionHost} from '../../../reflection';
1515

16+
import {validateAccessOfInitializerApiMember} from './initializer_function_access';
1617
import {tryParseInitializerApi} from './initializer_functions';
1718
import {parseAndValidateInputAndOutputOptions} from './input_output_parse_options';
1819

20+
// Outputs are accessed from parents, via the `listener` instruction.
21+
// Conceptually, the fields need to be publicly readable, but in practice,
22+
// accessing `protected` or `private` members works at runtime, so we can allow
23+
// such outputs that may not want to expose the `OutputRef` as part of the
24+
// component API, programmatically.
25+
// Note: `private` is omitted intentionally as this would be a conceptual confusion point.
26+
const allowedAccessLevels = [
27+
ClassMemberAccessLevel.PublicWritable,
28+
ClassMemberAccessLevel.PublicReadonly,
29+
ClassMemberAccessLevel.Protected,
30+
];
31+
1932
/**
2033
* Attempts to parse a signal output class member. Returns the parsed
2134
* input mapping if possible.
2235
*/
2336
export function tryParseInitializerBasedOutput(
24-
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
37+
member: Pick<ClassMember, 'name'|'value'|'accessLevel'>, reflector: ReflectionHost,
2538
importTracker: ImportedSymbolsTracker): {call: ts.CallExpression, metadata: InputOrOutput}|
2639
null {
27-
const output = member.value === null ?
28-
null :
29-
tryParseInitializerApi(
30-
[
31-
{functionName: 'output', owningModule: '@angular/core'},
32-
{functionName: 'outputFromObservable', owningModule: '@angular/core/rxjs-interop'},
33-
],
34-
member.value, reflector, importTracker);
40+
if (member.value === null) {
41+
return null;
42+
}
43+
44+
const output = tryParseInitializerApi(
45+
[
46+
{
47+
functionName: 'output',
48+
owningModule: '@angular/core',
49+
allowedAccessLevels,
50+
},
51+
{
52+
functionName: 'outputFromObservable',
53+
owningModule: '@angular/core/rxjs-interop',
54+
allowedAccessLevels
55+
},
56+
],
57+
member.value, reflector, importTracker);
3558
if (output === null) {
3659
return null;
3760
}
@@ -41,6 +64,8 @@ export function tryParseInitializerBasedOutput(
4164
`Output does not support ".required()".`);
4265
}
4366

67+
validateAccessOfInitializerApiMember(output, member);
68+
4469
// Options are the first parameter for `output()`, while for
4570
// the interop `outputFromObservable()` they are the second argument.
4671
const optionsNode = (output.api.functionName === 'output' ?

‎packages/compiler-cli/src/ngtsc/annotations/directive/src/query_functions.ts‎

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,10 @@ import ts from 'typescript';
1212

1313
import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
1414
import {ImportedSymbolsTracker} from '../../../imports';
15-
import {ClassMember, ReflectionHost, reflectObjectLiteral} from '../../../reflection';
15+
import {ClassMember, ClassMemberAccessLevel, ReflectionHost, reflectObjectLiteral} from '../../../reflection';
1616
import {tryUnwrapForwardRef} from '../../common';
1717

18+
import {validateAccessOfInitializerApiMember} from './initializer_function_access';
1819
import {tryParseInitializerApi} from './initializer_functions';
1920

2021
/** Possible query initializer API functions. */
@@ -24,6 +25,23 @@ export type QueryFunctionName = 'viewChild'|'contentChild'|'viewChildren'|'conte
2425
const queryFunctionNames: QueryFunctionName[] =
2526
['viewChild', 'viewChildren', 'contentChild', 'contentChildren'];
2627

28+
/** Possible query initializer API functions. */
29+
const initializerFns = queryFunctionNames.map(
30+
fnName => ({
31+
functionName: fnName,
32+
owningModule: '@angular/core' as const,
33+
// Queries are accessed from within static blocks, via the query definition functions.
34+
// Conceptually, the fields could access private members— even ES private fields.
35+
// Support for ES private fields requires special caution and complexity when partial
36+
// output is linked— hence not supported. TS private members are allowed in static blocks.
37+
allowedAccessLevels: [
38+
ClassMemberAccessLevel.PublicWritable,
39+
ClassMemberAccessLevel.PublicReadonly,
40+
ClassMemberAccessLevel.Protected,
41+
ClassMemberAccessLevel.Private,
42+
],
43+
}));
44+
2745
// The `descendants` option is enabled by default, except for content children.
2846
const defaultDescendantsValue = (type: QueryFunctionName) => type !== 'contentChildren';
2947

@@ -36,18 +54,20 @@ const defaultDescendantsValue = (type: QueryFunctionName) => type !== 'contentCh
3654
* @returns Resolved query metadata, or null if no query is declared.
3755
*/
3856
export function tryParseSignalQueryFromInitializer(
39-
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
57+
member: Pick<ClassMember, 'name'|'value'|'accessLevel'>, reflector: ReflectionHost,
4058
importTracker: ImportedSymbolsTracker):
4159
{name: QueryFunctionName, metadata: R3QueryMetadata, call: ts.CallExpression}|null {
42-
const initializerFns = queryFunctionNames.map(
43-
fnName => ({functionName: fnName, owningModule: '@angular/core' as const}));
44-
const query = member.value === null ?
45-
null :
46-
tryParseInitializerApi(initializerFns, member.value, reflector, importTracker);
60+
if (member.value === null) {
61+
return null;
62+
}
63+
64+
const query = tryParseInitializerApi(initializerFns, member.value, reflector, importTracker);
4765
if (query === null) {
4866
return null;
4967
}
5068

69+
validateAccessOfInitializerApiMember(query, member);
70+
5171
const {functionName} = query.api;
5272
const isSingleQuery = functionName === 'viewChild' || functionName === 'contentChild';
5373
const predicateNode = query.call.arguments[0] as ts.Expression | undefined;

‎packages/compiler-cli/src/transformers/jit_transforms/initializer_api_transforms/input_function.ts‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,14 @@ export const signalInputsTransform: PropertyTransform = (
3434
isCore,
3535
) => {
3636
// If the field already is decorated, we handle this gracefully and skip it.
37-
if (host.getDecoratorsOfDeclaration(member)?.some(d => isAngularDecorator(d, 'Input', isCore))) {
38-
return member;
37+
if (host.getDecoratorsOfDeclaration(member.node)
38+
?.some(d => isAngularDecorator(d, 'Input', isCore))) {
39+
return member.node;
3940
}
4041

41-
const inputMapping = tryParseSignalInputMapping(
42-
{name: member.name.text, value: member.initializer ?? null}, host, importTracker);
42+
const inputMapping = tryParseSignalInputMapping(member, host, importTracker);
4343
if (inputMapping === null) {
44-
return member;
44+
return member.node;
4545
}
4646

4747
const fields: Record<keyof Required<core.Input>, ts.Expression> = {
@@ -54,7 +54,7 @@ export const signalInputsTransform: PropertyTransform = (
5454
'transform': factory.createIdentifier('undefined'),
5555
};
5656

57-
const sourceFile = member.getSourceFile();
57+
const sourceFile = member.node.getSourceFile();
5858
const newDecorator = factory.createDecorator(
5959
factory.createCallExpression(
6060
createSyntheticAngularCoreDecoratorAccess(
@@ -72,11 +72,11 @@ export const signalInputsTransform: PropertyTransform = (
7272
);
7373

7474
return factory.updatePropertyDeclaration(
75-
member,
76-
[newDecorator, ...(member.modifiers ?? [])],
75+
member.node,
76+
[newDecorator, ...(member.node.modifiers ?? [])],
7777
member.name,
78-
member.questionToken,
79-
member.type,
80-
member.initializer,
78+
member.node.questionToken,
79+
member.node.type,
80+
member.node.initializer,
8181
);
8282
};

‎packages/compiler-cli/src/transformers/jit_transforms/initializer_api_transforms/model_function.ts‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -27,20 +27,20 @@ export const signalModelTransform: PropertyTransform = (
2727
classDecorator,
2828
isCore,
2929
) => {
30-
if (host.getDecoratorsOfDeclaration(member)?.some(d => {
30+
if (host.getDecoratorsOfDeclaration(member.node)?.some(d => {
3131
return isAngularDecorator(d, 'Input', isCore) || isAngularDecorator(d, 'Output', isCore);
3232
})) {
33-
return member;
33+
return member.node;
3434
}
3535

3636
const modelMapping = tryParseSignalModelMapping(
37-
{name: member.name.text, value: member.initializer ?? null},
37+
member,
3838
host,
3939
importTracker,
4040
);
4141

4242
if (modelMapping === null) {
43-
return member;
43+
return member.node;
4444
}
4545

4646
const inputConfig = factory.createObjectLiteralExpression([
@@ -52,7 +52,7 @@ export const signalModelTransform: PropertyTransform = (
5252
'required', modelMapping.input.required ? factory.createTrue() : factory.createFalse()),
5353
]);
5454

55-
const sourceFile = member.getSourceFile();
55+
const sourceFile = member.node.getSourceFile();
5656
const inputDecorator = createDecorator(
5757
'Input',
5858
// Config is cast to `any` because `isSignal` will be private, and in case this
@@ -67,12 +67,12 @@ export const signalModelTransform: PropertyTransform = (
6767
classDecorator, factory, sourceFile, importManager);
6868

6969
return factory.updatePropertyDeclaration(
70-
member,
71-
[inputDecorator, outputDecorator, ...(member.modifiers ?? [])],
72-
member.name,
73-
member.questionToken,
74-
member.type,
75-
member.initializer,
70+
member.node,
71+
[inputDecorator, outputDecorator, ...(member.node.modifiers ?? [])],
72+
member.node.name,
73+
member.node.questionToken,
74+
member.node.type,
75+
member.node.initializer,
7676
);
7777
};
7878

‎packages/compiler-cli/src/transformers/jit_transforms/initializer_api_transforms/output_function.ts‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,20 +30,21 @@ export const initializerApiOutputTransform: PropertyTransform = (
3030
isCore,
3131
) => {
3232
// If the field already is decorated, we handle this gracefully and skip it.
33-
if (host.getDecoratorsOfDeclaration(member)?.some(d => isAngularDecorator(d, 'Output', isCore))) {
34-
return member;
33+
if (host.getDecoratorsOfDeclaration(member.node)
34+
?.some(d => isAngularDecorator(d, 'Output', isCore))) {
35+
return member.node;
3536
}
3637

3738
const output = tryParseInitializerBasedOutput(
38-
{name: member.name.text, value: member.initializer ?? null},
39+
member,
3940
host,
4041
importTracker,
4142
);
4243
if (output === null) {
43-
return member;
44+
return member.node;
4445
}
4546

46-
const sourceFile = member.getSourceFile();
47+
const sourceFile = member.node.getSourceFile();
4748
const newDecorator = factory.createDecorator(
4849
factory.createCallExpression(
4950
createSyntheticAngularCoreDecoratorAccess(
@@ -52,11 +53,11 @@ export const initializerApiOutputTransform: PropertyTransform = (
5253
);
5354

5455
return factory.updatePropertyDeclaration(
55-
member,
56-
[newDecorator, ...(member.modifiers ?? [])],
57-
member.name,
58-
member.questionToken,
59-
member.type,
60-
member.initializer,
56+
member.node,
57+
[newDecorator, ...(member.node.modifiers ?? [])],
58+
member.node.name,
59+
member.node.questionToken,
60+
member.node.type,
61+
member.node.initializer,
6162
);
6263
};

0 commit comments

Comments
 (0)