Enforce numblock verification for ALL the cops that check block nodes

[gsamokovarov]

Ruby 2.7 introduced a new block syntax with access to implicit numbered arguments.

clients.map { |client| client['name'] } # Explicit arguments.
clients.map { _1['name'] }              # Numbered arguments.

The problem for rubocop is that blocks with numbered arguments clients.map { _1['name'] } are parsed with a different AST node called numblock and most of the block cops handle only block nodes and everything's allowed for numblock nodes. This is what the AST looks like:

;; clients.map { |client| client['name'] }
(block
  (send (send nil :clients) :map)
  (args (procarg0 (arg :client)))
  (index (lvar :client) (str "name")))

;; clients.map { _1['name'] }
(numblock
  (send (send nil :clients) :map) 1
  (index (lvar :_1) (str "name")))

Back in #10736, I found out that Layout/SpaceInsideBlockBraces wasn't being applied to numblocks. While adding numblock support for Style/BlockDelimiters in #10749 I figured out we have lots of such cops and decided to fix them all. In one big ass PR, nonetheless! 😅

To fix them all, I developed the an internal cop called InternalAffairs/NumblockHandler to check for cop definitions that handle block nodes but not numblock nodes. I found out around 45 offences at first.

It is perfectly legitimate to have cops that handle only block nodes. We have cops that verify block argument syntax or apply only to blocks without arguments. I decided to keep InternalAffairs/NumblockHandler enabled for everything and to disable it explicitly for the cops that don't need it. I think this is a good nudge that forces you to think about how block cops should deal with numblocks.

Those are the cops that needed to be fixed to support numblocks:

• 380201c1e Fix Style/HashEachMethods with numblock
• fa7f95601 Fix Style/EachWithObject with numblocks
• 961533223 Fix Style/CollectionMethods with numblocks
• c5a8c9ae9 Fix Style/InverseMethods with numblocks
• dbc5b1cb6 Fix Style/RedundantSortBy with numblocks
• 7db364ccd Fix Style/RedundantBegin with numblocks
• 9c5bef672 Fix Style/RedundantSelf with numblocks
• f191939da Fix Style/Next for numblocks
• 994bc9247 Fix Style/ObjectThen with numblocks
• 9b2f2cd94 Fix Style/MultilineBlockChain with numblocks
• 79a1df14a Fix Style/CombinableLoops with numblocks
• 1ad7bc74e Fix Style/TopLevelMethodDefinition
• cb67b2662 Fix Style/For with numblocks
• 2dd3e9275 Fix Style/Proc with numblocks
• e85536fa1 Fix Style/MethodCalledOnDoEndBlock with numblocks
• 0d296f35f Fix Metrics/AbcSize with numblocks
• 0d296f35f Fix Metrics/CyclomaticComplexity with numblocks
• f00058a33 Fix Lint/Void with numblocks
• bac0a0cc7 Fix Lint/UselessAccessModifier with numblocks
• 8cc720100 Fix Lint/UnreachableLoop for numblocks
• 0d7a06a64 Fix Lint/RedundantWithObject with numblocks
• 22c4afefe Fix Lint/RedundantWithIndex with numblocks
• 20fcfca81 Fix Lint/NonDeterministicRequireOrder with numblocks
• 9524f6026 Fix Lint/NextWithoutAccumulator with numblocks
• 824955b7a Fix Layout/SpaceBeforeBlockBraces with numblocks
• 5cc939cec Fix Layout/MultilineBlockLayout with numblocks
• 1154e95d4 Fix Layout/LineLength with numblocks
• 4b4310028 Fix Layout/IndentationWidth with numblocks
• d86c9141e Fix Layout/EmptyLinesAroundBlockBody with numblocks
• 3878f77af Fix Layout/EmptyLinesAroundAccessModifier with numblocks
• c2a5ef5d3 Fix Layout/BlockEndNewline with numblocks
• 4a70c16f4 Fix Layout/BlockAlignment with numblocks

Here are the cops I have disabled InternalAffairs/NumblockHandler for:

• ead257375 Disable InternalAffairs/NumblockComplement for Layout/SpaceAroundKeyword
• e1d0f5fa5 Disable InternalAffairs/NumblockComplement for HashTransformMethod
• 0f6b7a918 Disable InternalAffairs/NumblockComplement for Style/RedundantFetchBlock
• 37517d829 Disable InternalAffairs/NumblockComplement for Style/EachForSimpleLoop
• c51d8a1d7 Disable InternalAffairs/NumblockComplement for Style/NilLambda
• fc15f9d57 Disable InternalAffairs/NumblockComplement for Style/EmptyLambdaParameter
• 8e6e4c0ec Disable InternalAffairs/NumblockComplement for Style/EmptyBlockParameter
• 4cbbe2c05 Disable InternalAffairs/NumblockComplement for Style/SingleLineBlockParams
• 3829dbb8d Disable InternalAffairs/NumblockComplement for Style/TrailingCommaInBlockArgs
• 2d1986a70 Disable InternalAffairs/NumblockComplement for Naming/BlockParameterName
• 24ecdb147 Disable InternalAffairs/NumblockComplement for Lint/EmptyBlock
• 47f12fb63 Disable InternalAffairs/NumblockComplement for Layout/SpaceAroundBlockParameters
• 3f4e4d425 Disable InternalAffairs/NumblockComplement for Gemspec/RequireMFA

Along the way, I found a few bugs. One was in the autocompletion of Layout/LineLength:

# This is autocorrected to...
foo.select do 4444000039123123129993912312312999199291203123 end
    
# ... that 👇
foo.select d
o 4444000039123123129993912312312999199291203123 end

Another one was in MethodDispatchNode#macro? method not detecting macros in numblocks. I have fixed this in rubocop-ast and @marcandre released 1.21.0 with the fix. I have bumped its dependency for rubocop.

I know this is a big change but you can review it commit by commit. I introduced 2 changelog files – one with cops that are fixed to handle numblocks, and one for the rubocop-ast version bump.

[bbatsov]

Wow, that's one truly epic PR! The changes look good to me at a superficial examination, but we'll definitely have to think how to simplify the handling of both types of blocks going forward. Perhaps the commissioner can add some callback that gets triggered on both like we've done for some method definitions.

[dvandersluis]

This adds a lot of changelog noise, which on one hand makes sense because each line is a changed cop, but on the other hand might make it hard to find other things in the changelog. @bbatsov do you think we should collapse the entries?

[bbatsov]

@dvandersluis Yeah, that'd be fine by me. I guess we can also collapse all the commits that just add support for numblocks into one. Same with the commits that add ignores for the new internal affairs cop.

[gsamokovarov]

Wow, that's one truly epic PR! The changes look good to me at a superficial examination, but we'll definitely have to think how to simplify the handling of both types of blocks going forward. Perhaps the commissioner can add some callback that gets triggered on both like we've done for some method definitions.

At first I thought that an alias on_numblock on_block would fix most of the cases but it wasn't that simple. The nodes are different enough as numblocks don't have argument definitions, but do have arguments and that's weird. It's even weirder that the numblock nodes have straight integers (not Node objects) for the arguments count. See this hack in Lint/UnreachableLoop...

Notice the integer in the representation, it's the arguments count, but we may want a node object here.

;; clients.map { _1['name'] }
(numblock
  (send (send nil :clients) :map)
  1
  (index (lvar :_1) (str "name")))

For example, the Style/EachWithObject cop does a very clever autocorrection. It changes [1, 2].inject({}) { |a, e| a[e] = e; a } to [1, 2].each_with_object({}) { |e, a| a[e] = e } by rewriting the block arguments order without touching the body. Numblocks are represented by the Rubocop::AST::BlockNode and always have empty #arguments and falsy #arguments? but have some argument representation in #argument_list. This is something that we have to fix, it's confusing. Numblocks should have truthy #arguments? – we haven't declared the arguments but we do have them. We probably want some representation in #arguments as well, which is currently always empty for numblocks. A the moment #argument_list for numblocks contains synthetic ArgNodes representing the numbered local variables and their backing local variables. However, you cannot rewrite those synthetic nodes as they don't represent real source code. I ended up writing custom autocorrection for numblocks in Style/EachWithObject.

Differences like this can be tricky to get unified if we try to treat block and numblock nodes the same.

[gsamokovarov]

This adds a lot of changelog noise, which on one hand makes sense because each line is a changed cop, but on the other hand might make it hard to find other things in the changelog. @bbatsov do you think we should collapse the entries?

I can do that as well. I can add 2 entries and list the changed copes in a single changelog entry if this is fine with you all. To keep it a bit tidier and create only 2 files with the changes – 1 for the fixes and 1 for the rubocop-ast bump as a change, but there was a test enforcing single tries in the files. I think multiple entries per file can be a feature.

If you have other ideas about the changelog entry, I'm open to them.

[bbatsov]

🚀