Skip to content

Document Base.RefValue and clarify that this is public API #58101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bbrehm
Copy link

@bbrehm bbrehm commented Apr 14, 2025

Document Base.RefValue and clarify that this is public API.

This PR was triggered by https://discourse.julialang.org/t/ann-zerodimensionalarrays-jl-zero-dimensional-arrays-references-boxes/ and related discussions. Some people are questioning whether using Base.RefValue counts as "mucking with unstable runtime internals", so we better answer that conclusively.

(I wouldn't mind also exporting Base.RefValue, but that's a higher bar in terms of cluttering the namespace, and let's not have the good be the enemy of the adequate)

@nsajko
Copy link
Contributor

nsajko commented Apr 14, 2025

xref #40369

I don't see why this is necessary, especially now that a package is being registered. And considering all the issues with Ref, we need less of it, not more.

@nsajko nsajko added the feature Indicates new feature / enhancement requests label Apr 14, 2025
@favba
Copy link
Contributor

favba commented Apr 14, 2025

xref #40369

I don't see why this is necessary, especially now that a package is being registered. And considering all the issues with Ref, we need less of it, not more.

Even with the new package registered I think it is still good to clarify and make RefValue at least public for when you don't want simply a generic Reference in your struct, but an actual RefValue for the c interop aspect of it.

@bbrehm
Copy link
Author

bbrehm commented Apr 14, 2025

I feel that too much discussion of the ZeroDimensionalArrays.jl package might be a fruitless digression. The relevant questions should be:

  1. Is Base.RefValue already de-facto public, in the sense of being protected from breaking changes?
  2. Should Base.RefValue be public, in the sense of being protected from breaking changes and being documented?
  3. Do we need to spell that out?

My answers would have been: Yes, it's already de-facto public; yes, that's a good thing; and no, we shouldn't need to spell that out, but it is probably better to spell it out explicitly, at least for things like linters that warn about use of "private unstable APIs".

You @nsajko have provided the existence proof for (3): Stability of this API is undeniably not self-evident.

The reason why Base.RefValue is already de-facto public is: This is the return type of the widely advertised and used Base.Ref(x) method; it is referenced in the documentation at multiple points; it is an unfortunate but now unchangeable legacy fact that Base.Ref breaks the naming scheme that is otherwise prevalent in julia (it should be called Base.AbstractRef for the abstract type, and ref for the factory method).

So I would not see this a new feature, but rather a docfix for an oversight when the public keyword was introduced (having Ref public and Base.RefValue as unstable runtime internals that nobody should use is a nonsensical stance that I would not ascribe to anyone).

@nsajko
Copy link
Contributor

nsajko commented Apr 14, 2025

This is the return type

That doesn't mean it's public or "de-facto public", xref #55280.

it is referenced in the documentation at multiple points

That doesn't mean it's public: see the FAQ. In fact, preventing these kinds of arguments was one of the motivations for introducing public.

the naming scheme that is otherwise prevalent in julia

Lots of abstract types don't have the Abstract* prefix, e.g., Number, Signed, Unsigned, Real, etc.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There are almost always better choices (Some or FillArray or mutable), but it is valid to use this also in some occasional cases

@mcabbott
Copy link
Contributor

This note seems welcome, but perhaps still gets lost at the end of Ref's long docstring.

Could it start out by saying that Ref it's an abstract type, which is also the function for making something with these properties... and the return is of some concrete type <: Ref (if in fact that's guaranteed?). This is pretty unusual, as most other upper-case things you call make a struct of that exact name.

It would also help if the examples given would create a variety of types, as I think all of them now make Base.RefValue. Maybe some of these:

julia> x = [0, pi, 2pi];

julia> r = Ref(x, 2)
Base.RefArray{Float64, Vector{Float64}, Nothing}([0.0, 3.141592653589793, 6.283185307179586], 2, nothing)

julia> r isa Ref{Float64}
true

julia> r[]
3.141592653589793

julia> p = pointer(x, 2)
Ptr{Float64} @0x000000010c7fe7e8

julia> p isa Ref
true

Maybe the broadcasting heading can have its own block of examples, too. Maybe with and without Ref would make this clearer?

julia> isa.(Ref(x), [Array Dict Real Complex])  # Ref(x) is scalar under broadcasting
1×4 BitMatrix:
 1  0  0  0

julia> isa.(x, [Array Dict Real Complex])
3×4 BitMatrix:
 0  0  1  0
 0  0  1  0
 0  0  1  0

julia> println.("hello", ' ', ("world", "julia"))  # Strings are scalar under broadcasting...
hello world
hello julia
(nothing, nothing)

julia> Broadcast.broadcastable("string")  # ... because this calls Ref
Base.RefValue{String}("string")

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree with @mcabbott's suggestions, they are not strictly about this PR so @bbrehm, you are welcome to address them here or choose to proceed without an overhaul of the Ref docstring.

Currently, the documentation fails to build because the cross ref to RefValue is broken because RefValue is not included in the docs. It should go just after or just before Base.Ref in doc/src/manual/base/c.md.

Aside from that build issue this PR LGTM (Looks good to me). Thanks for making it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants