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

const interpreted as function typed via an interface declaration, yield unexpected comment summaries #2521

Closed
kraenhansen opened this issue Mar 8, 2024 · 8 comments
Labels
bug Functionality does not match expectation
Milestone

Comments

@kraenhansen
Copy link
Contributor

kraenhansen commented Mar 8, 2024

Search terms

const, variable, function, interface

This seems related to #1523.

Expected Behavior

Given the following interface and two exported const (interpreted as functions because of their type declaration).

/**
 * Original comment.
 */
export interface Foo {
    /** Overload 1 */
    (): void;
    /** Overload 2 */
    (baz: number): void;
}

export const fooWithoutComment: Foo;

/** 
 * New comment.
 */
export const fooWithComment: Foo;

I'd expect

  1. fooWithoutComment to copy the comment summary and signatures (including their comment summaries) from Foo.
  2. fooWithComment to override the comment summary, with "New comment." but copy the signatures (including their comment summaries) from Foo.

Actual Behavior

These are screenshots of the resulting documentation:

Screenshot 2024-03-08 at 22 20 58 Screenshot 2024-03-08 at 22 20 40 Screenshot 2024-03-08 at 22 20 30

Notice how neither fooWithoutComment nor fooWithComment have comment summaries of their own and how all signatures of fooWithComment has all signature comments copied from the declaration instead of being copied from the comments of Foo's signatures.

Steps to reproduce the bug

I found reproducing the bug using the "typedoc-repros" repo a bit too cumbersome, so I hope it's okay I made it a PR to the "typedoc" repo itself instead: #2522

Environment

  • Typedoc version: 0.25.10
  • TypeScript version: 5.3.3
  • Node.js version: v20.10.0
  • OS: macOS 14.3.1
@kraenhansen kraenhansen added the bug Functionality does not match expectation label Mar 8, 2024
@kraenhansen
Copy link
Contributor Author

I actually spent quite some time trying to fix this myself and I do have a fix, but it breaks ~30 regression tests:

My approach involves:

  • Storing a reflection.type from convertVariableAsFunction similarly to what convertVariable does when there are missing call signatures.
  • Remove the deletion of the reflection.comment in moveCommentToSignatures of the CommentPlugin:
    delete reflection.comment;
  • Condition the creation of signature comments, only on the presence of a declaration in createSignature:
    if (
    declaration &&
    (!parentReflection.comment ||
    !(
    parentReflection.conversionFlags &
    ConversionFlags.VariableOrPropertySource
    ))
    ) {
    sigRef.comment = context.getSignatureComment(declaration);
    }

I suspect I'm violating quite a few assumptions on how the reflections from other ASTs are supposed to look.
One thing that's not really clear to me is how the comments on a ReferenceType stored in declaration.type on a DeclarationReflection does/doesn't precedence over the declaration itself.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Mar 8, 2024

This is, unfortunately, working as designed today. TypeDoc's stance historically has been that functions do not have comments, signatures do. This is at least partially because of the decision (from before me) to allow specifying comments on the implementation of overloads that would apply to any signatures that don't have a comment.

This has been due for a redesign for a long time... it's probably time to do that. TypeDoc's current function rules are so convoluted that I have to rediscover them every time I have to deal with it, even if it's only been like a week... I think your idea to allow functions to have a comment is potentially the piece that was so obviously in front of my face that I've missed it for ages.

