Merging attributes with annotations fails

[vinistock]

When addressing ruby/prism#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/my_gem@0.1.0.rbi

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: Shopify/ruby-lsp#1953 (comment)