Project

General

Profile

Bug #13249

Access modifiers don't have an effect inside class methods in Ruby >= 2.3

Added by abotalov (Andrei Botalov) almost 3 years ago. Updated about 2 months ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
2.3.0, 2.4.0
[ruby-core:79751]

Description

Simple example:

class C
  def self.foo
    private
    def bar
    end
  end
end
C.foo
C.new.bar

This code runs fine on Ruby 2.3 and Ruby 2.4. It raises NoMethodError on Ruby 2.2 and prior versions.
I would expect an error to be raised.

Here is some code that actually uses private access modifier inside a class method - https://github.com/evolve75/RubyTree/blob/db48c35b0a3b96e4da473b095cc00e454d8a9996/lib/tree/utils/camel_case_method_handler.rb#L60

By the way, this code raises an error as expected on Ruby 2.3 and Ruby 2.4:

class C
  def self.foo
    private def bar
    end
  end
end
C.foo
C.new.bar # NoMethodError: private method `bar' called

Files

warn-scope-visibility-in-method-13249.patch (4.18 KB) warn-scope-visibility-in-method-13249.patch jeremyevans0 (Jeremy Evans), 08/25/2019 12:23 AM

Related issues

Related to Ruby master - Bug #11571: シングルトンメソッドの中で def を使用した時の可視性が変わっているClosedActions
Related to Ruby master - Bug #11754: Visibility scope is kept after lexical scope is closedClosedActions

Associated revisions

Revision 2993b24a
Added by jeremyevans (Jeremy Evans) about 2 months ago

Warn for calling public/protected/private/module_function without arguments inside method

Calling these methods without an argument does not have the
desired effect inside a method.

Fixes [Bug #13249]

History

Updated by abotalov (Andrei Botalov) almost 3 years ago

  • Description updated (diff)

Updated by abotalov (Andrei Botalov) almost 3 years ago

  • Description updated (diff)
#3

Updated by nobu (Nobuyoshi Nakada) almost 3 years ago

  • Related to Bug #11571: シングルトンメソッドの中で def を使用した時の可視性が変わっている added
#4

Updated by nobu (Nobuyoshi Nakada) almost 3 years ago

  • Related to Bug #11754: Visibility scope is kept after lexical scope is closed added

Updated by nobu (Nobuyoshi Nakada) almost 3 years ago

Hmmm, a class singleton method should have its own visibility per invocations?

Updated by abotalov (Andrei Botalov) almost 3 years ago

Well, ability to declare private methods inside class methods seems strange given that it's not possible to declare private methods inside instance methods:

class C
  def foo
    private def bar
    end
  end
end
C.new.foo # NoMethodError is raised

So I'm not sure which route would be better.

Updated by shevegen (Robert A. Heiler) almost 3 years ago

The examples confuse me a bit.

Does private actually make sense on any class-method / singleton method?

I understand it as a limitation for methods on the class, where outside
calls are not allowed, only internal ones (though ruby allows one to
bypass these anyway via .send).

I am also confused by the second example:

class C
  def self.foo
    private def bar
    end
  end
end
C.foo
C.new.bar

Is that not equivalent to

private
def self.bar

?

So why would this work on the C.new.bar() level?

It is however had interesting that this worked on 2.2 and below but
was changed past that point.

Updated by abotalov (Andrei Botalov) almost 3 years ago

  • Description updated (diff)

Updated by shyouhei (Shyouhei Urabe) over 2 years ago

  • Assignee set to ko1 (Koichi Sasada)
  • Status changed from Open to Assigned

We looked at this issue in yesterday's developer meeting.

The use of private in evolve75/RubyTree shown in the description is in fact wrong (methods are defined in a wrong place). That example made us think that the use of private in a method is a code smell.

We would forbid such usage in a future. For the time being, let us show warning message.

Updated by ko1 (Koichi Sasada) over 2 years ago

I will insert warning.
I will not change the current behavior.

Updated by jeremyevans0 (Jeremy Evans) 4 months ago

The warning discussed has not been added yet. Attached is a patch that implements it. While it passes make check, there may be corner cases it doesn't handle. I would appreciate review of the changes to vm_cref_set_visibility.

By adding this warning, I found a related issue in irb, which I submitted a pull request for: https://github.com/ruby/irb/pull/23

Updated by mame (Yusuke Endoh) about 2 months ago

  • Assignee changed from ko1 (Koichi Sasada) to nobu (Nobuyoshi Nakada)

nobu (Nobuyoshi Nakada) Could you review the patch?

Updated by nobu (Nobuyoshi Nakada) about 2 months ago

  • Is it necessary that check is placed inside the function vm_cref_set_visibility?
    What about calling a separate function where check_method flag is 1?

  • rb_frame_callee returns the called name, that may be an aliased name.
    Is it intentional?

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

nobu (Nobuyoshi Nakada) wrote:

  • Is it necessary that check is placed inside the function vm_cref_set_visibility? What about calling a separate function where check_method flag is 1?

I agree, that makes more sense.

  • rb_frame_callee returns the called name, that may be an aliased name. Is it intentional?

No. It would be better to use rb_frame_this_func instead, I think.

Thank you very much for your review. I've added the modified patch as a pull request (https://github.com/ruby/ruby/pull/2562). Assuming it passes CI, I will merge it.

#15

Updated by jeremyevans (Jeremy Evans) about 2 months ago

  • Status changed from Assigned to Closed

Applied in changeset git|2993b24a1ecc5fa3cc9f140bfd80669c3a3b7b9c.


Warn for calling public/protected/private/module_function without arguments inside method

Calling these methods without an argument does not have the
desired effect inside a method.

Fixes [Bug #13249]

Also available in: Atom PDF