def_node_matcher fails with NoMethodError on a non-matching node

[thomthom]

(I had originally added this as part of issue #5470; #5470 (comment). But I realized that I wasn't trying to match the same thing as described in that issue.)

I'm working on a RuboCop extension with a set of custom cops. I'm running into an unexpected error while trying to pattern match a send node.

        def_node_matcher :sketchup_require, <<-PATTERN
          (send
            (const nil? :Sketchup) :require
            (str $_)
          )
        PATTERN

        def_node_matcher :sketchup_require?, <<-PATTERN
          (send
            (const nil? :Sketchup) :require
            (str _)
          )
        PATTERN


        def_node_matcher :sketchup_extension_new, <<-PATTERN
          (:send
            (:const nil? :SketchupExtension) :new
            _
            (str $_))
        PATTERN

        def_node_search :sketchup_extension_new?, <<-PATTERN
          (:send
            (:const nil? :SketchupExtension) :new
            _
            (str _))
        PATTERN


        def on_send(node)
          if sketchup_require?(node)
            filename = sketchup_require(node)
          elsif sketchup_extension_new?(node)
            filename = sketchup_extension_new(node)
          else
            return
          end
          add_offense(node, location: :expression) unless valid_filename?(filename)
        end

Here are the tests that pass (as expected);

  it 'registers an offense when using `Sketchup.require` with file extension' do
    inspect_source('Sketchup.require("filename.rb")')
    expect(cop.offenses.size).to eq(1)
  end

  context 'SketchupExtension.new' do

    it 'registers an offense when using `SketchupExtension.new` with file extension' do
      inspect_source('extension = SketchupExtension.new("Example", "Example/main.rb")')
      expect(cop.offenses.size).to eq(1)
    end

  end

However, this tests fail with an error;

  it 'does not register an offense when using `require` without file extension' do
    inspect_source('require("filename")')
    expect(cop.offenses).to be_empty
  end

  1) RuboCop::Cop::SketchupSuggestions::SketchupRequire does not register an offense when using `require` without file extension
     Failure/Error: elsif sketchup_extension_new?(node)

     NoMethodError:
       undefined method `type' for nil:NilClass
     # C:3:in `block in sketchup_extension_new?'
     # C:2:in `sketchup_extension_new?'
     # ./lib/rubocop/sketchup/suggestions/sketchup_require.rb:47:in `on_send'
     # ./spec/rubocop/sketchup/suggestions/sketchup_require_spec.rb:38:in `block (2 levels) in <top (required)>'

$ bundle exec rubocop -V
0.53.0 (using Parser 2.5.0.3, running on ruby 2.4.3 i386-mingw32)

(If its of any use, I have the failing test checked into a dev branch here: https://github.com/SketchUp/rubocop-sketchup/tree/dev-sketchup-extension-filetype)

---

Expected behavior

I expected the :sketchup_extension_new and :sketchup_extension_new? patterns to return nil when inspecting inspect_source('require("filename")')

Actual behavior

Instead I'm getting an error:

     NoMethodError:
       undefined method `type' for nil:NilClass
     # C:3:in `block in sketchup_extension_new?'
     # C:2:in `sketchup_extension_new?'
     # ./lib/rubocop/sketchup/suggestions/sketchup_require.rb:47:in `on_send'
     # ./spec/rubocop/sketchup/suggestions/sketchup_require_spec.rb:38:in `block (2 levels) in <top (required)>'

Steps to reproduce the problem

I don't have a simple code snippet, I'm not sure how to exactly reproduce minimally the logic in def_node_matcher.

I have a failing test checked in this repo: https://github.com/SketchUp/rubocop-sketchup/tree/dev-sketchup-extension-filetype

If checking out that dev branch the tests can be run;

bundle exec rspec ./spec/rubocop/sketchup/suggestions/sketchup_require_spec.rb:37:42

When I hooked this up to the debugger I got lost when the code entered def_node_matcher. Any suggestions on how to proceed would be appreciated.

RuboCop version

Include the output of rubocop -V. Here's an example:

$ rubocop -V
0.50.0 (using Parser 2.4.0.0, running on ruby 2.4.2 x86_64-linux)

[thomthom]

I keep running into errors with node matcher.

The latest case is a snippet like this:

module Example
  msg = "Hello World"
  msg += "Foo Bar"
end

A matcher like this:

        def_node_search :sketchup_extension_new, <<-PATTERN
          ({:lvasgn :ivasgn :cvasgn :gvasgn :casgn} ...
            (:send
              (:const nil? :SketchupExtension) :new
              _
              _))
        PATTERN

Inspection like this:

def investigate(processed_source)
  source_node = processed_source.ast
  extension_nodes = sketchup_extension_new(source_node).to_a
  # ...
end

Which leads to error like this:

  1) RuboCop::Cop::SketchupRequirements::SketchupExtension Default source path does not throw an error when inspecting source
     Failure/Error: extension_nodes = sketchup_extension_new(source_node).to_a

     NoMethodError:
       undefined method `type' for :msg:Symbol
     # C:3:in `block in sketchup_extension_new'
     # C:2:in `sketchup_extension_new'
     # ./lib/rubocop/sketchup/requirements/sketchup_extension.rb:44:in `each'
     # ./lib/rubocop/sketchup/requirements/sketchup_extension.rb:44:in `to_a'
     # ./lib/rubocop/sketchup/requirements/sketchup_extension.rb:44:in `investigate'
     # ./spec/rubocop/sketchup/requirements/sketchup_extension_spec.rb:93:in `block (3 levels) in <top (required)>'

