Skip to content

Use brand types to clear up confusion about entity name expressions #10013

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
9 commits merged into from
Aug 11, 2016
Merged

Use brand types to clear up confusion about entity name expressions #10013

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
97bbbd7
Introduce the `EntityNameExpression` type
Jul 28, 2016
f9fd496
Allow `export =` and `export default` to alias any EntityNameExpressi…
Jul 29, 2016
c50ccbf
Simplify some code
Jul 29, 2016
91c9d76
Remove `SupportedExpressionWithTypeArguments` type; just check that t…
Aug 2, 2016
db44a71
Fix bug
Aug 2, 2016
dc19223
Use recursion, and fix error for undefined node
Aug 2, 2016
6814a9f
Rename function
Aug 2, 2016
7908257
Fix lint error
Aug 2, 2016
5b9bd82
Merge branch 'master' into resolve_entity_name
Aug 11, 2016
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
13 changes: 6 additions & 7 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1887,18 +1887,17 @@ namespace ts {
}

function bindExportAssignment(node: ExportAssignment | BinaryExpression) {
const boundExpression = node.kind === SyntaxKind.ExportAssignment ? (<ExportAssignment>node).expression : (<BinaryExpression>node).right;
if (!container.symbol || !container.symbol.exports) {
// Export assignment in some sort of block construct
bindAnonymousDeclaration(node, SymbolFlags.Alias, getDeclarationName(node));
}
else if (boundExpression.kind === SyntaxKind.Identifier && node.kind === SyntaxKind.ExportAssignment) {
// An export default clause with an identifier exports all meanings of that identifier
declareSymbol(container.symbol.exports, container.symbol, node, SymbolFlags.Alias, SymbolFlags.PropertyExcludes | SymbolFlags.AliasExcludes);
}
else {
// An export default clause with an expression exports a value
declareSymbol(container.symbol.exports, container.symbol, node, SymbolFlags.Property, SymbolFlags.PropertyExcludes | SymbolFlags.AliasExcludes);
const flags = node.kind === SyntaxKind.ExportAssignment && exportAssignmentIsAlias(<ExportAssignment>node)
// An export default clause with an EntityNameExpression exports all meanings of that identifier
? SymbolFlags.Alias
// An export default clause with any other expression exports a value
Copy link
Member

@DanielRosenwasser DanielRosenwasser Aug 2, 2016

Choose a reason for hiding this comment

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

Are these actually for default clauses, or just for export =?

Copy link
Author

@ghost ghost Aug 2, 2016

Choose a reason for hiding this comment

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

export default and export = are both instances of ExportAssignment.

: SymbolFlags.Property;
declareSymbol(container.symbol.exports, container.symbol, node, flags, SymbolFlags.PropertyExcludes | SymbolFlags.AliasExcludes);
}
}

