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

Correctly resolve tags for function overloads #30253

Merged
merged 6 commits into from
Jan 10, 2020
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
65 changes: 39 additions & 26 deletions src/services/symbolDisplay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ namespace ts.SymbolDisplay {
location: Node, semanticMeaning = getMeaningFromLocation(location), alias?: Symbol): SymbolDisplayPartsDocumentationAndSymbolKind {

const displayParts: SymbolDisplayPart[] = [];
let documentation: SymbolDisplayPart[] | undefined;
let tags: JSDocTagInfo[] | undefined;
let documentation: SymbolDisplayPart[] = [];
let tags: JSDocTagInfo[] = [];
const symbolFlags = getCombinedLocalAndExportSymbolFlags(symbol);
let symbolKind = semanticMeaning & SemanticMeaning.Value ? getSymbolKindOfConstructorPropertyMethodAccessorFunctionOrVar(typeChecker, symbol, location) : ScriptElementKind.unknown;
let hasAddedSymbolInfo = false;
Expand All @@ -135,6 +135,7 @@ namespace ts.SymbolDisplay {
let printer: Printer;
let documentationFromAlias: SymbolDisplayPart[] | undefined;
let tagsFromAlias: JSDocTagInfo[] | undefined;
let hasMultipleSignatures = false;

if (location.kind === SyntaxKind.ThisKeyword && !isThisExpression) {
return { displayParts: [keywordPart(SyntaxKind.ThisKeyword)], documentation: [], symbolKind: ScriptElementKind.primitiveType, tags: undefined };
Expand Down Expand Up @@ -230,6 +231,7 @@ namespace ts.SymbolDisplay {
addSignatureDisplayParts(signature, allSignatures);
}
hasAddedSymbolInfo = true;
hasMultipleSignatures = allSignatures.length > 1;
}
}
else if ((isNameOfFunctionDeclaration(location) && !(symbolFlags & SymbolFlags.Accessor)) || // name of function declaration
Expand Down Expand Up @@ -262,6 +264,7 @@ namespace ts.SymbolDisplay {

addSignatureDisplayParts(signature, allSignatures);
hasAddedSymbolInfo = true;
hasMultipleSignatures = allSignatures.length > 1;
}
}
}
Expand Down Expand Up @@ -489,6 +492,7 @@ namespace ts.SymbolDisplay {
const allSignatures = type.getNonNullableType().getCallSignatures();
if (allSignatures.length) {
addSignatureDisplayParts(allSignatures[0], allSignatures);
hasMultipleSignatures = allSignatures.length > 1;
}
}
}
Expand All @@ -498,42 +502,47 @@ namespace ts.SymbolDisplay {
}
}

