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.

Sign up for GitHub

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

RFC: new Attribute#ivar API #2110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

RFC: new Attribute#ivar API #2110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions lib/rbs/ast/members.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,21 @@ def update(name: self.name, type: self.type, ivar_name: self.ivar_name, kind: se
visibility: visibility
)
end

def ivar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the semantics are clear: You either get back an InstanceVariable object, or nil (meaning this attribute was declared with no backing ivar declaration, e.g. attr_reader foo(): Foo).

Open question: How should we model the difference between these two?

attr_reader a: Foo     # Ivar name is inferred
attr_reader a(@a): Foo # Same ivar name, but explicit

Copy link
Member

Choose a reason for hiding this comment

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

No difference between the two looks fine, if we consider this is a kind of utility class.
If we really need the detail of the syntax, we can use the old API.

@ivar ||= begin
ivar_name = case self.ivar_name
when false
return nil # Skip the instance variable declaration entirely
when nil
:"@#{name}" # Infer the instance variable name from the attribute name
else
self.ivar_name # Use the custom instance variable name given by the user
end

InstanceVariable.new(name: ivar_name, type: type, location: location, comment: comment)
Copy link
Contributor Author

@amomchilov amomchilov Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Open question: what should the location and comment be? The same as the attribute, or nil?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should have the comment attribute, because it's given to the attribute syntax, not to the instance variable name.

Not very sure about location, but I think going without location should work for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still have to fill in the parameters, are you suggesting I pass location: nil, comment: nil?

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I'm sorry. I was confused.

I thought the InstanceVariable is a new class for this API, but actually it is an existing class which models instance variable declaration.

end
end
end

class AttrReader < Base
Expand Down
2 changes: 2 additions & 0 deletions sig/members.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,8 @@ module RBS
include _HashEqual

def update: (?name: Symbol, ?type: Types::t, ?ivar_name: Symbol | false | nil, ?kind: kind, ?annotations: Array[Annotation], ?location: loc?, ?comment: Comment?, ?visibility: visibility?) -> instance

def ivar: () -> InstanceVariable?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simple return type! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it still make sense for ivar_name to accept false in the constructor and the attr_reader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, since there's backwards compatibility concerns there.

We can add a new keyword arg (perhaps explicit_ivar_name:?) and use that, while still supporting the old.

end

class AttrReader < Base
Expand Down
48 changes: 48 additions & 0 deletions test/rbs/signature_parsing_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,13 @@ def self?.three: -> bool
assert_equal :a, m.name
assert_nil m.ivar_name
assert_equal parse_type("Integer"), m.type

ivar = m.ivar
assert_instance_of Members::InstanceVariable, ivar
assert_equal :@a, ivar.name # Inferred from the attribute name
assert_equal m.type, ivar.type
assert_equal m.location, ivar.location
assert_equal m.comment, ivar.comment
end

module_decl.members[11].yield_self do |m|
Expand All @@ -369,6 +376,13 @@ def self?.three: -> bool
assert_equal :a, m.name
assert_equal :@A, m.ivar_name
assert_equal parse_type("String"), m.type

ivar = m.ivar
assert_instance_of Members::InstanceVariable, ivar
assert_equal :@A, ivar.name # Explicitly given ivar name
assert_equal m.type, ivar.type
assert_equal m.location, ivar.location
assert_equal m.comment, ivar.comment
end

module_decl.members[12].yield_self do |m|
Expand All @@ -377,6 +391,8 @@ def self?.three: -> bool
assert_equal :a, m.name
assert_equal false, m.ivar_name
assert_equal parse_type("bool"), m.type

assert_nil m.ivar # The instance variable was explicitly skipped
end

module_decl.members[13].yield_self do |m|
Expand All @@ -385,6 +401,13 @@ def self?.three: -> bool
assert_equal :b, m.name
assert_nil m.ivar_name
assert_equal parse_type("Integer"), m.type

ivar = m.ivar
assert_instance_of Members::InstanceVariable, ivar
assert_equal :@b, ivar.name # Inferred from the attribute name
assert_equal m.type, ivar.type
assert_equal m.location, ivar.location
assert_equal m.comment, ivar.comment
end

module_decl.members[14].yield_self do |m|
Expand All @@ -393,6 +416,13 @@ def self?.three: -> bool
assert_equal :b, m.name
assert_equal :@B, m.ivar_name
assert_equal parse_type("String"), m.type

ivar = m.ivar
assert_instance_of Members::InstanceVariable, ivar
assert_equal :@B, ivar.name # Explicitly given ivar name
assert_equal m.type, ivar.type
assert_equal m.location, ivar.location
assert_equal m.comment, ivar.comment
end

module_decl.members[15].yield_self do |m|
Expand All @@ -401,6 +431,8 @@ def self?.three: -> bool
assert_equal :b, m.name
assert_equal false, m.ivar_name
assert_equal parse_type("bool"), m.type

assert_nil m.ivar # The instance variable was explicitly skipped
end

module_decl.members[16].yield_self do |m|
Expand All @@ -409,6 +441,13 @@ def self?.three: -> bool
assert_equal :c, m.name
assert_nil m.ivar_name
assert_equal parse_type("Integer"), m.type

ivar = m.ivar
assert_instance_of Members::InstanceVariable, ivar
assert_equal :@c, ivar.name # Inferred from the attribute name
assert_equal m.type, ivar.type
assert_equal m.location, ivar.location
assert_equal m.comment, ivar.comment
end

module_decl.members[17].yield_self do |m|
Expand All @@ -417,6 +456,13 @@ def self?.three: -> bool
assert_equal :c, m.name
assert_equal :@C, m.ivar_name
assert_equal parse_type("String"), m.type

ivar = m.ivar
assert_instance_of Members::InstanceVariable, ivar
assert_equal :@C, ivar.name # Explicitly given ivar name
assert_equal m.type, ivar.type
assert_equal m.location, ivar.location
assert_equal m.comment, ivar.comment
end

module_decl.members[18].yield_self do |m|
Expand All @@ -425,6 +471,8 @@ def self?.three: -> bool
assert_equal :c, m.name
assert_equal false, m.ivar_name
assert_equal parse_type("bool"), m.type

assert_nil m.ivar # The instance variable was explicitly skipped
end
end
end
Expand Down
Loading