Skip to content

Allow resolving of ES5 default values in functions #25048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alxhub wants to merge 1 commit into from
Closed

Allow resolving of ES5 default values in functions #25048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 72 additions & 3 deletions packages/compiler-cli/src/ngtsc/host/src/reflection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,9 @@ export interface ClassMember {
}

/**
* A parameter to a function or constructor.
* A parameter to a constructor.
*/
export interface Parameter {
export interface CtorParameter {
/**
* Name of the parameter, if available.
*
Expand Down Expand Up @@ -180,6 +180,53 @@ export interface Parameter {
decorators: Decorator[]|null;
}

/**
* Definition of a function or method, including its body if present and any parameters.
*
* In TypeScript code this metadata will be a simple reflection of the declarations in the node
* itself. In ES5 code this can be more complicated, as the default values for parameters may
* be extracted from certain body statements.
*/
export interface FunctionDefinition<T extends ts.MethodDeclaration|ts.FunctionDeclaration|ts.FunctionExpression> {
/**
* A reference to the node which declares the function.
*/
node: T;

/**
* Statements of the function body, if a body is present, or null if no body is present.
*
* This list may be filtered to exclude statements which perform parameter default value
Copy link
Copy Markdown
Member

@JoostK JoostK Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be reworded to This list may have been filtered (...) to indicate the ReflectionHost is expected to do that. Currently it reads to me as if consumers of ReflectionHost are responsible for doing so, in which it doesn't make much sense to tell here wat consumers can do with an array :)

Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this in my PR

* initialization.
*/
body: ts.Statement[] | null;

/**
* Metadata regarding the function's parameters, including possible default value expressions.
*/
parameters: Parameter[];
}

/**
* A parameter to a function or method.
*/
export interface Parameter {
/**
* Name of the parameter, if available.
*/
name: string|null;

/**
* Declaration which created this parameter.
*/
node: ts.ParameterDeclaration;

/**
* Expression which represents the default value of the parameter, if any.
*/
initializer: ts.Expression|null;
}

/**
* The source of an imported symbol, including the original symbol name and the module from which it
* was imported.
Expand Down Expand Up @@ -273,7 +320,29 @@ export interface ReflectionHost {
* a constructor exists. If the constructor exists and has 0 parameters, this array will be empty.
* If the class has no constructor, this method returns `null`.
*/
getConstructorParameters(declaration: ts.Declaration): Parameter[]|null;
getConstructorParameters(declaration: ts.Declaration): CtorParameter[]|null;

/**
* Reflect over a function and return metadata about its parameters and body.
*
* Functions in TypeScript and ES5 code have different AST representations, in particular around
* default values for parameters. A TypeScript function has its default value as the initializer
* on the parameter declaration, whereas an ES5 function has its default value set in a statement
* of the form:
*
* if (param === void 0) { param = 3; }
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps stick some triple back ticks around this.

*
* This method abstracts over these details, and interprets the function declaration and body to
* extract parameter default values and the "real" body.
*
* A current limitation is that this metadata has no representation for shorthand assignment of
* parameter objects in the function signature.
*
* @param node a TypeScript `ts.Declaration` node representing the function over which to reflect.
Copy link
Copy Markdown
Member

@gkalpak gkalpak Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The param's name is fn 😁

Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this in my PR

*
* @returns a `FunctionDefinition` giving metadata about the function definition.
*/
getDefinitionOfFunction<T extends ts.MethodDeclaration|ts.FunctionDeclaration|ts.FunctionExpression>(fn: T): FunctionDefinition<T>;

/**
* Determine if an identifier was imported from another module and return `Import` metadata
Expand Down
16 changes: 14 additions & 2 deletions packages/compiler-cli/src/ngtsc/metadata/src/reflector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import * as ts from 'typescript';

import {ClassMember, ClassMemberKind, Declaration, Decorator, Import, Parameter, ReflectionHost} from '../../host';
import {ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, Import, Parameter, ReflectionHost, FunctionDefinition} from '../../host';

/**
* reflector.ts implements static reflection of declarations using the TypeScript `ts.TypeChecker`.
Expand All @@ -31,7 +31,7 @@ export class TypeScriptReflectionHost implements ReflectionHost {
.filter((member): member is ClassMember => member !== null);
}

getConstructorParameters(declaration: ts.Declaration): Parameter[]|null {
getConstructorParameters(declaration: ts.Declaration): CtorParameter[]|null {
const clazz = castDeclarationToClassOrDie(declaration);

// First, find the constructor.
Expand Down Expand Up @@ -141,6 +141,18 @@ export class TypeScriptReflectionHost implements ReflectionHost {
return this._getDeclarationOfSymbol(symbol);
}

getDefinitionOfFunction<T extends ts.FunctionDeclaration|ts.MethodDeclaration|ts.FunctionExpression>(node: T): FunctionDefinition<T> {
return {
node,
body: node.body !== undefined ? Array.from(node.body.statements) : null,
parameters: node.parameters.map(node => {
Copy link
Copy Markdown
Member

@gkalpak gkalpak Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider renaming the second node to paramNode to avoid confusion below.

Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this in my PR

const name = parameterName(node.name);
const initializer = node.initializer || null;
return {name, node, initializer};
}),
};
}

/**
* Resolve a `ts.Symbol` to its declaration, keeping track of the `viaModule` along the way.
*
Expand Down
14 changes: 7 additions & 7 deletions packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,14 +521,14 @@ class StaticInterpreter {
`calling something that is not a function declaration? ${ts.SyntaxKind[lhs.node.kind]}`);
}

const fn = lhs.node;
const fn = this.host.getDefinitionOfFunction(lhs.node);

// If the function is foreign (declared through a d.ts file), attempt to resolve it with the
// foreignFunctionResolver, if one is specified.
if (fn.body === undefined) {
if (fn.body === null) {
let expr: ts.Expression|null = null;
if (context.foreignFunctionResolver) {
expr = context.foreignFunctionResolver(fn);
expr = context.foreignFunctionResolver(fn.node);
}
if (expr === null) {
throw new Error(`could not resolve foreign function declaration`);
Expand All @@ -545,10 +545,10 @@ class StaticInterpreter {
}

const body = fn.body;
if (body.statements.length !== 1 || !ts.isReturnStatement(body.statements[0])) {
if (body.length !== 1 || !ts.isReturnStatement(body[0])) {
throw new Error('Function body must have a single return statement only.');
}
const ret = body.statements[0] as ts.ReturnStatement;
const ret = body[0] as ts.ReturnStatement;

const newScope: Scope = new Map<ts.ParameterDeclaration, ResolvedValue>();
fn.parameters.forEach((param, index) => {
Expand All @@ -557,10 +557,10 @@ class StaticInterpreter {
const arg = node.arguments[index];
value = this.visitExpression(arg, context);
}
if (value === undefined && param.initializer !== undefined) {
if (value === undefined && param.initializer !== null) {
value = this.visitExpression(param.initializer, context);
}
newScope.set(param, value);
newScope.set(param.node, value);
});

return ret.expression !== undefined ?
Expand Down
4 changes: 2 additions & 2 deletions packages/compiler-cli/src/ngtsc/metadata/test/reflector_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import * as ts from 'typescript';

import {Parameter} from '../../host';
import {CtorParameter} from '../../host';
import {getDeclaration, makeProgram} from '../../testing/in_memory_typescript';
import {TypeScriptReflectionHost} from '../src/reflector';

Expand Down Expand Up @@ -165,7 +165,7 @@ describe('reflector', () => {
});

function expectParameter(
param: Parameter, name: string, type?: string, decorator?: string,
param: CtorParameter, name: string, type?: string, decorator?: string,
decoratorFrom?: string): void {
expect(param.name !).toEqual(name);
if (type === undefined) {
Expand Down