Skip to content

[Master] Fix resolve entity name to not dive inside property access expression when the expression is not entity name #14692

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
merged 3 commits into from
Mar 29, 2017
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
36 changes: 31 additions & 5 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1493,9 +1493,23 @@ namespace ts {
}
}
else if (name.kind === SyntaxKind.QualifiedName || name.kind === SyntaxKind.PropertyAccessExpression) {
const left = name.kind === SyntaxKind.QualifiedName ? (<QualifiedName>name).left : (<PropertyAccessEntityNameExpression>name).expression;
const right = name.kind === SyntaxKind.QualifiedName ? (<QualifiedName>name).right : (<PropertyAccessExpression>name).name;
let left: EntityNameOrEntityNameExpression;

if (name.kind === SyntaxKind.QualifiedName) {
left = (<QualifiedName>name).left;
}
else if (name.kind === SyntaxKind.PropertyAccessExpression &&
(name.expression.kind === SyntaxKind.ParenthesizedExpression || isEntityNameExpression(name.expression))) {
left = name.expression;
}
else {
// If the expression in property-access expression is not entity-name or parenthsizedExpression (e.g. it is a call expression), it won't be able to successfully resolve the name.
// This is the case when we are trying to do any language service operation in heritage clauses. By return undefined, the getSymbolOfEntityNameOrPropertyAccessExpression
// will attempt to checkPropertyAccessExpression to resolve symbol.
// i.e class C extends foo()./*do language service operation here*/B {}
return undefined;
}
const right = name.kind === SyntaxKind.QualifiedName ? name.right : name.name;
const namespace = resolveEntityName(left, SymbolFlags.Namespace, ignoreErrors, /*dontResolveAlias*/ false, location);
if (!namespace || nodeIsMissing(right)) {
return undefined;
Expand All @@ -1511,6 +1525,15 @@ namespace ts {
return undefined;
}
}
else if (name.kind === SyntaxKind.ParenthesizedExpression) {
// If the expression in parenthsizedExpression is not an entity-name (e.g. it is a call expression), it won't be able to successfully resolve the name.
Copy link
Member

Choose a reason for hiding this comment

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

typo:parenthesized

// This is the case when we are trying to do any language service operation in heritage clauses. By return undefined, the getSymbolOfEntityNameOrPropertyAccessExpression
// will attempt to checkPropertyAccessExpression to resolve symbol.
// i.e class C extends foo()./*do language service operation here*/B {}
return isEntityNameExpression(name.expression) ?
resolveEntityName(name.expression as EntityNameOrEntityNameExpression, meaning, ignoreErrors, dontResolveAlias, location) :
undefined;
Copy link
Member

Choose a reason for hiding this comment

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

should we also give an error in the undefined case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't because the caller will already handle this case.

}
else {
Debug.fail("Unknown entity name kind.");
}
Expand Down Expand Up @@ -21069,7 +21092,6 @@ namespace ts {

if (isHeritageClauseElementIdentifier(<EntityName>entityName)) {
let meaning = SymbolFlags.None;

// In an interface or class, we're definitely interested in a type.
if (entityName.parent.kind === SyntaxKind.ExpressionWithTypeArguments) {
meaning = SymbolFlags.Type;
Expand All @@ -21084,9 +21106,13 @@ namespace ts {
}

meaning |= SymbolFlags.Alias;
return resolveEntityName(<EntityName>entityName, meaning);
const entityNameSymbol = resolveEntityName(<EntityName>entityName, meaning);
if (entityNameSymbol) {
return entityNameSymbol;
}
}
else if (isPartOfExpression(entityName)) {

if (isPartOfExpression(entityName)) {
if (nodeIsMissing(entityName)) {
// Missing entity name.
return undefined;
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1395,7 +1395,7 @@ namespace ts {
multiLine?: boolean;
}

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

export interface PropertyAccessExpression extends MemberExpression, Declaration {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
//// [emitClassDeclarationWithPropertyAccessInHeritageClause1.ts]
interface I {}
interface CTor {
new (hour: number, minute: number): I
}
var x: {
B : CTor
};
class B {}
function foo() {
return {B: B};
}
class C extends (foo()).B {}

//// [emitClassDeclarationWithPropertyAccessInHeritageClause1.js]
var __extends = (this && this.__extends) || (function () {
var extendStatics = Object.setPrototypeOf ||
({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
return function (d, b) {
extendStatics(d, b);
function __() { this.constructor = d; }
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
})();
var x;
var B = (function () {
function B() {
}
return B;
}());
function foo() {
return { B: B };
}
var C = (function (_super) {
__extends(C, _super);
function C() {
return _super !== null && _super.apply(this, arguments) || this;
}
return C;
}((foo()).B));
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
=== tests/cases/conformance/es6/classDeclaration/emitClassDeclarationWithPropertyAccessInHeritageClause1.ts ===
interface I {}
>I : Symbol(I, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 0, 0))

interface CTor {
>CTor : Symbol(CTor, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 0, 14))

new (hour: number, minute: number): I
>hour : Symbol(hour, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 2, 9))
>minute : Symbol(minute, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 2, 22))
>I : Symbol(I, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 0, 0))
}
var x: {
>x : Symbol(x, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 4, 3))

B : CTor
>B : Symbol(B, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 4, 8))
>CTor : Symbol(CTor, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 0, 14))

};
class B {}
>B : Symbol(B, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 6, 2))

function foo() {
>foo : Symbol(foo, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 7, 10))

return {B: B};
>B : Symbol(B, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 9, 12))
>B : Symbol(B, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 6, 2))
}
class C extends (foo()).B {}
>C : Symbol(C, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 10, 1))
>(foo()).B : Symbol(B, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 9, 12))
>foo : Symbol(foo, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 7, 10))
>B : Symbol(B, Decl(emitClassDeclarationWithPropertyAccessInHeritageClause1.ts, 9, 12))

Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
=== tests/cases/conformance/es6/classDeclaration/emitClassDeclarationWithPropertyAccessInHeritageClause1.ts ===
interface I {}
>I : I

