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

Implement no args Struct.new constructor in Ruby 3.3 #3491

Merged
merged 1 commit into from
Apr 19, 2024
Merged

Implement no args Struct.new constructor in Ruby 3.3 #3491

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
1 change: 1 addition & 0 deletions spec/truffleruby.next-specs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
# Use spec/ruby/core/nil/nil_spec.rb as a dummy file to avoid being empty (what causes mspec to error)
spec/ruby/core/nil/nil_spec.rb

spec/ruby/core/struct/new_spec.rb
spec/ruby/core/warning/element_reference_spec.rb
spec/ruby/core/warning/element_set_spec.rb

Expand Down
2 changes: 1 addition & 1 deletion src/main/ruby/truffleruby/core/struct.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class << self
alias_method :subclass_new, :new
end

def self.new(klass_name, *attrs, keyword_init: nil, &block)
def self.new(klass_name = nil, *attrs, keyword_init: nil, &block)
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem a correct change. And adding more specs would catch the issue.

Optional parameter will always consume the first argument and will be nil only if there are no positional arguments at all. But it isn't how it's supposed to work:

Struct.new(:foo, :bar).name # => nil

Handling the first argument (if any) depends on its type. Non-Symbol argument will be handled as a class name. A Symbol argument - as a member name.

So it's more like:

if klass_name.is_a?(Symbol)
  attrs.unshift(klass_name)
  klass_name = nil
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. We'll add more specs and fix this.

Copy link
Collaborator

@nirvdrum nirvdrum Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@andrykonchin I took another look at this and I'm a little confused. The code appears to already do what you suggest with regards to checking if the first argument is a Symbol and I couldn't see a behavioral difference in the Struct.new(:foo, :bar).name example. Can you please elaborate on what the problem is?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure exactly what Andrii meant but the current logic + optional argument makes it really hard to reason about.

Optional + rest arguments seems quite difficult to follow, how about def self.new(*attrs, keyword_init: nil, &block) and then below check if attrs.first is a Symbol (if so it's an attribute, so nothing special to do, if not try StringValue and if that succeeds use it as the name and .shift).
Another advantage of that is the common case by far is no klass_name, so then we avoid an extra unshift for that case + make the logic clearer.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, my bad. Sorry for such delaying. The PR will be merged ASAP.

if klass_name
if Primitive.is_a?(klass_name, Symbol) # Truffle: added to avoid exception and match MRI
attrs.unshift klass_name
Expand Down
Loading