Skip to content

Commit

Permalink
fix(compiler-cli): report errors when initializer APIs are used on pr…
Browse files Browse the repository at this point in the history
…ivate 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
  • Loading branch information
devversion authored and dylhunn committed Mar 28, 2024
1 parent 75d1cae commit b478dfb
Show file tree
Hide file tree
Showing 14 changed files with 420 additions and 95 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ import ts from 'typescript';

import {ImportedSymbolsTracker} from '../../../imports';
import {InputMapping} from '../../../metadata';
import {ClassMember, ReflectionHost} from '../../../reflection';
import {ClassMember, ClassMemberAccessLevel, ReflectionHost} from '../../../reflection';

import {validateAccessOfInitializerApiMember} from './initializer_function_access';
import {tryParseInitializerApi} from './initializer_functions';
import {parseAndValidateInputAndOutputOptions} from './input_output_parse_options';

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

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

validateAccessOfInitializerApiMember(signalInput, member);

const optionsNode = (signalInput.isRequired ? signalInput.call.arguments[0] :
signalInput.call.arguments[1]) as ts.Expression |
undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,43 @@ import ts from 'typescript';

import {ImportedSymbolsTracker} from '../../../imports';
import {ModelMapping} from '../../../metadata';
import {ClassMember, ReflectionHost} from '../../../reflection';
import {ClassMember, ClassMemberAccessLevel, ReflectionHost} from '../../../reflection';

import {validateAccessOfInitializerApiMember} from './initializer_function_access';
import {tryParseInitializerApi} from './initializer_functions';
import {parseAndValidateInputAndOutputOptions} from './input_output_parse_options';