interface CTor {
>CTor : CTor

new (hour: number, minute: number): I
>hour : number
>minute : number
>I : I
}
var x: {
>x : { B: CTor; }

B : CTor
>B : CTor
>CTor : CTor

};
class B {}
>B : B

function foo() {
>foo : () => { B: typeof B; }

return {B: B};
>{B: B} : { B: typeof B; }
>B : typeof B
>B : typeof B
}
class C extends (foo()).B {}
>C : C
>(foo()).B : B
>(foo()) : { B: typeof B; }
>foo() : { B: typeof B; }
>foo : () => { B: typeof B; }
>B : typeof B

2 changes: 2 additions & 0 deletions tests/baselines/reference/reexportClassDefinition.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import foo2 = require('./foo2')

class x extends foo2.x {}
>x : Symbol(x, Decl(foo3.ts, 0, 31))
>foo2.x : Symbol(x, Decl(foo2.ts, 2, 10))
>foo2 : Symbol(foo2, Decl(foo3.ts, 0, 0))
>x : Symbol(x, Decl(foo2.ts, 2, 10))


=== tests/cases/conformance/externalModules/foo1.ts ===
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
interface I {}
interface CTor {
new (hour: number, minute: number): I
}
var x: {
B : CTor
};
Copy link
Member

Choose a reason for hiding this comment

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

turns out lines 1-7 aren't used here either!

class B {}
function foo() {
return {B: B};
}
class C extends (foo()).B {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/// <reference path="fourslash.ts" />

//// interface I {}
//// interface CTor {
//// new (hour: number, minute: number): I
//// }
//// var x: {
//// B : CTor
//// };
Copy link
Member

Choose a reason for hiding this comment

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

lines 1-9 don't seem necessary for this test

//// class B {}
//// function foo() {
//// return {[|B|]: B};
//// }
//// class C extends (foo()).[|B|] {}
//// class C1 extends foo().[|B|] {}

const [def, ref1, ref2] = test.ranges();
Copy link
Member

Choose a reason for hiding this comment

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

Try this instead:

const rs = test.ranges();
for (const r of rs) {
  verify.referencesOf(r, rs));
}

verify.referencesOf(ref1, [def, ref1, ref2]);
verify.referencesOf(ref2, [def, ref1, ref2]);
verify.referencesOf(def, [def, ref1, ref2]);
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path='fourslash.ts' />

//// interface I {}
//// interface CTor {
//// new (hour: number, minute: number): I
//// }
//// var x: {
//// B : CTor
//// };
Copy link
Member

Choose a reason for hiding this comment

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

lines 1-9 don't seem necessary for this test

//// class B {}
//// function foo() {
//// return {/*refB*/B: B};
//// }
//// class C extends (foo())./*B*/B {}
//// class C1 extends foo()./*B1*/B {}

verify.goToDefinition([["B", "refB"], ["B1", "refB"]]);
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path="fourslash.ts" />

//// interface I {}
//// interface CTor {
//// new (hour: number, minute: number): I
//// }
//// var x: {
//// B : CTor
//// };
Copy link
Member

Choose a reason for hiding this comment

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

lines 1-9 don't seem necessary for this test

//// class B {}
//// function foo() {
//// return {[|B|]: B};
//// }
//// class C extends (foo()).[|B|] {}
//// class C1 extends foo().[|B|] {}

verify.rangesAreRenameLocations();