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

jsdoc/require-param adds missing parameters multiple times #770

Open
RunDevelopment opened this issue Jul 18, 2021 · 10 comments
Open

jsdoc/require-param adds missing parameters multiple times #770

RunDevelopment opened this issue Jul 18, 2021 · 10 comments

Comments

@RunDevelopment
Copy link

RunDevelopment commented Jul 18, 2021

Expected behavior

The jsdoc/require-param should add exactly one new @param tag - one for each missing parameter.

Actual behavior

For each missing parameter, it adds all missing parameters. For n missing parameters, it will add n*n @param tags.

ESLint Config

/** @type {import("eslint").Linter.Config} */
module.exports = {
	env: {
		browser: true,
		es6: true
	},
	root: true,
	parser: "@typescript-eslint/parser",
	plugins: [
		"@typescript-eslint",
		"jsdoc"
	],
	parserOptions: {
		ecmaVersion: 2018,
		sourceType: "module",
		ecmaFeatures: {
			node: true,
			spread: true
		}
	},
	rules: {
		"jsdoc/require-param": "error"
	},
	settings: {
		jsdoc: {
			mode: "typescript"
		}
	},
}

ESLint sample

/**
 * Foo.
 */
function foo(a: number, b: number, c:number):void {
	throw new Error();
}

Output:

/**
 * Foo.
 * @param a
 * @param b
 * @param c
 * @param a
 * @param b
 * @param c
 * @param a
 * @param b
 * @param c
 */
function foo(a: number, b: number, c:number):void {
	throw new Error();
}

Environment

  • Node version: v14.15.4
  • ESLint version v7.26.0
  • eslint-plugin-jsdoc version: 35.4.1

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@brettz9
Copy link
Collaborator

brettz9 commented Jul 18, 2021

I have run your exact config locally, and only see 3 params being added, as expected.

What OS are you on?

You might want to prepare a small repo to reproduce in case there are any subtle differences.

@RunDevelopment
Copy link
Author

What OS are you on?

Win10

You might want to prepare a small repo to reproduce in case there are any subtle differences.

Here is a small example project.

However, I found something quite interesting. The main function in the example is

/**
 * Foo.
 */
function foo(a, b, c) {}

If I run npx eslint .\index.js --fix, ESLint will produce

/**
 * Foo.
 * @param a
 * @param b
 * @param c
 */
function foo(a, b, c) {}

which is correct. However, when using the ESLint extension in VSCode, I get

/**
 * Foo.
 * @param a
 * @param b
 * @param c
 * @param a
 * @param b
 * @param c
 * @param a
 * @param b
 * @param c
 */
function foo(a, b, c) {}

So it seems like this plugin is interacting poorly with the extension.

It seems like there is a subtle difference in how ESLint and the ESLint extension apply fixes.

Could you please provide me information on how exactly your plugin does fixes, so I can make an accurate bug report on their issue page? Alternatively, you could also make the report.

@brettz9
Copy link
Collaborator

brettz9 commented Jul 19, 2021

Thanks for the update and info. The one difference I can think of that might possibly be of relevance is that this rule does fix multiple lines at once.

@RunDevelopment
Copy link
Author

Q: What exactly is each fix supposed to do?

I get reports like this:

  1:1  error  Missing JSDoc @param "a" declaration  jsdoc/require-param
  1:1  error  Missing JSDoc @param "b" declaration  jsdoc/require-param
  1:1  error  Missing JSDoc @param "c" declaration  jsdoc/require-param

What exactly is the fix for let's say report Missing JSDoc @param "a" declaration supposed to do? Does it only add an @param a?

I'm asking because the fixer logic for require-param is complex and recursive.

Let's say ESLint runs the fix function for both parameters a and b. Would those fixes influence each other?

@brettz9
Copy link
Collaborator

brettz9 commented Jul 20, 2021

We actually have a goal to do so with more fixers.

This I see was justified based on this comment, i.e., that by recursively fixing things ourselves, we get the advantage that both our tests, and the docs which are based on the tests, do not indicate a confusing result that makes it appear that only one fix will be made.

This problem might be mitigated by using what I recall are VSC's ability to delay fixes (i.e., to run on save or on type), but this is not an ideal solution so hopefully it can be solved (which I think ought to be doable on their end). I don't think this is really a problem on our end, though do please let us know the results of any inquiry.

@RunDevelopment
Copy link
Author

Let's see if I understand this correctly:

The report of each one parameter will fix all parameters (e.g. the report for "a" will fix "a", "b", and "c"). You did this because ESLint will only do one pass in tests and documentation, so it would look strange for some fixes not to be applied (because of conflicts).

