Add new InternalAffairs/NodeMatcherDirective cop.

[dvandersluis]

As discussed on discord, this cop looks for node matchers that do not have an appropriate @!method directive.

This is really useful for code editors which support YARD directives, as it allows them to be able to jump to the pattern matcher definition from the usage, as well as introspect what arguments are necessary. Even without one of these editors, it makes arguments explicit in the definition (so it's not a guess what %1, etc. mean in the pattern).

I am going to create PRs for the other gems (especially rubocop-ast) next.

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[koic]

Maybe it's because I'm not using YARD's editor jump, it's a bit hard to force this directive...

[marcandre]

Good stuff 💪
Seems like a small price to pay for those that can use this.

[marcandre]

Not sure it's worth the extra work, but you could use NodePattern#positional_parameters and #named_parameters to build an exact signature, even for the more complex cases...

[koic]

Unfortunately, I honestly don't feel that way. Most matchers are only used within the defined one cop (that is, within a single file). It's the rule I'm not happy to be able to repeatedly write (and read) comments similar to what is written in the code. So I pay homage to this work, but it's not fun the directive code comment.

[koic]

I've been thinking about it for a while, but it doesn't seem like a good approach. It's a slightly different perspective. For example, type signature has been introduced in Ruby 3.0, but as far as I know, Matz doesn't recommend writing type annotation in Ruby code itself using annotation comment feature. Similarly, I don't think signature annotation should be written in Ruby code itself for support editor. I think that external development environments (like ctags, LSP, and others) should be enhancement developed. For me, one of Ruby language design's favorite points is that it can read and write to DRY.

[koic]

The above comment is a mistake of not approve.

[marcandre]

So maybe these comments should be added only to the nodepatterns that are "global" (i.e. defined on Node or a subclass)?

[dvandersluis]

I want to demonstrate why I think this is useful. I understand that the majority of pattern matchers only are used within a single file, but even with that constraint, I am constantly using my editor to jump between usages and definitions of any methods I encounter. (FWIW, most cops define 0-2 node matchers, and the annotations are only required or added by the cop for def_node_matcher and def_node_search).

Pattern matchers dynamically define methods, in a way that the static analysis editors do (if any) cannot detect, so the editor does not know that a matcher exists. I am not very familiar with ctags, etc. but I would guess that these methods can't easily be detected by anything generating ctags either, for the same reason (nothing to tell any analysis system that def_node_matcher creates a dynamic method). I don't really see this as the same as a type annotation, but I understand your feelings there.

Without annotations:

With annotations:

BTW, YARD annotations are already in quite a few places in the rubocop code (I found 331 instances of yard comments excluding @example or @!method, 196 in cops). I understand that none of the other ones are forced as this would cop would do, and that's probably a big part of your dislike. I tried to make it so that the cop could auto-generate a good tag itself without needing user input, hoping to alleviate any issue there.

So maybe these comments should be added only to the nodepatterns that are "global" (i.e. defined on Node or a subclass)?

I would rather do this than nothing at all, but I still think it's useful and beneficial to have these for every pattern matcher definition.

[marcandre]

Maybe @koic is mostly opposed to the obligation of doing this, so maybe it could be done now on all patterns, and enforced in the future only for rubocop-ast?

[bbatsov]

For the record - I'm fine with the proposed change, but I won't merge the PR until we address @koic's concerns. I don't mind adding metadata here and there (I used to write Java for a living :D ), but I understand his perspective as well.

[dvandersluis]

I think @marcandre's suggestion is a good compromise. It will likely get out of sync, but we (I?) can fix new ones as they're encountered.

@koic would you be ok with that?

[koic]

Yeah, I also wrote Java at work until Rails came along. At that time, Java 1.5 annotations and zero configuration helped us escape from XML hell. On the other hand, Rails's CoC and Ruby's DSL have made it unnecessary to write redundant configurations.

I understand that dynamic definitions difficult work well with code analysising, but I'm still concerned about annotation document and code repeats. (This is a tough one that is to solve redundancy using CoC.)

Maybe in the future it may be hoped that growth of RBS and LSP will solve it, but it may be a little further.

Thank you for your consideration and kind comments. Let me think for a while.

[koic]

I still have the same opinion, but it's a better choice to move forward, choosing maintainer to benefit rather than difference of opinion. So, I believe it will be.
In addition, since the maintenance cost has been reduced by this cop's auto-correction, I think that the current implementation policy can be applied (or a better policy). Thank you always for your activity ❤️

[dvandersluis]

@koic great, thanks! Just to clarify, are you okay with keeping the IA cop enabled, or do you want it to be disabled by default?

[koic]

@dvandersluis I'll leave it to you :-) IMO this IA cop can be enabled by default. It can be chosen to be auto-corrected or disabled for each repository that used IA cops.