Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rewriter to transform attributes into methods #308

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

amomchilov
Copy link
Contributor

@amomchilov amomchilov commented Jun 6, 2024

Part of Shopify/tapioca#1918

This PR introduces a new rewriter which transforms attr_reader/attr_writer/attr_accessor RBIs into methods with the corresponding sigs. This has two benefits:

  1. It naturally solves Merging attributes with annotations fails tapioca#1709. If Tapioca only has to deal with methods (which can already be merged), then we don't have to write bespoke logic to merge attributes with methods, or methods with attributes.
  2. It'll leave less work for Sorbet, which internally rewrites attributes into methods. We can do this on its behalf, once up-front, rather than on every parse of the RBIs.

class AttrWriter
sig { returns(T::Array[Method]) }
def convert_to_methods
raise "How can there be multiple?" unless sigs.one?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sig overloads are allowed in RBI files: sorbet/sorbet#7412

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for attributes, though. Sorbet.run

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In .rbi files only. With sorbet.run you're testing with a .rb file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, looks like it allows it, but the revealed type is whichever one you listed first. I think we should warn in that case, it likely wasn't intended.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently do not do any linting / warning inside rbi, it's up to the client to make sure that what is produced is correct.

This could be a warning in Tapioca when pulling the gem's hand-written annotations. Or it could be done by Sorbet.

@amomchilov amomchilov force-pushed the Alex/rewrite-attributes branch from 61d982d to a425158 Compare June 6, 2024 16:13
lib/rbi/model.rb Outdated Show resolved Hide resolved
test/rbi/rewriters/replace_attributes_with_methods_test.rb Outdated Show resolved Hide resolved
lib/rbi/rewriters/replace_attributes_with_methods.rb Outdated Show resolved Hide resolved
lib/rbi/rewriters/replace_attributes_with_methods.rb Outdated Show resolved Hide resolved
lib/rbi/rewriters/replace_attributes_with_methods.rb Outdated Show resolved Hide resolved
test/rbi/rewriters/replace_attributes_with_methods_test.rb Outdated Show resolved Hide resolved
@amomchilov
Copy link
Contributor Author

amomchilov commented Jun 6, 2024

Here were the previously open questions. Spoke offline, decisions in-line.

1. Which attr_writer sig should we accept?

Decision: Be flexible, and accept both .void and .returns(Type). Reevaluate if Sorbet changes its behaviour to prefer one or the other

Sorbet accepts both of these (should it?):

sig { params(a: Integer).void }
attr_writer :a

sig { params(b: Integer).returns(Integer) }
attr_writer :b

Sorbet.run

According to Sorbet, the result of these expressions is always just whatever is assigned to them, ignoring the return type of the sig (so perhaps we should only accept void?):

result_of_a = (self.a = 123)
T.reveal_type(result_of_a) # => Integer

result_of_b = (self.b = 123)
T.reveal_type(result_of_b) # => Integer

But the writer methods themselves do actually return the value:

class C
  attr_writer :foo
end

C.new.send(:foo=, 123) # => 123

So both seem reasonable. Should we permissively accept either one?

2. Which sig should we generate for writers?

Decision: Always emit .void.

It's not obvious which of these would be better. Consider "normal" Ruby code (not RBIs), which needs to implement a setter method:

class C
  def s=(_)
    "This return value isn't normally used"
  end
end

object = C.new

result_of_setter = (object.s = "This value is always returned")
result_of_setter # => "This value is always returned"

# ... makes sense, except:

result_of_send = object.send(:s=, "Now this isn't returned")
result_of_send # => "This return value isn't normally used"
  • From the caller perspective .returns(String) is correct
  • But from an implementer's side, .void is correct (you're not obligated to return a string)

@amomchilov amomchilov force-pushed the Alex/rewrite-attributes branch 5 times, most recently from b8d34ae to d321f86 Compare June 7, 2024 18:17
lib/rbi/model.rb Outdated Show resolved Hide resolved
lib/rbi/model.rb Outdated Show resolved Hide resolved
lib/rbi/model.rb Outdated Show resolved Hide resolved
comments: T::Array[Comment],
).returns(Method)
end
def create_setter_method(name, sig, attribute_type, visibility, loc, comments) # rubocop:disable Metrics/ParameterLists
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 params, but the upper limit is 5. Worth overriding?