It's definitely a strange solution. I would even say that the reports are misleading. The fix of one report should only fix the reported problem.

How about you only do one report for all missing parameters? That would solve fixer conflicts and the IDE problem, and make an accurate report. I imagine an error like this:

  1:1  error  Missing JSDoc @param declaration for parameter(s) "a", "b", "c"  jsdoc/require-param

@brettz9
Copy link
Collaborator

brettz9 commented Jul 20, 2021

Let's see if I understand this correctly:

The report of each one parameter will fix all parameters (e.g. the report for "a" will fix "a", "b", and "c"). You did this because ESLint will only do one pass in tests and documentation, so it would look strange for some fixes not to be applied (because of conflicts).

Actually, it is sort of the opposite. Oftentimes rules only fix and report one item, and ESLint fixes them recursively until there are no more errors.

We, however, report multiple times. ESLint has no problem with this (nor do the docs suggest there is anything wrong with this), and ESLint applies the fixes we report.

But as mentioned, ESLint's rule tester (which we use) expects only one change to be made in the resulting output field. If our example has 3 params, it will therefore only show one param being fixed. This is strange both for someone looking at the test files, and is especially confusing for regular users looking at our docs which extract the tests.

It's definitely a strange solution. I would even say that the reports are misleading. The fix of one report should only fix the reported problem.

We report multiple times as ESLint allows, and ESLint fixes them.

How about you only do one report for all missing parameters? That would solve fixer conflicts and the IDE problem, and make an accurate report. I imagine an error like this:

  1:1  error  Missing JSDoc @param declaration for parameter(s) "a", "b", "c"  jsdoc/require-param

I think the bigger problem is that VSC shouldn't be choking on this. I expect it may be queuing multiple executions but applying fixes without using the fully updated code in the next run.

@RunDevelopment
Copy link
Author

But as mentioned, ESLint's rule tester (which we use) expects only one change to be made in the resulting output field. If our example has 3 params, it will therefore only show one param being fixed.

That's not the case. ESLint's RuleTester uses the normal Linter class under the hood. That linter will apply as many fixes as it can (be it 1, 2, 3, or more). The reason why it only fixes one parameter is that the fixes for different parameters conflict (same/overlapping/adjacent region in source code).

The reason why ESLint's RuleTester doesn't apply conflicting fixes is that it only does one linting pass, while ESLint CLI does 10 (by default).

It's definitely a strange solution. I would even say that the reports are misleading. The fix of one report should only fix the reported problem.

We report multiple times as ESLint allows, and ESLint fixes them.

I have no problem with you reporting multiple times. I have a problem with each report fixing all reports. This is unexpected. Nobody would expect that a report called Missing JSDoc @param "a" declaration will add 3 @param tags. This is what I meant by "misleading".

I think the bigger problem is that VSC shouldn't be choking on this.

Definitely.

@brettz9
Copy link
Collaborator

brettz9 commented Jul 20, 2021

But as mentioned, ESLint's rule tester (which we use) expects only one change to be made in the resulting output field. If our example has 3 params, it will therefore only show one param being fixed.

That's not the case. ESLint's RuleTester uses the normal Linter class under the hood. That linter will apply as many fixes as it can (be it 1, 2, 3, or more). The reason why it only fixes one parameter is that the fixes for different parameters conflict (same/overlapping/adjacent region in source code).

The reason why ESLint's RuleTester doesn't apply conflicting fixes is that it only does one linting pass, while ESLint CLI does 10 (by default).

It's definitely a strange solution. I would even say that the reports are misleading. The fix of one report should only fix the reported problem.

We report multiple times as ESLint allows, and ESLint fixes them.

I have no problem with you reporting multiple times. I have a problem with each report fixing all reports. This is unexpected. Nobody would expect that a report called Missing JSDoc @param "a" declaration will add 3 @param tags. This is what I meant by "misleading".

Ah, ok, thanks for the extra info. I'll hopefully be able to take a closer look later as I may have some energy.

@brettz9
Copy link
Collaborator

brettz9 commented Jul 21, 2021

Unfortunately, I don't see any quick fix to this which still allows us to avoid the single-fix-only docs problem. Because, as you say, they are overlapping, it seems we felt the need to apply these adjacent fixes together.

Also, though we're admittedly not currently reporting the precise line numbers, but one of the ideas was to be able to get precise line numbers in addition to the itemized listing.

That being said, I think we'd probably accept a PR to fix this, given VSC's popularity (if we at least see if they may be able to do anything on their end). We should really be overhauling this code anyways, ideally to also address #540 and #762 . Due to other commitments and other reasons, I've pretty much been confining myself to low-hanging fruit with the project, so don't think I'm up for taking this on myself at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants