Fix validation.notDocumentated signature logic

Gerrit0 · Gerrit0 · commit 98841f5b6c79 · 2022-03-26T16:15:29.000-06:00
Resolves #1895
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,7 +3,9 @@
 ### Bug Fixes
 
 -   Fixed missing comments on `@enum` style enum members defined in declaration files, #1880.
+-   Fixed `--validation.notDocumented` warnings for functions/methods, #1895.
 -   Search results will no longer include random items when the search bar is empty, #1881.
+-   Comments on overloaded constructors will now be detected in the same way that overloaded functions/methods are.
 
 ### Thanks!
 
diff --git a/src/lib/converter/plugins/CommentPlugin.ts b/src/lib/converter/plugins/CommentPlugin.ts
@@ -175,7 +175,11 @@ export class CommentPlugin extends ConverterComponent {
         reflection: Reflection,
         node?: ts.Node
     ) {
-        if (reflection.kindOf(ReflectionKind.FunctionOrMethod)) {
+        if (
+            reflection.kindOf(
+                ReflectionKind.FunctionOrMethod | ReflectionKind.Constructor
+            )
+        ) {
             // We only want a comment on functions/methods if this is a set of overloaded functions.
             // In that case, TypeDoc lets you put a comment on the implementation, and will copy it over to
             // the available signatures so that you can avoid documenting things multiple times.
@@ -184,15 +188,18 @@ export class CommentPlugin extends ConverterComponent {
             let specialOverloadCase = false;
             if (
                 node &&
-                (ts.isFunctionDeclaration(node) || ts.isMethodDeclaration(node))
+                (ts.isFunctionDeclaration(node) ||
+                    ts.isMethodDeclaration(node) ||
+                    ts.isConstructorDeclaration(node))
             ) {
                 const symbol =
                     node.name && context.checker.getSymbolAtLocation(node.name);
                 if (symbol && symbol.declarations) {
                     const declarations = symbol.declarations.filter(
                         (d) =>
                             ts.isFunctionDeclaration(d) ||
-                            ts.isMethodDeclaration(d)
+                            ts.isMethodDeclaration(d) ||
+                            ts.isConstructorDeclaration(d)
                     );
                     if (
                         declarations.length > 1 &&
diff --git a/src/lib/converter/symbols.ts b/src/lib/converter/symbols.ts
@@ -509,10 +509,12 @@ function convertClassOrInterface(
             reflection
         );
         reflectionContext.addChild(constructMember);
-        // The symbol is already taken by the class.
-        context.registerReflection(constructMember, undefined);
 
         const ctors = staticType.getConstructSignatures();
+        context.registerReflection(
+            constructMember,
+            ctors?.[0]?.declaration?.symbol
+        );
 
         // Modifiers are the same for all constructors
         if (ctors.length && ctors[0].declaration) {
diff --git a/src/lib/models/reflections/abstract.ts b/src/lib/models/reflections/abstract.ts
@@ -364,7 +364,8 @@ export abstract class Reflection {
     }
 
     /**
-     * Return the full name of this reflection.
+     * Return the full name of this reflection. Intended for use in debugging. For log messages
+     * intended to be displayed to the user for them to fix, prefer {@link getFriendlyFullName} instead.
      *
      * The full name contains the name of this reflection and the names of all parent reflections.
      *
@@ -379,6 +380,29 @@ export abstract class Reflection {
         }
     }
 
+    /**
+     * Return the full name of this reflection, with signature names dropped if possible without
+     * introducing ambiguity in the name.
+     */
+    getFriendlyFullName(): string {
+        if (this.parent && !this.parent.isProject()) {
+            if (
+                this.kindOf(
+                    ReflectionKind.ConstructorSignature |
+                        ReflectionKind.CallSignature |
+                        ReflectionKind.GetSignature |
+                        ReflectionKind.SetSignature
+                )
+            ) {
+                return this.parent.getFriendlyFullName();
+            }
+
+            return this.parent.getFriendlyFullName() + "." + this.name;
+        } else {
+            return this.name;
+        }
+    }
+
     /**
      * Set a flag on this reflection.
      */
diff --git a/src/lib/output/plugins/MarkedLinksPlugin.ts b/src/lib/output/plugins/MarkedLinksPlugin.ts
@@ -135,7 +135,7 @@ export class MarkedLinksPlugin extends ContextAwareRendererComponent {
                 }
             } else {
                 const fullName = (this.reflection ||
-                    this.project)!.getFullName();
+                    this.project)!.getFriendlyFullName();
                 this.warnings.push(`In ${fullName}: ${original}`);
                 return original;
             }
diff --git a/src/lib/utils/options/sources/typedoc.ts b/src/lib/utils/options/sources/typedoc.ts
@@ -372,8 +372,7 @@ export function addTypeDocOptions(options: Pick<Options, "addDeclaration">) {
             "Interface",
             "Property",
             "Method",
-            "GetSignature",
-            "SetSignature",
+            "Accessor",
             "TypeAlias",
         ],
     });
diff --git a/src/lib/validation/documentation.ts b/src/lib/validation/documentation.ts
@@ -1,23 +1,66 @@
 import * as path from "path";
 import * as ts from "typescript";
-import { ProjectReflection, ReflectionKind } from "../models";
+import {
+    DeclarationReflection,
+    ProjectReflection,
+    ReflectionKind,
+    SignatureReflection,
+} from "../models";
 import { Logger, normalizePath } from "../utils";
+import { removeFlag } from "../utils/enum";
 
 export function validateDocumentation(
     project: ProjectReflection,
     logger: Logger,
     requiredToBeDocumented: readonly (keyof typeof ReflectionKind)[]
 ): void {
-    const kinds = requiredToBeDocumented.reduce(
-        (prev, cur) => (prev |= ReflectionKind[cur]),
+    let kinds = requiredToBeDocumented.reduce(
+        (prev, cur) => prev | ReflectionKind[cur],
         0
     );
 
+    // Functions, Constructors, and Accessors never have comments directly on them.
+    // If they are required to be documented, what's really required is that their
+    // contained signatures have a comment.
+    if (kinds & ReflectionKind.FunctionOrMethod) {
+        kinds |= ReflectionKind.CallSignature;
+        kinds = removeFlag(kinds, ReflectionKind.FunctionOrMethod);
+    }
+    if (kinds & ReflectionKind.Constructor) {
+        kinds |= ReflectionKind.ConstructorSignature;
+        kinds = removeFlag(kinds, ReflectionKind.Constructor);
+    }
+    if (kinds & ReflectionKind.Accessor) {
+        kinds |= ReflectionKind.GetSignature | ReflectionKind.SetSignature;
+        kinds = removeFlag(kinds, ReflectionKind.Accessor);
+    }
+
     for (const ref of project.getReflectionsByKind(kinds)) {
-        const symbol = project.getSymbolFromReflection(ref);
-        if (!ref.comment && symbol?.declarations) {
-            const decl = symbol.declarations[0];
+        let symbol = project.getSymbolFromReflection(ref);
+        let index = 0;
+
+        // Signatures don't have symbols associated with them, so get the parent and then
+        // maybe also adjust the declaration index that we care about.
+        if (!symbol && ref.kindOf(ReflectionKind.SomeSignature)) {
+            symbol = project.getSymbolFromReflection(ref.parent!);
+
+            const parentIndex = (
+                ref.parent as DeclarationReflection
+            ).signatures?.indexOf(ref as SignatureReflection);
+            if (parentIndex) {
+                index = parentIndex;
+            }
+        }
+
+        const decl = symbol?.declarations?.[index];
+
+        if (!ref.hasComment() && decl) {
             const sourceFile = decl.getSourceFile();
+
+            if (sourceFile.fileName.includes("node_modules")) {
+                continue;
+            }
+
             const { line } = ts.getLineAndCharacterOfPosition(
                 sourceFile,
                 decl.getStart()
@@ -26,13 +69,9 @@ export function validateDocumentation(
                 path.relative(process.cwd(), sourceFile.fileName)
             );
 
-            if (file.includes("node_modules")) {
-                continue;
-            }
-
             const loc = `${file}:${line + 1}`;
             logger.warn(
-                `${ref.name}, defined at ${loc}, does not have any documentation.`
+                `${ref.getFriendlyFullName()}, defined at ${loc}, does not have any documentation.`
             );
         }
     }
diff --git a/src/test/TestLogger.ts b/src/test/TestLogger.ts
@@ -0,0 +1,32 @@
+import { Logger, LogLevel } from "../lib/utils";
+import { deepStrictEqual as equal, fail } from "assert";
+
+const levelMap: Record<LogLevel, string> = {
+    [LogLevel.Error]: "error: ",
+    [LogLevel.Warn]: "warn: ",
+    [LogLevel.Info]: "info: ",
+    [LogLevel.Verbose]: "debug: ",
+};
+
+export class TestLogger extends Logger {
+    messages: string[] = [];
+
+    expectMessage(message: string) {
+        const index = this.messages.indexOf(message);
+        if (index === -1) {
+            const messages = this.messages.join("\n\t") || "(none logged)";
+            fail(
+                `Expected "${message}" to be logged. The logged messages were:\n\t${messages}`
+            );
+        }
+        this.messages.splice(index, 1);
+    }
+
+    expectNoOtherMessages() {
+        equal(this.messages, [], "Expected no other messages to be logged.");
+    }
+
+    override log(message: string, level: LogLevel): void {
+        this.messages.push(levelMap[level] + message);
+    }
+}
diff --git a/src/test/converter/exports/mod.ts b/src/test/converter/exports/mod.ts
@@ -20,9 +20,7 @@ export { a as c } from "./mod";
 /**
  * Will not be re-exported from export.ts using export * from...
  */
-export default function () {
-    console.log("Default");
-}
+export default function () {}
 
 /**
  * This is annotated with the hidden tag and will therefore not be included in the generated documentation.
diff --git a/src/test/converter2/issues/gh1547.ts b/src/test/converter2/issues/gh1547.ts
@@ -19,7 +19,5 @@ export class Test {
      *
      * @param things - Array of things or a thing.
      */
-    log_thing(things: ValueOrArray<Things>): void {
-        console.log(things);
-    }
+    log_thing(things: ValueOrArray<Things>): void {}
 }
diff --git a/src/test/converter2/issues/gh1580.ts b/src/test/converter2/issues/gh1580.ts
@@ -5,16 +5,13 @@ class A {
     prop!: string;
 
     /** Run docs */
-    run(): void {
-        console.log("A");
-    }
+    run(): void {}
 }
 
 class B extends A {
     declare prop: "B";
 
     run(): void {
         super.run();
-        console.log("B");
     }
 }
diff --git a/src/test/converter2/validation/class.ts b/src/test/converter2/validation/class.ts
@@ -0,0 +1,8 @@
+export class Foo {
+    /** Comment */
+    constructor();
+    constructor(x: string);
+    constructor(x?: string) {}
+}
+
+export class Bar {}
diff --git a/src/test/converter2/validation/function.ts b/src/test/converter2/validation/function.ts
@@ -0,0 +1,7 @@
+/** Foo docs */
+export function foo() {}
+
+export function bar();
+/** Bar docs */
+export function bar(x: string);
+export function bar() {}
diff --git a/src/test/validation.test.ts b/src/test/validation.test.ts
@@ -1,15 +1,12 @@
 import { equal, fail, ok } from "assert";
 import { join, relative } from "path";
 import { Logger, LogLevel, normalizePath } from "..";
+import { validateDocumentation } from "../lib/validation/documentation";
 import { validateExports } from "../lib/validation/exports";
 import { getConverter2App, getConverter2Program } from "./programs";
+import { TestLogger } from "./TestLogger";
 
-function expectWarning(
-    typeName: string,
-    file: string,
-    referencingName: string,
-    intentionallyNotExported: readonly string[] = []
-) {
+function convertValidationFile(file: string) {
     const app = getConverter2App();
     const program = getConverter2Program();
     const sourceFile = program.getSourceFile(
@@ -26,6 +23,17 @@ function expectWarning(
         },
     ]);
 
+    return project;
+}
+
+function expectWarning(
+    typeName: string,
+    file: string,
+    referencingName: string,
+    intentionallyNotExported: readonly string[] = []
+) {
+    const project = convertValidationFile(file);
+
     let sawWarning = false;
     const regex =
         /(.*?), defined at (.*?):\d+, is referenced by (.*?) but not included in the documentation\./;
@@ -186,3 +194,38 @@ describe("validateExports", () => {
         ok(sawWarning, "Never saw warning.");
     });
 });
+
+describe("validateDocumentation", () => {
+    it("Should correctly handle functions", () => {
+        const project = convertValidationFile("function.ts");
+        const logger = new TestLogger();
+        validateDocumentation(project, logger, ["Function"]);
+
+        logger.expectMessage(
+            "warn: bar, defined at src/test/converter2/validation/function.ts:4, does not have any documentation."
+        );
+        logger.expectNoOtherMessages();
+    });
+
+    it("Should correctly handle accessors", () => {
+        const project = convertValidationFile("getSignature.ts");
+        const logger = new TestLogger();
+        validateDocumentation(project, logger, ["Accessor"]);
+
+        logger.expectMessage(
+            "warn: Foo.foo, defined at src/test/converter2/validation/getSignature.ts:2, does not have any documentation."
+        );
+        logger.expectNoOtherMessages();
+    });
+
+    it("Should correctly handle constructors", () => {
+        const project = convertValidationFile("class.ts");
+        const logger = new TestLogger();
+        validateDocumentation(project, logger, ["Constructor"]);
+
+        logger.expectMessage(
+            "warn: Foo.constructor, defined at src/test/converter2/validation/class.ts:4, does not have any documentation."
+        );
+        logger.expectNoOtherMessages();
+    });
+});

Original file line number	Diff line number	Diff line change
`@@ -135,7 +135,7 @@ export class MarkedLinksPlugin extends ContextAwareRendererComponent {`
`135`	`135`	`}`
`136`	`136`	`} else {`
`137`	`137`	`const fullName = (this.reflection \|\|`
`138`		`- this.project)!.getFullName();`
	`138`	`+ this.project)!.getFriendlyFullName();`
`139`	`139`	this.warnings.push(`In ${fullName}: ${original}`);
`140`	`140`	`return original;`
`141`	`141`	`}`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,5 @@ export class Test {`
`19`	`19`	`*`
`20`	`20`	`* @param things - Array of things or a thing.`
`21`	`21`	`*/`
`22`		`- log_thing(things: ValueOrArray<Things>): void {`
`23`		`- console.log(things);`
`24`		`- }`
	`22`	`+ log_thing(things: ValueOrArray<Things>): void {}`
`25`	`23`	`}`