Skip to content

Commit

Permalink
Add warnings for bad @param tags
Browse files Browse the repository at this point in the history
Resolves #2368
  • Loading branch information
Gerrit0 committed Dec 30, 2023
1 parent 1963bd1 commit 5e108b6
Show file tree
Hide file tree
Showing 6 changed files with 544 additions and 1,086 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## Features

- TypeDoc now provides warnings if a signature comment is directly specified on a signature and contains `@param` tags which do not apply, #2368.
- Extended reflection preview view for interfaces to include type parameters, #2455.
- Added special cases for converting methods which are documented as returning `this` or accepting `this` as a parameter, #2458.
Note: This will only happen if a method is declared as `method(): this`, it will not happen if the method implicitly returns `this`
Expand Down
107 changes: 78 additions & 29 deletions src/lib/converter/plugins/CommentPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
removeIfPresent,
unique,
partition,
removeIf,
} from "../../utils";
import { CategoryPlugin } from "./CategoryPlugin";

Expand Down Expand Up @@ -388,6 +389,7 @@ export class CommentPlugin extends ConverterComponent {
: reflection.comment;

for (const signature of signatures) {
const signatureHadOwnComment = !!signature.comment;
const childComment = (signature.comment ||= comment?.clone());
if (!childComment) continue;

Expand All @@ -405,7 +407,6 @@ export class CommentPlugin extends ConverterComponent {
}
}

moveNestedParamTags(childComment, parameter);
const tag = childComment.getIdentifiedTag(
parameter.name,
"@param",
Expand Down Expand Up @@ -439,6 +440,13 @@ export class CommentPlugin extends ConverterComponent {
}
}

this.validateParamTags(
signature,
childComment,
signature.parameters || [],
signatureHadOwnComment,
);

childComment?.removeTags("@param");
childComment?.removeTags("@typeParam");
childComment?.removeTags("@template");
Expand Down Expand Up @@ -590,6 +598,33 @@ export class CommentPlugin extends ConverterComponent {

return excludeCategories.some((cat) => categories.has(cat));
}

private validateParamTags(
signature: SignatureReflection,
comment: Comment,
params: ParameterReflection[],
signatureHadOwnComment: boolean,
) {
const paramTags = comment.blockTags.filter(
(tag) => tag.tag === "@param",
);

removeIf(paramTags, (tag) =>
params.some((param) => param.name === tag.name),
);

moveNestedParamTags(/* in-out */ paramTags, params);

if (signatureHadOwnComment && paramTags.length) {
for (const tag of paramTags) {
this.application.logger.warn(
`The signature ${signature.getFriendlyFullName()} has an @param with name "${
tag.name
}", which was not used.`,
);
}
}
}
}

