Skip to content

valid-types: Only checks the name of typedef but not the definition #305

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

Closed
l1bbcsg opened this issue Jul 3, 2019 · 7 comments · Fixed by #308
Closed

valid-types: Only checks the name of typedef but not the definition #305

l1bbcsg opened this issue Jul 3, 2019 · 7 comments · Fixed by #308

Comments

@l1bbcsg
Copy link
Contributor

l1bbcsg commented Jul 3, 2019

The following comment has two types: typedef name (Foo) and its value (String):

/**
 * @typedef {String} Foo
 */

valid-types only checks typedef name (Foo) for validity and not the value.

I debugged this a bit and figured valid-types rule when iterating over tags goes into isNamepathTag execution branch while checking of they type value is done in isTagWithType branch. @typedef is essentially both and should go through both.

@l1bbcsg
Copy link
Contributor Author

l1bbcsg commented Jul 3, 2019

Upon further investigation, the following tags also skip type checks because they are listed as namepathDefiningTags:
[ 'class', 'constant', 'member', 'namespace', 'typedef' ].

For example, here type is not checked at all because utils.passesEmptyNamepathCheck returns true for @constant

/**
 * @constant {number}
 */
var bar;

@l1bbcsg
Copy link
Contributor Author

l1bbcsg commented Jul 3, 2019

Also, of note, this behaviour leads to Syntax error when validating GCC variant of typedef syntax:

/**
 * @typedef {String}
 */
let Foo;

Rather than parsing String type here, it passes empty string (would-be typedef name) to jsdoctypeparser which results in linter error.

@l1bbcsg
Copy link
Contributor Author

l1bbcsg commented Jul 3, 2019

Another Syntax Error I got on @extends/@augments with generic types:

/**
 * @template A
 */
class Parent {}

/**
 * @extends {Parent<string>}
 */
class Child extends Parent {}

Here it fails to properly set up the parser for @extends tag. Both commentparser and jsdoctypeparser can handle this syntax. This is specific to GCC, but it technically might be valid in base jsdoc too.

@brettz9
Copy link
Collaborator

brettz9 commented Jul 3, 2019

Regarding @extends, per https://jsdoc.app/tags-augments.html (of which @extends is an alias), there are no brackets around the type. comment-parser is generic and jsdoctypeparser is only for the types themselves, but the expectation of how the tag is structured is more of a (jsdoc) linting issue.

The other concerns look valid though note that constant does not need to have a type, but of course if it has one, it makes sense to validate it.

I'd strongly prefer to have a cross-rule setting to set the mode to jsdoc, typescript, or gcc, so we could auto-tweak accordingly, and not be overly lenient if the desired environment doesn't support the type (unless the user opts in, e.g., it would be ok for a rule to add @template as a valid tag within their particular jsdoc environment, since jsdoc allows adding custom tags, but not to automatically treat it as a valid tag (unless the setting is for GCC), since it is not standard jsdoc).

@l1bbcsg
Copy link
Contributor Author

l1bbcsg commented Jul 3, 2019

Regarding @extends, per https://jsdoc.app/tags-augments.html (of which @extends is an alias), there are no brackets around the type.

Yes, there’s a bit of caveat is that per the same spec JSDoc supports angled bracket notation to specify some templa-ty classes like Array. So, Array<string> is a valid JSDoc without GCC extension. This kinda brings templates into JSDoc without @template tag. I think this might technically be a valid standard jsdoc, at least from syntax point of view:

/**
 * @extends Array<string>
 */
class StringArray {}

@brettz9
Copy link
Collaborator

brettz9 commented Jul 3, 2019

Yes, that latter example should be valid, but I was not clear, I actually meant curly brackets were not supposed to be as part of @augments/@extends, as in your prior example with @extends {Parent<string>}.

@gajus
Copy link
Owner

gajus commented Sep 1, 2019

🎉 This issue has been resolved in version 15.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Sep 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants