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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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

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

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
132 changes: 67 additions & 65 deletions src/compiler/checker.ts

Large diffs are not rendered by default.

13 changes: 12 additions & 1 deletion src/compiler/core.ts
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
8 changes: 4 additions & 4 deletions src/compiler/declarationEmitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ namespace ts {
}
}

function emitEntityName(entityName: EntityName | PropertyAccessExpression) {
function emitEntityName(entityName: EntityNameOrEntityNameExpression) {
const visibilityResult = resolver.isEntityNameVisible(entityName,
// Aliases can be written asynchronously so use correct enclosing declaration
entityName.parent.kind === SyntaxKind.ImportEqualsDeclaration ? entityName.parent : enclosingDeclaration);
Expand All @@ -452,9 +452,9 @@ namespace ts {
}

function emitExpressionWithTypeArguments(node: ExpressionWithTypeArguments) {
if (isSupportedExpressionWithTypeArguments(node)) {
if (isEntityNameExpression(node.expression)) {
Debug.assert(node.expression.kind === SyntaxKind.Identifier || node.expression.kind === SyntaxKind.PropertyAccessExpression);
emitEntityName(<Identifier | PropertyAccessExpression>node.expression);
emitEntityName(node.expression);
if (node.typeArguments) {
write("<");
emitCommaList(node.typeArguments, emitType);
Expand Down Expand Up @@ -1019,7 +1019,7 @@ namespace ts {
}

function emitTypeOfTypeReference(node: ExpressionWithTypeArguments) {
if (isSupportedExpressionWithTypeArguments(node)) {
if (isEntityNameExpression(node.expression)) {
emitTypeWithNewGetSymbolAccessibilityDiagnostic(node, getHeritageClauseVisibilityError);
}
else if (!isImplementsList && node.expression.kind === SyntaxKind.NullKeyword) {
Expand Down
14 changes: 10 additions & 4 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -982,13 +982,19 @@ namespace ts {
multiLine?: boolean;
}

export type EntityNameExpression = Identifier | PropertyAccessEntityNameExpression;
export type EntityNameOrEntityNameExpression = EntityName | EntityNameExpression;

// @kind(SyntaxKind.PropertyAccessExpression)
export interface PropertyAccessExpression extends MemberExpression, Declaration {
expression: LeftHandSideExpression;
name: Identifier;
Copy link
Author

Choose a reason for hiding this comment

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

This didn't seem to be used, but is it possible that a third party is using this type?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, this is a better question for @vladima


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

}

export type IdentifierOrPropertyAccess = Identifier | PropertyAccessExpression;
/** Brand for a PropertyAccessExpression which, like a QualifiedName, consists of a sequence of identifiers separated by dots. */
export interface PropertyAccessEntityNameExpression extends PropertyAccessExpression {
_propertyAccessExpressionLikeQualifiedNameBrand?: any;
expression: EntityNameExpression;
}

// @kind(SyntaxKind.ElementAccessExpression)
export interface ElementAccessExpression extends MemberExpression {
Expand Down Expand Up @@ -2031,7 +2037,7 @@ namespace ts {
writeTypeOfExpression(expr: Expression, enclosingDeclaration: Node, flags: TypeFormatFlags, writer: SymbolWriter): void;
writeBaseConstructorTypeOfClass(node: ClassLikeDeclaration, enclosingDeclaration: Node, flags: TypeFormatFlags, writer: SymbolWriter): void;
isSymbolAccessible(symbol: Symbol, enclosingDeclaration: Node, meaning: SymbolFlags): SymbolAccessibilityResult;
isEntityNameVisible(entityName: EntityName | Expression, enclosingDeclaration: Node): SymbolVisibilityResult;
isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node): SymbolVisibilityResult;
// Returns the constant value this property access resolves to, or 'undefined' for a non-constant
getConstantValue(node: EnumMember | PropertyAccessExpression | ElementAccessExpression): number;
getReferencedValueDeclaration(reference: Identifier): Declaration;
Expand All @@ -2040,7 +2046,7 @@ namespace ts {
moduleExportsSomeValue(moduleReferenceExpression: Expression): boolean;
isArgumentsLocalBinding(node: Identifier): boolean;
getExternalModuleFileFromDeclaration(declaration: ImportEqualsDeclaration | ImportDeclaration | ExportDeclaration | ModuleDeclaration): SourceFile;
getTypeReferenceDirectivesForEntityName(name: EntityName | PropertyAccessExpression): string[];
getTypeReferenceDirectivesForEntityName(name: EntityNameOrEntityNameExpression): string[];
getTypeReferenceDirectivesForSymbol(symbol: Symbol, meaning?: SymbolFlags): string[];
}

Expand Down
35 changes: 13 additions & 22 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1033,14 +1033,14 @@ namespace ts {
&& (<PropertyAccessExpression | ElementAccessExpression>node).expression.kind === SyntaxKind.SuperKeyword;
}


export function getEntityNameFromTypeNode(node: TypeNode): EntityName | Expression {
export function getEntityNameFromTypeNode(node: TypeNode): EntityNameOrEntityNameExpression {
if (node) {
switch (node.kind) {
case SyntaxKind.TypeReference:
return (<TypeReferenceNode>node).typeName;
case SyntaxKind.ExpressionWithTypeArguments:
return (<ExpressionWithTypeArguments>node).expression;
Debug.assert(isEntityNameExpression((<ExpressionWithTypeArguments>node).expression));
return <EntityNameExpression>(<ExpressionWithTypeArguments>node).expression;
case SyntaxKind.Identifier:
case SyntaxKind.QualifiedName:
return (<EntityName><Node>node);
Expand Down Expand Up @@ -1694,16 +1694,20 @@ namespace ts {
// import * as <symbol> from ...
// import { x as <symbol> } from ...
// export { x as <symbol> } from ...
// export = ...
// export default ...
// export = <EntityNameExpression>
// export default <EntityNameExpression>
export function isAliasSymbolDeclaration(node: Node): boolean {
return node.kind === SyntaxKind.ImportEqualsDeclaration ||
node.kind === SyntaxKind.NamespaceExportDeclaration ||
node.kind === SyntaxKind.ImportClause && !!(<ImportClause>node).name ||
node.kind === SyntaxKind.NamespaceImport ||
node.kind === SyntaxKind.ImportSpecifier ||
node.kind === SyntaxKind.ExportSpecifier ||
node.kind === SyntaxKind.ExportAssignment && (<ExportAssignment>node).expression.kind === SyntaxKind.Identifier;
node.kind === SyntaxKind.ExportAssignment && exportAssignmentIsAlias(<ExportAssignment>node);
}

export function exportAssignmentIsAlias(node: ExportAssignment): boolean {
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.

exportAssignmentIsAlias [](start = 20, length = 23)

it seems weird that there are 3 functions whose bodies are all just isEntityNameExpression(node.expression). If you express them as a single function, does any common name look at all good? #Closed

return isEntityNameExpression(node.expression);
}

export function getClassExtendsHeritageClauseElement(node: ClassLikeDeclaration | InterfaceDeclaration) {
Expand Down Expand Up @@ -2681,22 +2685,9 @@ namespace ts {
isClassLike(node.parent.parent);
}

// Returns false if this heritage clause element's expression contains something unsupported
// (i.e. not a name or dotted name).
export function isSupportedExpressionWithTypeArguments(node: ExpressionWithTypeArguments): boolean {
return isSupportedExpressionWithTypeArgumentsRest(node.expression);
}

function isSupportedExpressionWithTypeArgumentsRest(node: Expression): boolean {
if (node.kind === SyntaxKind.Identifier) {
return true;
}
else if (isPropertyAccessExpression(node)) {
return isSupportedExpressionWithTypeArgumentsRest(node.expression);
}
else {
return false;
}
export function isEntityNameExpression(node: Expression): node is EntityNameExpression {
Copy link
Author

@ghost ghost Jul 28, 2016

Choose a reason for hiding this comment

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

This would be a one-liner with recursion, but I guess we're worried about performance? Note that recursion should be shallow (assuming a.b.c.d.e.f.g is rare in real code).

Copy link
Member

Choose a reason for hiding this comment

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

Again, a better question for @vladima since he knows more about performance.


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

return node.kind === SyntaxKind.Identifier ||
node.kind === SyntaxKind.PropertyAccessExpression && isEntityNameExpression((<PropertyAccessExpression>node).expression);
}

export function isRightSideOfQualifiedNameOrPropertyAccess(node: Node) {
Expand Down
76 changes: 76 additions & 0 deletions tests/baselines/reference/exportDefaultProperty.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
//// [tests/cases/compiler/exportDefaultProperty.ts] ////

//// [declarations.d.ts]
// This test is just like exportEqualsProperty, but with `export default`.

declare namespace foo.bar {
export type X = number;
export const X: number;
}

declare module "foobar" {
export default foo.bar;
}

declare module "foobarx" {
export default foo.bar.X;
}

//// [a.ts]
namespace A {
export class B { constructor(b: number) {} }
export namespace B { export const b: number = 0; }
}
export default A.B;

//// [b.ts]
export default "foo".length;

//// [index.ts]
/// <reference path="declarations.d.ts" />
import fooBar from "foobar";
import X = fooBar.X;
import X2 from "foobarx";
const x: X = X;
const x2: X2 = X2;

import B from "./a";
const b: B = new B(B.b);

import fooLength from "./b";
fooLength + 1;


//// [a.js]
"use strict";
var A;
(function (A) {
var B = (function () {
function B(b) {
}
return B;
}());
A.B = B;
var B;
(function (B) {
B.b = 0;
})(B = A.B || (A.B = {}));
})(A || (A = {}));
exports.__esModule = true;
exports["default"] = A.B;
//// [b.js]
"use strict";
exports.__esModule = true;
exports["default"] = "foo".length;
//// [index.js]
"use strict";
/// <reference path="declarations.d.ts" />
var foobar_1 = require("foobar");
var X = foobar_1["default"].X;
var foobarx_1 = require("foobarx");
var x = X;
var x2 = foobarx_1["default"];
var a_1 = require("./a");
var b = new a_1["default"](a_1["default"].b);
var b_1 = require("./b");
b_1["default"] + 1;
92 changes: 92 additions & 0 deletions tests/baselines/reference/exportDefaultProperty.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
=== tests/cases/compiler/index.ts ===
/// <reference path="declarations.d.ts" />
import fooBar from "foobar";
>fooBar : Symbol(fooBar, Decl(index.ts, 1, 6))

import X = fooBar.X;
>X : Symbol(X, Decl(index.ts, 1, 28))
>fooBar : Symbol(fooBar, Decl(index.ts, 1, 6))
>X : Symbol(fooBar.X, Decl(declarations.d.ts, 2, 27), Decl(declarations.d.ts, 4, 16))

import X2 from "foobarx";
>X2 : Symbol(X2, Decl(index.ts, 3, 6))

const x: X = X;
>x : Symbol(x, Decl(index.ts, 4, 5))
>X : Symbol(X, Decl(index.ts, 1, 28))
>X : Symbol(X, Decl(index.ts, 1, 28))

const x2: X2 = X2;
>x2 : Symbol(x2, Decl(index.ts, 5, 5))
>X2 : Symbol(X2, Decl(index.ts, 3, 6))
>X2 : Symbol(X2, Decl(index.ts, 3, 6))

import B from "./a";
>B : Symbol(B, Decl(index.ts, 7, 6))

const b: B = new B(B.b);
>b : Symbol(b, Decl(index.ts, 8, 5))
>B : Symbol(B, Decl(index.ts, 7, 6))
>B : Symbol(B, Decl(index.ts, 7, 6))
>B.b : Symbol(B.b, Decl(a.ts, 2, 37))
>B : Symbol(B, Decl(index.ts, 7, 6))
>b : Symbol(B.b, Decl(a.ts, 2, 37))

import fooLength from "./b";
>fooLength : Symbol(fooLength, Decl(index.ts, 10, 6))

fooLength + 1;
>fooLength : Symbol(fooLength, Decl(index.ts, 10, 6))

=== tests/cases/compiler/declarations.d.ts ===
// This test is just like exportEqualsProperty, but with `export default`.

declare namespace foo.bar {
>foo : Symbol(foo, Decl(declarations.d.ts, 0, 0))
>bar : Symbol(bar, Decl(declarations.d.ts, 2, 22))

export type X = number;
>X : Symbol(X, Decl(declarations.d.ts, 2, 27), Decl(declarations.d.ts, 4, 16))

export const X: number;
>X : Symbol(X, Decl(declarations.d.ts, 2, 27), Decl(declarations.d.ts, 4, 16))
}

declare module "foobar" {
export default foo.bar;
>foo.bar : Symbol(default, Decl(declarations.d.ts, 2, 22))
>foo : Symbol(foo, Decl(declarations.d.ts, 0, 0))
>bar : Symbol(default, Decl(declarations.d.ts, 2, 22))
}

declare module "foobarx" {
export default foo.bar.X;
>foo.bar.X : Symbol(default, Decl(declarations.d.ts, 2, 27), Decl(declarations.d.ts, 4, 16))
>foo.bar : Symbol(foo.bar, Decl(declarations.d.ts, 2, 22))
>foo : Symbol(foo, Decl(declarations.d.ts, 0, 0))
>bar : Symbol(foo.bar, Decl(declarations.d.ts, 2, 22))
>X : Symbol(default, Decl(declarations.d.ts, 2, 27), Decl(declarations.d.ts, 4, 16))
}

=== tests/cases/compiler/a.ts ===
namespace A {
>A : Symbol(A, Decl(a.ts, 0, 0))

export class B { constructor(b: number) {} }
>B : Symbol(B, Decl(a.ts, 0, 13), Decl(a.ts, 1, 48))
>b : Symbol(b, Decl(a.ts, 1, 33))

export namespace B { export const b: number = 0; }
>B : Symbol(B, Decl(a.ts, 0, 13), Decl(a.ts, 1, 48))
>b : Symbol(b, Decl(a.ts, 2, 37))
}
export default A.B;
>A.B : Symbol(default, Decl(a.ts, 0, 13), Decl(a.ts, 1, 48))
>A : Symbol(A, Decl(a.ts, 0, 0))
>B : Symbol(default, Decl(a.ts, 0, 13), Decl(a.ts, 1, 48))

=== tests/cases/compiler/b.ts ===
export default "foo".length;
>"foo".length : Symbol(String.length, Decl(lib.d.ts, --, --))
>length : Symbol(String.length, Decl(lib.d.ts, --, --))

Loading