ProcessedSource#ast_with_comments doesn't differentiate between identical nodes

[dvandersluis]

This spec passes but is strange behaviour:

RSpec.describe RuboCop::AST::ProcessedSource do
  describe '#ast_with_comments' do
    include RuboCop::AST::Sexp

    let(:source) do
      <<~RUBY
        # comment 1
        do_a
        # comment 2
        do_b
        # comment 3
        do_a
      RUBY
    end

    it do
      expect(processed_source.ast_with_comments).to include(
        s(:send, nil, :do_a) => [instance_of(Parser::Source::Comment), instance_of(Parser::Source::Comment)],
        s(:send, nil, :do_b) => [instance_of(Parser::Source::Comment)]
      )
    end
  end
end

Here is the actual hash created:

{
  s(:send, nil, :do_a) => [
    #<Parser::Source::Comment ast/and_node_spec.rb:1:1 "# comment 1">,
    #<Parser::Source::Comment ast/and_node_spec.rb:5:1 "# comment 3">
  ],
  s(:send, nil, :do_b) => [
    #<Parser::Source::Comment ast/and_node_spec.rb:3:1 "# comment 2">
  ]
}

Because there are two s(:send, nil, :do_a) nodes, both comments are associated with the same node in the hash, and then when this is used in rubocop (for instance CommentsHelp.begin_pos_with_comment), it returns potentially the wrong comment node.

I don't know that there is a good way to fix this necessarily without changing how the keys are constructed; this might also not be a RuboCop::AST bug directly either.

[marcandre]

Indeed, this is surprising 😅

This was noted in whitequark/parser#184: "Note that {associate} produces unexpected result for nodes which are equal but have distinct locations; comments for these nodes are merged."

The proposed fix was associate_locations that does the same but uses the location as key.

IMO, a better method than either of them would be associate_by_identity, which does not use eql? but equal?. I'm proposing a PR for this: whitequark/parser#798

I believe that we could modify our ast_with_comments to use that without any incompatibility, but I didn't double check yet.

[marcandre]

As I expected, tests pass on master.
Released in 1.4.2 👍

[marcandre]

Released 1.5.0...

[dvandersluis]

Thanks for the quick work @marcandre! 🎉