My spec to reproduce:

    it 'does not throw an error when inspecting source' do
      inspect_source(['module Example',
                      '  msg = "Hello World"',
                      '  msg += "Foo Bar"',
                      'end'],
                      './src/hello.rb')
      expect(cop.offenses.size).to eq(1)
    end

Any ideas? (I get lost when I try to debug into the matchers.)

[thomthom]

I inspected the code the NodePattern::Compiler generates for:

({:lvasgn :ivasgn :cvasgn :gvasgn} ...
  (:send (:const nil? :SketchupExtension) :new _ _))

After trying add some line breaks for readability it looks like this:

def bugout()
  return nil unless
    (temp1 = temp1 = self
      
      (temp2 = temp2 = temp1
        (temp2.type == :lvasgn) || (temp2.type == :ivasgn) || (temp2.type == :cvasgn) || (temp2.type == :gvasgn)) &&
        
      (temp1.children.size > 0) && (temp3 = temp3 = temp1.children.last
        (temp3.type == :send) && (temp4 = temp4 = temp3.children[0]
          (temp4.type == :const) && (temp4.children[0].nil?) && (temp4.children[1] == :SketchupExtension) && (temp4.children.size == 2)) &&
          (temp3.children[1] == :new) && true && true && (temp3.children.size == 4)))
              block_given? ? yield() : (return true)
    
end

From how I read this:

• temp1 is the object itself, this cannot be nil.
• temp2 is referring to temp1m so temp2.type should also be safe
• temp3 is the last of temp1's children, and there a check for this temp1.children.size > 0
• temp4 on the other hand is assigned as temp4 = temp4 = temp3.children[0] without any checks for there being children. Which makes me suspect temp4.type == :const is throwing the error.

Since the NodePattern::Compiler generate code on the fly it's not possible to step through this with a debugger. What methods of debugging can I use as an alternative?

(Btw, what's the reason behind temp1 = temp1 = and temp2 = temp2 = etc?)

[thomthom]

I have been trying to set up a test in RuboCop for the failure, but to trigger it I need the logic of def_node_search from the Macros mix-in. The tests in node_pattern_spec didn't seem to cover that code path, nor have I found an easy way to do so.

My current speculation is that it relates tocompile_seq_terms_with_size and/or compile_expr_with_index;
https://github.com/bbatsov/rubocop/blob/3323d943ba4233970ddeb8ce07e1676283414dd0/lib/rubocop/node_pattern.rb#L202-L225

compile_expr_with_index appear to be the only expression performing the direct index access which I suspect is throwing the errors:
https://github.com/bbatsov/rubocop/blob/3323d943ba4233970ddeb8ce07e1676283414dd0/lib/rubocop/node_pattern.rb#L222

[thomthom]

I've dug deeper and narrowed down what part of the Compiler generates the code that cause the error;

https://github.com/bbatsov/rubocop/blob/1df6f7d29a9f5084a187a1ffa01af77fcd874db0/lib/rubocop/node_pattern.rb#L351-L353

This code generates something like this:

(temp4.type == :const)

The problem is that when using def_node_search the node being inspected (in this case temp4) might not be of Node type.

I experimented by injecting a type check and that avoided the errors I have been running into;

https://github.com/thomthom/rubocop/blob/61a00d422282c1b411120c78652c544bd1ac17b9/lib/rubocop/node_pattern.rb#L351-L353

I created a branch on my fork that replicate the error in a test and making a change that avoids it:
https://github.com/thomthom/rubocop/tree/dev-issue5656

The diff:

thomthom@61a00d4

      def compile_literal(cur_node, literal, seq_head)
        "(#{cur_node + '.is_a?(RuboCop::AST::Node) && ' if seq_head}#{cur_node}#{'.type' if seq_head} == #{literal})"
      end

All of RuboCop's tests continued to pass after my tweak.

I didn't create a pull request because so far the test is a hack and I'm not sure how this project would ideally set up a test for def_node_search.

I also wasn't sure if it was the correct place to add a type check, of whether it could be elsewhere while traversing nodes for def_node_search.

Any feedback would be welcome, and I can try to tidy this up.

[thomthom]

I had incorrectly used :send and :const instead of send and const in my node pattern. Not sure why they seemed to match some node, but correcting them made the matching work without errors.

[marcandre]

FWIW, I believe #6834 fixed this.