Merging attributes with annotations fails

[vinistock]

When addressing https://github.com/ruby/prism/pull/1767, I noticed that all of the type annotations associated with attr_reader were being lost. I believe we may be incorrectly merging type signatures for attributes.

Example:

# my_gem/rbi/some_file.rbi

class Foo
  sig { returns(Integer) }
  attr_reader :bar
end

When Tapioca generates the resulting RBI for my_gem, it should include the signature

# sorbet/rbi/gems/[email protected]

class Foo
  sig { returns(Integer) } # this is missing
  def bar; end
end

[Morriar]

This is by design.

When trying to merge two definitions such as this:

# rbi/foo.rbi
class Foo
  attr_reader :foo; end
end

# lib/foo.rb
class Foo
  def foo; end
end

Tapioca will see that the shim says it's an attribute accessor yet the code says it's a method and will discard the shim as it indicate it may be out of date.

If the actual code uses a method the rbi file must use a method as well or the definition will be discarded.

[vinistock]

I'm not sure that was the exact case we were seeing in Prism. The shims under rbi were using attributes

    sig { returns(<%= field.rbi_class %>) }
    attr_reader :<%= field.name %>

And so is the actual code. Both were defined as attributes, but Tapioca seemed to be dropping them.

[Morriar]

Yeah, at the moment it seems that Tapioca can't see if a method comes from an attribute accessor or not.

If you look at the generated RBI for prism this attributes are actually generated as methods. I'm not sure we can easily change this behavior.

But indeed, we could be more lenient when it comes to merging and allow the merging of the signatures. This raises a question though. Should we keep the def of the attr_*? Our guideline right now is always to trust the generated RBI more than the shim but this may need to change.

[paracycle]

In Tapioca, we've always defaulted to generating reader/writer methods instead of attr_xxx declarations, because it is harder to figure out if a reader/writer method was defined via an attr_xxx call, but also because if Tapioca generated attr_xxx declarations, Sorbet would then do more work to turn them into reader/writer method definitions in its rewriter anyway.

Basically, you don't get any advantage from using attr_xxx declarations in RBIs, you actually get slightly worse performance because of the extra rewriter phases that need to happen.

As a result, I think we should:

1. Merge attr_xxx declarations with the corresponding reader/writer method definitions, AND
2. Always leave method definitions behind.

[andyw8]

We ran into this for Prism: https://github.com/Shopify/ruby-lsp/pull/1953#discussion_r1572697529

[amomchilov]

So, my initial inclination here was to first fix the "sig getting dropped problem" in this issue, then make a new issue for the "performance improvement: rewrite attr_* into def methods" (from Ufuk's comment).

After looking into this, I don't think that approach is tenable, and I suggest we go right for rewriting attr_* into def methods, and making sure the sigs work on that.

---

The current implementation says that Attr is incompatible with Method, and vice versa.

This means they're never merged at all. Whatever was on the "left" side of the comparison, is what was kept.

example_tests.rb

# typed: true
# frozen_string_literal: true

require "test_helper"

module RBI
  class NodeMergingTest < Minitest::Test
    LEFT = Rewriters::Merge::Keep::LEFT

    def test_merge_attr_with_attr
      attr1 = Parser.parse_string(<<~RBI)
        attr_reader :a
      RBI

      attr2 = Parser.parse_string(<<~RBI)
        sig { returns(Integer) }
        attr_reader :a
      RBI

      # Attributes merge correctly with other attributes
      assert_equal(attr2.string, attr1.merge(attr2).string)
      assert_equal(attr2.string, attr2.merge(attr1).string)
    end

    def test_merge_attr_and_method_characterization_test
      attr = Parser.parse_string(<<~RBI)
        sig { returns(Integer) }
        attr_reader :a
      RBI

      method = Parser.parse_string(<<~RBI)
        def a; end
      RBI

      # The "left" is just always kept, the right is discarded.
      # Nothing is merged, so the `sig` gets lost.
      assert_equal(attr.string, attr.merge(method, keep: LEFT).string)
      assert_equal(method.string, method.merge(method, keep: LEFT).string)
    end
  end
end

If we wanted to fix the sig getting lost, but still keep the current behaviour of out-putting attr_*, we would need to modify both of these methods to handle both Attr and Method, and merge them. This is more complicated than it's worth IMO, especially when you consider a case like:

sig { returns(Integer) }
attr_reader :a, :b, :c

If this was merged with a tree that had a def a; end, we'd need to pull the a out of that list.

[amomchilov]

Closing this in favour of #1918, which will make it so that we never emit attributes to begin with. When it's all methods, those will automatically merge correctly, without us needing to do anything special.