Expand Down
131 changes: 69 additions & 62 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,29 +968,37 @@ namespace ts {


function checkAndReportErrorForExtendingInterface(errorLocation: Node): boolean {
let parentClassExpression = errorLocation;
while (parentClassExpression) {
const kind = parentClassExpression.kind;
if (kind === SyntaxKind.Identifier || kind === SyntaxKind.PropertyAccessExpression) {
parentClassExpression = parentClassExpression.parent;
continue;
}
if (kind === SyntaxKind.ExpressionWithTypeArguments) {
break;
}
return false;
}
if (!parentClassExpression) {
return false;
}
const expression = (<ExpressionWithTypeArguments>parentClassExpression).expression;
if (resolveEntityName(expression, SymbolFlags.Interface, /*ignoreErrors*/ true)) {
const expression = climbToEntityNameOfExpressionWithTypeArguments(errorLocation);
const isError = !!(expression && resolveEntityName(expression, SymbolFlags.Interface, /*ignoreErrors*/ true));
if (isError) {
error(errorLocation, Diagnostics.Cannot_extend_an_interface_0_Did_you_mean_implements, getTextOfNode(expression));
return true;
}
return false;
return isError;
}
/**
* Climbs up parents to an ExpressionWithTypeArguments, and returns its expression,
* but returns undefined if that expression is not an EntityNameExpression.
*/
function climbToEntityNameOfExpressionWithTypeArguments(node: Node): EntityNameExpression | undefined {
Copy link
Member

@sandersn sandersn Aug 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

climbToEntityNameOfExpressionWithTypeArguments [](start = 17, length = 46)

getEntityNameParent[OfExpressionWithTypeArguments]? (get...Parent is the usual pattern used rather than climbTo...)

Copy link
Author

@ghost ghost Aug 2, 2016

Choose a reason for hiding this comment

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

This one would also be simpler with recursion.

Copy link
Author

@ghost ghost Aug 2, 2016

Choose a reason for hiding this comment

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

We're getting the child of an ancestor (e.g. an aunt) which is an ExpressionWithTypeArguments. So it's more like getExpressionOfExpressionWithTypeArgumentsParent...

Copy link
Member

@sandersn sandersn Aug 2, 2016

Choose a reason for hiding this comment

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

good point, I missed that. Maybe move the .expression access to the caller? This is more like how it works with the rest of your new code post-commit 4 anyway.

In reply to: 73203379 [](ancestors = 73203379)

Copy link
Author

@ghost ghost Aug 2, 2016

Choose a reason for hiding this comment

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

Don't want to move .expression to the caller because climbToEntityNameOfExpressionWithTypeArguments is responsible for ensuring that .expression is a EntityNameExpression. Maybe it could just be named getEntityNameForExtendingInterface?

switch (node.kind) {
Copy link
Member

@DanielRosenwasser DanielRosenwasser Aug 2, 2016

Choose a reason for hiding this comment

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

Why? Use while (true)

case SyntaxKind.Identifier:
case SyntaxKind.PropertyAccessExpression:
if (node.parent) {
node = node.parent;
}
else {
return undefined;
}
break;
case SyntaxKind.ExpressionWithTypeArguments:
Debug.assert(isEntityNameExpression((<ExpressionWithTypeArguments>node).expression));
return <EntityNameExpression>(<ExpressionWithTypeArguments>node).expression;
default:
return undefined;
}
}


function checkResolvedBlockScopedVariable(result: Symbol, errorLocation: Node): void {
Debug.assert((result.flags & SymbolFlags.BlockScopedVariable) !== 0);
// Block-scoped variables cannot be used before their definition
Expand Down Expand Up @@ -1033,7 +1041,7 @@ namespace ts {
}

function getDeclarationOfAliasSymbol(symbol: Symbol): Declaration {
return forEach(symbol.declarations, d => isAliasSymbolDeclaration(d) ? d : undefined);
return find(symbol.declarations, d => isAliasSymbolDeclaration(d) ? d : undefined);
}

function getTargetOfImportEqualsDeclaration(node: ImportEqualsDeclaration): Symbol {
Expand Down Expand Up @@ -1164,7 +1172,7 @@ namespace ts {
}

function getTargetOfExportAssignment(node: ExportAssignment): Symbol {
return resolveEntityName(<Identifier>node.expression, SymbolFlags.Value | SymbolFlags.Type | SymbolFlags.Namespace);
return resolveEntityName(<EntityNameExpression>node.expression, SymbolFlags.Value | SymbolFlags.Type | SymbolFlags.Namespace);
}

function getTargetOfAliasDeclaration(node: Declaration): Symbol {
Expand Down Expand Up @@ -1274,7 +1282,7 @@ namespace ts {
}

// Resolves a qualified name and any involved aliases
function resolveEntityName(name: EntityName | Expression, meaning: SymbolFlags, ignoreErrors?: boolean, dontResolveAlias?: boolean): Symbol {
function resolveEntityName(name: EntityNameOrEntityNameExpression, meaning: SymbolFlags, ignoreErrors?: boolean, dontResolveAlias?: boolean): Symbol | undefined {
if (nodeIsMissing(name)) {
return undefined;
}
Expand All @@ -1289,7 +1297,7 @@ namespace ts {
}
}
else if (name.kind === SyntaxKind.QualifiedName || name.kind === SyntaxKind.PropertyAccessExpression) {
const left = name.kind === SyntaxKind.QualifiedName ? (<QualifiedName>name).left : (<PropertyAccessExpression>name).expression;
const left = name.kind === SyntaxKind.QualifiedName ? (<QualifiedName>name).left : (<PropertyAccessEntityNameExpression>name).expression;
const right = name.kind === SyntaxKind.QualifiedName ? (<QualifiedName>name).right : (<PropertyAccessExpression>name).name;

const namespace = resolveEntityName(left, SymbolFlags.Namespace, ignoreErrors);
Expand Down Expand Up @@ -1845,7 +1853,7 @@ namespace ts {
}
}

function isEntityNameVisible(entityName: EntityName | Expression, enclosingDeclaration: Node): SymbolVisibilityResult {
function isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node): SymbolVisibilityResult {
// get symbol of the first identifier of the entityName
let meaning: SymbolFlags;
if (entityName.parent.kind === SyntaxKind.TypeQuery || isExpressionWithTypeArgumentsInClassExtendsClause(entityName.parent)) {
Expand Down Expand Up @@ -3675,7 +3683,7 @@ namespace ts {
const baseTypeNodes = getInterfaceBaseTypeNodes(<InterfaceDeclaration>declaration);
if (baseTypeNodes) {
for (const node of baseTypeNodes) {
if (isSupportedExpressionWithTypeArguments(node)) {
if (isEntityNameExpression(node.expression)) {
const baseSymbol = resolveEntityName(node.expression, SymbolFlags.Type, /*ignoreErrors*/ true);
if (!baseSymbol || !(baseSymbol.flags & SymbolFlags.Interface) || getDeclaredTypeOfClassOrInterface(baseSymbol).thisType) {
return false;
Expand Down Expand Up @@ -5022,7 +5030,7 @@ namespace ts {
return getDeclaredTypeOfSymbol(symbol);
}

function getTypeReferenceName(node: TypeReferenceNode | ExpressionWithTypeArguments | JSDocTypeReference): LeftHandSideExpression | EntityName {
function getTypeReferenceName(node: TypeReferenceNode | ExpressionWithTypeArguments | JSDocTypeReference): EntityNameOrEntityNameExpression | undefined {
switch (node.kind) {
case SyntaxKind.TypeReference:
return (<TypeReferenceNode>node).typeName;
Expand All @@ -5031,8 +5039,9 @@ namespace ts {
case SyntaxKind.ExpressionWithTypeArguments:
// We only support expressions that are simple qualified names. For other
// expressions this produces undefined.
if (isSupportedExpressionWithTypeArguments(<ExpressionWithTypeArguments>node)) {
return (<ExpressionWithTypeArguments>node).expression;
const expr = (<ExpressionWithTypeArguments>node).expression;
if (isEntityNameExpression(expr)) {
return expr;
}

// fall through;
Expand All @@ -5043,7 +5052,7 @@ namespace ts {

function resolveTypeReferenceName(
node: TypeReferenceNode | ExpressionWithTypeArguments | JSDocTypeReference,
typeReferenceName: LeftHandSideExpression | EntityName) {
typeReferenceName: EntityNameExpression | EntityName) {

if (!typeReferenceName) {
return unknownSymbol;
Expand Down Expand Up @@ -5084,15 +5093,14 @@ namespace ts {
const typeReferenceName = getTypeReferenceName(node);
symbol = resolveTypeReferenceName(node, typeReferenceName);
type = getTypeReferenceType(node, symbol);

links.resolvedSymbol = symbol;
links.resolvedType = type;
}
else {
// We only support expressions that are simple qualified names. For other expressions this produces undefined.
const typeNameOrExpression = node.kind === SyntaxKind.TypeReference ? (<TypeReferenceNode>node).typeName :
isSupportedExpressionWithTypeArguments(<ExpressionWithTypeArguments>node) ? (<ExpressionWithTypeArguments>node).expression :
undefined;
const typeNameOrExpression: EntityNameOrEntityNameExpression = node.kind === SyntaxKind.TypeReference
? (<TypeReferenceNode>node).typeName
: isEntityNameExpression((<ExpressionWithTypeArguments>node).expression)
? <EntityNameExpression>(<ExpressionWithTypeArguments>node).expression
: undefined;
symbol = typeNameOrExpression && resolveEntityName(typeNameOrExpression, SymbolFlags.Type) || unknownSymbol;
type = symbol === unknownSymbol ? unknownType :
symbol.flags & (SymbolFlags.Class | SymbolFlags.Interface) ? getTypeFromClassOrInterfaceReference(node, symbol) :
Expand Down Expand Up @@ -16244,7 +16252,7 @@ namespace ts {
const implementedTypeNodes = getClassImplementsHeritageClauseElements(node);
if (implementedTypeNodes) {
for (const typeRefNode of implementedTypeNodes) {
if (!isSupportedExpressionWithTypeArguments(typeRefNode)) {
if (!isEntityNameExpression(typeRefNode.expression)) {
error(typeRefNode.expression, Diagnostics.A_class_can_only_implement_an_identifier_Slashqualified_name_with_optional_type_arguments);
}
checkTypeReferenceNode(typeRefNode);
Expand Down Expand Up @@ -16486,7 +16494,7 @@ namespace ts {
checkObjectTypeForDuplicateDeclarations(node);
}
forEach(getInterfaceBaseTypeNodes(node), heritageElement => {
if (!isSupportedExpressionWithTypeArguments(heritageElement)) {
if (!isEntityNameExpression(heritageElement.expression)) {
error(heritageElement.expression, Diagnostics.An_interface_can_only_extend_an_identifier_Slashqualified_name_with_optional_type_arguments);
}
checkTypeReferenceNode(heritageElement);
Expand Down Expand Up @@ -16951,20 +16959,21 @@ namespace ts {
}
}

function getFirstIdentifier(node: EntityName | Expression): Identifier {
while (true) {
if (node.kind === SyntaxKind.QualifiedName) {
node = (<QualifiedName>node).left;
}
else if (node.kind === SyntaxKind.PropertyAccessExpression) {
node = (<PropertyAccessExpression>node).expression;
}
else {
break;
}
function getFirstIdentifier(node: EntityNameOrEntityNameExpression): Identifier {
switch (node.kind) {
case SyntaxKind.Identifier:
return <Identifier>node;
case SyntaxKind.QualifiedName:
do {
node = (<QualifiedName>node).left;
} while (node.kind !== SyntaxKind.Identifier);
return <Identifier>node;
case SyntaxKind.PropertyAccessExpression:
do {
node = (<PropertyAccessEntityNameExpression>node).expression;
} while (node.kind !== SyntaxKind.Identifier);
return <Identifier>node;
}
Debug.assert(node.kind === SyntaxKind.Identifier);
return <Identifier>node;
}

function checkExternalImportOrExportDeclaration(node: ImportDeclaration | ImportEqualsDeclaration | ExportDeclaration): boolean {
Expand Down Expand Up @@ -17663,7 +17672,7 @@ namespace ts {
return getLeftSideOfImportEqualsOrExportAssignment(node) !== undefined;
}

function getSymbolOfEntityNameOrPropertyAccessExpression(entityName: EntityName | PropertyAccessExpression): Symbol {
function getSymbolOfEntityNameOrPropertyAccessExpression(entityName: EntityName | PropertyAccessExpression): Symbol | undefined {
if (isDeclarationName(entityName)) {
return getSymbolOfNode(entityName.parent);
}
Expand All @@ -17682,22 +17691,20 @@ namespace ts {
}
}

if (entityName.parent.kind === SyntaxKind.ExportAssignment) {
return resolveEntityName(<Identifier>entityName,
if (entityName.parent.kind === SyntaxKind.ExportAssignment && isEntityNameExpression(<Identifier | PropertyAccessExpression>entityName)) {
return resolveEntityName(<EntityNameExpression>entityName,
/*all meanings*/ SymbolFlags.Value | SymbolFlags.Type | SymbolFlags.Namespace | SymbolFlags.Alias);
}

if (entityName.kind !== SyntaxKind.PropertyAccessExpression) {
if (isInRightSideOfImportOrExportAssignment(<EntityName>entityName)) {
// Since we already checked for ExportAssignment, this really could only be an Import
const importEqualsDeclaration = <ImportEqualsDeclaration>getAncestor(entityName, SyntaxKind.ImportEqualsDeclaration);
Debug.assert(importEqualsDeclaration !== undefined);
return getSymbolOfPartOfRightHandSideOfImportEquals(<EntityName>entityName, importEqualsDeclaration, /*dontResolveAlias*/ true);
}
if (entityName.kind !== SyntaxKind.PropertyAccessExpression && isInRightSideOfImportOrExportAssignment(<EntityName>entityName)) {
// Since we already checked for ExportAssignment, this really could only be an Import
const importEqualsDeclaration = <ImportEqualsDeclaration>getAncestor(entityName, SyntaxKind.ImportEqualsDeclaration);
Debug.assert(importEqualsDeclaration !== undefined);
return getSymbolOfPartOfRightHandSideOfImportEquals(<EntityName>entityName, importEqualsDeclaration, /*dontResolveAlias*/ true);
}

if (isRightSideOfQualifiedNameOrPropertyAccess(entityName)) {
entityName = <QualifiedName | PropertyAccessExpression>entityName.parent;
entityName = <QualifiedName | PropertyAccessEntityNameExpression>entityName.parent;
}

if (isHeritageClauseElementIdentifier(<EntityName>entityName)) {
Expand Down Expand Up @@ -18410,7 +18417,7 @@ namespace ts {
};

// defined here to avoid outer scope pollution
function getTypeReferenceDirectivesForEntityName(node: EntityName | PropertyAccessExpression): string[] {
function getTypeReferenceDirectivesForEntityName(node: EntityNameOrEntityNameExpression): string[] {
// program does not have any files with type reference directives - bail out
if (!fileToDirective) {
return undefined;
Expand Down
15 changes: 13 additions & 2 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ namespace ts {
* returns a truthy value, then returns that value.
* If no such value is found, the callback is applied to each element of array and undefined is returned.
*/
export function forEach<T, U>(array: T[], callback: (element: T, index: number) => U): U {
export function forEach<T, U>(array: T[] | undefined, callback: (element: T, index: number) => U | undefined): U | undefined {
if (array) {
for (let i = 0, len = array.length; i < len; i++) {
const result = callback(array[i], i);
Expand All @@ -93,6 +93,17 @@ namespace ts {
return undefined;
}

/** Like `forEach`, but assumes existence of array and fails if no truthy value is found. */
export function find<T, U>(array: T[], callback: (element: T, index: number) => U | undefined): U {
for (let i = 0, len = array.length; i < len; i++) {
const result = callback(array[i], i);
if (result) {
return result;
}
}
Debug.fail();
}

export function contains<T>(array: T[], value: T): boolean {
if (array) {
for (const v of array) {
Expand Down Expand Up @@ -941,7 +952,7 @@ namespace ts {
* [^./] # matches everything up to the first . character (excluding directory seperators)
* (\\.(?!min\\.js$))? # matches . characters but not if they are part of the .min.js file extension
*/
const singleAsteriskRegexFragmentFiles = "([^./]|(\\.(?!min\\.js$))?)*";
const singleAsteriskRegexFragmentFiles = "([^./]|(\\.(?!min\\.js$))?)*";
const singleAsteriskRegexFragmentOther = "[^/]*";

export function getRegularExpressionForWildcard(specs: string[], basePath: string, usage: "files" | "directories" | "exclude") {
Expand Down
Loading