Provide a way to declare visibility of attributes defined by attr* methods in a single expression

[radarek]

Description

Many of us (me included) declare class attributes, even if they are private, on the top of class definition. When reading source code it's convinient to see what kind of attributes class has.

To declare private attributes we can:

• declare them with one of attr* methods and later change visiblity calling private
• call private without argument, then declare attributes and finally call (in most cases) public to keep defining public methods
• declare attribute on top of the class but make them private in private section later in a file

clsss Foo
  attr_accessor :foo
  private :foo, :foo= # we have to remember about :foo= too

  private

  attr_accessor :bar

  public

  # rest of the code
end

To simplify it and create other possibilites I propose to:

• change attr* methods so as they return array of defined methods names
• allow public/protected/private methods to receive array of methods names (single argument)

With requested feature we could write code like this:

class Foo
  private attr_accessor :foo, :bar
end

Additionaly you could use attr* with your own methods. Something like this:

class Module
  def traceable(names)
    # ...
    names
  end
end

class Foo
  traceable attr_accessor :foo
  # it can be mixed with public/protected/private too
  protected traceable attr_accessor :bar
end

Backward compatibility

• attr* methods currently return nil so there should be no problem with changing them
• public/protected/private methods receive multiple positional arguments and convert all non symbol/string objects to strings. I can imagine only one case where compatibility would be broken:

class Foo
  def foo; end
  def bar; end

  arr = [:foo]
  def arr.to_str
    'bar'
  end

  private arr
end

p [Foo.public_instance_methods(false), Foo.private_instance_methods(false)]

Currently [[:foo], [:bar]] would be displayed, [[:bar], [:foo]] after requested feature is implemented.

[marcandre]

Could you please create an issue on bugs.ruby-lang.org ?

I'll even second your request

[radarek]

I've already created issue but I forgot to post it here. Thanks for reminding me.

https://bugs.ruby-lang.org/issues/17314

[marcandre]

Great news, this was accepted by Matz 🎉

Missing from the PR:

• Module#alias_method returning the symbol for new alias
• Module#public, protected and private to accept Array.

Did you want to amend this PR and do you have time to do it quickly? Otherwise we can merge this and add the rest in another commit.

[radarek]

This is great news!

Module#public, protected and private to accept Array.

It is covered in this PR. Am I missing something?

Module#alias_method returning the symbol for new alias

I will add this.

[marcandre]

This is great news!

Module#public, protected and private to accept Array.

It is covered in this PR. Am I missing something?

My mistake 🤦‍♂️

Module#alias_method returning the symbol for new alias

I will add this.

Thanks!

Also missing, if you want to add: entries in NEWS.md

[radarek]

@marcandre I pushed 2 commits (I will squash all of them when this PR will satisfy all requirements) with following changes:

• replace strict array type checkin in favour of rb_check_array_type
• alias_method now returns symbol instead of module/class
• added usage of ruby version guard in changed/added rubyspecs previously - now they should pass on all ruby versions

[marcandre]

Thanks for the quick changes.
I checked more closely the doc and there are some updates to do...

[radarek]

@marcandre Thank you for review. I made all requested changes. Additionaly I added rdoc for returned type to Module#{public,protected,private} methods. Let me know If anything should be changed.

[marcandre]

LGTM. I need sleep; maybe others will have a look, otherwise I intend to commit in a day or two...

[radarek]

Thanks for your assist. Should I squash commits or will it be done during merging?

[marcandre]

Oh, I got distracted, sorry. Could you please squash your commits and rebase to resolve the conflict with NEWS?

[radarek]

No problem. I've just done it.

[marcandre]

Merged, thank you for the PR and your prompt updates ♥️

[mame]

Hi,

Unfortunately, a CI (Solaris10 with gcc) started failing randomly after this commit 81739ad.

http://rubyci.s3.amazonaws.com/solaris10-gcc/ruby-master/recent.html

  1) Error:
TestAutoload#test_autoload_same_file:
fatal: No live threads left. Deadlock?
3 threads, 3 sleeps current:0x02681d58 main thread:0x003c3a88
* #<Thread:0x00483d20 sleep_forever>
   rb_thread_t:0x003c3a88 native:0x00000001 int:0
   
* #<Thread:0x02663f40 -:9 sleep_forever>
   rb_thread_t:0x02681d58 native:0x00000002 int:0
    depended by: tb_thread_id:0x003c3a88
   
* #<Thread:0x02663e38 -:10 sleep_forever>
   rb_thread_t:0x0259bbd0 native:0x00000003 int:0 mutex:0x0260e0f8 cond:1
   

    /export/home/users/chkbuild/cb-gcc/tmp/build/20201217T190004Z/ruby/test/ruby/test_autoload.rb:400:in `join'
    /export/home/users/chkbuild/cb-gcc/tmp/build/20201217T190004Z/ruby/test/ruby/test_autoload.rb:400:in `<main>'
    /export/home/users/chkbuild/cb-gcc/tmp/build/20201217T190004Z/ruby/test/ruby/test_autoload.rb:394:in `block (2 levels) in test_autoload_same_file'
    /export/home/users/chkbuild/cb-gcc/tmp/build/20201217T190004Z/ruby/test/ruby/test_autoload.rb:393:in `times'
    /export/home/users/chkbuild/cb-gcc/tmp/build/20201217T190004Z/ruby/test/ruby/test_autoload.rb:393:in `block in test_autoload_same_file'
    /export/home/users/chkbuild/cb-gcc/tmp/build/20201217T190004Z/ruby/lib/tmpdir.rb:96:in `mktmpdir'
    /export/home/users/chkbuild/cb-gcc/tmp/build/20201217T190004Z/ruby/test/ruby/test_autoload.rb:387:in `test_autoload_same_file'

Finished tests in 1206.255071s, 17.0851 tests/s, 2204.1839 assertions/s.
20609 tests, 2658808 assertions, 0 failures, 1 errors, 71 skips

I'm now investigating the issue on the CI environment, but I have not been able to reproduce this issue manually. As far as I see, the commit itself looks benign, but to isolate the problem, I may revert the commit temporarily. Thank you for your understanding.

[marcandre]

Oh, interesting. I'm curious to see what could possible cause this in the commit. Thank you so much for looking into it @mame

[mame]

I've reverted the commit 81739ad and related ones (34f0606, e042e84, and 66090b9), and now the CI succeeded.

http://rubyci.s3.amazonaws.com/solaris10-gcc/ruby-master/log/20201218T080004Z.fail.html.gz

The failure is flaky, so I'll watch a few successive builds. I guess this changeset may include something bad, though I can't find offhand... I'll check it deeply tonight.

[mame]

If possible, it would be helpful to split the changeset to each part (making attr* return symbols, making public accept symbol array, and making alias_method return a symbol) and to commit and test each by each.

[radarek]

@mame Will it be sufficient if I split this PR into multiple commits? Or should I commit only 1 change, wait for CI, commit 2nd change, wait for CI and so on? Also, should it be done in this PR (which is already merged) or create new one?

[mame]

The CI build has been executed six times since I reverted, and no failure occurred so far. So, I think this PR includes something bad. However, I've gone through this PR and I couldn't find any wrong code. Solaris 10 uses GCC 4 which is very old, so it might accidentally trigger a compiler bug, but I'm unsure.

Or should I commit only 1 change, wait for CI, commit 2nd change, wait for CI and so on?

I think this approach is good. Could you please create a new PR for one change (whichever you like)? Thank you for your ooperation.

[radarek]

Ok, no problem. I created new PR#3934 and will push each change as separate commit to find out which one fails on CI.