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

no-unused-vars wrong error when using imported JSDoc definition as @property type. #507

Closed
ernestostifano opened this issue Mar 23, 2020 · 13 comments
Labels

Comments

@ernestostifano
Copy link

ernestostifano commented Mar 23, 2020

eslint-plugin-jsdoc/no-undefined-types is correctly added to ESLint configuration.

import {myTypesA} from '../internal/file.js'; // ERROR
import {myTypesB} from '../internal/file.js'; // NO ERROR

/**
* @typedef newType
* @property {myTypesA.someType} someProp - Some prop.
*/

/**
* @param {newType} arg - Arg.
*/
function myFunctionA(arg) {
    return arg;
}

/**
* @param {myTypesB.someType} arg - Arg.
*/
function myFunctionB(arg) {
    return arg;
}

What did you expect to happen?

There should not be an error.
JSDoc definition is correctly imported and used even when the error is present.
The error only appears when imported type is used as a type for a @property tag.
Error does not appear is used in @param tag.

What actually happened? Please include the actual, raw output from ESLint.

/path/to/issue/file.js
  1:9  error  'fiberRegExpTypes' is defined but never used  no-unused-vars

Please see this ESLint issue for more details.

@brettz9

This comment was marked as outdated.

@brettz9 brettz9 closed this as completed Mar 24, 2020
@brettz9 brettz9 reopened this Mar 24, 2020
@brettz9

This comment was marked as outdated.

@brettz9
Copy link
Collaborator

brettz9 commented Mar 24, 2020

I've reduced the test case to this:

{
      code: `
      import {myTypesA} from '../internal/file.js'; // ERROR
      import {myTypesB} from '../internal/file.js'; // NO ERROR

      /**
      * @typedef newType
      * @property {myTypesA.someType} someProp - Some prop.
      */

      /**
      * @param {newType} arg - Arg.
      */
      function myFunctionA(arg) {
          return arg;
      }

      /**
      * @param {myTypesB.someType} arg - Arg.
      */
      function myFunctionB(arg) {
          return arg;
      }
      export {myFunctionA, myFunctionB};
      `,
      parserOptions: {
        sourceType: 'module',
      },
      rules: {
        'no-unused-vars': ['error'],
      },
    },

...and am indeed getting an error that myTypesA is unused.

(Please note that I have added the second asterisk to the /** block as jsdoc requires it (and therefore so do we. I've also added an export line for myFunctionA, and myFunctionB so they wouldn't get marked as unused as well.)

@brettz9
Copy link
Collaborator

brettz9 commented Mar 24, 2020

The issue seems to be, not with it being on @property vs. @param but due to it not being attached to a regular context like a function and instead to a @typedef comment block not above any function or such context.

The strange thing is that I see that myTypesA is in fact found and added to the line context.markVariableAsUsed(name) within the no-undefined-types rule, but it seems not to be registered with eslint for some reason.

@brettz9
Copy link
Collaborator

brettz9 commented Mar 24, 2020

This looks like it may be related to eslint/eslint#9810 .

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Mar 24, 2020
@ernestostifano
Copy link
Author

@brettz9 thank you for your response and sorry that my report wasn't clear at first sight. I've just added the mandatory second asterisks, it was just a typo.

As far as I know @typedef blocks are not intended to be attached to any other JavaScript context. Indeed, my example is very similar to the @typedef usage example in JSDoc official documentation.

Since your mentioned ESLint issue has been archived, shouldn't we fix this by our own means?

I'm not familiar with your codebase, but if you think that I might be of help in some way, please let me know.

@ernestostifano
Copy link
Author

Side note: WebStorm correctly marks myTypesA as used (no error), auto-completion works as intended and even TypeScript generates correct declaration files without complaining. This is just to reinforce the fact that myTypesA seems to be correctly used and that "no error" should be the correct behaviour.

@brettz9
Copy link
Collaborator

brettz9 commented Mar 24, 2020

No worries @ernestostifano --yeah, I figured it may have been a typo as you got the essential right.

Yes, you're right about @typedef's not being attached. But I think that is the cause of our problem, as those are the ones processed during Program:exit which has that timing issue.

And yes, hopefully we can handle the issue ourselves in some way.

And yes, I think there should indeed be no errors here.

What I am thinking, and in the spirit of that archived issue, is that we really shouldn't be using this approach of having our rule impact other rules (no-unused-vars) at all anyways (eslint is designed to normally not work that way in having rules interact).

Variables that aren't actually used by real code should indeed remain marked as "unused".

Then how should we get awareness of the @typedef's? As mentioned in my comment above: have an option to feed files to the rule (whether by whitelist, by pointing to the jsdoc config which indicates the files that contain docs, or, probably most appealingly, a file iterator to actually visit any imported files and collect their @typdef's, taking care not to recurse over already visited files, doing caching for performance, etc.). In such a case, we only need worry about the @typedef's and any variables marked in scope (including /* globals */ and such as I believe we are already gathering), and not forcing people to have their code do useless things like making imports which don't actually get used (this is not TypeScript which could compile out type info not used at runtime but will actually slow things down).

