Have namepath checks in `valid-types` only allow strict namepaths

brettz9 commented 5 years ago

As raised in #308, contexts where we are checking namepaths in valid-types also mistakenly allow for more broad types, e.g.,Array<string> should get reported in @typedef {StringArray} Array<string> but it currently is not.

~I've filed https://github.com/jsdoctypeparser/jsdoctypeparser/issues/86 to possibly have jsdoctypeparser facilitate strict namepath checking rather than examining the parse results more closely here.~

Ideally, @memberof variation pointers would be valid as well (see #521 ).

--- Want to back this issue? **[Post a bounty on it!](https://app.bountysource.com/issues/79701427-have-namepath-checks-in-valid-types-only-allow-strict-namepaths?utm_campaign=plugin&utm_content=tracker%2F23037809&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://app.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F23037809&utm_medium=issues&utm_source=github).

jaydenseric commented 3 years ago

Also, we should lint that certain kinds of child members have valid relationships to the parent in the namepath; for example (I'm not 100% sure about these because the JSDoc spec is poor):

It is only valid for an event to have an instance membership (#).
- Valid: @event FooClass#event:EventName or @event FooClass#EventName
- Invalid: @event FooClass~event:EventName or @event FooClass~EventName
As a side note, it would also be nice to autofix event names to either have or not have the event: prefix for consistency. It's my preference to use the prefix everywhere instead of remembering it can be omitted in the @event and @fires tags.
A typedef can only have an inner membership (~).
- Valid: @typedef {Object} FooClass~FooType
- Invalid: @typedef {Object} FooClass#FooType

I spent a lot of time in jsdoc-md coming up with an elegant regex to reliably deconstruct and validate JSDoc namepaths, maybe you will find it useful:

If you do want to use it, let me know and I will publish a package for it. It's my preference to share a proper dependency than have you cut and paste the code in with the MIT license mandated credit in a comment or something.

brettz9 commented 3 years ago

Also, we should lint that certain kinds of child members have valid relationships to the parent in the namepath; for example (I'm not 100% sure about these because the JSDoc spec is poor):
* It is only valid for an event to have an instance membership (`#`).

  * Valid: `@event FooClass#event:EventName` or `@event FooClass#EventName`
  * Invalid: `@event FooClass~event:EventName` or `@event FooClass~EventName`

I think it should be possible that an event is defined as an inner object yet such an event instance is fired by a public method on that class and thus needs to document it is arising from a private.

class EventName {}

class FooClass {
  triggerEventName () {
    return new EventName();
  }
}

// ...

class PublicAPI {
  /**
   * @event FooClass~EventName
   */
  publicMethod () {
    return new FooClass().triggerEventName();
  }
}

  As a side note, it would also be nice to autofix event names to either have or not have the `event:` prefix for consistency. It's my preference to use the prefix everywhere instead of remembering it can be omitted in the `@event` and `@fires` tags.

Feel free to file a separate issue for this. We couldn't really determine if the event prefix was missing, e.g., on a @param unless on a tag like event, fires, listens (since we wouldn't know whether it was an event or something else).

* A typedef can only have an inner membership (`~`).

  * Valid: `@typedef {Object} FooClass~FooType`
  * Invalid: `@typedef {Object} FooClass#FooType`

I guess the idea here is that a public item would not just be abstract, and so a @typedef would not be suitable in such a case, right? (i.e., one would have to use a @namespace instead to define such a visible object.) Yes, I see the docs do at least suggest this is often the case even if they are not explicitly or firmly recommending this.

Such a change sounds reasonable though and would fit well with the change I think will be needed for this issue–to supply startRule: 'NamepathExpr' to our jsdoctypeparser call (we'd have to introspect on the owner of the parsed result to determine whether it is "MEMBER", "INNER_MEMBER", or "INSTANCE_MEMBER").

However, I think it should probably be added as a default-on option since the docs are not adamant that instance members cannot be used.

I spent a lot of time in jsdoc-md coming up with an elegant regex to reliably deconstruct and validate JSDoc namepaths, maybe you will find it useful:
* Source: https://github.com/jaydenseric/jsdoc-md/blob/v8.0.0/private/deconstructJsdocNamepath.js

* Tests: https://github.com/jaydenseric/jsdoc-md/blob/v8.0.0/test/private/deconstructJsdocNamepath.test.js
If you do want to use it, let me know and I will publish a package for it. It's my preference to share a proper dependency than have you cut and paste the code in with the MIT license mandated credit in a comment or something.

Thank you for the offer, but I really think this should be handled in jsdoctypeparser. The grammar makes it easy to understand and maintain (and ensure parsing improvements benefit other parsing uses within our project(s))--might even be useful for your project.

Btw, if anyone wants to help with a real bottleneck on jsdoctypeparser that is stalling some things on this project, we haven't been able to get semantic-release working, so any suggestions or help would be appreciated: https://github.com/jsdoctypeparser/jsdoctypeparser/pull/122 . With such a fix in place, we'd be in a better position to make releases there which can be more quickly surfaced in eslint-plugin-jsdoc.

I do have to mention that my health/energy has not been great though, so I'm having to slow down and am not necessarily going to be able to be reliable.

gajus / eslint-plugin-jsdoc

Have namepath checks in `valid-types` only allow strict namepaths #374