test/rbi/rewriters/replace_attributes_with_methods_test.rb Outdated Show resolved Hide resolved
@amomchilov amomchilov requested a review from Morriar June 7, 2024 19:01
@amomchilov amomchilov marked this pull request as ready for review June 7, 2024 19:01
@amomchilov amomchilov requested a review from a team as a code owner June 7, 2024 19:01
@amomchilov amomchilov requested a review from KaanOzkan June 7, 2024 19:01
lib/rbi/model.rb Outdated Show resolved Hide resolved
lib/rbi/model.rb Outdated Show resolved Hide resolved
lib/rbi/model.rb Outdated Show resolved Hide resolved
lib/rbi/model.rb Outdated Show resolved Hide resolved
lib/rbi/rewriters/replace_attributes_with_methods.rb Outdated Show resolved Hide resolved
lib/rbi/rewriters/replace_attributes_with_methods.rb Outdated Show resolved Hide resolved
lib/rbi/rewriters/replace_attributes_with_methods.rb Outdated Show resolved Hide resolved
lib/rbi/rewriters/replace_attributes_with_methods.rb Outdated Show resolved Hide resolved
lib/rbi/rewriters/replace_attributes_with_methods.rb Outdated Show resolved Hide resolved
@amomchilov amomchilov force-pushed the Alex/rewrite-attributes branch 3 times, most recently from 866256c to f1f606f Compare June 17, 2024 20:39
test/rbi/rewriters/replace_attributes_with_methods_test.rb Outdated Show resolved Hide resolved
def visit(node)
case node
when Tree
visit_all(node.nodes)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why does this work correctly without needing a .dup?

Suggested change
visit_all(node.nodes)
visit_all(node.nodes.dup)

There seems to be a risk of concurrently modifying an array while it's being iterated by the visitor. Perhaps we just don't hit that edge case in our tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need a test iterating over multiple nodes:

class Foo
  def foo; end
  attr_accessor :bar
  def baz; end
end

Copy link
Contributor Author

@amomchilov amomchilov Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I tried that, and a bunch of more complex cases, and they all work fine lol

Copy link
Contributor Author

@amomchilov amomchilov Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's needed, so I added.

This case reproduces it:

      rbi = Parser.parse_string(<<~RBI)
        class Foo
          sig { returns(Integer) }
          attr_reader :i1

          sig { returns(Integer) }
          attr_reader :i2
        end
      RBI

I thought I by alternating def and attr I'd have the highest chance of hitting it, but in fact, that's the only safe configuration haha

lib/rbi/model.rb Outdated Show resolved Hide resolved
def visit(node)
case node
when Tree
visit_all(node.nodes)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need a test iterating over multiple nodes:

class Foo
  def foo; end
  attr_accessor :bar
  def baz; end
end

lib/rbi/rewriters/attr_to_methods.rb Outdated Show resolved Hide resolved
lib/rbi/rewriters/attr_to_methods.rb Outdated Show resolved Hide resolved
lib/rbi/rewriters/attr_to_methods.rb Outdated Show resolved Hide resolved
case node
when Tree
visit_all(node.nodes.dup)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change


module RBI
class UnexpectedMultipleSigsError < Error
attr_reader :node
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
attr_reader :node
sig { returns(Node) }
attr_reader :node

@amomchilov amomchilov force-pushed the Alex/rewrite-attributes branch from b1807ac to e572c2c Compare July 17, 2024 13:50
@amomchilov amomchilov enabled auto-merge July 17, 2024 13:50
@amomchilov amomchilov force-pushed the Alex/rewrite-attributes branch from e572c2c to c47cd74 Compare July 17, 2024 15:07
@amomchilov amomchilov merged commit ec75e3b into main Jul 17, 2024
8 checks passed
@amomchilov amomchilov deleted the Alex/rewrite-attributes branch July 17, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants