https://redmine.ruby-lang.org/https://redmine.ruby-lang.org/favicon.ico?17113305112020-06-12T22:18:54ZRuby Issue Tracking SystemRuby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861302020-06-12T22:18:54Zheadius (Charles Nutter)headius@headius.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/86130/diff?detail_id=57332">diff</a>)</li></ul> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861312020-06-12T23:19:02Zchrisseaton (Chris Seaton)chris@chrisseaton.com
<ul></ul><blockquote>
<p>I suspect the addition of the specs led to folks starting to use this private API.</p>
</blockquote>
<p>I think it's the other way around - the specs were added because people were using it - <a href="https://github.com/oracle/truffleruby/issues/1385" class="external">https://github.com/oracle/truffleruby/issues/1385</a> and <a href="https://github.com/oracle/truffleruby/issues/1958" class="external">https://github.com/oracle/truffleruby/issues/1958</a> is what led to the specs being written.</p>
<p>The example you give which all lead back to the Rails PR was written well <em>before</em> the specs - April 2019, and the specs were just written I believe, but maybe there is more context to why it was merged that is not referenced?</p>
<p>So I don't think it's right to blame the specs here. The specs reflect reality.</p>
<p>But I do think it could be worth having a conversation about moving some specs to an optional namespace - the C extension API is in there. It'd mark intent at least.</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861332020-06-13T00:41:13Zheadius (Charles Nutter)headius@headius.com
<ul></ul><blockquote>
<p>So I don't think it's right to blame the specs here. The specs reflect reality.</p>
</blockquote>
<p>I'm not sure I'm seeing what you're seeing, since the specs appear to have been created and merged in mid-April and the Rails PR was created in May.</p>
<p><a href="https://github.com/ruby/spec/commits/dd8437628a6f2de5b74b338d4960682bb1590a60/core/objectspace/weakmap" class="external">https://github.com/ruby/spec/commits/dd8437628a6f2de5b74b338d4960682bb1590a60/core/objectspace/weakmap</a></p>
<p>I can't tell if the merging of those specs in any way led to the Rails usage, so I'll retract that theory. There certainly are many other uses of WeakMap that predate the specs, including the Mustermann usage that (presumably) led to the specs being created.</p>
<p>The specs should reflect official, public Ruby APIs... not private APIs that were added unilaterally by one implementation without any discussion. We should always be vigilant about hidden features of Ruby becoming de facto standards, given the mess that RubyVM has become for alternative Rubies.</p>
<p>JRuby isn't entirely blameless here, since we implemented WeakMap shortly after it was introduced almost a decade ago (mostly in hopes of sharing the pure-Ruby weakref.rb, which didn't end up panning out). WeakMap itself was actually introduced because I and others proved that the previous implementation of weakref.rb was fundamentally broken. We did not push for specs then largely because discussions were still ongoing about how to support weak references in Ruby.</p>
<p>We should really make Weakref a core API and build WeakMap on top of it (in pure Ruby)... not the other way around.</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861342020-06-13T00:46:20Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>I think it's also worth pointing out that the gc.c comment I linked above does propagate to the official documentation, which states that users should go to Weakref for the public API:</p>
<p><a href="https://docs.ruby-lang.org/en/2.7.0/ObjectSpace/WeakMap.html" class="external">https://docs.ruby-lang.org/en/2.7.0/ObjectSpace/WeakMap.html</a></p>
<p>We should avoid adding specs for APIs documented as non-public... and we should also avoid adding "private" APIs to Ruby without at least a "require" opt-in.</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861352020-06-13T00:55:18Zchrisseaton (Chris Seaton)chris@chrisseaton.com
<ul></ul><p>The PR was created recently, but the code was written in April 2019 <a href="https://github.com/rails/rails/pull/39121/commits/77f7b2df3aa3b7eb318cc830f239fd5ec2a7f28f" class="external">https://github.com/rails/rails/pull/39121/commits/77f7b2df3aa3b7eb318cc830f239fd5ec2a7f28f</a>, a year before it was specified. Sorry if it seems pedantic and thanks for retracting but I'm carefully pointing out that the specs definitely came second here because I don't want people to get the idea that the process of writing specs by itself cause problems like this or that people writing specs is a bad thing.</p>
<blockquote>
<p>We should avoid adding specs for APIs documented as non-public...</p>
</blockquote>
<p>Given that people are using it in code in the wild, TruffleRuby needs specs or tests to figure out what MRI is doing. They could go into a private part of the TruffleRuby repo, not upstreamed to ruby/specs, but then does every implementation independently spec these things that we're not supposed to spec but we all have to as there's code using it?</p>
<p>But overall what you're saying does make sense and I can see your problem.</p>
<p>Maybe new specs should go through more PRs and community oversight? At the moment they do just tend to appear from downstream implementations. So then you'd have been able to see the new specs and have said 'wait are people using this, is this the behaviour we wanted, who is using this and why'.</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861372020-06-13T01:17:19Zheadius (Charles Nutter)headius@headius.com
<ul></ul><blockquote>
<p>Maybe new specs should go through more PRs and community oversight?</p>
</blockquote>
<p>Well, clearly they should if they're not public APIs. Perhaps the spec author did not realize that was the case here.</p>
<p>This does illustrate a problem with each impl keeping their own copy of the specs and syncing periodically; it circumvents all review processes designed to ensure we're only adding "good" specs. Maybe we should revisit using something like git submodules for ruby/spec in alternative Ruby repos. Then we have the option to roll forward the submodule to pick up new specs <em>after</em> they're approved and merged.</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861382020-06-13T01:18:25Zheadius (Charles Nutter)headius@headius.com
<ul></ul><blockquote>
<p>or that people writing specs is a bad thing</p>
</blockquote>
<p>I clearly didn't say that, unless you mean writing specs for private features...</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861392020-06-13T01:20:33Zheadius (Charles Nutter)headius@headius.com
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/86139/diff?detail_id=57334">diff</a>)</li></ul> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861442020-06-13T09:28:47Zzverok (Victor Shepelev)zverok.offline@gmail.com
<ul></ul><p>Probably not only specs are "guilty", but also Ruby 2.7's changelog (and maybe, a small bit, <a href="https://rubyreferences.github.io/rubychanges/2.7.html#objectspaceweakmap-now-accepts-non-gc-able-objects" class="external">my rendering of it</a>): it included changes in "private" class as a part of official changelog, and people start noticing it can be used for all kind of stuff...</p>
<p>Though, the ticket <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Allow non-finalizable objects such as Integer, static Symbol etc in ObjectSpace::WeakMap (Closed)" href="https://redmine.ruby-lang.org/issues/16035">#16035</a> that caused the changelog entry implies that people was <em>already</em> using it. And Matz's answer there goes as:</p>
<blockquote>
<p>I think this is the required behavior for WeakMap to implement a cache for example. Accepted.</p>
</blockquote>
<p>(Also implying the class is publicly usable.)</p>
<p>I had clarifying this on my post-2.7 TODO list, but haven't acted on it, yet.</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861452020-06-13T11:07:12ZEregon (Benoit Daloze)
<ul></ul><p>When APIs like this are used in the wild, for many years, the docs can state "private API" (well actually these docs don't, they just suggest using weakref.rb, which has its own issues) but it's simply not accurate anymore.</p>
<p>mustermann has been using ObjectSpace::WeakMap since 2016, and the "tool" gem before since 2014 or earlier: <a href="https://github.com/rkh/tool/issues/2" class="external">https://github.com/rkh/tool/issues/2</a>.<br>
There was a fallback with a Hash but that's been clearly incompatible in many subtle ways.</p>
<p>JRuby implemented ObjectSpace::WeakMap so that further made it "API any gem can use".</p>
<p>And indeed recently there has been a few improvements to ObjectSpace::WeakMap discussed on this tracker (<a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Allow non-finalizable objects such as Integer, static Symbol etc in ObjectSpace::WeakMap (Closed)" href="https://redmine.ruby-lang.org/issues/16035">#16035</a>, <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Provide a public WeakMap that compares by equality rather than by identity (Closed)" href="https://redmine.ruby-lang.org/issues/16038">#16038</a>), which clearly shows this API is considered public.<br>
And it was also mentioned as part of the 2.7 NEWS file: <a href="https://github.com/ruby/ruby/blob/master/doc/NEWS-2.7.0" class="external">https://github.com/ruby/ruby/blob/master/doc/NEWS-2.7.0</a></p>
<p>The specs are reviewed by me during synchronization with upstream ruby/spec.<br>
I think it makes perfect sense to add specs for ObjectSpace::WeakMap and e.g., other ObjectSpace features as long as >1 Ruby implementation implements them, and it's not specifically annotated as MRI-only (currently, that's only RubyVM).</p>
<hr>
<p>So concretely, what problems do you see with adapting the documentation of ObjectSpace::WeakMap to mark it "non-private"?<br>
Any issue in the API?<br>
AFAIK the only problem is the documentation doesn't clearly mention what's weak.</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861462020-06-13T11:07:34ZEregon (Benoit Daloze)
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/86146/diff?detail_id=57337">diff</a>)</li></ul> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861472020-06-13T11:12:08ZEregon (Benoit Daloze)
<ul></ul><p>In any case, having specs in ruby/spec doesn't mean it's public APIs.<br>
That's why e.g., some private methods are also tested in ruby/spec.<br>
Users should refer to documentation to find out if an API is public.</p>
<p>IMHO, every public constant and method is public API, or at least it is in practice with enough time.<br>
RubyVM is the only module that is clearly documented as MRI-specific, might break APIs anytime, everything there is experimental.</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861492020-06-13T18:10:45Zheadius (Charles Nutter)headius@headius.com
<ul></ul><p>Please don't edit my description.</p>
<p>I personally do not believe the specs should be removed. But I also do not feel that WeakMap is the right abstraction. If Weakref had support for a reference queue, then WeakMap would be a pure-Ruby library that all implementations could share. Further, there would not just be the one WeakMap... Users could build their own weak arrays and other data structures. What I am asking for here is that we don't just accept this private API as official public API without going through the process that every other API must go through. This API was never discussed for public use, and has limitations that could be addressed quickly by making Weakref a bit more robust.</p>
<p>The bottom line is that WeakMap was added as a private API to make Weakref work better, but was never discussed and approved to become a public API. Yes, people have been using it, and that's not good. Yes, those uses predate the recent specs, but the specs and the 2.7 changelog make it even more likely this will become a de facto standard API, completely circumventing all processes for adding public APIs.</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=861502020-06-13T20:54:18ZEregon (Benoit Daloze)
<ul></ul><p>I don't see much the value of having the WeakMap implementation shared between Ruby implements, it's a tiny class, and CRuby/JRuby/TruffleRuby already all have their own WeakMap implementation.</p>
<p>So I guess your main point is having some sort of reference queue in addition of the existing WeakRef would be more flexible and allow building other weak data structures.<br>
That makes sense to me.<br>
I'd suggest to restart the discussion/review the PR on <a class="issue tracker-2 status-2 priority-4 priority-default" title="Feature: Add a reference queue for weak references (Assigned)" href="https://redmine.ruby-lang.org/issues/6309">#6309</a> then.</p>
<p>WeakMap itself is useful in its own right, that's why a handful gems use it, and so I think there is no hope to remove it.</p>
<p>IMHO ObjectSpace::WeakMap became public API as soon as it was introduced, i.e., nothing recent.<br>
Although to be fair I don't know the exact history of how ObjectSpace::WeakMap was added in CRuby.</p>
<blockquote>
<p>completely circumventing all processes for adding public APIs.</p>
</blockquote>
<p>What's that process exactly?<br>
As far as I know, once it's implemented in CRuby and in a release it's pretty much public API, discussed or not.<br>
I agree discussion would be useful, but AFAIK there is no formal process to introduce new APIs in Ruby, just needs an approval from matz.</p> Ruby master - Bug #16959: Weakmap has specs and third-party usage despite being a private APIhttps://redmine.ruby-lang.org/issues/16959?journal_id=1030522023-05-14T03:58:15Zheadius (Charles Nutter)headius@headius.com
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Rejected</i></li></ul><p>WeakMap has now been in the wild with documentation and specs for several years, so I think it has become a de-facto public API. I'm fine closing this, though I still would like to see the other features I have described added to Weakref.</p>