if (!documentation) {
if (documentation.length === 0 && !hasMultipleSignatures) {
documentation = symbol.getDocumentationComment(typeChecker);
Copy link
Member

Choose a reason for hiding this comment

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

I think you should add the same code here.

tags = symbol.getJsDocTags();
if (documentation.length === 0 && symbolFlags & SymbolFlags.Property) {
// For some special property access expressions like `exports.foo = foo` or `module.exports.foo = foo`
// there documentation comments might be attached to the right hand side symbol of their declarations.
// The pattern of such special property access is that the parent symbol is the symbol of the file.
if (symbol.parent && forEach(symbol.parent.declarations, declaration => declaration.kind === SyntaxKind.SourceFile)) {
for (const declaration of symbol.declarations) {
if (!declaration.parent || declaration.parent.kind !== SyntaxKind.BinaryExpression) {
continue;
}
}

const rhsSymbol = typeChecker.getSymbolAtLocation((<BinaryExpression>declaration.parent).right);
if (!rhsSymbol) {
continue;
}
if (documentation.length === 0 && symbolFlags & SymbolFlags.Property) {
// For some special property access expressions like `exports.foo = foo` or `module.exports.foo = foo`
// there documentation comments might be attached to the right hand side symbol of their declarations.
// The pattern of such special property access is that the parent symbol is the symbol of the file.
if (symbol.parent && forEach(symbol.parent.declarations, declaration => declaration.kind === SyntaxKind.SourceFile)) {
for (const declaration of symbol.declarations) {
if (!declaration.parent || declaration.parent.kind !== SyntaxKind.BinaryExpression) {
continue;
}

documentation = rhsSymbol.getDocumentationComment(typeChecker);
tags = rhsSymbol.getJsDocTags();
if (documentation.length > 0) {
break;
}
const rhsSymbol = typeChecker.getSymbolAtLocation((<BinaryExpression>declaration.parent).right);
if (!rhsSymbol) {
continue;
}

documentation = rhsSymbol.getDocumentationComment(typeChecker);
tags = rhsSymbol.getJsDocTags();
if (documentation.length > 0) {
break;
}
}
}
}

if (tags.length === 0 && !hasMultipleSignatures) {
tags = symbol.getJsDocTags();
}

if (documentation.length === 0 && documentationFromAlias) {
documentation = documentationFromAlias;
}
if (tags!.length === 0 && tagsFromAlias) {

if (tags.length === 0 && tagsFromAlias) {
tags = tagsFromAlias;
}

return { displayParts, documentation, symbolKind, tags: tags!.length === 0 ? undefined : tags };
return { displayParts, documentation, symbolKind, tags: tags.length === 0 ? undefined : tags };

function getPrinter() {
if (!printer) {
Expand Down Expand Up @@ -614,9 +623,13 @@ namespace ts.SymbolDisplay {
displayParts.push(textPart(allSignatures.length === 2 ? "overload" : "overloads"));
displayParts.push(punctuationPart(SyntaxKind.CloseParenToken));
}
const docComment = signature.getDocumentationComment(typeChecker);
documentation = docComment.length === 0 ? undefined : docComment;
documentation = signature.getDocumentationComment(typeChecker);
tags = signature.getJsDocTags();

if (allSignatures.length > 1 && documentation.length === 0 && tags.length === 0) {
documentation = allSignatures[0].getDocumentationComment(typeChecker);
tags = allSignatures[0].getJsDocTags();
}
}

function writeTypeParametersOfSymbol(symbol: Symbol, enclosingDeclaration: Node | undefined) {
Expand Down
30 changes: 15 additions & 15 deletions tests/cases/fourslash/commentsOverloads.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,11 +237,11 @@ verify.signatureHelp({ marker: "4", overloadsCount: 2 });
verify.signatureHelp({ marker: "o4", overloadsCount: 2, docComment: "this is signature 1", parameterDocComment: "param a" });

verify.quickInfos({
5: ["function f2(a: number): number (+1 overload)", "this is signature 2\nthis is f2 var comment"],
5: "function f2(a: number): number (+1 overload)",
6: ["function f2(b: string): number (+1 overload)", "this is signature 2"],
7: ["function f2(a: number): number (+1 overload)", "this is signature 2\nthis is f2 var comment"],
7: "function f2(a: number): number (+1 overload)",
"8q": ["function f2(b: string): number (+1 overload)", "this is signature 2"],
o8q: ["function f2(a: number): number (+1 overload)", "this is signature 2\nthis is f2 var comment"],
o8q: "function f2(a: number): number (+1 overload)",
});

verify.signatureHelp(
Expand Down Expand Up @@ -277,15 +277,15 @@ verify.completions(
marker: "17",
includes: [
{ name: "f1", text: "function f1(a: number): number (+1 overload)", documentation: "this is signature 1" },
{ name: "f2", text: "function f2(a: number): number (+1 overload)", documentation: "this is signature 2\nthis is f2 var comment" },
{ name: "f2", text: "function f2(a: number): number (+1 overload)" },
{ name: "f3", text: "function f3(a: number): number (+1 overload)" },
{ name: "f4", text: "function f4(a: number): number (+1 overload)", documentation: "this is signature 4 - with number parameter" },
],
},
{
marker: "18",
includes: [
{ name: "i1_i", text: "var i1_i: i1\nnew (b: number) => any (+1 overload)" },
{ name: "i1_i", text: "var i1_i: i1\nnew (b: number) => any (+1 overload)", documentation: "new 1" },
Copy link
Member

Choose a reason for hiding this comment

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

this documentation is supposed to be on new (a: string), not new (b: number).

Copy link
Contributor Author

@jeanp413 jeanp413 Jan 10, 2020

Choose a reason for hiding this comment

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

I took a look at this and it's working as intended following:

the jsdoc for the first signature of the overload should be applied to all others that do not themselves have jsdoc

Is this an exception to the above rule?
Should this exception only be applied when useConstructSignatures === true?

if (useConstructSignatures) {
displayParts.push(keywordPart(SyntaxKind.NewKeyword));
displayParts.push(spacePart());
}
addSignatureDisplayParts(signature, allSignatures, TypeFormatFlags.WriteArrowStyleSignature);

Copy link
Member

Choose a reason for hiding this comment

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

No, I just missed that rule.

{ name: "i2_i", text: "var i2_i: i2\nnew (a: string) => any (+1 overload)" },
{ name: "i3_i", text: "var i3_i: i3\nnew (a: string) => any (+1 overload)", documentation: "new 1" },
{ name: "i4_i", text: "var i4_i: i4\nnew (a: string) => any (+1 overload)" },
Expand All @@ -295,7 +295,7 @@ verify.completions(
);

verify.signatureHelp({ marker: "19", overloadsCount: 2 });
verify.quickInfoAt("19q", "var i1_i: i1\nnew (b: number) => any (+1 overload)");
verify.quickInfoAt("19q", "var i1_i: i1\nnew (b: number) => any (+1 overload)", "new 1");

verify.signatureHelp({ marker: "20", overloadsCount: 2, docComment: "new 1" });
verify.quickInfoAt("20q", "var i1_i: i1\nnew (a: string) => any (+1 overload)", "new 1");
Expand All @@ -311,7 +311,7 @@ verify.completions({
marker: "23",
includes: [
{ name: "foo" , text: "(method) i1.foo(a: number): number (+1 overload)", documentation: "foo 1" },
{ name: "foo2", text: "(method) i1.foo2(a: number): number (+1 overload)", documentation: "foo2 2" },
{ name: "foo2", text: "(method) i1.foo2(a: number): number (+1 overload)" },
{ name: "foo3", text: "(method) i1.foo3(a: number): number (+1 overload)" },
{ name: "foo4", text: "(method) i1.foo4(a: number): number (+1 overload)", documentation: "foo4 1" },
],
Expand All @@ -324,7 +324,7 @@ verify.signatureHelp({ marker: "25", overloadsCount: 2, docComment: "foo 2" });
verify.quickInfoAt("25q", "(method) i1.foo(b: string): number (+1 overload)", "foo 2");

verify.signatureHelp({ marker: "26", overloadsCount: 2 });
verify.quickInfoAt("26q", "(method) i1.foo2(a: number): number (+1 overload)", "foo2 2");
verify.quickInfoAt("26q", "(method) i1.foo2(a: number): number (+1 overload)");

verify.signatureHelp({ marker: "27", overloadsCount: 2, docComment: "foo2 2" });
verify.quickInfoAt("27q", "(method) i1.foo2(b: string): number (+1 overload)", "foo2 2");
Expand Down Expand Up @@ -363,7 +363,7 @@ verify.signatureHelp({ marker: "38", overloadsCount: 2, docComment: "this is sig
verify.quickInfoAt("38q", "var i3_i: i3\n(a: number) => number (+1 overload)", "this is signature 1");

verify.signatureHelp({ marker: "39", overloadsCount: 2 });
verify.quickInfoAt("39q", "var i3_i: i3\n(b: string) => number (+1 overload)");
verify.quickInfoAt("39q", "var i3_i: i3\n(b: string) => number (+1 overload)", "this is signature 1");
Copy link
Member

Choose a reason for hiding this comment

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

same: this documentation is supposed to be on (a: number) not (b: string).


verify.signatureHelp({ marker: "40", overloadsCount: 2 });
verify.quickInfoAt("40q", "var i4_i: i4\nnew (b: number) => any (+1 overload)");
Expand All @@ -382,7 +382,7 @@ verify.completions({
exact: [
{ name: "prop1", text: "(method) c.prop1(a: number): number (+1 overload)" },
{ name: "prop2", text: "(method) c.prop2(a: number): number (+1 overload)", documentation: "prop2 1" },
{ name: "prop3", text: "(method) c.prop3(a: number): number (+1 overload)", documentation: "prop3 2" },
{ name: "prop3", text: "(method) c.prop3(a: number): number (+1 overload)" },
{ name: "prop4", text: "(method) c.prop4(a: number): number (+1 overload)", documentation: "prop4 1" },
{ name: "prop5", text: "(method) c.prop5(a: number): number (+1 overload)", documentation: "prop5 1" },
],
Expand All @@ -401,7 +401,7 @@ verify.signatureHelp({ marker: "48", overloadsCount: 2 });
verify.quickInfoAt("48q", "(method) c.prop2(b: string): number (+1 overload)", "prop2 1");

verify.signatureHelp({ marker: "49", overloadsCount: 2 });
verify.quickInfoAt("49q", "(method) c.prop3(a: number): number (+1 overload)", "prop3 2");
verify.quickInfoAt("49q", "(method) c.prop3(a: number): number (+1 overload)");

verify.signatureHelp({ marker: "50", overloadsCount: 2, docComment: "prop3 2" });
verify.quickInfoAt("50q", "(method) c.prop3(b: string): number (+1 overload)", "prop3 2");
Expand All @@ -428,7 +428,7 @@ verify.signatureHelp({ marker: "57", overloadsCount: 2, docComment: "c2 1" });
verify.quickInfoAt("57q", "constructor c2(a: number): c2 (+1 overload)", "c2 1");

verify.signatureHelp({ marker: "58", overloadsCount: 2 });
verify.quickInfoAt("58q", "constructor c2(b: string): c2 (+1 overload)");
verify.quickInfoAt("58q", "constructor c2(b: string): c2 (+1 overload)", "c2 1");
Copy link
Member

Choose a reason for hiding this comment

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

same: (a: number) is the one with jsdoc, not (b: string)


verify.signatureHelp({ marker: "59", overloadsCount: 2 });
verify.quickInfoAt("59q", "constructor c3(a: number): c3 (+1 overload)");
Expand Down Expand Up @@ -490,7 +490,7 @@ verify.quickInfos({
79: "constructor c1(b: string): c1 (+1 overload)",
80: "constructor c1(a: number): c1 (+1 overload)",
81: ["constructor c2(a: number): c2 (+1 overload)", "c2 1"],
82: "constructor c2(b: string): c2 (+1 overload)",
82: ["constructor c2(b: string): c2 (+1 overload)", "c2 1"],
83: ["constructor c2(a: number): c2 (+1 overload)", "c2 1"],
84: "constructor c3(a: number): c3 (+1 overload)",
85: ["constructor c3(b: string): c3 (+1 overload)", "c3 2"],
Expand All @@ -507,9 +507,9 @@ verify.quickInfos({
96: ["(method) c.prop2(a: number): number (+1 overload)", "prop2 1"],
97: ["(method) c.prop2(b: string): number (+1 overload)", "prop2 1"],
98: ["(method) c.prop2(a: number): number (+1 overload)", "prop2 1"],
99: ["(method) c.prop3(a: number): number (+1 overload)", "prop3 2"],
99: "(method) c.prop3(a: number): number (+1 overload)",
100: ["(method) c.prop3(b: string): number (+1 overload)", "prop3 2"],
101: ["(method) c.prop3(a: number): number (+1 overload)", "prop3 2"],
101: "(method) c.prop3(a: number): number (+1 overload)",
102: ["(method) c.prop4(a: number): number (+1 overload)", "prop4 1"],
103: ["(method) c.prop4(b: string): number (+1 overload)", "prop4 2"],
104: ["(method) c.prop4(a: number): number (+1 overload)", "prop4 1"],
Expand Down
66 changes: 66 additions & 0 deletions tests/cases/fourslash/quickInfoJsDocTagsFunctionOverload01.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/// <reference path='fourslash.ts'/>

// @Filename: quickInfoJsDocTagsFunctionOverload01.ts
/////**
//// * Doc foo
//// */
////declare function /*1*/foo(): void;
////
/////**
//// * Doc foo overloaded
//// * @tag Tag text
//// */
////declare function /*2*/foo(x: number): void

goTo.marker("1");
verify.verifyQuickInfoDisplayParts(
"function",
"declare",
{ start: 36, length: 3 },
[
{text:"function",kind:"keyword"},
{text:" ",kind:"space"},
{text:"foo",kind:"functionName"},
{text:"(",kind:"punctuation"},
{text:")",kind:"punctuation"},
{text:":",kind:"punctuation"},
{text:" ",kind:"space"},
{text:"void",kind:"keyword"},
{text:" ",kind:"space"},
{text:"(",kind:"punctuation"},
{text:"+",kind:"operator"},
{text:"1",kind:"numericLiteral"},
{text:" ",kind:"space"},
{text:"overload",kind:"text"},
{text:")",kind:"punctuation"}
],
[{ text: "Doc foo", kind: "text" }]);

goTo.marker("2");
verify.verifyQuickInfoDisplayParts(
"function",
"declare",
{ start: 114, length: 3 },
[
{text:"function",kind:"keyword"},
{text:" ",kind:"space"},
{text:"foo",kind:"functionName"},
{text:"(",kind:"punctuation"},
{text:"x",kind:"parameterName"},
{text:":",kind:"punctuation"},
{text:" ",kind:"space"},
{text:"number",kind:"keyword"},
{text:")",kind:"punctuation"},
{text:":",kind:"punctuation"},
{text:" ",kind:"space"},
{text:"void",kind:"keyword"},
{text:" ",kind:"space"},
{text:"(",kind:"punctuation"},
{text:"+",kind:"operator"},
{text:"1",kind:"numericLiteral"},
{text:" ",kind:"space"},
{text:"overload",kind:"text"},
{text:")",kind:"punctuation"}
],
[{ text: "Doc foo overloaded", kind: "text" }],
[{ name: "tag", text: "Tag text" }]);
64 changes: 64 additions & 0 deletions tests/cases/fourslash/quickInfoJsDocTagsFunctionOverload02.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/// <reference path='fourslash.ts'/>

// @Filename: quickInfoJsDocTagsFunctionOverload02.ts
/////**
//// * Doc foo
//// */
////declare function /*1*/foo(): void;
////
/////**
//// * Doc foo overloaded
//// */
////declare function /*2*/foo(x: number): void

goTo.marker("1");
verify.verifyQuickInfoDisplayParts(
"function",
"declare",
{ start: 36, length: 3 },
[
{text:"function",kind:"keyword"},
{text:" ",kind:"space"},
{text:"foo",kind:"functionName"},
{text:"(",kind:"punctuation"},
{text:")",kind:"punctuation"},
{text:":",kind:"punctuation"},
{text:" ",kind:"space"},
{text:"void",kind:"keyword"},
{text:" ",kind:"space"},
{text:"(",kind:"punctuation"},
{text:"+",kind:"operator"},
{text:"1",kind:"numericLiteral"},
{text:" ",kind:"space"},
{text:"overload",kind:"text"},
{text:")",kind:"punctuation"}
],
[{ text: "Doc foo", kind: "text" }]);

goTo.marker("2");
verify.verifyQuickInfoDisplayParts(
"function",
"declare",
{ start: 97, length: 3 },
[
{text:"function",kind:"keyword"},
{text:" ",kind:"space"},
{text:"foo",kind:"functionName"},
{text:"(",kind:"punctuation"},
{text:"x",kind:"parameterName"},
{text:":",kind:"punctuation"},
{text:" ",kind:"space"},
{text:"number",kind:"keyword"},
{text:")",kind:"punctuation"},
{text:":",kind:"punctuation"},
{text:" ",kind:"space"},
{text:"void",kind:"keyword"},
{text:" ",kind:"space"},
{text:"(",kind:"punctuation"},
{text:"+",kind:"operator"},
{text:"1",kind:"numericLiteral"},
{text:" ",kind:"space"},
{text:"overload",kind:"text"},
{text:")",kind:"punctuation"}
],
[{ text: "Doc foo overloaded", kind: "text" }]);
Loading