on_index, on_indexasgn, on_lambda callbacks seem to be missing?

[alfondotnet]

---

Expected behavior

Have callbacks available for whitequark/parser@7d72eba and _type?

Actual behavior

Above callbacks don't seem to be called for appropiate nodes

Steps to reproduce the problem

module RuboCop
  module Cop
    module Deprecation
       class MutatedStrings < Cop
           MSG = "Don't mutate strings!"
           
           def on_indexasgn(node)
              binding.pry
           end
        end
       end
    end
  end
end

And a quick test with:

  it 'register an offense when indexed assignment is being used on a string' do
    expect_offense(<<~RUBY)
      foo[1] = 'c'
    RUBY
 end

It appears that the callback is not being called, nor a node as indexasgn_type? available

RuboCop version

$ rubocop -V
0.57.2 (using Parser 2.5.1.0, running on ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux]

I am fairly new to ruby and definitely to rubocop, so excuse the issue / vs potential PR, I would love to contribute with some tips!.

[bbatsov]

First time I hear about those callbacks. :-) I guess we'll need to update Commissioner to support them.

[alfondotnet]

First time I hear about those callbacks. :-) I guess we'll need to update Commissioner to support them.

I will totally try to take a stab at this then!

[Ruffeng]

Hi! I can try to have a look!

[Drenmi]

First time I hear about those callbacks.

They were only added a few months back. A lot of good work being done on parser. 🙂

[Ruffeng]

After checking a while, I am not feeling confident to understand what's going on with AST, and therefore apply these 3 rules. Sorry

[Drenmi]

Thanks anyway @Ruffeng. Feel free to chip in on other parts of the codebase. 🙂

[paolobrasolin]

Hi @bbatsov and @Ruffeng , I'm trying to tackle this.

I think the reason for the missing callbacks is that the most recent AST format is opt-in: https://github.com/whitequark/parser#usage

This has two consequences:

• one must explicitly activate it - https://github.com/paolobrasolin/rubocop/commit/02ef76c9a691f2c2da58b1a7aa952b595d536cee#diff-0bd5991adbd8e115afc676f92b3fd224R18
• the node types are missing from Parser::Meta::NODE_TYPES so they must be added - https://github.com/paolobrasolin/rubocop/commit/02ef76c9a691f2c2da58b1a7aa952b595d536cee#diff-14e01ec7acd1c0ac3dfabe7a3a357d69R74 and https://github.com/paolobrasolin/rubocop/commit/02ef76c9a691f2c2da58b1a7aa952b595d536cee#diff-553b19f5326d7b15f2364680ae682341R29

Once that is done, the following tests do pass: https://github.com/paolobrasolin/rubocop/commit/02ef76c9a691f2c2da58b1a7aa952b595d536cee#diff-a5c2712427b22a714e59aaebd99527c1

There is good reason for the feature to be opt-in: it is breaking, and in fact 268 specs fail.

The reason can be deduced by the documentation in https://github.com/whitequark/parser/blob/master/lib/parser/builders/default.rb. TL;DR: Parser::Builders::Default.modernize activates four flags and

• if Parser::Builders::Default.emit_lambda == true then -> {} is emitted as

  s(:block, s(:lambda), s(:args), nil)
  

  instead of

  s(:block, s(:send, nil, :lambda), s(:args), nil)
  

• if Parser::Builders::Default.emit_procarg0 == true then m { |a| } is emitted as

  s(:args, s(:procarg0, :a))
  

  instead of

  s(:args, s(:arg, :a))
  

• if Parser::Builders::Default.emit_encoding == true then __ENCODING__ is emitted as

  s(:__ENCODING__)
  

  instead of

  s(:const, s(:const, nil, :Encoding), :UTF_8)
  

• if Parser::Builders::Default.emit_index == true then self[1] is emitted as

  s(:index, s(:self), s(:int, 1))
  

  instead of

  s(:send, s(:self), :[], s(:int, 1))
  

• if Parser::Builders::Default.emit_index == true then self[1] = 2 is emitted as

  s(:indexasgn, s(:self), s(:int, 1), s(:int, 2))
  

  instead of

  s(:send, s(:self), :[]=, s(:int, 1), s(:int, 2))
  

So presumably there is fixing to do wherever there is some assumption on the emission of trees like these.

I think this broadly defines the direction for a solution, but there clearly is a bunch of work to do.
I'd like to contribute and take care of that, but I have some questions.

• The existence of these features depend upon the version of parser - should the minimum required version be bumped up?
• If that is not the case, should these features be opt-in in rubocop too (i.e. should the implications be handled conditionally everywhere)?
• I sketched the classification of nodes for AST traversal, but I'm new to the codebase so I'd like a double check on that: https://github.com/paolobrasolin/rubocop/commit/02ef76c9a691f2c2da58b1a7aa952b595d536cee#diff-0e6728539dbdfb2c60ab2ca784fc75ec

Thanks. ❤️

[marcandre]

FYI, the missing NODETYPES have just been added.

[paolobrasolin]

Thanks @marcandre! This will make things easier and much cleaner when I get back on this.

[marcandre]

Transferred to rubocop-ast. I'll handle it.