Feature #19261: `Data#members` is not important

• Status: Open
• Assignee: -
• Target version: -

[ko1 (Koichi Sasada)]

Data#members is defined but it is calculated by self.class.members (in other words, #members is a shorthand for self.class.members).
So it is better to remove this method.

P = Data.define(:x, :y)
p P.new(1, 2).members #=> [:x, :y]

Group = Data.define(:name, :members)
gs = Group.new('SasadaFamily', %w(ko1 yuki))
p gs.members #=> ["ko1", "yuki"]

[fabiormoura (Fabio Moura Maia)]

• File data-members-remove-method.patch data-members-remove-method.patch added

I've attached a patch with the changes to remove the aforementioned method. Also, I run the test suite in test_data.rb to validate my changes but there was one test failure that I'm unable to explain. However, the test failure looks to be unrelated:

% bin/ruby ../ruby/test/ruby/test_data.rb
Ignoring debug-1.7.1 because its extensions are not built. Try: gem pristine debug --version 1.7.1
Ignoring rbs-2.8.3 because its extensions are not built. Try: gem pristine rbs --version 2.8.3
Loaded suite ../ruby/test/ruby/test_data
Started
.E
==============================================================================================================================================================================================================================================================
Error: test_define_edge_cases(TestData): NoMethodError: undefined method `<=' for /duplicate member/:Regexp
../ruby/test/ruby/test_data.rb:44:in `test_define_edge_cases'
     41:     assert_same(x, o.b!)
     42:
     43:     assert_raise(ArgumentError) { Data.define(:x=) }
  => 44:     assert_raise(ArgumentError, /duplicate member/) { Data.define(:x, :x) }
     45:   end
     46:
     47:   def test_define_with_block
==============================================================================================================================================================================================================================================================
...........

I've not been able to reproduce the error with this simple ad-hoc script:

% bin/ruby -e "Data.define(:x, :x)"
-e:1:in `define': duplicate member: x (ArgumentError)

Data.define(:x, :x)
            ^^^^^^
	from -e:1:in `<main>'
% bin/ruby -e "Data.define(:x, :x)"
-e:1:in `define': duplicate member: x (ArgumentError)

Data.define(:x, :x)

[k0kubun (Takashi Kokubun)]

If we were to remove this from Data instances, I'd like Struct instances to not have that method either for consistency. Let's say you replace Struct.new with Data.define for something that's already immutable, I don't want that change to break anything. But Data randomly lacking some shorthand methods that exist for Struct would make such refactoring harder.

[fabiormoura (Fabio Moura Maia)]

• File data-and-struct-remove-members-method.patch data-and-struct-remove-members-method.patch added

k0kubun (Takashi Kokubun) wrote in #note-2:

If we were to remove this from Data instances, I'd like Struct instances to not have that method either for consistency. Let's say you replace Struct.new with Data.define for something that's already immutable, I don't want that change to break anything. But Data randomly lacking some shorthand methods that exist for Struct would make such refactoring harder.

I'm a newbie at ruby's codebase, but this sounds a reasonable suggestion so, I've attached a revised patch with the requested modifications. Both test suites for Data (test_data.rb) and Struct (test_struct.rb) passed.

build % make test-all TESTS=ruby/test_struct.rb
Run options:
  --seed=7424
  "--ruby=./miniruby -I../ruby/lib -I. -I.ext/common  ../ruby/tool/runruby.rb --extout=.ext  -- --disable-gems"
  --excludes-dir=../ruby/test/excludes
  --name=!/memory_leak/

# Running tests:

Finished tests in 0.037040s, 2591.7927 tests/s, 14848.8121 assertions/s.
96 tests, 550 assertions, 0 failures, 0 errors, 0 skips

ruby -v: ruby 3.3.0dev (2023-01-08T15:02:29Z master 8f6a9ad35d) [x86_64-darwin21]
build % make test-all TESTS=ruby/test_data.rb
Run options:
  --seed=63192
  "--ruby=./miniruby -I../ruby/lib -I. -I.ext/common  ../ruby/tool/runruby.rb --extout=.ext  -- --disable-gems"
  --excludes-dir=../ruby/test/excludes
  --name=!/memory_leak/

# Running tests:

Finished tests in 0.008879s, 1464.1288 tests/s, 12726.6584 assertions/s.
13 tests, 113 assertions, 0 failures, 0 errors, 0 skips

ruby -v: ruby 3.3.0dev (2023-01-08T15:02:29Z master 8f6a9ad35d) [x86_64-darwin21]

[zverok (Victor Shepelev)]

But Data randomly lacking some shorthand methods that exist for Struct would make such refactoring harder.

Yes, that was the only reason I preserved the method in Data. Don't recall using it myself, but don't see harm in its existence either.