@brettz9
Copy link
Collaborator

brettz9 commented Mar 24, 2020

So we should be able to find the typedefs referenced by a file like:

import './typedefs.js';

...regardless of whether the import adds specific variables into scope.

But that still requires an import within the file using a @typedef from elsewhere. My idea in #507 is to allow iterating over one's whole codebase (or targeted portions) to collect the typedefs, e.g., if the rule had an option projectEntryPoints: ['filesFromWhichToBeginLookingRecursivelyThroughImportsForTypedefs.js].

Another option we could provide would be allowing import() within jsdoc as TypeScript does--though I don't think we should do that in jsdoc mode.

@brettz9
Copy link
Collaborator

brettz9 commented Mar 24, 2020

If this doesn't make sense, let me know and I can clarify.

@ernestostifano
Copy link
Author

ernestostifano commented Mar 25, 2020

@brettz9 I have carefully read your comments and I think that we belong to the same school of thought. Indeed, everything makes a lot of sense. Specifically,

I can not reject the following statement:

Variables that aren't actually used by real code should indeed remain marked as "unused".

event if it goes against my particular use case, from which this issue was raised in the first place, and the currently implemented logic of eslint-plugin-jsdoc/no-undefined-types.

I am particularly enthusiastic about JSDoc, the idea of having extensive type checking, auto-completion, commented code and automatic documentation, all at the same time, without using any JS superset and with no extra compilation stages involved, is huge. It is a shame that JSDoc is not being actively developed.

With that being said,

Another option we could provide would be allowing import() within jsdoc as TypeScript does--though I don't think we should do that in jsdoc mode.

I would ban this option too! To be compliant with current specs.

Long story short: Alas, I would completely remove the no-unsused-vars suppression feature. (Yes, for all tags and blocks).

Background details:

You could be asking yourself why am I "importing a JSDoc type". And the reason is that, in the optic of the JSDoc paradigm, I'm studying code patterns and structures that would allow using JSDoc for all the above mentioned functionalities, not only during development, but also at the time of consuming the code (for example, if you are publishing a library).

The biggest challenge is that transpilers and bundlers will most likely break JSDoc because of non trivial comment handling issues. The idea now was to "use TypeScript without using TypeScript", only to generate declaration files from your already existent JS files with JSDoc annotations.

The only remaining aspect was modularity and type declarations re-utilisation. And there is where I discovered that JSDoc types attached to a JS context (a variable, class, function, etc) gets exported/imported along with that context. And that it is a common behaviour between JSDoc and TypeScript! (Everything has to be supported by both specifications for this strategy to work).

If that context is being normally used in your code, no unused error are showed. The problem is when you only need the type. (This issue occurs).

My final solution was to implement a custom runtime type checking utility, like schema-utils, but based on JSDoc specifications. So now I export/import JSDoc types along with JS real objects containing analog type definitions for the above mentioned utility, that are actually being used. No errors, and no schools of thought contradictions this way. (Obviously, this was a way for justifying that additionally created context for type export/import. At the end, I decided to bet on this strategy, results seems promising so far, code is clean, scalable, sharable and most importantly, type safe with pure JavaScript!)

@brettz9
Copy link
Collaborator

brettz9 commented Mar 26, 2020

I am particularly enthusiastic about JSDoc, the idea of having extensive type checking, auto-completion, commented code and automatic documentation, all at the same time, without using any JS superset and with no extra compilation stages involved, is huge.

Yes, great stuff.

It is a shame that JSDoc is not being actively developed.

JSDoc seems to have activity periodically over time, but it seems the project is mostly the maintainer, so it seems it doesn't keep pace with the demand.

With that being said,

Another option we could provide would be allowing import() within jsdoc as TypeScript does--though I don't think we should do that in jsdoc mode.

I would ban this option too! To be compliant with current specs.

Yeah. JSDoc has added features over the years, and there are issues filed to add TypeScript features including import(), but for now at the very least, this isn't supported.

Long story short: Alas, I would completely remove the no-unsused-vars suppression feature. (Yes, for all tags and blocks).

Cool. I'll close then since we already have an issue for getting files fed to the rule so their typedefs can be processed.

Plan to reply to your background info comments as I have a chance.

