Skip to content
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

Add new special assignment kinds for recognizing Object.defineProperty calls #27208

Merged
merged 7 commits into from
Oct 19, 2018
Merged

Add new special assignment kinds for recognizing Object.defineProperty calls #27208

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
66a5531
Add new special assignment kinds for recognizing Object.definePropert…
weswigham Sep 19, 2018
f91debf
Merge branch 'master' into special-object-assignment
weswigham Sep 24, 2018
a1780fe
Merge branch 'master' into special-object-assignment
weswigham Sep 24, 2018
66d2bb7
Add support for prototype assignments, fix nits
weswigham Sep 24, 2018
01589fb
Fix code review comments
weswigham Oct 10, 2018
71e8d8c
Merge branch 'master' into special-object-assignment
weswigham Oct 19, 2018
8da77c6
Add test documenting behavior in a few more odd scenarios
weswigham Oct 19, 2018
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
60 changes: 51 additions & 9 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2112,7 +2112,7 @@ namespace ts {
// Nothing to do
break;
default:
Debug.fail("Unknown special property assignment kind");
Debug.fail("Unknown binary expression special property assignment kind");
}
return checkStrictModeBinaryExpression(<BinaryExpression>node);
case SyntaxKind.CatchClause:
Expand Down Expand Up @@ -2188,6 +2188,17 @@ namespace ts {
return bindFunctionExpression(<FunctionExpression>node);

case SyntaxKind.CallExpression:
const assignmentKind = getAssignmentDeclarationKind(node as CallExpression);
switch (assignmentKind) {
case AssignmentDeclarationKind.ObjectDefinePropertyValue:
return bindObjectDefinePropertyAssignment(node as BindableObjectDefinePropertyCall);
case AssignmentDeclarationKind.ObjectDefinePropertyExports:
return bindObjectDefinePropertyExport(node as BindableObjectDefinePropertyCall);
case AssignmentDeclarationKind.None:
break; // Nothing to do
default:
return Debug.fail("Unknown call expression assignment declaration kind");
Copy link
Member

Choose a reason for hiding this comment

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

Debug.assertNever is better here I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

assignmentKind has all the non-object-define values still "possible" (they're not, because it's a call expression and not a binary expression) and so isn't never; so not quite?

}
if (isInJSFile(node)) {
bindCallExpression(<CallExpression>node);
}
Expand Down Expand Up @@ -2361,6 +2372,22 @@ namespace ts {
return true;
}

function bindObjectDefinePropertyExport(node: BindableObjectDefinePropertyCall) {
if (!setCommonJsModuleIndicator(node)) {
return;
}
const symbol = forEachIdentifierInEntityName(node.arguments[0], /*parent*/ undefined, (id, symbol) => {
if (symbol) {
addDeclarationToSymbol(symbol, id, SymbolFlags.Module | SymbolFlags.Assignment);
}
return symbol;
});
if (symbol) {
const flags = SymbolFlags.Property | SymbolFlags.ExportValue;
declareSymbol(symbol.exports!, symbol, node, flags, SymbolFlags.None);
}
}

function bindExportsPropertyAssignment(node: BinaryExpression) {
// When we create a property via 'exports.foo = bar', the 'exports.foo' property access
// expression is the declaration
Expand Down Expand Up @@ -2486,6 +2513,12 @@ namespace ts {
bindPropertyAssignment(constructorFunction, lhs, /*isPrototypeProperty*/ true);
}

function bindObjectDefinePropertyAssignment(node: BindableObjectDefinePropertyCall) {
let namespaceSymbol = lookupSymbolForPropertyAccess(node.arguments[0]);
const isToplevel = node.parent.parent.kind === SyntaxKind.SourceFile;
namespaceSymbol = bindPotentiallyMissingNamespaces(namespaceSymbol, node.arguments[0], isToplevel, /*isPrototypeProperty*/ false);
bindPotentiallyNewExpandoMemberToNamespace(node, namespaceSymbol, /*isPrototypeProperty*/ false);
}

function bindSpecialPropertyAssignment(node: BinaryExpression) {
const lhs = node.left as PropertyAccessEntityNameExpression;
Expand Down Expand Up @@ -2517,16 +2550,12 @@ namespace ts {
bindPropertyAssignment(node.expression, node, /*isPrototypeProperty*/ false);
}

function bindPropertyAssignment(name: EntityNameExpression, propertyAccess: PropertyAccessEntityNameExpression, isPrototypeProperty: boolean) {
let namespaceSymbol = lookupSymbolForPropertyAccess(name);
const isToplevel = isBinaryExpression(propertyAccess.parent)
? getParentOfBinaryExpression(propertyAccess.parent).parent.kind === SyntaxKind.SourceFile
: propertyAccess.parent.parent.kind === SyntaxKind.SourceFile;
function bindPotentiallyMissingNamespaces(namespaceSymbol: Symbol | undefined, entityName: EntityNameExpression, isToplevel: boolean, isPrototypeProperty: boolean) {
if (isToplevel && !isPrototypeProperty && (!namespaceSymbol || !(namespaceSymbol.flags & SymbolFlags.Namespace))) {
// make symbols or add declarations for intermediate containers
const flags = SymbolFlags.Module | SymbolFlags.Assignment;
const excludeFlags = SymbolFlags.ValueModuleExcludes & ~SymbolFlags.Assignment;
namespaceSymbol = forEachIdentifierInEntityName(propertyAccess.expression, namespaceSymbol, (id, symbol, parent) => {
namespaceSymbol = forEachIdentifierInEntityName(entityName, namespaceSymbol, (id, symbol, parent) => {
if (symbol) {
addDeclarationToSymbol(symbol, id, flags);
return symbol;
Expand All @@ -2538,6 +2567,10 @@ namespace ts {
}
});
}
return namespaceSymbol;
}

function bindPotentiallyNewExpandoMemberToNamespace(declaration: PropertyAccessEntityNameExpression | CallExpression, namespaceSymbol: Symbol | undefined, isPrototypeProperty: boolean) {
if (!namespaceSymbol || !isExpandoSymbol(namespaceSymbol)) {
return;
}
Expand All @@ -2547,10 +2580,19 @@ namespace ts {
(namespaceSymbol.members || (namespaceSymbol.members = createSymbolTable())) :
(namespaceSymbol.exports || (namespaceSymbol.exports = createSymbolTable()));

const isMethod = isFunctionLikeDeclaration(getAssignedExpandoInitializer(propertyAccess)!);
const isMethod = isFunctionLikeDeclaration(getAssignedExpandoInitializer(declaration)!);
const includes = isMethod ? SymbolFlags.Method : SymbolFlags.Property;
const excludes = isMethod ? SymbolFlags.MethodExcludes : SymbolFlags.PropertyExcludes;
declareSymbol(symbolTable, namespaceSymbol, propertyAccess, includes | SymbolFlags.Assignment, excludes & ~SymbolFlags.Assignment);
declareSymbol(symbolTable, namespaceSymbol, declaration, includes | SymbolFlags.Assignment, excludes & ~SymbolFlags.Assignment);
}

function bindPropertyAssignment(name: EntityNameExpression, propertyAccess: PropertyAccessEntityNameExpression, isPrototypeProperty: boolean) {
let namespaceSymbol = lookupSymbolForPropertyAccess(name);
const isToplevel = isBinaryExpression(propertyAccess.parent)
? getParentOfBinaryExpression(propertyAccess.parent).parent.kind === SyntaxKind.SourceFile
: propertyAccess.parent.parent.kind === SyntaxKind.SourceFile;
namespaceSymbol = bindPotentiallyMissingNamespaces(namespaceSymbol, propertyAccess.expression, isToplevel, isPrototypeProperty);
bindPotentiallyNewExpandoMemberToNamespace(propertyAccess, namespaceSymbol, isPrototypeProperty);
}

/**
Expand Down
76 changes: 70 additions & 6 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4841,7 +4841,7 @@ namespace ts {
let jsdocType: Type | undefined;
let types: Type[] | undefined;
for (const declaration of symbol.declarations) {
const expression = isBinaryExpression(declaration) ? declaration :
const expression = (isBinaryExpression(declaration) || isCallExpression(declaration)) ? declaration :
isPropertyAccessExpression(declaration) ? isBinaryExpression(declaration.parent) ? declaration.parent : declaration :
undefined;
if (!expression) {
Expand All @@ -4857,9 +4857,11 @@ namespace ts {
definedInMethod = true;
}
}
jsdocType = getJSDocTypeFromAssignmentDeclaration(jsdocType, expression, symbol, declaration);
if (!isCallExpression(expression)) {
jsdocType = getJSDocTypeFromAssignmentDeclaration(jsdocType, expression, symbol, declaration);
}
if (!jsdocType) {
(types || (types = [])).push(isBinaryExpression(expression) ? getInitializerTypeFromAssignmentDeclaration(symbol, resolvedSymbol, expression, kind) : neverType);
(types || (types = [])).push((isBinaryExpression(expression) || isCallExpression(expression)) ? getInitializerTypeFromAssignmentDeclaration(symbol, resolvedSymbol, expression, kind) : neverType);
}
}
let type = jsdocType;
Expand Down Expand Up @@ -4922,7 +4924,32 @@ namespace ts {
}

/** If we don't have an explicit JSDoc type, get the type from the initializer. */
function getInitializerTypeFromAssignmentDeclaration(symbol: Symbol, resolvedSymbol: Symbol | undefined, expression: BinaryExpression, kind: AssignmentDeclarationKind) {
function getInitializerTypeFromAssignmentDeclaration(symbol: Symbol, resolvedSymbol: Symbol | undefined, expression: BinaryExpression | CallExpression, kind: AssignmentDeclarationKind) {
if (isCallExpression(expression)) {
if (resolvedSymbol) {
return getTypeOfSymbol(resolvedSymbol); // This shouldn't happen except under some hopefully forbidden merges of export assignments and object define assignments
}
Copy link
Member

Choose a reason for hiding this comment

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

is there a test for this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK I can't construct something that can actually hit this codepath currently (since something like Object.defineProperty(module, "exports", { value: "no" }) won't be bound to the module in the first place); but I wanna avoid crashing and burning if we change something, and this seems reasonable (ignoring the reassignment). I'll add a test with similar content that could trigger it if we ever do start binding it a bit differently, though.

const objectLitType = checkExpressionCached((expression as BindableObjectDefinePropertyCall).arguments[2]);
const valueType = getTypeOfPropertyOfType(objectLitType, "value" as __String);
if (valueType) {
return valueType;
}
const getFunc = getTypeOfPropertyOfType(objectLitType, "get" as __String);
if (getFunc) {
const getSig = getSingleCallSignature(getFunc);
if (getSig) {
return getReturnTypeOfSignature(getSig);
}
}
const setFunc = getTypeOfPropertyOfType(objectLitType, "set" as __String);
if (setFunc) {
const setSig = getSingleCallSignature(setFunc);
if (setSig) {
return getTypeOfFirstParameterOfSignature(setSig);
}
}
return anyType;
}
const type = resolvedSymbol ? getTypeOfSymbol(resolvedSymbol) : getWidenedLiteralType(checkExpressionCached(expression.right));
if (type.flags & TypeFlags.Object &&
kind === AssignmentDeclarationKind.ModuleExports &&
Expand Down Expand Up @@ -5176,7 +5203,7 @@ namespace ts {
}
let type: Type | undefined;
if (isInJSFile(declaration) &&
(isBinaryExpression(declaration) || isPropertyAccessExpression(declaration) && isBinaryExpression(declaration.parent))) {
(isCallExpression(declaration) || isBinaryExpression(declaration) || isPropertyAccessExpression(declaration) && isBinaryExpression(declaration.parent))) {
type = getWidenedTypeFromAssignmentDeclaration(symbol);
}
else if (isJSDocPropertyLikeTag(declaration)
Expand Down Expand Up @@ -16535,6 +16562,9 @@ namespace ts {
}
const thisType = checkThisExpression(thisAccess.expression);
return thisType && getTypeOfPropertyOfContextualType(thisType, thisAccess.name.escapedText) || false;
case AssignmentDeclarationKind.ObjectDefinePropertyValue:
case AssignmentDeclarationKind.ObjectDefinePropertyExports:
return Debug.fail("Unimplemented");
default:
return Debug.assertNever(kind);
}
Expand Down Expand Up @@ -21206,18 +21236,52 @@ namespace ts {
return true;
}

function isReadonlyAssignmentDeclaration(d: Declaration) {
if (!isCallExpression(d)) {
return false;
}
if (!isBindableObjectDefinePropertyCall(d)) {
return false;
}
const objectLitType = checkExpressionCached(d.arguments[2]);
const valueType = getTypeOfPropertyOfType(objectLitType, "value" as __String);
if (valueType) {
const writableType = getTypeOfPropertyOfType(objectLitType, "writable" as __String);
if (!writableType || writableType === falseType || writableType === regularFalseType) {
return true;
}
// We include this definition whereupon we walk back and check the type at the declaration because
// The usual definition of `Object.defineProperty` will _not_ cause literal type to be preserved in the
// argument types, should the type be contextualized by the call itself.
const writableProp = getPropertyOfType(objectLitType, "writable" as __String);
if (writableProp!.valueDeclaration && isPropertyAssignment(writableProp!.valueDeclaration)) {
const initializer = (writableProp!.valueDeclaration as PropertyAssignment).initializer;
const rawOriginalType = checkExpression(initializer);
if (rawOriginalType === falseType || rawOriginalType === regularFalseType) {
return true;
}
}
return false;
}
const setProp = getPropertyOfType(objectLitType, "set" as __String);
return !setProp;
}

function isReadonlySymbol(symbol: Symbol): boolean {
// The following symbols are considered read-only:
// Properties with a 'readonly' modifier
// Variables declared with 'const'
// Get accessors without matching set accessors
// Enum members
// JS Assignments with writable false or no setter
Copy link
Member

Choose a reason for hiding this comment

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

pedantically (since I spent a day thinking about terms), it's better to say // define-property assignments with writable false or no setter. This parallels terminology of things that are parllel, like "this-property assignments" or "prototype assignments".

// Unions and intersections of the above (unions and intersections eagerly set isReadonly on creation)
return !!(getCheckFlags(symbol) & CheckFlags.Readonly ||
symbol.flags & SymbolFlags.Property && getDeclarationModifierFlagsFromSymbol(symbol) & ModifierFlags.Readonly ||
symbol.flags & SymbolFlags.Variable && getDeclarationNodeFlagsFromSymbol(symbol) & NodeFlags.Const ||
symbol.flags & SymbolFlags.Accessor && !(symbol.flags & SymbolFlags.SetAccessor) ||
symbol.flags & SymbolFlags.EnumMember);
symbol.flags & SymbolFlags.EnumMember ||
some(symbol.declarations, isReadonlyAssignmentDeclaration)
);
}

function isReferenceToReadonlyEntity(expr: Expression, symbol: Symbol): boolean {
Expand Down
13 changes: 12 additions & 1 deletion src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ namespace ts {

export type PropertyName = Identifier | StringLiteral | NumericLiteral | ComputedPropertyName;

export type DeclarationName = Identifier | StringLiteral | NumericLiteral | ComputedPropertyName | BindingPattern;
export type DeclarationName = Identifier | StringLiteralLike | NumericLiteral | ComputedPropertyName | BindingPattern;

export interface Declaration extends Node {
_declarationBrand: any;
Expand Down Expand Up @@ -1697,6 +1697,10 @@ namespace ts {
arguments: NodeArray<Expression>;
}

/** @internal */
export type BindableObjectDefinePropertyCall = CallExpression & { arguments: { 0: EntityNameExpression, 1: StringLiteralLike | NumericLiteral, 2: ObjectLiteralExpression } };


Copy link
Member

Choose a reason for hiding this comment

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

extra newline

// see: https://tc39.github.io/ecma262/#prod-SuperCall
export interface SuperCall extends CallExpression {
expression: SuperExpression;
Expand Down Expand Up @@ -4252,6 +4256,13 @@ namespace ts {
Property,
// F.prototype = { ... }
Prototype,
// Object.defineProperty(x, 'name', { value: any, writable?: boolean (false by default) });
// Object.defineProperty(x, 'name', { get: Function, set: Function });
// Object.defineProperty(x, 'name', { get: Function });
// Object.defineProperty(x, 'name', { set: Function });
ObjectDefinePropertyValue,
// Object.defineProperty(exports || module.exports, 'name', ...);
ObjectDefinePropertyExports,
}

/** @deprecated Use FileExtensionInfo instead. */
Expand Down
Loading