Add support for checking destructured properties and TypeScript literal object types

[dstaley]

This PR modifies the check-param-names rule to verify that destructured object properties are documented with a @param directive. Additionally, it verifies that the properties on TypeScript object literal types also have accompanying documentation, and checks that there aren't any @param directives for non-existent properties.

With this PR, the following are now errors:

/**
 * @param cfg
 * @param cfg.foo
 */
function quux ({foo, bar}) {
}
// Message: Missing @param "cfg.bar"

export class SomeClass {
  /**
   * @param prop
   * @param prop.foo
   */
  constructor(prop: { foo: string, bar: string }) {}
}
// Message: Missing @param "prop.bar"

export class SomeClass {
  /**
   * @param options
   * @param options.foo
   * @param options.bar
   */
  constructor(options: { foo: string }) {}
}
// Message: @param "options.bar" does not exist on options

getFunctionParameterNames will now report destructured parameters and object literal types as a tuple [?string, Array<string>], where the first element represents the parameter name if provided, otherwise undefined in the case of destructiring. The second element represents the properties of the parameter.

function quux({ foo, bar }) {} // [undefined, ["foo", "bar"]]

function quux(options: { foo: string, bar: string }) {} // ["options", ["foo", "bar"]]

I tried to be as exhaustive as possible, but please let me know if there's a particular case I didn't write a test-case for. I also didn't touch and code related to providing fixes, so please let me know if I ended up breaking something there that wasn't accounted for in the unit tests.

[brettz9]

Looks great, thanks! I need to take a closer look, but for now, I've rebased on some just-updated master changes, fixing the Travis errors.

While there may be some duplication involved, I think it would be better if require-param could be made to report the destructuring-ish errors rather than ignore them. It already is reporting some param mismatches, so I see no reason we should not do so there also if we are doing validation in both. I would probably want to add that, so if you could add it, I expect it would expedite my review.

Thanks!

[dstaley]

Sure, I'm not opposed to that! I'm a bit confused though: would that mean both rules would report the same errors? Or would one handle the TypeScript object literal property checks and the other handle destructuring?

If both rules report the same errors, would the tests for each rule confirm that errors from the other rule are also reported?

[brettz9]

I think both should report the same kind of errors (as they both report errors now--just not with destructuring).

However, require-param only iterates through the actual function parameter names, not concerning itself if the user has added too many @param's (as does check-param-names). I think we can keep this behavior.

So in summary, I think the same tests can be used for both with the exception of tests checking extra @param tags (require-param should be checked to ensure it gives no errors whatever these extra params say and check-param-names should report problems with extra @param tags (as it already does with non-destructuring-type params)).

[dstaley]

Just so that I fully understand before I dive in, you're asking for the following to be errors, correct?

[
    {
      code: `
          /**
           *
           */
          function quux ({foo}) {

          }
      `,
      errors: [
        {
          message: 'Missing JSDoc @param "foo" declaration.',
        },
      ]
    },
    {
      code: `
          /**
           * @param
           */
          function quux ({foo}) {

          }
      `,
      errors: [
        {
          message: 'Missing JSDoc @param "foo" declaration.',
        },
      ]
    },
    {
      code: `
          /**
           * @param options
           */
          function quux ({foo}) {

          }
      `,
      errors: [
        {
          message: 'Missing JSDoc @param "options.foo" declaration.',
        },
      ]
    }
]

If so, what should the error for the second test case be since we don't have a name for the argument?

[brettz9]

Right, those should be errors.

