Make copymutable public and add clarification to some docstrings

[LilithHafner]

Fixes #51932

• update sort docstring
• make copymutable public
• clarify that users should not typically need to override copymutable

[nsajko]

The Base.copymutable doc string currently describes the function like so:

Make a mutable copy of an array or iterable a.

I think the meaning of "mutable copy" here should be clarified somewhat. I gather that it basically means that the returned value must compare equal to the original (thus "copy") and the returned value must be mutable.

[Tokazama]

Would it make sense to also include emptymutable in this?

[PallHaraldsson]

make copymutable public

You're exporting, implicitly making public. It's probably ok, but should the original text say "export copymutable", be clear, or should the PR actually only made public, and NOT export? I'm not worried in this case (about conflict), but I am in others, about exporting from Base.

[LilithHafner]

Would it make sense to also include emptymutable in this?

It would make sense to use mark emptymutable as public if there are compelling reasons to include it in the public API. Let's leave that to another PR, though, as it is acceptable to have copymutable public but not emptymutable.

You're exporting, implicitly making public.

No. I'm explicitly marking it as public. Please check the diff.

[PallHaraldsson]

No. I'm explicitly marking it as public.

Sorry for the noise, yes PR and description matches (my intention was to alert to syncing, and it's good to know you're NOT exporting, and things match).

I saw you editing "base/exports.jl" and missed seeing "public", so I assumed. It would be helpful to have separate file for that base/public.jl, which could import base/exports.jl.

[You were editing line 1117 and public not visible until I expand/scroll up to line 1075, and it's not marked red as other keywords...]

[LilithHafner]

It would be helpful to have separate file for that base/public.jl, which could import base/exports.jl

Agreed, though I would have Base.jl include both exports.jl and public.jl. Care to make a PR?

[LilithHafner]

@nsajko is it more clear now?

[jakobnissen]

Just some minor comments:

• The docstring mentions that it's like collect, except for some other array types. I think that's not right - for non-array types it's certainly not equivalent to collect:

julia> s = Set([1,2,3]);

julia> s2 = Base.copymutable(s);

julia> typeof(s2)
Set{Int64}

julia> s === s2
false

• Maybe the example could use a Base type, and not require loading an external package?

[rfourquet]

Maybe I don't have a recent-enough julia version installed, but sort(::Dict) for me produces a vector of Pair and not a sorted dictionary, do you confirm this changed?

[rfourquet]

Related question, does sort(::Dict) returning a vector works thanks to the fact that copymutable is not yet specialized for Dict? Would implementing copymutable(d::Dict) = copy(d) break this? Would it make sense to support sort(Set(1)), and does it currently fail only because copymutable is used (instead of collect) for Set?

[rfourquet]

In other words, while copymutable seems fine for arrays, I wonder whether collect wouldn't be better than copymutable for other iterables. As copymutable for arrays is just a thin wrapper atop similar, mentioning "similar for arrays and collect for iterable" might be simpler than introducing another copymutable concept.

[LilithHafner]

Haha, it's piracy by OrderedCollections that allows sort(::Dict) to work. OrderedCollections just gets loaded as a transitive dependency in my startup.jl file so I didn't realize. Also the pirated sort(::Dict) method is deprecated.

I don't like sort(::Dict) returning a vector of pairs, that is rarely what the user wants and it precludes the possibility of creating a better data structure instead.

[rfourquet]

I'm hesitant to make copymutable public now, at least as long as it's not implemented for AbstractDict.

[LilithHafner]

Closing because #52098 makes the changes here obsolete.