https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17097754782016-11-19T13:18:28ZRuby Issue Tracking SystemRuby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=615852016-11-19T13:18:28Zmatthewd (Matthew Draper)matthew@trebex.net
<ul></ul><p>Draft implementation:</p>
<pre><code class="diff syntaxhl" data-language="diff"><span class="gh">diff --git a/include/ruby/intern.h b/include/ruby/intern.h
index 8776a59..fa5b1dc 100644
</span><span class="gd">--- a/include/ruby/intern.h
</span><span class="gi">+++ b/include/ruby/intern.h
</span><span class="p">@@ -588,6 +588,7 @@</span> VALUE rb_any_to_s(VALUE);
VALUE rb_inspect(VALUE);
VALUE rb_obj_is_instance_of(VALUE, VALUE);
VALUE rb_obj_is_kind_of(VALUE, VALUE);
<span class="gi">+VALUE rb_obj_is_friend_of(VALUE, VALUE);
</span> VALUE rb_obj_alloc(VALUE);
VALUE rb_obj_clone(VALUE);
VALUE rb_obj_dup(VALUE);
<span class="gh">diff --git a/object.c b/object.c
index 05bef4d..257db8a 100644
</span><span class="gd">--- a/object.c
</span><span class="gi">+++ b/object.c
</span><span class="p">@@ -698,6 +698,41 @@</span> rb_class_search_ancestor(VALUE cl, VALUE c)
return class_search_ancestor(cl, RCLASS_ORIGIN(c));
}
<span class="gi">+/*
+ *
+ */
+
+VALUE
+rb_obj_is_friend_of(VALUE obj, VALUE c)
+{
+ VALUE defined_class;
+
+ c = class_or_module_required(c);
+
+ defined_class = RB_TYPE_P(c, T_ICLASS) ? RBASIC(c)->klass : rb_class_real(c);
+ if (rb_obj_is_kind_of(obj, defined_class)) return Qtrue;
+
+ while (c) {
+ VALUE mod = RB_TYPE_P(c, T_ICLASS) ? RBASIC(c)->klass : c;
+ VALUE ary = rb_ivar_get(mod, rb_intern_const("friends"));
+
+ if (RB_TYPE_P(ary, T_ARRAY)) {
+ int i;
+ long len = RARRAY_LEN(ary);
+
+ for (i=0; i<len; i++) {
+ VALUE friend = RARRAY_AREF(ary, i);
+
+ if (rb_obj_is_kind_of(obj, friend)) return Qtrue;
+ }
+ }
+
+ c = RCLASS_SUPER(c);
+ }
+
+ return Qfalse;
+}
+
</span> /*
* call-seq:
* obj.tap{|x|...} -> obj
<span class="p">@@ -1529,6 +1564,61 @@</span> rb_mod_to_s(VALUE klass)
return rb_str_dup(rb_class_name(klass));
}
<span class="gi">+/*
+ * call-seq:
+ * mod.friend(module, ...) -> mod
+ *
+ * Makes the given modules friends of <i>mod</i>. Instances of friends
+ * are permitted to call protected methods on instances of <i>mod</i>.
+ */
+
+static VALUE
+rb_mod_friend(int argc, const VALUE *argv, VALUE mod)
+{
+ VALUE ary;
+ int i;
+
+ ID id = rb_intern_const("friends");
+ ary = rb_ivar_get(mod, id);
+ if (!RB_TYPE_P(ary, T_ARRAY)) ary = rb_ary_new();
+ for (i = 0; i < argc; i++) {
+ rb_ary_push(ary, class_or_module_required(argv[i]));
+ }
+ rb_ivar_set(mod, id, ary);
+ return Qnil;
+}
+
+/*
+ * call-seq:
+ * mod.friends -> array
+ *
+ * Returns the list of modules and classes whose instances are
+ * permitted to call protected methods on <i>mod</i>. The returned list
+ * does not include <i>mod</i>, which is always considered a friend of
+ * itself.
+ */
+
+static VALUE
+rb_mod_friends(VALUE c)
+{
+ VALUE result = rb_ary_new();
+
+ c = class_or_module_required(c);
+
+ while (c) {
+ VALUE mod = RB_TYPE_P(c, T_ICLASS) ? RBASIC(c)->klass : c;
+ VALUE ary = rb_ivar_get(mod, rb_intern_const("friends"));
+
+ if (RB_TYPE_P(ary, T_ARRAY)) {
+ rb_ary_concat(result, ary);
+ }
+
+ c = RCLASS_SUPER(c);
+ }
+
+ return result;
+}
+
</span> /*
* call-seq:
* mod.freeze -> mod
<span class="p">@@ -3512,6 +3602,8 @@</span> InitVM_Object(void)
rb_define_global_const("NIL", Qnil);
rb_deprecate_constant(rb_cObject, "NIL");
<span class="gi">+ rb_define_method(rb_cModule, "friend", rb_mod_friend, -1);
+ rb_define_method(rb_cModule, "friends", rb_mod_friends, 0);
</span> rb_define_method(rb_cModule, "freeze", rb_mod_freeze, 0);
rb_define_method(rb_cModule, "===", rb_mod_eqq, 1);
rb_define_method(rb_cModule, "==", rb_obj_equal, 1);
<span class="gh">diff --git a/vm_eval.c b/vm_eval.c
index ea398e0..48ee2df 100644
</span><span class="gd">--- a/vm_eval.c
</span><span class="gi">+++ b/vm_eval.c
</span><span class="p">@@ -593,13 +593,7 @@</span> rb_method_call_status(rb_thread_t *th, const rb_callable_method_entry_t *me, cal
/* self must be kind of a specified form for protected method */
if (visi == METHOD_VISI_PROTECTED && scope == CALL_PUBLIC) {
<span class="gd">- VALUE defined_class = klass;
-
- if (RB_TYPE_P(defined_class, T_ICLASS)) {
- defined_class = RBASIC(defined_class)->klass;
- }
-
- if (self == Qundef || !rb_obj_is_kind_of(self, defined_class)) {
</span><span class="gi">+ if (self == Qundef || !rb_obj_is_friend_of(self, klass)) {
</span> return MISSING_PROTECTED;
}
}
<span class="gh">diff --git a/vm_insnhelper.c b/vm_insnhelper.c
index 43db728..cdbcfd1 100644
</span><span class="gd">--- a/vm_insnhelper.c
</span><span class="gi">+++ b/vm_insnhelper.c
</span><span class="p">@@ -2274,7 +2274,7 @@</span> vm_call_method(rb_thread_t *th, rb_control_frame_t *cfp, struct rb_calling_info
case METHOD_VISI_PROTECTED:
if (!(ci->flag & VM_CALL_OPT_SEND)) {
<span class="gd">- if (!rb_obj_is_kind_of(cfp->self, cc->me->defined_class)) {
</span><span class="gi">+ if (!rb_obj_is_friend_of(cfp->self, cc->me->defined_class)) {
</span> cc->aux.method_missing_reason = MISSING_PROTECTED;
return vm_call_method_missing(th, cfp, calling, ci, cc);
}
<span class="p">@@ -2776,7 +2776,7 @@</span> vm_defined(rb_thread_t *th, rb_control_frame_t *reg_cfp, rb_num_t op_type, VALUE
case METHOD_VISI_PRIVATE:
break;
case METHOD_VISI_PROTECTED:
<span class="gd">- if (!rb_obj_is_kind_of(GET_SELF(), rb_class_real(klass))) {
</span><span class="gi">+ if (!rb_obj_is_friend_of(GET_SELF(), klass)) {
</span> break;
}
case METHOD_VISI_PUBLIC:
</code></pre> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=615862016-11-19T17:34:35Zshevegen (Robert A. Heiler)shevegen@gmail.com
<ul></ul><p>The terminology is a bit peculiar - friendly modules? Do we have unfriendly modules as well?<br>
Is that a new terminology altogether? I never read friend-methods before.</p>
<p>However had, leaving aside the choice of names, I do not like syntax constructs such as:</p>
<p>"protected def foo"</p>
<p>Looks fairly Java-ish.</p>
<p>This is also probably only a minor issue because e. g. "private" keyword identifier<br>
allows all subsequent methods be defined as private, so I suppose the same would<br>
apply for protected.</p>
<p>I agree with you in one regards - using .send() as workarounds. Not on your comment<br>
that it would make it "untidy", I love .send(), but I agree that it is a bit strange<br>
that there is also .public_send() altogether and also why sometimes method calls<br>
work via .send() but not otherwise. I am just not fully sure that the above proposal<br>
adds a lot; then again your mileage may vary and thankfully I don't have to make any<br>
decisions that impact anyone else really other than in my gems.</p>
<p>The code does look sorta alien to me, perhaps it is less alien for rails people.</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=615892016-11-19T23:09:58Zmatthewd (Matthew Draper)matthew@trebex.net
<ul></ul><p>Robert A. Heiler wrote:</p>
<blockquote>
<p>The terminology is a bit peculiar - friendly modules? Do we have unfriendly modules as well?<br>
Is that a new terminology altogether? I never read friend-methods before.</p>
</blockquote>
<p>It has slightly different semantics (more in line, IMO, with ruby's existing definitions of private & protected), but I didn't invent the term: <a href="https://en.wikipedia.org/wiki/Friend_class" class="external">https://en.wikipedia.org/wiki/Friend_class</a></p>
<blockquote>
<p>However had, leaving aside the choice of names, I do not like syntax constructs such as:</p>
<p>"protected def foo"</p>
</blockquote>
<p>That's existing ruby syntax. I used it here mostly because it's a line shorter.</p>
<p>To be clear, the change introduces no new syntax, and two new methods: <code>Module#friend(*modules)</code>, and <code>Module#friends</code>. Beyond that, it's about making some method calls succeed where they would previously have raised a NoMethodError due to visibility.</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=625972017-01-20T03:29:31Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>We looked at this issue yesterday at developers meeting.</p>
<p>While I understand the needs to distinguish official API and internal ones, (ab)using <code>protected</code> for that purpose was not recommended by the attendees. If you could isolate the "friendship"-ness into a single file, maybe refinements can solve the issue.</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=626152017-01-20T09:54:04Zdsferreira (Daniel Ferreira)
<ul></ul><p>My proposal of <a href="https://bugs.ruby-lang.org/issues/9992" class="external">Internal intefaces</a> comes in line with this proposal.<br>
I would love to have this functionality in place.<br>
Would make code integrity so much better.</p>
<p>Maybe the use of the new <code>internal</code> access modifier would make things more clear?</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=626162017-01-20T10:10:44Zdsferreira (Daniel Ferreira)
<ul></ul><p>The use of <code>:nodoc:</code> for these situations it is not a valid option in my opinion.<br>
The reason being because <code>Object#public_methods</code> will not be inline with the documentation.<br>
From that fact the developer cannot anymore trust the code.</p>
<p>Code should be 100% reliable in the feedback it gives to the developer.</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=964882022-02-14T14:23:26ZEregon (Benoit Daloze)
<ul></ul><p>Do you have an example in Rails?</p>
<p>This sounds like it can be solved by moving such private-but-shared method to <code>Internals</code> or some other module.</p>
<p>Calling "private/protected" methods on another object feels rather wrong to me.</p>
<p>Maybe <code>private_constant</code> can also be leveraged to e.g. have a private class/module but all methods on it public.</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=964902022-02-14T15:02:08Zmatthewd (Matthew Draper)matthew@trebex.net
<ul></ul><blockquote>
<p>Calling "private/protected" methods on another object feels rather wrong to me.</p>
</blockquote>
<p>The whole point of <code>protected</code> is that it allows you to call methods on another object. But I assume you meant another object that is not an instance of the caller's class.</p>
<p>A quick grep for <code>:nodoc:</code> turned up this example: <a href="https://github.com/ruby/ruby/blob/26187a8520b8c6645206a2064c11a7ab86a89845/lib/net/http/response.rb#L163" class="external">https://github.com/ruby/ruby/blob/26187a8520b8c6645206a2064c11a7ab86a89845/lib/net/http/response.rb#L163</a></p>
<p>In my experience it's just not unusual for two collaborating objects to need to talk to each other in more detail than their "user"-facing public API intends to expose. In a typed OO language, this is where the objects communicate using a private concrete class, while only "publishing" a more focused Interface.</p>
<p>As in that Net::HTTP example, a separate module to stash the methods is not really viable, because we're talking about instance methods that need access to ivars -- that's why it's a method on the current target object, and not a private method on the caller.</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=964922022-02-14T15:30:44ZEregon (Benoit Daloze)
<ul></ul><p>For this Net::HTTP example IMHO it'd be better to use <code>send</code>, that would make it a private method and be explicit that code is calling into partly-internals.</p>
<p>IMHO friend-like visibility is too magic and the linear search could be a significant overhead on such calls (+ rb_obj_is_kind_of, so could be O(n*m)).<br>
Also the additional state per module/class seems to add non-trivial complexity.<br>
<code>send</code> can be easily optimized OTOH (especially with a literal Symbol as name).</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=965022022-02-15T13:25:39Zmatthewd (Matthew Draper)matthew@trebex.net
<ul></ul><p>The trouble with using <code>send</code> in this [general] situation is that it makes it too equally-easy to reach into <em>all</em> internals: you want access to methods that are suitable for a collaborator but not a downstream user, but you can also immediately access truly-private methods that cause the object to violate its intended invariants. In a large project, you still want <em>some</em> guard-rails for your collaborators, even as you give them more abilities than usual.</p>
<p>Widespread use of <code>send</code> is also just unpleasant to read and maintain. In practice, people don't like writing code like that: I can see additional uses of <code>:nodoc:</code> for this purpose in stdlib (the chosen example was just particularly illustrative because of its comment), yet <code>send(:</code> turns up remarkably little. Given the currently-available options, it seems that more than just Rails chooses "secretly public" methods over send+private.</p>
<p>On performance, <code>protected</code> already performs a linear <code>rb_obj_is_kind_of</code> search. As you note, the total search becomes O(n*m) here... but the <code>m</code> is the number of friends (so only becomes > 0 when friendship is in use), and is only even considered when the target method is protected (or, in an alternate variant of this proposal, some specific fourth method visibility level).</p>
<p>I would expect this check to be easier to optimize than <code>send</code>, for monomorphic call-sites, by leaning on existing call-site caching. (If a friendship change bumps the class serial, there's no need to recheck the cached call.)<br>
Even setting aside the syntactic noise of repeated <code>send(:sym)</code>, I'm not aware of any current optimization effort that could make it competitive.</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=965062022-02-15T14:53:33Zp8 (Petrik de Heus)
<ul></ul><p>I can imagine having a <code>protected_send</code> just like we have a <code>public_send</code> and <code>send</code> (which is basically <code>private_send</code>).<br>
Although that probably is a lot more difficult to optimize.</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=965072022-02-15T15:14:10ZEregon (Benoit Daloze)
<ul></ul><p>matthewd (Matthew Draper) wrote in <a href="#note-10">#note-10</a>:</p>
<blockquote>
<p>The trouble with using <code>send</code> in this [general] situation is that it makes it too equally-easy to reach into <em>all</em> internals: you want access to methods that are suitable for a collaborator but not a downstream user, but you can also immediately access truly-private methods that cause the object to violate its intended invariants. In a large project, you still want <em>some</em> guard-rails for your collaborators, even as you give them more abilities than usual.</p>
</blockquote>
<p>I see. Such semi-private methods could have a comment explaining they are also used by other parts of the gem.<br>
Or even an alias of <code>private</code> to indicate that but be clearer on the intention.</p>
<blockquote>
<p>Widespread use of <code>send</code> is also just unpleasant to read and maintain. In practice, people don't like writing code like that: I can see additional uses of <code>:nodoc:</code> for this purpose in stdlib (the chosen example was just particularly illustrative because of its comment), yet <code>send(:</code> turns up remarkably little. Given the currently-available options, it seems that more than just Rails chooses "secretly public" methods over send+private.</p>
</blockquote>
<p>It might also be written as <code>__send__(:</code>.<br>
There are not many occurrences in stdlib, but stdlibs are also fairly small compared to larger gems, and probably don't need this much.</p>
<blockquote>
<p>I would expect this check to be easier to optimize than <code>send</code>, for monomorphic call-sites, by leaning on existing call-site caching. (If a friendship change bumps the class serial, there's no need to recheck the cached call.)</p>
</blockquote>
<p>Right, at least this can be cached for a call site since it's add-only.<br>
It would likely be very difficult to cache when trying to persist JITed code though, as it depends on lots of live values.</p>
<blockquote>
<p>I'm not aware of any current optimization effort that could make it competitive.</p>
</blockquote>
<p>There is <a class="issue tracker-2 status-2 priority-4 priority-default" title="Feature: Optimize __send__ call (Assigned)" href="https://bugs.ruby-lang.org/issues/17291">#17291</a>, but it would need a redefinition check to be semantically correct.<br>
TruffleRuby already optimizes send and <code>__send__</code>, in JITed code there is no difference and only a small overhead in interpreter (when using a constant method name).</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=965082022-02-15T15:19:23ZEregon (Benoit Daloze)
<ul></ul><p>p8 (Petrik de Heus) wrote in <a href="#note-11">#note-11</a>:</p>
<blockquote>
<p>I can imagine having a <code>protected_send</code> just like we have a <code>public_send</code> and <code>send</code> (which is basically <code>private_send</code>).</p>
</blockquote>
<p>Yeah, I was also thinking to something like:</p>
<pre><code class="ruby syntaxhl" data-language="ruby"><span class="n">friend</span><span class="o">/</span><span class="n">internal</span><span class="o">/</span><span class="kp">protected</span> <span class="k">def</span> <span class="nf">my_method</span>
<span class="o">...</span>
<span class="k">end</span>
<span class="k">def</span> <span class="nf">foo</span>
<span class="n">obj</span><span class="p">.</span><span class="nf">send_friend</span><span class="o">/</span><span class="n">send_internal</span><span class="o">/</span><span class="n">send_protected</span><span class="p">(</span><span class="ss">:my_method</span><span class="p">)</span>
<span class="k">end</span>
</code></pre>
<p>and such method could only be called by <code>send_friend/send_internal/send_protected</code>, and that would be very efficient and simple to check.</p>
<p>I don't think it's hard to optimize <code>send</code> and similar, e.g. there could be an extra call cache when parsing send/<code>__send__</code>/etc, and that could be used to cache the second lookup too.</p> Ruby master - Feature #12962: Feature Proposal: Extend 'protected' to support module friendshiphttps://bugs.ruby-lang.org/issues/12962?journal_id=965382022-02-17T12:11:15Zmatz (Yukihiro Matsumoto)matz@ruby.or.jp
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Rejected</i></li></ul><p>I revisited this issue at the developer meeting and concluded to reject this proposal.<br>
I understand OP's need for the <code>friend</code> visibility but considering the dynamic nature of the language (plus current usage of <code>protected</code> visibility), this visibility provides the value far less than the pain of complexity. I'd rather like to remove <code>protected</code> visibility, if possible (I know it's too late).</p>
<p>Thus this is rejected. Sorry.</p>
<p>Matz.</p>