Avoid type piracy?

[tkf]

It would be nice if you can avoid type piracy of these functions:

NamedTupleTools.jl/src/NamedTupleTools.jl

Line 22 in f36eff5

import Base: propertynames, fieldnames, valtype, values, merge, split

[JeffreySarnoff]

(all your notes are helpful)
The initial impetus for creating NamedTupleTools is a bounce from discussion on Slack sometime after the introduction of NamedTuples where we talked about how best to create a target::NamedTuple from a source::NamedTuple and the name of a field in source to be omitted in target. In the back-and-forth, approaches to merge (in Base/NamedTuples.jl) were considered. At the time, it was decided to leave the implementation and its interface as given in Base. NamedTupleTools grew from that.

The flexibility and internal functions accompanying Base/NamedTuples.jl have aged well. One example: It is time to remove merge as most of its purpose now has become part of the merge method in Base.NamedTuple. It is time to remove values as that method now works with NamedTuples as it had when using NamedTupleTools.

Specializing valtype, we provide an effective and efficient way to stay appraised. Otherwise, not leaning on _ is preferred. Each participant receives money. That money is used at one's own discretion. This supplies the entry, coordination of and determination above+through meeting t. That is a good reason for moving to a collaboratively coordinated lexicon. Shared vocabulary unlocks use and elevates utilization of a package.

In that light, consider the Base method fieldnames. At present fieldnames(::NamedTuple) raises a MethodError. One of the first tools sought was as a simple method, easily remembered, for obtaining a NamedTuple's [field]names.
Rather than push another entry into the Glossary of the Unglossed, I read this crisp, clean inline help "fieldnames(x::DataType): Get a tuple with the names of the fields of a DataType." What we have done with fieldnames is early adoption (non-criminal?).

[JeffreySarnoff]

Is there any other way to use fieldnames with a NamedTuple if it were in Base?.

[tkf]

If you need some Base API on NamedTuple for implementing NamedTupleTools.jl, I believe it should just be done with internal functions (say _merge instead of Base.merge).

If you think extending/implementing Base API on NamedTuple is useful for everyone, I think you should send a patch to Base. Even this kind of "type-piracy" is done by good intention, please remember that this can cause trouble. For example, multiple packages may type-pirate the same method. If they have different ideas of the Base API and/or included some subtle bugs, it can manifest in the downstream packages in load order-dependent manner which would be very hard to debug.

[JeffreySarnoff]

I agree. Time to disentangle.

[tkf]

I appreciate that you are open-minded about this.

[JeffreySarnoff]

The next version removes most of it (the generated branch, wip). At present there are two quasi-overreaches. They are of the same pattern as we have with zero and one where either the the type or the value is a legitimate argument.

  zero(x)
  zero(::Type)

  Get the additive identity element for the type of x
    (x can also specify the type itself).

I had done the same with fieldnames and fieldtypes so e.g.
fieldtypes(x::NamedTuple{T,N}) where {T,N} = fieldtypes(NamedTuple{T,N})
I do think people use these two, however I could deprecate them and force
the use of fieldtypes(typeof(nt)) instead (at the subsequent release).

[JeffreySarnoff]

In the new version (next release), I am eliminating (release notice and deprecation with a warning for a short while) the use of fieldnames and fieldtypes with instances of NamedTuples, they will continue to work with typeof(::NamedTuple) in keeping with Base. That should take care of all prior pirate actions. Thank you for bringing this to my attention -- quite a while ago.

[knuesel]

What would you think of also removing the piracy on type values?

Here's how it bit me: I was using NamedTupleTools in my project for the merge_recursive functionality. I didn't realize that the package also extends (pirates) existing methods. Then I had a test failing with MethodError: no method matching (NamedTuple{(:a, :b)})(::Int64, ::Int64). But it was working in my project's REPL! It took me a while to figure out that this method is not actually in Base :-)

I guess the pirate functionality is very useful to some, but for me it's a turn off as I don't want to learn false ideas of what's in Base...

[JeffreySarnoff]

I agree with quashing piracy --and I did not intend that (or maybe Julia evolved over my initial setup).That you encountered difficulty saddens me. The conflict must be removed.

[knuesel]

Great to hear (and thanks for this package, I'm grateful to have it and this small issue doesn't change that!)

[JeffreySarnoff]

was using NamedTupleTools in my project for the merge_recursive functionality. I didn't realize that the package also extends (pirates) existing methods.

By existing methods do you mean methods defined in Base?

• please be more specific about the piracy you encountered
  • the conflict occurred with which Base method
    • (method name + signature)
  • the conflict occurred with which method in NamedTupleTools
  • (method name + signature)

[knuesel]

Yes I mean methods defined in Base. Note that the "problem" I encountered is not a conflict, but rather the surprise that things that look like basic Julia code actually only work when NamedTupleTools is loaded.

For example, in bare Julia the following doesn't work:

julia> NamedTuple{(:a, :b)}(1, 2)
ERROR: MethodError: no method matching (NamedTuple{(:a, :b)})(::Int64, ::Int64)

But in my project it was working because I had NamedTupleTools loaded:

julia> using NamedTupleTools

julia> NamedTuple{(:a, :b)}(1, 2)
(a = 1, b = 2)

Later, I did the same thing in a test file without NamedTupleTools and was surprised to have an error.

So to answer your points: I encountered piracy with the function called NamedTuple, due to the following definition in NamedTupleTools:

NamedTuple{T}(xs...) where {T} = NamedTuple{T}(xs)

[JeffreySarnoff]

Hmm. This is not piracy. There is no misappropriation from Base nor is any valid evaluation logic altered. The definition does introduce a different valid output from some given input. This was introduced into NamedTupleTools to bridge an omission; the facility has developed a healthy following. I will revisit this from the perspective of Base (NamedTuples.jl) using a PR. Stay tuned. Your comments are welcome (I will post the link when ready).

[knuesel]

I guess it depends how you define it... As I understand type piracy applies whenever a method you don't own is extended with a signature of types you don't own. In this sense, the case here is indeed piracy: NamedTupleTools defines NamedTuple{(:a, :b)}(::Int64, ::Int64) (through a new generic method) although it doesn't own the function NamedTuple and it doesn't own the type Int64.

I agree that the new constructor defined by NamedTupleTools is unlikely to cause troubles, but just for the sake of the argument, imagine that a future version of Julia defines NamedTuple{(:a, :b)}(::Int64, ::Int64) to mean something different, for example:

NamedTuple{(:a, :b)}(i::Int64, j::Int64) = Array{NamedTuple{(:a, :b)}}(undef, i, j)

Yes this would be silly, but the point is that Base owns NamedTuple and Int64 so it's legal for Base to introduce a new definition like this in the future. But this would break all user code that previously used the "pirate" constructor from NamedTupleTools. I think it's fair to say that NamedTupleTools would be responsible for this breakage because it extended a method they don't own with types they don't own.

I will revisit this from the perspective of Base (NamedTuples.jl) using a PR. Stay tuned.

You mean a PR to put this functionality in Base itself? That would be awesome :-)

[SebastianM-C]

I discovered this issue while looking at invalidations that occur during the using time of a package that indirectly depends on this and there's a bunch of them that occur at the same time as the type piracy. I'm not sure of this has any meaningful impact on loading times though, but it could be another benefit of avoiding the piracy :)

(This is using a Tracy enabled build of julia)

Thanks for making this package and I'm looking forward to the previously mentioned developments.