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.

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

Completion list show correct entry for function expression and class expression #3643

Merged
merged 25 commits into from
Jul 10, 2015
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
aab2100
Add name of function expression into completion list
Jun 24, 2015
9c9e298
Add test for completion in function expression
Jun 26, 2015
34489fa
Add test for completion in class expression
Jun 26, 2015
5467e1d
Support completion in named class expression and named function expre…
Jun 26, 2015
45182b8
Add todo comment to use getDeclaredName
Jun 26, 2015
553085f
Merge branch 'master' into completionListWithLocalName
Jun 29, 2015
389e446
Merge branch 'master' into completionListWithLocalName
Jun 29, 2015
744f640
Add comment to clarify why we don't use copySymbol
Jul 1, 2015
514d054
Address CR: Use getDeclaredName and getDeclarationOfKind
Jul 1, 2015
605ab0b
Fix rename for class expression
Jul 1, 2015
7747ad4
Clean up stripQuote and add comments
Jul 1, 2015
3079607
Update verification function to be able to test that the only symbol …
Jul 1, 2015
2e0c390
Address code review
Jul 1, 2015
06c9876
Address code review
Jul 2, 2015
6da98ce
Fix comments
Jul 6, 2015
7c52aaa
Merge branch 'master' into completionListWithLocalName
Jul 6, 2015
b01e4d8
Address code review
Jul 6, 2015
f4cd1ac
Address code review, handle type parameter in completion list inside …
Jul 7, 2015
0148915
Fix comment
Jul 7, 2015
872fdcf
Merge branch 'master' into completionListWithLocalName
Jul 7, 2015
b5b1b7b
Use symbol.getName for classExpression and functionExpression since i…
Jul 7, 2015
d0b8002
Address code review
Jul 9, 2015
41bedd2
Use getDecalredName in service layer instead of symbol.getName
Jul 10, 2015
d0d1ee9
Add test for using unicode escape as function name
Jul 10, 2015
f16f9d1
Merge branch 'master' of https://github.com/Microsoft/TypeScript into…
Jul 10, 2015
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
14 changes: 7 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12170,20 +12170,20 @@ namespace ts {
case SyntaxKind.EnumDeclaration:
copySymbols(getSymbolOfNode(location).exports, meaning & SymbolFlags.EnumMember);
break;
case SyntaxKind.ClassExpression:
if ((<ClassExpression>location).name) {
copySymbol(location.symbol, meaning);
}
// Fall through
case SyntaxKind.ClassDeclaration:
case SyntaxKind.InterfaceDeclaration:
if (!(memberFlags & NodeFlags.Static)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what does line do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which line ?

Copy link
Member

Choose a reason for hiding this comment

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

the logic below, GitHub doesn't show it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @JsonFreeman for explaining this. This is the logic so that if we didn't come from static member of class or interface, add the type parameters into the symbol table (type-parameters of class Declaration and interface are in member property of the symbol.
Note: that the memberFlags come from previous iteration

Copy link
Contributor

Choose a reason for hiding this comment

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

Also note that for type aliases and signatures, the type parameters are in the locals, but for classes and interfaces, the type parameters are in the members.

copySymbols(getSymbolOfNode(location).members, meaning & SymbolFlags.Type);
}
break;
case SyntaxKind.FunctionExpression:
if ((<FunctionExpression>location).name) {
copySymbol(location.symbol, meaning);
case SyntaxKind.ClassExpression:
Copy link
Contributor

Choose a reason for hiding this comment

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

class expression listed twice

Copy link
Contributor

Choose a reason for hiding this comment

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

Uh oh. It'd be good to have a feature that catches this.

Copy link
Member

Choose a reason for hiding this comment

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

@JsonFreeman it's being tracked in #2854

let name = (<FunctionExpression|ClassExpression>location).name;
if (name) {
let symbol = location.symbol;
if (symbol.flags & meaning && !hasProperty(symbols, name.text)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i would add a comment why we are not using copySymbol.. cause i will forget :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌵

symbols[name.text] = symbol;
}
}
break;
}
Expand Down
33 changes: 31 additions & 2 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1423,6 +1423,9 @@ namespace ts {
// class X {}
export const classElement = "class";

// var x = class X {}
export const localClassElement = "local class";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have a separate tag for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be consistent with function expression which has kind of "local function":cactus:

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why we do it for either one then.

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree. let's take them both out.


Copy link
Contributor

Choose a reason for hiding this comment

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

note. you need to handle this on the managed side as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you mean be handling in the managed side?

Copy link
Member

Choose a reason for hiding this comment

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

Probably best to discuss this in person with someone but I believe the idea is that VS needs to handle styles differently depending on what comes in to the managed side.

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you see in navbar when you do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌵 file a bug

// interface Y {}
export const interfaceElement = "interface";

Expand Down Expand Up @@ -2807,6 +2810,22 @@ namespace ts {
}
}

// Special case for function expression and class expression because despite sometimes having a name, the binder
// binds them to a symbol with the name "__function" and "__class" respectively. However, for completion entry, we want
// to display its declared name rather than "__function" and "__class".
// var x = function foo () {
// fo$ <- completion list should contain local name "foo"
// }
// foo$ <- completion list should not contain "foo"
if (displayName === "__function" || displayName === "__class") {
Copy link
Member

Choose a reason for hiding this comment

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

Look into getDeclaredName instead, including the above for the default export

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getDeclaredName doesn't handle function expression correctly. Talk with @mhegazy, the function needed some rewrite and I don't want to have it done in this PR. I will put a TODO here to have it change once getDeclaredName is done

Copy link
Contributor

Choose a reason for hiding this comment

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

@DanielRosenwasser should have a fix for this in a pending review. you should synchronize.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, it's in my PR. Speaking of which, can you guys review it? 😃

#3367

Copy link
Member

Choose a reason for hiding this comment

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

You should be clear to use getDeclaredName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌹 yeah
🌵

displayName = symbol.declarations[0].name.getText();

// At this point, we expect that all completion list entries have declared name including function expression
// because when we gather all relevant symbols, we check that the function expression must have declared name
// before adding the symbol into our symbols table. (see: getSymbolsInScope)
Debug.assert(displayName !== undefined,"Expected this function expression to have declared name");
}

let firstCharCode = displayName.charCodeAt(0);
// First check of the displayName is not external module; if it is an external module, it is not valid entry
if ((symbol.flags & SymbolFlags.Namespace) && (firstCharCode === CharacterCodes.singleQuote || firstCharCode === CharacterCodes.doubleQuote)) {
Expand Down Expand Up @@ -3552,7 +3571,8 @@ namespace ts {
function getSymbolKind(symbol: Symbol, location: Node): string {
let flags = symbol.getFlags();

if (flags & SymbolFlags.Class) return ScriptElementKind.classElement;
if (flags & SymbolFlags.Class) return symbol.declarations[0].kind === SyntaxKind.ClassExpression ?
Copy link
Member

Choose a reason for hiding this comment

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

Use getDeclarationOfKind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌵

ScriptElementKind.localClassElement : ScriptElementKind.classElement;
if (flags & SymbolFlags.Enum) return ScriptElementKind.enumElement;
if (flags & SymbolFlags.TypeAlias) return ScriptElementKind.typeElement;
if (flags & SymbolFlags.Interface) return ScriptElementKind.interfaceElement;
Expand Down Expand Up @@ -3761,7 +3781,16 @@ namespace ts {
}
}
if (symbolFlags & SymbolFlags.Class && !hasAddedSymbolInfo) {
displayParts.push(keywordPart(SyntaxKind.ClassKeyword));
// Special case for class expressions because we would like to indicate that
// the class name is local to the class body (similar to function expression)
// (local class) class <className>
Copy link
Member

Choose a reason for hiding this comment

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

Move into the if body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌵

if (symbol.getName() === "__class") {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use getDeclarationOfKind(symbol, SyntaxKind.ClassExpression) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🌵

pushTypePart(ScriptElementKind.localClassElement);
}
else {
// Class declaration has name which is not local.
displayParts.push(keywordPart(SyntaxKind.ClassKeyword));
}
displayParts.push(spacePart());
addFullSymbolName(symbol);
writeTypeParametersOfSymbol(symbol, sourceFile);
Expand Down
14 changes: 14 additions & 0 deletions tests/cases/fourslash/completionListInNamedClassExpression.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
///<reference path="fourslash.ts" />

//// var x = class myClass {
//// getClassName (){
//// m/*0*/
//// }
//// /*1*/
//// }

goTo.marker("0");
verify.completionListContains("myClass", "(local class) myClass", /*documentation*/ undefined, "local class");

goTo.marker("1");
verify.completionListContains("myClass", "(local class) myClass", /*documentation*/ undefined, "local class");
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
///<reference path="fourslash.ts" />

//// class myClass { /*0*/ }
//// /*1*/
//// var x = class myClass {
//// getClassName (){
//// m/*2*/
//// }
//// /*3*/
//// }
//// var y = class {
//// getSomeName() {
//// /*4*/
//// }
//// /*5*/
//// }

goTo.marker("0");
verify.completionListContains("myClass", "class myClass", /*documentation*/ undefined, "class");

goTo.marker("1");
verify.completionListContains("myClass", "class myClass", /*documentation*/ undefined, "class");

goTo.marker("2");
verify.completionListContains("myClass", "(local class) myClass", /*documentation*/ undefined, "local class");

goTo.marker("3");
verify.completionListContains("myClass", "(local class) myClass", /*documentation*/ undefined, "local class");

goTo.marker("4");
verify.completionListContains("myClass", "class myClass", /*documentation*/ undefined, "class");

goTo.marker("5");
verify.completionListContains("myClass", "class myClass", /*documentation*/ undefined, "class");
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
///<reference path="fourslash.ts" />

//// var x = function foo() {
//// /*1*/
//// }

goTo.marker("1");
verify.completionListContains("foo", "(local function) foo(): void", /*documentation*/ undefined, "local function");
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
///<reference path="fourslash.ts" />

//// function foo() {}
//// /*0*/
//// var x = function foo() {
//// /*1*/
//// }
//// var y = function () {
//// /*2*/
//// }

goTo.marker("0");
verify.completionListContains("foo", "function foo(): void", /*documentation*/ undefined, "function");

goTo.marker("1");
verify.completionListContains("foo", "(local function) foo(): void", /*documentation*/ undefined, "local function");

goTo.marker("2");
verify.completionListContains("foo", "function foo(): void", /*documentation*/ undefined, "function")