/**
* Attempts to parse a model class member. Returns the parsed model mapping if possible.
*/
export function tryParseSignalModelMapping(
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
member: Pick<ClassMember, 'name'|'value'|'accessLevel'>, reflector: ReflectionHost,
importTracker: ImportedSymbolsTracker): ModelMapping|null {
const model = member.value === null ?
null :
tryParseInitializerApi(
[{functionName: 'model', owningModule: '@angular/core'}], member.value, reflector,
importTracker);
if (member.value === null) {
return null;
}

const model = tryParseInitializerApi(
[{
functionName: 'model',
owningModule: '@angular/core',
// Inputs are accessed from parents, via the `property` instruction.
// Conceptually, the fields need to be publicly readable, but in practice,
// accessing `protected` or `private` members works at runtime, so we can allow
// cases where the input is intentionally not part of the public API, programmatically.
allowedAccessLevels: [
ClassMemberAccessLevel.PublicWritable,
ClassMemberAccessLevel.PublicReadonly,
ClassMemberAccessLevel.Protected,
],
}],
member.value, reflector, importTracker);
if (model === null) {
return null;
}

validateAccessOfInitializerApiMember(model, member);

const optionsNode =
(model.isRequired ? model.call.arguments[0] : model.call.arguments[1]) as ts.Expression |
undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,27 +11,50 @@ import ts from 'typescript';
import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
import {ImportedSymbolsTracker} from '../../../imports';
import {InputOrOutput} from '../../../metadata';
import {ClassMember, ReflectionHost} from '../../../reflection';
import {ClassMember, ClassMemberAccessLevel, ReflectionHost} from '../../../reflection';

import {validateAccessOfInitializerApiMember} from './initializer_function_access';
import {tryParseInitializerApi} from './initializer_functions';
import {parseAndValidateInputAndOutputOptions} from './input_output_parse_options';

// Outputs are accessed from parents, via the `listener` instruction.
// Conceptually, the fields need to be publicly readable, but in practice,
// accessing `protected` or `private` members works at runtime, so we can allow
// such outputs that may not want to expose the `OutputRef` as part of the
// component API, programmatically.
// Note: `private` is omitted intentionally as this would be a conceptual confusion point.
const allowedAccessLevels = [
ClassMemberAccessLevel.PublicWritable,
ClassMemberAccessLevel.PublicReadonly,
ClassMemberAccessLevel.Protected,
];

/**
* Attempts to parse a signal output class member. Returns the parsed
* input mapping if possible.
*/
export function tryParseInitializerBasedOutput(
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
member: Pick<ClassMember, 'name'|'value'|'accessLevel'>, reflector: ReflectionHost,
importTracker: ImportedSymbolsTracker): {call: ts.CallExpression, metadata: InputOrOutput}|
null {
const output = member.value === null ?
null :
tryParseInitializerApi(
[
{functionName: 'output', owningModule: '@angular/core'},
{functionName: 'outputFromObservable', owningModule: '@angular/core/rxjs-interop'},
],
member.value, reflector, importTracker);
if (member.value === null) {
return null;
}

const output = tryParseInitializerApi(
[
{
functionName: 'output',
owningModule: '@angular/core',
allowedAccessLevels,
},
{
functionName: 'outputFromObservable',
owningModule: '@angular/core/rxjs-interop',
allowedAccessLevels
},
],
member.value, reflector, importTracker);
if (output === null) {
return null;
}
Expand All @@ -41,6 +64,8 @@ export function tryParseInitializerBasedOutput(
`Output does not support ".required()".`);
}

validateAccessOfInitializerApiMember(output, member);

// Options are the first parameter for `output()`, while for
// the interop `outputFromObservable()` they are the second argument.
const optionsNode = (output.api.functionName === 'output' ?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ import ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../../diagnostics';
import {ImportedSymbolsTracker} from '../../../imports';
import {ClassMember, ReflectionHost, reflectObjectLiteral} from '../../../reflection';
import {ClassMember, ClassMemberAccessLevel, ReflectionHost, reflectObjectLiteral} from '../../../reflection';
import {tryUnwrapForwardRef} from '../../common';

import {validateAccessOfInitializerApiMember} from './initializer_function_access';
import {tryParseInitializerApi} from './initializer_functions';

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

/** Possible query initializer API functions. */
const initializerFns = queryFunctionNames.map(
fnName => ({
functionName: fnName,
owningModule: '@angular/core' as const,
// Queries are accessed from within static blocks, via the query definition functions.
// Conceptually, the fields could access private members— even ES private fields.
// Support for ES private fields requires special caution and complexity when partial
// output is linked— hence not supported. TS private members are allowed in static blocks.
allowedAccessLevels: [
ClassMemberAccessLevel.PublicWritable,
ClassMemberAccessLevel.PublicReadonly,
ClassMemberAccessLevel.Protected,
ClassMemberAccessLevel.Private,
],
}));

// The `descendants` option is enabled by default, except for content children.
const defaultDescendantsValue = (type: QueryFunctionName) => type !== 'contentChildren';

Expand All @@ -36,18 +54,20 @@ const defaultDescendantsValue = (type: QueryFunctionName) => type !== 'contentCh
* @returns Resolved query metadata, or null if no query is declared.
*/
export function tryParseSignalQueryFromInitializer(
member: Pick<ClassMember, 'name'|'value'>, reflector: ReflectionHost,
member: Pick<ClassMember, 'name'|'value'|'accessLevel'>, reflector: ReflectionHost,
importTracker: ImportedSymbolsTracker):
{name: QueryFunctionName, metadata: R3QueryMetadata, call: ts.CallExpression}|null {
const initializerFns = queryFunctionNames.map(
fnName => ({functionName: fnName, owningModule: '@angular/core' as const}));
const query = member.value === null ?
null :
tryParseInitializerApi(initializerFns, member.value, reflector, importTracker);
if (member.value === null) {
return null;
}

const query = tryParseInitializerApi(initializerFns, member.value, reflector, importTracker);
if (query === null) {
return null;
}

validateAccessOfInitializerApiMember(query, member);

const {functionName} = query.api;
const isSingleQuery = functionName === 'viewChild' || functionName === 'contentChild';
const predicateNode = query.call.arguments[0] as ts.Expression | undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ export const signalInputsTransform: PropertyTransform = (
isCore,
) => {
// If the field already is decorated, we handle this gracefully and skip it.
if (host.getDecoratorsOfDeclaration(member)?.some(d => isAngularDecorator(d, 'Input', isCore))) {
return member;
if (host.getDecoratorsOfDeclaration(member.node)
?.some(d => isAngularDecorator(d, 'Input', isCore))) {
return member.node;
}

const inputMapping = tryParseSignalInputMapping(
{name: member.name.text, value: member.initializer ?? null}, host, importTracker);
const inputMapping = tryParseSignalInputMapping(member, host, importTracker);
if (inputMapping === null) {
return member;
return member.node;
}

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

const sourceFile = member.getSourceFile();
const sourceFile = member.node.getSourceFile();
const newDecorator = factory.createDecorator(
factory.createCallExpression(
createSyntheticAngularCoreDecoratorAccess(
Expand All @@ -72,11 +72,11 @@ export const signalInputsTransform: PropertyTransform = (
);

return factory.updatePropertyDeclaration(
member,
[newDecorator, ...(member.modifiers ?? [])],
member.node,
[newDecorator, ...(member.node.modifiers ?? [])],
member.name,
member.questionToken,
member.type,
member.initializer,
member.node.questionToken,
member.node.type,
member.node.initializer,
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,20 @@ export const signalModelTransform: PropertyTransform = (
classDecorator,
isCore,
) => {
if (host.getDecoratorsOfDeclaration(member)?.some(d => {
if (host.getDecoratorsOfDeclaration(member.node)?.some(d => {
return isAngularDecorator(d, 'Input', isCore) || isAngularDecorator(d, 'Output', isCore);
})) {
return member;
return member.node;
}

const modelMapping = tryParseSignalModelMapping(
{name: member.name.text, value: member.initializer ?? null},
member,
host,
importTracker,
);

if (modelMapping === null) {
return member;
return member.node;
}

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

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

return factory.updatePropertyDeclaration(
member,
[inputDecorator, outputDecorator, ...(member.modifiers ?? [])],
member.name,
member.questionToken,
member.type,
member.initializer,
member.node,
[inputDecorator, outputDecorator, ...(member.node.modifiers ?? [])],
member.node.name,
member.node.questionToken,
member.node.type,
member.node.initializer,
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,21 @@ export const initializerApiOutputTransform: PropertyTransform = (
isCore,
) => {
// If the field already is decorated, we handle this gracefully and skip it.
if (host.getDecoratorsOfDeclaration(member)?.some(d => isAngularDecorator(d, 'Output', isCore))) {
return member;
if (host.getDecoratorsOfDeclaration(member.node)
?.some(d => isAngularDecorator(d, 'Output', isCore))) {
return member.node;
}

const output = tryParseInitializerBasedOutput(
{name: member.name.text, value: member.initializer ?? null},
member,
host,
importTracker,
);
if (output === null) {
return member;
return member.node;
}

const sourceFile = member.getSourceFile();
const sourceFile = member.node.getSourceFile();
const newDecorator = factory.createDecorator(
factory.createCallExpression(
createSyntheticAngularCoreDecoratorAccess(
Expand All @@ -52,11 +53,11 @@ export const initializerApiOutputTransform: PropertyTransform = (
);

return factory.updatePropertyDeclaration(
member,
[newDecorator, ...(member.modifiers ?? [])],
member.name,
member.questionToken,
member.type,
member.initializer,
member.node,
[newDecorator, ...(member.node.modifiers ?? [])],
member.node.name,
member.node.questionToken,
member.node.type,
member.node.initializer,
);
};

0 comments on commit b478dfb

Please sign in to comment.