I think for case no. 2, it might be nice to report Missing JSDoc @param declaration for root of "foo". (I think it would be sufficient to report the first destructured name rather than needing to say Missing JSDoc @param declaration for root of "foo", "bar", and "baz". had the argument looked like function quux ({foo, bar, baz: boo}) {)

Btw, further to my example, I think we ought to be showing the key instead of the value (i.e., for {baz: boo}, use "baz" in reporting names) since the key is what consumers of the function need to be concerned about, and knowing the value is just about tracing it back to where it is defined (even though it is quite early and used throughout the function, it is still after baz is destructured, like a local variable had been defined at the top of the function; i.e., the param is still really baz).

[dstaley]

Gotcha!

As far as displaying "the key instead of the value": could you explain a different way or provide an example? I'm not sure what you mean.

[brettz9]

To change your last example, if it were:

{
      code: `
          /**
           * @param options
           */
          function quux ({foo: bar}) {

          }
      `,
      errors: [
        {
          message: 'Missing JSDoc @param "options.foo" declaration.',
        },
      ]
    }

...the error still references foo even though it destructures (immediately) to bar. We want the key foo in the message (unless you think listing both will not be confusing, e.g., Missing JSDoc @param "options.foo (bar)" declaration.).

[dstaley]

Ah, okay yeah that makes sense now! I think I have all the answers I need to go through this over the next few days. Thanks again for your help!

[brettz9]

Also, not just in messages:

          /**
           * @param options
           * @param options.bar
           */
          function quux ({foo: bar}) {

          }

...should be tested and give an error (since the @param should have been named options.foo).

[brettz9]

And, btw, your examples made me realize I think we want a test (unrelated to destructuring) like this for require-param too to check for the case when there are the same number of arguments, just named differently:

{
      code: `
          /**
           * @param bar
           */
          function quux (foo) {

          }
      `,
      errors: [
        {
          message: 'Missing JSDoc @param "foo" declaration.',
        },
      ],
      output: `
          /**
           * @param foo
           */
          function quux (foo) {

          }
      `,
    },

[dstaley]

Good news! I finally managed to get some time to implement checking properties for arbitrarily nested destructuring assignments! The tests are now passing, including all the cases you've mentioned here.

There's probably a few places where performance can be improved, so I'm gonna take a few more days and see if anything stands out to me.

[dstaley]

Alrighty, I think it's ready for a review! If there's anything that's unclear, please let me know. I also added a new TypeScript test case using an actual function definition from one of my codebases, and it's really exciting to see it accurately reporting missing parameters!

[brettz9]

I've added most of the proposed changes now. The behavior of unnamedRootBase still needs adjusting and enableRootFixer should be added. Now added also.

I'd ideally like to see the proposed (and documented but not yet implemented) checkRestProperty and enableRestElementFixer added as well along with tests. (I've added tests and document changes in my own branch already, but need to implement/get tests and coverage passing.)

Will see if I can get to it, but I hope you can review my changes to provide an extra pair of eyes before we merge.

[brettz9]

Ok, all proposed fixes/docs/additions/tests have been added. I think this should mostly be ready now.

The one remaining issue... While not a really big problem, the one real world issue I came across when applying this to a pretty large code base (svgedit)--and it worked nicely to catch a few doc problems we had--is that sometimes one may wish to just use the parent object and let the type speak to the properties in use.

For example, we had destructuring in this function's parameters:

/**
* Converts a `SVGRect` into an object.
* @function module:utilities.bboxToObj
* @param {SVGRect} bbox - a SVGRect
* @returns {module:utilities.BBoxObject} An object with properties names x, y, width, height.
*/
export const bboxToObj = function ({x, y, width, height}) {
  return {x, y, width, height};
};

But doc errors were given for the fact that x, y, etc. were not documented. But the SVGRect type ought to indicate this information (or in other cases where the type was a module: namepath to one of our own types).

I could refactor the above as follows to avoid needing to repeat the docs, but I think we should probably provide an option so the rule only applies if the type is missing or is "object" or "array" (or some whitelist) so that refactoring is not required:

/**
* Converts a `SVGRect` into an object.
* @function module:utilities.bboxToObj
* @param {SVGRect} bbox - a SVGRect
* @returns {module:utilities.BBoxObject} An object with properties names x, y, width, height.
*/
export const bboxToObj = function (bbox) {
  const {x, y, width, height} = bbox;
  return {x, y, width, height};
};

[brettz9]

Ok, I've implemented for check-param-names and require-param with a checkTypesPattern regex string option.

So this issue is now fully ready for review. For a cleaner comparison (and as I was having some difficulties seeing the right diffs in Github), I've squashed the commits in https://github.com/brettz9/eslint-plugin-jsdoc/tree/destructured-squashed , with a complete diff of changes introduced by the PR available at https://github.com/gajus/eslint-plugin-jsdoc/compare/master...brettz9:destructured-squashed?expand=1

[brettz9]

This has now been incorporated in 30835cd . Thanks so much for your work on this!