@brettz9 brettz9 closed this as completed Mar 26, 2020
@brettz9
Copy link
Collaborator

brettz9 commented Mar 26, 2020

Background details:

You could be asking yourself why am I "importing a JSDoc type". And the reason is that, in the optic of the JSDoc paradigm, I'm studying code patterns and structures that would allow using JSDoc for all the above mentioned functionalities, not only during development, but also at the time of consuming the code (for example, if you are publishing a library).

I guess you mean runtime checking?

The biggest challenge is that transpilers and bundlers will most likely break JSDoc because of non trivial comment handling issues. The idea now was to "use TypeScript without using TypeScript", only to generate declaration files from your already existent JS files with JSDoc annotations.

Sounds great. Curious if there are limitations in the specificity one can get out of jsdoc compared to TypeScript. But TypeScript declaration generation would be different from runtime type checking, no?

The only remaining aspect was modularity and type declarations re-utilisation. And there is where I discovered that JSDoc types attached to a JS context (a variable, class, function, etc) gets exported/imported along with that context. And that it is a common behaviour between JSDoc and TypeScript! (Everything has to be supported by both specifications for this strategy to work).

If that context is being normally used in your code, no unused error are showed. The problem is when you only need the type. (This issue occurs).

My final solution was to implement a custom runtime type checking utility, like schema-utils, but based on JSDoc specifications. So now I export/import JSDoc types along with JS real objects containing analog type definitions for the above mentioned utility, that are actually being used. No errors, and no schools of thought contradictions this way. (Obviously, this was a way for justifying that additionally created context for type export/import. At the end, I decided to bet on this strategy, results seems promising so far, code is clean, scalable, sharable and most importantly, type safe with pure JavaScript!)

It sounds very interesting, though it'd be great to see some examples. If it is not proprietary, would love to see a repo for it!

It sounds like it might be similar to something I've wanted which is the ability to avoid redundancy in specifying jsdocs for my main programmatic API while also having a separate schema.

I currently use command-line-args, as I can get help generation with command-line-usage, SVG or HTML generation with my command-line-publish, and command-line-basics so I can avoid redundancy in always-needed CLI features across CLI projects.

But it'd be great if there were a tool to parse a jsdoc typedef definition file and turn that into such schema code, whether by compile or runtime processing (there is already https://www.npmjs.com/package/jsdoc-to-json-schema but I'd need another step to convert that to my CLI option processing schema format).

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 22, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Sep 13, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Sep 14, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Sep 27, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Sep 28, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Sep 28, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Sep 28, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Sep 28, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Sep 29, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Sep 29, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Sep 29, 2020
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):

test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 7, 2021
	1. Add tests (for desired features added to docs) -> implement
	2. (apparent problem in eslint itself with our use of Program:exit):
        3. Could also use this approach for checking consistency between function calls and jsdoc argument signature types; compile to WebAssembly
test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 7, 2021
1. Add tests (for desired features added to docs) -> implement
2. (apparent problem in eslint itself with our use of Program:exit):
3. Could also use this approach for checking consistency between function calls and jsdoc argument signature types; compile to WebAssembly
test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 7, 2021
1. Add tests (for desired features added to docs) -> implement
2. (apparent problem in eslint itself with our use of Program:exit):
3. Could also use this approach for checking consistency between function calls and jsdoc argument signature types; compile to WebAssembly
test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 12, 2021
1. Add tests (for desired features added to docs) -> implement
2. (apparent problem in eslint itself with our use of Program:exit):
3. Could also use this approach for checking consistency between function calls and jsdoc argument signature types; compile to WebAssembly
test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 15, 2021
1. Add tests (for desired features added to docs) -> implement
2. (apparent problem in eslint itself with our use of Program:exit):
3. Could also use this approach for checking consistency between function calls and jsdoc argument signature types; compile to WebAssembly
test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 15, 2021
1. Add tests (for desired features added to docs) -> implement
2. (apparent problem in eslint itself with our use of Program:exit):
3. Could also use this approach for checking consistency between function calls and jsdoc argument signature types; compile to WebAssembly
test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 15, 2021
1. Add tests (for desired features added to docs) -> implement
2. (apparent problem in eslint itself with our use of Program:exit):
3. Could also use this approach for checking consistency between function calls and jsdoc argument signature types; compile to WebAssembly
test(`no-undefined-types`): issues gajus#507
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 15, 2021
1. Add tests (for desired features added to docs) -> implement
2. (apparent problem in eslint itself with our use of Program:exit):
3. Could also use this approach for checking consistency between function calls and jsdoc argument signature types; compile to WebAssembly
test(`no-undefined-types`): issues gajus#507
@brettz9 brettz9 mentioned this issue May 3, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants