Some predicates return nil

[marcandre]

Found by @dvandersluis in #317.

Unfortunately our specs don't catch these.

My plan:

1. Turn on strict_predictes rspec config
2. Add def_node_predicate to be used instead of def_node_matcher where it makes sense
3. Fix spec failures
4. analyse the remaining cases which are either redundant, or missing a test.

Offenses:

  lib/rubocop/ast/node/array_node.rb:37:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.begin&.is?('[')
^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/array_node.rb:55:11: C: Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.begin&.source&.start_with?('%')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/block_node.rb:93:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.end&.is?('}')
^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/block_node.rb:100:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.end&.is?('end')
^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/for_node.rb:20:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.begin&.is?('do')
^^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/hash_node.rb:118:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.end&.is?('}')
^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/in_pattern_node.rb:27:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.begin&.is?('then')
^^^^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/mixin/method_dispatch_node.rb:144:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  receiver&.self_type?
^^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/mixin/method_dispatch_node.rb:153:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  receiver&.const_type?
^^^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/mixin/method_identifier_predicates.rb:187:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  receiver&.self_type?
^^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/mixin/method_identifier_predicates.rb:194:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  receiver&.const_type?
^^^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/mixin/parameterized_node.rb:16:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.end&.is?(')')
^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/until_node.rb:31:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.begin&.is?('do')
^^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/when_node.rb:36:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.begin&.is?('then')
^^^^^^^^^^^^^^^^^^^^^^
  lib/rubocop/ast/node/while_node.rb:31:9: C: [Correctable] Style/ReturnNilInPredicateMethodDefinition: Predicate method may return nil due to &. operator.
  loc.begin&.is?('do')
^^^^^^^^^^^^^^^^^^^^

  165 files inspected, 15 offenses detected, 14 offenses autocorrectable

[marcandre]

First, I'm also planning on adding def_node_predicate that will generate code that always return a boolean.

Now there are a bunch of issues.

I note that the doc of def_node_matcher states "If the node matches, and no block is provided, the new method will return the captures, or true if there were none.". That's a bit misleading, as if there is a single capture, that capture will be returned not within an array. I plan on fixing the doc.

Overall, I'm seeing different cases, and I'd love input @rubocop/rubocop-core

1. Some code that return nil/false/true, e.g. dot?, or other combinations, e.g. camel_case_method? that returns nil or MatchData. These are clearly bugs and will be fixed
2. def_node_matcher :assignment_or_similar? which don't capture, hence returning nil or true: I think we should consider these as bugs and they should be changed to def_node_predicate.
3. def_node_matcher :class_constructor? and similar that have a single capture, hence returning nil or the capture: these were clearly meant to be used to get the interesting capture, as well as to be used within other node patterns.

Should we change class_constructor? to return a boolean and add class_constructor that would return nil/capture? I think this would be the best course of action, and but I think we should consider this a breaking change, so we'd go from 1.31 to 2.0 for rubocop-ast, but what about rubocop itself?...
FWIW, rubocop's code calls class_constructor? 5 times, none of which would need to be changed to class_constructor. I haven't checked other cases though.

[dvandersluis]

That all sounds reasonable and well thought out to me.

When I write patterns I tend to only use the ? form if I'm not expecting to use a capture (I'm sure there are examples I've written where this doesn't strictly apply but that's the intention anyways). I like the idea of def_node_predicate to make this more explicit. Would it be a syntax error if a def_node_predicate has captures?

Should we change class_constructor? to return a boolean and add class_constructor that would return nil/capture?

Given that it's capture form is not used I think it's probably a reasonable change. Of course the risk is if it's used in another one of the rubocop gems or a third party extension. Making it a breaking change sounds right to me.

[marcandre]

Would it be a syntax error if a def_node_predicate has captures?

Very good point, I think it should

Making it a breaking change sounds right to me.

Would that warrant a major version upgrade for Rubocop itself?

[dvandersluis]

Would that warrant a major version upgrade for Rubocop itself?

I'd say no given the breaking behaviour isn't used in core.