Skip to content

Support some late-bound special property assignments #33220

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

Merged
merged 9 commits into from
Sep 27, 2019
Merged

Support some late-bound special property assignments #33220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
37674e0
Support some late-bound special property assignments
weswigham Sep 3, 2019
a2e3797
Integrate PR feedback
weswigham Sep 23, 2019
5d4ba79
Merge branch 'master' into js-this-latebound-property-assignment
weswigham Sep 24, 2019
489a01b
PR feedback
weswigham Sep 24, 2019
108ea20
Merge branch 'master' into js-this-latebound-property-assignment
weswigham Sep 26, 2019
d4ed5e9
Enable declaration on core tests
weswigham Sep 26, 2019
5b1f766
Specialize type of binary expression used for late binding, speculati…
weswigham Sep 27, 2019
b8be6d9
Merge branch 'master' into js-this-latebound-property-assignment
weswigham Sep 27, 2019
11b3448
Add test showing current nav bar behavior (specifically the lack ther…
weswigham Sep 27, 2019
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
35 changes: 32 additions & 3 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2622,7 +2622,12 @@ namespace ts {
// Declare a 'member' if the container is an ES5 class or ES6 constructor
constructorSymbol.members = constructorSymbol.members || createSymbolTable();
// It's acceptable for multiple 'this' assignments of the same identifier to occur
declareSymbol(constructorSymbol.members, constructorSymbol, node, SymbolFlags.Property | SymbolFlags.Assignment, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property);
if (hasDynamicName(node)) {
bindDynamicallyNamedThisPropertyAssignment(node, constructorSymbol);
}
else {
declareSymbol(constructorSymbol.members, constructorSymbol, node, SymbolFlags.Property | SymbolFlags.Assignment, SymbolFlags.PropertyExcludes & ~SymbolFlags.Property);
}
addDeclarationToSymbol(constructorSymbol, constructorSymbol.valueDeclaration, SymbolFlags.Class);
}
break;
Expand All @@ -2636,7 +2641,12 @@ namespace ts {
// Bind this property to the containing class
const containingClass = thisContainer.parent;
const symbolTable = hasModifier(thisContainer, ModifierFlags.Static) ? containingClass.symbol.exports! : containingClass.symbol.members!;
declareSymbol(symbolTable, containingClass.symbol, node, SymbolFlags.Property | SymbolFlags.Assignment, SymbolFlags.None, /*isReplaceableByMethod*/ true);
if (hasDynamicName(node)) {
bindDynamicallyNamedThisPropertyAssignment(node, containingClass.symbol);
}
else {
declareSymbol(symbolTable, containingClass.symbol, node, SymbolFlags.Property | SymbolFlags.Assignment, SymbolFlags.None, /*isReplaceableByMethod*/ true);
}
break;
case SyntaxKind.SourceFile:
// this.property = assignment in a source file -- declare symbol in exports for a module, in locals for a script
Expand All @@ -2653,6 +2663,18 @@ namespace ts {
}
}

function bindDynamicallyNamedThisPropertyAssignment(node: BinaryExpression | DynamicNamedDeclaration, symbol: Symbol) {
bindAnonymousDeclaration(node, SymbolFlags.Property, InternalSymbolName.Computed);
addLateBoundAssignmentDeclarationToSymbol(node, symbol);
}

function addLateBoundAssignmentDeclarationToSymbol(node: BinaryExpression | DynamicNamedDeclaration, symbol: Symbol | undefined) {
if (symbol) {
const members = symbol.assignmentDeclarationMembers || (symbol.assignmentDeclarationMembers = createMap());
Copy link
Member

Choose a reason for hiding this comment

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

changing the shape of Symbol makes me think we should run the perf tests as a precaution.

members.set("" + getNodeId(node), node);
}
}

function bindSpecialPropertyDeclaration(node: PropertyAccessExpression) {
if (node.expression.kind === SyntaxKind.ThisKeyword) {
bindThisPropertyAssignment(node);
Expand Down Expand Up @@ -2722,7 +2744,14 @@ namespace ts {
bindExportsPropertyAssignment(node);
}
else {
bindStaticPropertyAssignment(lhs);
if (hasDynamicName(node)) {
bindAnonymousDeclaration(node, SymbolFlags.Property | SymbolFlags.Assignment, InternalSymbolName.Computed);
const sym = bindPotentiallyMissingNamespaces(parentSymbol, lhs.expression, isTopLevelNamespaceAssignment(lhs), /*isPrototype*/ false, /*containerIsClass*/ false);
addLateBoundAssignmentDeclarationToSymbol(node, sym);
}
else {
bindStaticPropertyAssignment(lhs);
}
}
}

Expand Down
60 changes: 44 additions & 16 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2860,7 +2860,7 @@ namespace ts {
}

function getExportsOfSymbol(symbol: Symbol): SymbolTable {
return symbol.flags & SymbolFlags.Class ? getResolvedMembersOrExportsOfSymbol(symbol, MembersOrExportsResolutionKind.resolvedExports) :
return symbol.flags & SymbolFlags.LateBindingContainer ? getResolvedMembersOrExportsOfSymbol(symbol, MembersOrExportsResolutionKind.resolvedExports) :
symbol.flags & SymbolFlags.Module ? getExportsOfModule(symbol) :
symbol.exports || emptySymbols;
}
Expand Down Expand Up @@ -4223,7 +4223,15 @@ namespace ts {
if (context.tracker.trackSymbol && getCheckFlags(propertySymbol) & CheckFlags.Late) {
const decl = first(propertySymbol.declarations);
if (hasLateBindableName(decl)) {
trackComputedName(decl.name, saveEnclosingDeclaration, context);
if (isBinaryExpression(decl)) {
const name = getNameOfDeclaration(decl);
if (name && isElementAccessExpression(name) && isPropertyAccessEntityNameExpression(name.argumentExpression)) {
trackComputedName(name.argumentExpression, saveEnclosingDeclaration, context);
}
}
else {
trackComputedName(decl.name.expression, saveEnclosingDeclaration, context);
}
}
}
const propertyName = symbolToName(propertySymbol, context, SymbolFlags.Value, /*expectsIdentifier*/ true);
Expand Down Expand Up @@ -4439,7 +4447,7 @@ namespace ts {
return <BindingName>elideInitializerAndSetEmitFlags(node);
function elideInitializerAndSetEmitFlags(node: Node): Node {
if (context.tracker.trackSymbol && isComputedPropertyName(node) && isLateBindableName(node)) {
trackComputedName(node, context.enclosingDeclaration, context);
trackComputedName(node.expression, context.enclosingDeclaration, context);
}
const visited = visitEachChild(node, elideInitializerAndSetEmitFlags, nullTransformationContext, /*nodesVisitor*/ undefined, elideInitializerAndSetEmitFlags)!;
const clone = nodeIsSynthesized(visited) ? visited : getSynthesizedClone(visited);
Expand All @@ -4451,10 +4459,10 @@ namespace ts {
}
}

function trackComputedName(node: LateBoundName, enclosingDeclaration: Node | undefined, context: NodeBuilderContext) {
function trackComputedName(accessExpression: EntityNameOrEntityNameExpression, enclosingDeclaration: Node | undefined, context: NodeBuilderContext) {
if (!context.tracker.trackSymbol) return;
// get symbol of the first identifier of the entityName
const firstIdentifier = getFirstIdentifier(node.expression);
const firstIdentifier = getFirstIdentifier(accessExpression);
const name = resolveName(firstIdentifier, firstIdentifier.escapedText, SymbolFlags.Value | SymbolFlags.ExportValue, /*nodeNotFoundErrorMessage*/ undefined, /*nameArg*/ undefined, /*isUse*/ true);
if (name) {
context.tracker.trackSymbol(name, enclosingDeclaration, SymbolFlags.Value);
Expand Down Expand Up @@ -7181,8 +7189,10 @@ namespace ts {
if (declaration.kind === SyntaxKind.ExportAssignment) {
type = widenTypeForVariableLikeDeclaration(checkExpressionCached((<ExportAssignment>declaration).expression), declaration);
}
else if (isInJSFile(declaration) &&
(isCallExpression(declaration) || isBinaryExpression(declaration) || isPropertyAccessExpression(declaration) && isBinaryExpression(declaration.parent))) {
else if (
isBinaryExpression(declaration) ||
(isInJSFile(declaration) &&
(isCallExpression(declaration) || isPropertyAccessExpression(declaration) && isBinaryExpression(declaration.parent)))) {
type = getWidenedTypeForAssignmentDeclaration(symbol);
}
else if (isJSDocPropertyLikeTag(declaration)
Expand Down Expand Up @@ -8200,9 +8210,12 @@ namespace ts {
* - The type of its expression is a string or numeric literal type, or is a `unique symbol` type.
*/
function isLateBindableName(node: DeclarationName): node is LateBoundName {
return isComputedPropertyName(node)
&& isEntityNameExpression(node.expression)
&& isTypeUsableAsPropertyName(checkComputedPropertyName(node));
if (!isComputedPropertyName(node) && !isElementAccessExpression(node)) {
return false;
}
const expr = isComputedPropertyName(node) ? node.expression : node.argumentExpression;
return isEntityNameExpression(expr)
&& isTypeUsableAsPropertyName(isComputedPropertyName(node) ? checkComputedPropertyName(node) : checkExpressionCached(expr));
}

function isLateBoundName(name: __String): boolean {
Expand All @@ -8214,7 +8227,7 @@ namespace ts {
/**
* Indicates whether a declaration has a late-bindable dynamic name.
*/
function hasLateBindableName(node: Declaration): node is LateBoundDeclaration {
function hasLateBindableName(node: Declaration): node is LateBoundDeclaration | LateBoundBinaryExpressionDeclaration {
const name = getNameOfDeclaration(node);
return !!name && isLateBindableName(name);
}
Expand Down Expand Up @@ -8251,7 +8264,7 @@ namespace ts {
* late-bound members that `addDeclarationToSymbol` in binder.ts performs for early-bound
* members.
*/
function addDeclarationToLateBoundSymbol(symbol: Symbol, member: LateBoundDeclaration, symbolFlags: SymbolFlags) {
function addDeclarationToLateBoundSymbol(symbol: Symbol, member: LateBoundDeclaration | BinaryExpression, symbolFlags: SymbolFlags) {
Debug.assert(!!(getCheckFlags(symbol) & CheckFlags.Late), "Expected a late-bound symbol.");
symbol.flags |= symbolFlags;
getSymbolLinks(member.symbol).lateSymbol = symbol;
Expand Down Expand Up @@ -8296,14 +8309,15 @@ namespace ts {
* @param lateSymbols The late-bound symbols of the parent.
* @param decl The member to bind.
*/
function lateBindMember(parent: Symbol, earlySymbols: SymbolTable | undefined, lateSymbols: SymbolTable, decl: LateBoundDeclaration) {
function lateBindMember(parent: Symbol, earlySymbols: SymbolTable | undefined, lateSymbols: SymbolTable, decl: LateBoundDeclaration | LateBoundBinaryExpressionDeclaration) {
Debug.assert(!!decl.symbol, "The member is expected to have a symbol.");
const links = getNodeLinks(decl);
if (!links.resolvedSymbol) {
// In the event we attempt to resolve the late-bound name of this member recursively,
// fall back to the early-bound name of this member.
links.resolvedSymbol = decl.symbol;
const type = checkComputedPropertyName(decl.name);
const declName = isBinaryExpression(decl) ? decl.left : decl.name;
const type = isElementAccessExpression(declName) ? checkExpressionCached(declName.argumentExpression) : checkComputedPropertyName(declName);
if (isTypeUsableAsPropertyName(type)) {
const memberName = getPropertyNameFromType(type);
const symbolFlags = decl.symbol.flags;
Expand All @@ -8320,9 +8334,9 @@ namespace ts {
// If we have an existing early-bound member, combine its declarations so that we can
// report an error at each declaration.
const declarations = earlySymbol ? concatenate(earlySymbol.declarations, lateSymbol.declarations) : lateSymbol.declarations;
const name = !(type.flags & TypeFlags.UniqueESSymbol) && unescapeLeadingUnderscores(memberName) || declarationNameToString(decl.name);
const name = !(type.flags & TypeFlags.UniqueESSymbol) && unescapeLeadingUnderscores(memberName) || declarationNameToString(declName);
forEach(declarations, declaration => error(getNameOfDeclaration(declaration) || declaration, Diagnostics.Property_0_was_also_declared_here, name));
error(decl.name || decl, Diagnostics.Duplicate_property_0, name);
error(declName || decl, Diagnostics.Duplicate_property_0, name);
lateSymbol = createSymbol(SymbolFlags.None, memberName, CheckFlags.Late);
}
lateSymbol.nameType = type;
Expand Down Expand Up @@ -8364,6 +8378,20 @@ namespace ts {
}
}
}
const assignments = symbol.assignmentDeclarationMembers;
if (assignments) {
const decls = arrayFrom(assignments.values());
for (const member of decls) {
const assignmentKind = getAssignmentDeclarationKind(member as BinaryExpression | CallExpression);
const isInstanceMember = assignmentKind === AssignmentDeclarationKind.PrototypeProperty
|| assignmentKind === AssignmentDeclarationKind.ThisProperty
|| assignmentKind === AssignmentDeclarationKind.ObjectDefinePrototypeProperty
|| assignmentKind === AssignmentDeclarationKind.Prototype; // A straight `Prototype` assignment probably can never have a computed name
if (isStatic === !isInstanceMember && hasLateBindableName(member)) {
lateBindMember(symbol, earlySymbols, lateSymbols, member);
}
}
}

links[resolutionKind] = combineSymbolTables(earlySymbols, lateSymbols) || emptySymbols;
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/transformers/declarations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1130,7 +1130,7 @@ namespace ts {
fakespace.symbol = props[0].parent!;
const declarations = mapDefined(props, p => {
if (!isPropertyAccessExpression(p.valueDeclaration)) {
return undefined;
return undefined; // TODO GH#33569: Handle element access expressions that created late bound names (rather than silently omitting them)
}
getSymbolAccessibilityDiagnostic = createGetSymbolAccessibilityDiagnosticForNode(p.valueDeclaration);
const type = resolver.createTypeOfDeclaration(p.valueDeclaration, fakespace, declarationEmitNodeBuilderFlags, symbolTracker);
Expand Down
20 changes: 18 additions & 2 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,7 @@ namespace ts {

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

export type DeclarationName = Identifier | StringLiteralLike | NumericLiteral | ComputedPropertyName | BindingPattern;
export type DeclarationName = Identifier | StringLiteralLike | NumericLiteral | ComputedPropertyName | ElementAccessExpression | BindingPattern;
Copy link
Contributor

Choose a reason for hiding this comment

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

navigationBar uses DeclarationName and getNameOfDeclaration. Is it affected by this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, it uses nodeText on the name, which probably works fine for ComputedPropertyDeclarations, but probably falls a little short for ElementAccessExpressions. Can you point me to a test for computed property name tests for navigationBar? I'll (speculatively) build in the fix, but I'd like to add a test.

Copy link
Contributor

Choose a reason for hiding this comment

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

tests\cases\fourslash\navigationBarItemsSymbols1.ts

Copy link
Member

Choose a reason for hiding this comment

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

FWIW: https://github.com/microsoft/TypeScript/pull/33537/files#diff-098ee0b770157ee6b5ef687045ee074b

Copy link
Member Author

@weswigham weswigham Sep 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So, turns out js assignment members aren't syntactically children, so they can't be in the nav bar (which only looks at syntax members). Additionally, the nav bar excludes dynamically named members, so on two fronts these shouldn't appear in the nav bar. I've added a test showing the behavior~


export interface Declaration extends Node {
_declarationBrand: any;
Expand All @@ -829,12 +829,27 @@ namespace ts {
name: ComputedPropertyName;
}

/* @internal */
export interface DynamicNamedBinaryExpression extends BinaryExpression {
left: ElementAccessExpression;
}

/* @internal */
// A declaration that supports late-binding (used in checker)
export interface LateBoundDeclaration extends DynamicNamedDeclaration {
name: LateBoundName;
}

/* @internal */
export interface LateBoundBinaryExpressionDeclaration extends DynamicNamedBinaryExpression {
left: LateBoundElementAccessExpression;
}

/* @internal */
export interface LateBoundElementAccessExpression extends ElementAccessExpression {
argumentExpression: EntityNameExpression;
}

export interface DeclarationStatement extends NamedDeclaration, Statement {
name?: Identifier | StringLiteral | NumericLiteral;
}
Expand Down Expand Up @@ -3827,7 +3842,7 @@ namespace ts {
Classifiable = Class | Enum | TypeAlias | Interface | TypeParameter | Module | Alias,

/* @internal */
LateBindingContainer = Class | Interface | TypeLiteral | ObjectLiteral,
LateBindingContainer = Class | Interface | TypeLiteral | ObjectLiteral | Function,
}

export interface Symbol {
Expand All @@ -3847,6 +3862,7 @@ namespace ts {
/* @internal */ isReferenced?: SymbolFlags; // True if the symbol is referenced elsewhere. Keeps track of the meaning of a reference in case a symbol is both a type parameter and parameter.
/* @internal */ isReplaceableByMethod?: boolean; // Can this Javascript class property be replaced by a method symbol?
/* @internal */ isAssigned?: boolean; // True if the symbol is a parameter with assignments
/* @internal */ assignmentDeclarationMembers?: Map<Declaration>; // detected late-bound assignment declarations associated with the symbol
}

/* @internal */
Expand Down
Loading