(I actually much prefer the PR to add a failing test! It's easier for most people to just set up a project that I can clone, which the typedoc-repros project is meant to be)

@kraenhansen
Copy link
Contributor Author

kraenhansen commented Mar 9, 2024

First off. Thanks a lot for stewarding and maintaining a valuable tool for the community!

TypeDoc's current function rules are so convoluted that I have to rediscover them every time I have to deal with it, even if it's only been like a week...

I got that feeling too, when trying to wrap my head around the current implementation.

If you're interested, I'd love to:

  1. discuss the desired design / output,
  2. review any PR related to this (now that I've already spent some time familiarizing myself with the codebase)
  3. take an initial stab at implementing a redesign,
  4. sit back, relax and wait for the next version to solve all my problems 😅🤞

Feel free to pick any permutation of the above ...

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Mar 10, 2024

Thanks! I'm definitely interested in improving how this works.. unfortunately it's going to be a rather major change which will probably break >50% of TypeDoc's tests... and really needs to wait for a minor release to go with other breaking changes (0.26?)

I suspect I'm going to have to just try to make the change and see what falls out of it in order to get a sense for how it'll go... I'm getting fairly close to being done with #2475, which is the other big thing I want to get into 0.26, so hopefully will be able to look at this in more detail next/nextnext weekend

@kraenhansen
Copy link
Contributor Author

kraenhansen commented Mar 11, 2024

a rather major change which will probably break >50% of TypeDoc's tests

Yeah - it would probably be good to signal quite explicitly to developers, as they might have employed workarounds that could break (or are obsolete) after such a change.

hopefully will be able to look at this in more detail next/nextnext weekend

That sounds great. It might not be that urgent, but I believe this is an important issue to get resolved nonetheless. Let me know if / how I can help as you progress 👍

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Apr 29, 2024

Few weeks later than I wanted... but I finally spent some time looking at this today, and I think it's a promising way forward. The approach I'm taking is to formalize the difference between signature comments and symbol comments. Functions generally only have signature comments, but in the fooWithComment example, it will actually have a symbol comment too.

A very hacky attempt at this only requires three specs changes, and causes 11 tests to fail (working on the beta branch):

The logic for how to decide whether a comment is a symbol or signature comment isn't quite right yet, but I suspect once that's nailed down this ought to actually be a pretty easy update (however, getting that right is likely going to be pretty tricky)

Diff
diff --git a/src/lib/converter/comments/discovery.ts b/src/lib/converter/comments/discovery.ts
index e37a9b60..0fccce2e 100644
--- a/src/lib/converter/comments/discovery.ts
+++ b/src/lib/converter/comments/discovery.ts
@@ -190,7 +190,10 @@ export function discoverComment(
                     ts.SyntaxKind.MethodDeclaration,
                     ts.SyntaxKind.Constructor,
                 ].includes(node.kind) &&
-                !(node as ts.FunctionDeclaration).body
+                (symbol.declarations!.filter((d) =>
+                    wantedKinds[kind].includes(d.kind),
+                ).length === 1 ||
+                    !(node as ts.FunctionDeclaration).body)
             ) {
                 continue;
             }
diff --git a/src/lib/converter/plugins/CommentPlugin.ts b/src/lib/converter/plugins/CommentPlugin.ts
index 07d1343f..ca61ba5f 100644
--- a/src/lib/converter/plugins/CommentPlugin.ts
+++ b/src/lib/converter/plugins/CommentPlugin.ts
@@ -364,19 +364,19 @@ export class CommentPlugin extends ConverterComponent {
         }
 
         if (reflection.type instanceof ReflectionType) {
-            this.moveCommentToSignatures(
+            this.moveSignatureParamComments(
                 reflection,
                 reflection.type.declaration.getNonIndexSignatures(),
             );
         } else {
-            this.moveCommentToSignatures(
+            this.moveSignatureParamComments(
                 reflection,
                 reflection.getNonIndexSignatures(),
             );
         }
     }
 
-    private moveCommentToSignatures(
+    private moveSignatureParamComments(
         reflection: DeclarationReflection,
         signatures: SignatureReflection[],
     ) {
@@ -384,13 +384,9 @@ export class CommentPlugin extends ConverterComponent {
             return;
         }
 
-        const comment = reflection.kindOf(ReflectionKind.ClassOrInterface)
-            ? undefined
-            : reflection.comment;
-
         for (const signature of signatures) {
             const signatureHadOwnComment = !!signature.comment;
-            const childComment = (signature.comment ||= comment?.clone());
+            const childComment = signature.comment;
             if (!childComment) continue;
 
             signature.parameters?.forEach((parameter, index) => {
@@ -440,40 +436,17 @@ export class CommentPlugin extends ConverterComponent {
                 }
             }
 
-            this.validateParamTags(
-                signature,
-                childComment,
-                signature.parameters || [],
-                signatureHadOwnComment,
-            );
-
-            childComment?.removeTags("@param");
-            childComment?.removeTags("@typeParam");
-            childComment?.removeTags("@template");
-        }
-
-        // Since this reflection has signatures, we need to remove the comment from the non-primary
-        // declaration location. For functions/methods/constructors, this means removing it from
-        // the wrapping reflection. For type aliases, classes, and interfaces, this means removing
-        // it from the contained signatures... if it's the same as what is on the signature.
-        // This is important so that in type aliases we don't end up with a comment rendered twice.
-        if (reflection.kindOf(ReflectionKind.SignatureContainer)) {
-            delete reflection.comment;
-        } else {
-            reflection.comment?.removeTags("@param");
-            reflection.comment?.removeTags("@typeParam");
-            reflection.comment?.removeTags("@template");
+            if (signatureHadOwnComment) {
+                this.validateParamTags(
+                    signature,
+                    childComment,
+                    signature.parameters || [],
+                    signatureHadOwnComment,
+                );
 
-            const parentComment = Comment.combineDisplayParts(
-                reflection.comment?.summary,
-            );
-            for (const sig of signatures) {
-                if (
-                    Comment.combineDisplayParts(sig.comment?.summary) ===
-                    parentComment
-                ) {
-                    delete sig.comment;
-                }
+                childComment.removeTags("@param");
+                childComment.removeTags("@typeParam");
+                childComment.removeTags("@template");
             }
         }
     }

Gerrit0 added a commit that referenced this issue May 2, 2024
Gerrit0 added a commit that referenced this issue May 3, 2024
…nterface

Adding a failing test to reproduce #2521
@Gerrit0
Copy link
Collaborator

Gerrit0 commented May 3, 2024

That wasn't that bad at all, only like 3 hours to straighten out the remaining edge cases.

One comment from the test case you added:

/**
 * Original comment.
 */
export interface Foo {
    /** Overload 1 */
    (): void;
    /** Overload 2 */
    (baz: number): void;
}

export const fooWithoutComment: Foo;

In this case, fooWithoutComment should not inherit the original comment from the Foo interface. You could argue that it should, since the signatures are inherited, but doing so would imply that we should also inherit the comment from any assignments, which is infeasible. It's possible to tell TypeDoc to inherit the comment via @inheritDoc, which I think makes this a minor issue.

/** {@inheritDoc Foo} */
export const fooWithoutComment: Foo;

The signature comments are inherited because the only declaration of the signature is in the base class, so TypeDoc picks it up there. If they had declarations in the implementation, TypeDoc would look there for the comment, and wouldn't inherit one.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 16, 2024

Fixed with TypeDoc 0.26, which is releasing 2024/06/21

@Gerrit0 Gerrit0 closed this as completed Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Functionality does not match expectation
Projects
None yet
Development

No branches or pull requests

2 participants