Skip to content

Only show first overload in a series of consecutive overload signatures #804

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
DanielRosenwasser merged 2 commits into from
Oct 7, 2014
Merged

Only show first overload in a series of consecutive overload signatures #804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7822759
Only show first overload in a series of consecutive overload signatur…
DanielRosenwasser Oct 2, 2014
4069e0a
Finished up "overload collapsing" for navigate-to.
DanielRosenwasser Oct 3, 2014
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
42 changes: 30 additions & 12 deletions src/services/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,47 +504,65 @@ module ts {
if (!this.namedDeclarations) {
var sourceFile = this;
var namedDeclarations: Declaration[] = [];
var isExternalModule = ts.isExternalModule(sourceFile);

forEachChild(sourceFile, function visit(node: Node): boolean {
forEachChild(sourceFile, function visit(node: Node): void {
Copy link
Copy Markdown
Contributor

@yuit yuit Oct 7, 2014

Choose a reason for hiding this comment

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

I would pull the visit function out

switch (node.kind) {
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.Method:
var functionDeclaration = <FunctionDeclaration>node;

if (functionDeclaration.name && functionDeclaration.name.kind !== SyntaxKind.Missing) {
var lastDeclaration = namedDeclarations.length > 0 ?
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Oct 7, 2014

Choose a reason for hiding this comment

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

? and : should be on the next line.

namedDeclarations[namedDeclarations.length - 1] :
undefined;

// Check whether this declaration belongs to an "overload group".
if (lastDeclaration && functionDeclaration.symbol === lastDeclaration.symbol) {
// Overwrite the last declaration if it was an overload
// and this one is an implementation.
if (functionDeclaration.body && !(<FunctionDeclaration>lastDeclaration).body) {
namedDeclarations[namedDeclarations.length - 1] = functionDeclaration;
}
}
else {
namedDeclarations.push(node);
}

forEachChild(node, visit);
Copy link
Copy Markdown
Contributor

@yuit yuit Oct 7, 2014

Choose a reason for hiding this comment

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

Why do we have to recurse here?

Copy link
Copy Markdown
Member Author

@DanielRosenwasser DanielRosenwasser Oct 7, 2014

Choose a reason for hiding this comment

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

Because we need to continue to aggregate things like functions within other functions, and eventually things like class expressions.

}
break;

Copy link
Copy Markdown
Contributor

@mhegazy mhegazy Oct 2, 2014

Choose a reason for hiding this comment

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

We are not walking the body of the function declaration this way, correct? If so we are missing inner functions.

Copy link
Copy Markdown
Member Author

@DanielRosenwasser DanielRosenwasser Oct 2, 2014

Choose a reason for hiding this comment

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

Right, this isn't finished, I was thinking about finishing this at home but I think I need to think out some of the semantics when dealing with incorrect code.

Copy link
Copy Markdown
Contributor

@JsonFreeman JsonFreeman Oct 2, 2014

Choose a reason for hiding this comment

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

This is going to get the first overload, not the last one. I think we should get the last one.

case SyntaxKind.ClassDeclaration:
case SyntaxKind.InterfaceDeclaration:
case SyntaxKind.EnumDeclaration:
case SyntaxKind.ModuleDeclaration:
case SyntaxKind.ImportDeclaration:
case SyntaxKind.Method:
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.Constructor:
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.TypeLiteral:
if ((<Declaration>node).name) {
namedDeclarations.push(<Declaration>node);
}
forEachChild(node, visit);
break;

// fall through
case SyntaxKind.Constructor:
case SyntaxKind.VariableStatement:
case SyntaxKind.ModuleBlock:
case SyntaxKind.FunctionBlock:
forEachChild(node, visit);
break;

case SyntaxKind.Parameter:
// Only consider properties defined as constructor parameters
if (!(node.flags & NodeFlags.AccessibilityModifier)) {
// Only consider properties defined as constructor parameters
break;
}
// fall through
Copy link
Copy Markdown
Contributor

@yuit yuit Oct 7, 2014

Choose a reason for hiding this comment

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

we only fall through in certain condition. Could you comment more on this?

Copy link
Copy Markdown
Member Author

@DanielRosenwasser DanielRosenwasser Oct 7, 2014

Choose a reason for hiding this comment

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

I think this is explained decently by the preceding comment about constructor parameters.

case SyntaxKind.VariableDeclaration:
case SyntaxKind.EnumMember:
case SyntaxKind.Property:
namedDeclarations.push(<Declaration>node);
break;
}

// do not go any deeper
return undefined;
});

this.namedDeclarations = namedDeclarations;
Expand Down
30 changes: 30 additions & 0 deletions tests/cases/fourslash/navigationItemsOverloads1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/// <reference path="fourslash.ts"/>

////function overload(a: string): boolean;
////function overload(b: boolean): boolean;
////function overload(b: number): boolean;
////function overload(f: typeof overload): boolean;
////function overload(x: any, b = (function overload() { return false })): boolean {
//// throw overload;
////}
////
////interface I {
//// interfaceMethodSignature(a: string): boolean;
//// interfaceMethodSignature(b: boolean): boolean;
//// interfaceMethodSignature(b: number): boolean;
//// interfaceMethodSignature(f: I): boolean;
////}
////
////class C {
//// methodOverload(a: string): boolean;
//// methodOverload(b: boolean): boolean;
//// methodOverload(b: number): boolean;
//// methodOverload(f: I): boolean;
//// methodOverload(x: any, b = (function overload() { return false })): boolean {
//// throw C;
//// }
////}

verify.navigationItemsListCount(1, "overload", "exact");
verify.navigationItemsListCount(1, "interfaceMethodSignature", "exact");
verify.navigationItemsListCount(1, "methodOverload", "exact");
12 changes: 12 additions & 0 deletions tests/cases/fourslash/navigationItemsOverloads2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/// <reference path="fourslash.ts"/>

////interface I {
//// interfaceMethodSignature(a: string): boolean;
//// interfaceMethodSignature(b: number): boolean;
//// interfaceMethodSignature(f: I): boolean;
////}
////interface I {
//// interfaceMethodSignature(b: boolean): boolean;
////}

verify.navigationItemsListCount(2, "interfaceMethodSignature", "exact");
29 changes: 29 additions & 0 deletions tests/cases/fourslash/navigationItemsOverloadsBroken1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/// <reference path="fourslash.ts"/>

////function overload1(a: string): boolean;
////function overload1(b: boolean): boolean;
////function overload1(b: number): boolean;
////
////var heyImNotInterruptingAnythingAmI = '?';
////
////function overload1(f: typeof overload): boolean;
////function overload1(x: any, b = (function overload() { return false })): boolean {
//// throw overload;
////}

////function overload2(a: string): boolean;
////function overload2(b: boolean): boolean;
////function overload2(b: number): boolean;
////
////function iJustRuinEverything(x: any, b = (function overload() { return false })): boolean {
//// throw overload;
////}
////
////function overload2(f: typeof overload): boolean;
////function overload2(x: any, b = (function overload() { return false })): boolean {
//// throw overload;
////}

verify.navigationItemsListCount(2, "overload1", "exact");
verify.navigationItemsListCount(2, "overload2", "exact");
verify.navigationItemsListCount(4, "overload", "prefix");
23 changes: 23 additions & 0 deletions tests/cases/fourslash/navigationItemsOverloadsBroken2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/// <reference path="fourslash.ts"/>

////function overload1(a: string): boolean;
////function overload1(b: boolean): boolean;
////function overload1(x: any, b = (function overload() { return false })): boolean {
//// throw overload1;
////}
////function overload1(b: number): boolean;
////function overload1(f: typeof overload): boolean;

////function overload2(a: string): boolean;
////function overload2(b: boolean): boolean;
////function overload2(x: any, b = (function overload() { return false })): boolean {
Copy link
Copy Markdown
Contributor

@yuit yuit Oct 7, 2014

Choose a reason for hiding this comment

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

I would have another case to test on nesting without another overload following. Also I will add another case when inside function has body

//// function overload2(): boolean;
//// function overload2(x: any): boolean;
//// throw overload2;
////}
////function overload2(b: number): boolean;
////function overload2(f: typeof overload): boolean;

verify.navigationItemsListCount(1, "overload1", "exact");
verify.navigationItemsListCount(3, "overload2", "exact");
verify.navigationItemsListCount(4, "overload", "prefix");