function inTypeLiteral(refl: Reflection | undefined) {
Expand All @@ -603,37 +638,51 @@ function inTypeLiteral(refl: Reflection | undefined) {
}

// Moves tags like `@param foo.bar docs for bar` into the `bar` property of the `foo` parameter.
function moveNestedParamTags(comment: Comment, parameter: ParameterReflection) {
const visitor: Partial<TypeVisitor> = {
reflection(target) {
const tags = comment.blockTags.filter(
(t) =>
t.tag === "@param" &&
t.name?.startsWith(`${parameter.name}.`),
);
function moveNestedParamTags(
/* in-out */ paramTags: CommentTag[],
parameters: ParameterReflection[],
) {
const used = new Set<number>();

for (const param of parameters) {
const visitor: Partial<TypeVisitor> = {
reflection(target) {
const tags = paramTags.filter(
(t) => t.name?.startsWith(`${param.name}.`),
);

for (const tag of tags) {
const path = tag.name!.split(".");
path.shift();
const child = target.declaration.getChildByName(path);
for (const tag of tags) {
const path = tag.name!.split(".");
path.shift();
const child = target.declaration.getChildByName(path);

if (child && !child.comment) {
child.comment = new Comment(
Comment.cloneDisplayParts(tag.content),
);
if (child && !child.comment) {
child.comment = new Comment(
Comment.cloneDisplayParts(tag.content),
);
used.add(paramTags.indexOf(tag));
}
}
}
},
// #1876, also do this for unions/intersections.
union(u) {
u.types.forEach((t) => t.visit(visitor));
},
intersection(i) {
i.types.forEach((t) => t.visit(visitor));
},
};

parameter.type?.visit(visitor);
},
// #1876, also do this for unions/intersections.
union(u) {
u.types.forEach((t) => t.visit(visitor));
},
intersection(i) {
i.types.forEach((t) => t.visit(visitor));
},
};

param.type?.visit(visitor);
}

const toRemove = Array.from(used)
.sort((a, b) => a - b)
.reverse();

for (const index of toRemove) {
paramTags.splice(index, 1);
}
}

function movePropertyTags(comment: Comment, container: Reflection) {
Expand Down
39 changes: 39 additions & 0 deletions src/test/behavior.c2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1046,4 +1046,43 @@ describe("Behavior Tests", () => {
"this",
);
});

it("Handles renaming of destructured parameters via @param tag name inference", () => {
const project = convert("destructuredParamRenames");

const params = (name: string) =>
querySig(project, name).parameters?.map((p) => p.name);

equal(params("functionWithADestructuredParameter"), [
"destructuredParam",
]);

equal(params("functionWithADestructuredParameterAndExtraParameters"), [
"__namedParameters",
"extraParameter",
]);

equal(
params(
"functionWithADestructuredParameterAndAnExtraParamDirective",
),
["__namedParameters"],
);

const logs = [
'warn: The signature functionWithADestructuredParameterAndExtraParameters has an @param with name "destructuredParam", which was not used.',
'warn: The signature functionWithADestructuredParameterAndExtraParameters has an @param with name "destructuredParam.paramZ", which was not used.',
'warn: The signature functionWithADestructuredParameterAndExtraParameters has an @param with name "destructuredParam.paramG", which was not used.',
'warn: The signature functionWithADestructuredParameterAndExtraParameters has an @param with name "destructuredParam.paramA", which was not used.',
'warn: The signature functionWithADestructuredParameterAndAnExtraParamDirective has an @param with name "fakeParameter", which was not used.',
'warn: The signature functionWithADestructuredParameterAndAnExtraParamDirective has an @param with name "destructuredParam", which was not used.',
'warn: The signature functionWithADestructuredParameterAndAnExtraParamDirective has an @param with name "destructuredParam.paramZ", which was not used.',
'warn: The signature functionWithADestructuredParameterAndAnExtraParamDirective has an @param with name "destructuredParam.paramG", which was not used.',
'warn: The signature functionWithADestructuredParameterAndAnExtraParamDirective has an @param with name "destructuredParam.paramA", which was not used.',
];
for (const log of logs) {
logger.expectMessage(log);
}
logger.expectNoOtherMessages();
});
});
96 changes: 0 additions & 96 deletions src/test/converter/function/function.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,102 +79,6 @@ export function functionWithRest(...rest: string[]): string {
return rest.join(", ");
}

/**
* This is a function with a destructured parameter.
*
* @param destructuredParam - This is the parameter that is destructured.
* @param destructuredParam.paramZ - This is a string parameter.
* @param destructuredParam.paramG - This is a parameter flagged with any.
* This sentence is placed in the next line.
*
* @param destructuredParam.paramA
* This is a **parameter** pointing to an interface.
*
* ```
* const value:BaseClass = new BaseClass('test');
* functionWithArguments('arg', 0, value);
* ```
*
* @returns This is the return value of the function.
*/
export function functionWithADestructuredParameter({
paramZ,
paramG,
paramA,
}: {
paramZ: string;
paramG: any;
paramA: Object;
}): number {
return 0;
}

/**
* This is a function with a destructured parameter and additional undocumented parameters.
* The `@param` directives are ignored because we cannot be certain which parameter they refer to.
*
* @param destructuredParam - This is the parameter that is destructured.
* @param destructuredParam.paramZ - This is a string parameter.
* @param destructuredParam.paramG - This is a parameter flagged with any.
* This sentence is placed in the next line.
*
* @param destructuredParam.paramA
* This is a **parameter** pointing to an interface.
*
* ```
* const value:BaseClass = new BaseClass('test');
* functionWithArguments('arg', 0, value);
* ```
*
* @returns This is the return value of the function.
*/
export function functionWithADestructuredParameterAndExtraParameters(
{
paramZ,
paramG,
paramA,
}: {
paramZ: string;
paramG: any;
paramA: Object;
},
extraParameter: string,
): number {
return 0;
}

/**
* This is a function with a destructured parameter and an extra `@param` directive with no corresponding parameter.
* The `@param` directives are ignored because we cannot be certain which corresponds to the real parameter.
*
* @param fakeParameter - This directive does not have a corresponding parameter.
* @param destructuredParam - This is the parameter that is destructured.
* @param destructuredParam.paramZ - This is a string parameter.
* @param destructuredParam.paramG - This is a parameter flagged with any.
* This sentence is placed in the next line.
*
* @param destructuredParam.paramA
* This is a **parameter** pointing to an interface.
*
* ```
* const value:BaseClass = new BaseClass('test');
* functionWithArguments('arg', 0, value);
* ```
*
* @returns This is the return value of the function.
*/
export function functionWithADestructuredParameterAndAnExtraParamDirective({
paramZ,
paramG,
paramA,
}: {
paramZ: string;
paramG: any;
paramA: Object;
}): number {
return 0;
}

/**
* This is the first signature of a function with multiple signatures.
*
Expand Down
Loading

0 comments on commit 5e108b6

Please sign in to comment.