tk3369 / BinaryTraits.jl

Can do or not? It's easy. See https://tk3369.github.io/BinaryTraits.jl/dev/
MIT License
51 stars 3 forks source link

Simplify trait types with a parametric design? #23

Closed tk3369 closed 4 years ago

tk3369 commented 4 years ago

From Janis Erdmanis via Slack:

The API is rather clear and seems I could find it useful for enforcing interface. However it also feels a bit verbose. Looking at the under the hood section I wonder wouldn't it be possible to have an API:

struct Lack{T} end
struct Has{T} end
abstract type FlyTrait <: Ability end
FlyTrait(x) = Lack{FlyTrait}

And then the methods would have a form:

tickle(::Has{FlyTrait}, ::Has{SwimTrait}, x) = "Flying high and diving deep"
tk3369 commented 4 years ago

While it looks simpler, it does not seem to be much easier than the current design. Further, this is forcing a grouping of can-types together (subtype of Has) and likewise for cannot-types (subtype of Lack). Current design groups the can-type/cannot-type under the same custom abstract type, for which I feel that it's more organized.

JanisErdmanis commented 4 years ago

Current design groups the can-type/cannot-type under the same custom abstract type, for which I feel that it's more organized.

Have you considered an option of introducing an abstract type like AbstractProperty:

abstract type AbstractProperty end
struct Lack{T} <: AbstractProperty end
struct Has{T} <: AbstractProperty end

IMO it is a considerable drawback that types are generated with a macro ina kinda invisible way for the user.

tk3369 commented 4 years ago

IMO it is a considerable drawback that types are generated with a macro ina kinda invisible way for the user.

What if we can output something to the console when it's used in the REPL?

Taking your idea above, I guess we can group things together like this:

abstract type AbstractProperty{T} end
struct Lack{T} <: AbstractProperty{T} end
struct Has{T} <: AbstractProperty{T} end

Then,

julia> subtypes(AbstractProperty{FlyTrait})
2-element Array{Any,1}:
 Has{FlyTrait}
 Lack{FlyTrait}

My concern about such design is that it makes the code look more complex for general users who are not keen with parametric types. The usage of other macros now becomes:

@implement Has{FlyTrait} by fly(_, destination::Location, speed::Float64)
@assign Duck with Has{FlyTrait}

rather than

@implement CanFly by fly(_, destination::Location, speed::Float64)
@assign Duck with CanFly

One primary goal of this package is ease of use. Having said that, I continue to keep an open mind. If there's any substantial benefits for adopting this design, I love to hear that and reconsider as necessary. Thanks for the note.

@KlausC - your thoughts?

KlausC commented 4 years ago

it is a considerable drawback that types are generated with a macro ina kinda invisible way

I must admit, that was also one of my feelings when I saw the syntax design for the first time. That is the case for all generated names XyzTrait, xyztrait, CanXyz, CannotXyz, which are all derived from the user's @trait Xyz. From a technical point of view, it would look cleaner, if we had for example Xyz, trait(Xyz, x), Can{Xyz} and Cannot{Xyz}.

For the user it may be a little less convenient and readable, if he has to handle the curly brackets, but he would not have much fun with Julia anyhow, if that bothered him. In the macros, we could even give more readable syntax replacing @assign Duck with CanFly with

    @implement can Fly by fly ...
    @assign Duck can Fly

which would be equivalent to

    @implement Can{Fly} by fly ...
    @assign Duck with Can{Fly}

The names Has{FlyTrait} and Lack{FlyTrait} are not very generic, I think. Maybe there are two better words available in English.

And I don't understand, why their common supertype is called AbstractProperty{FlyTrait} and not AbstractTrait{Fly}, for example.

KlausC commented 4 years ago

We could also provide synonyms for user friendliness.

    struct Can{T} <: AbstractTrait{T} end
    struct Cannot{T} <: AbstractTrait end
    const Has{T} = Can{T}
    const Is{T} = Can{T}
    const HasNot{T} = Cannot{T}
    const Lack{T} = Cannot{T}

The @trait macro would be only needed for composite traits; but we could also use it to define some extra constants like const CanFly = Can{Fly}. For the trait types like Fly, Array it would be possible to use any type; either defined for the traits purpose by the user or from any pre-existing type.

JanisErdmanis commented 4 years ago

If Can and Cannot are the only options for the trait then macros could be simplified:

@implement FlyTrait by fly(_, destination::Location, speed::Float64)
@assign Duck with FlyTrait

then only when methods would be defined user gets exposed with parametric types.

tk3369 commented 4 years ago

Re-opening issue since we are having some high quality discussions here.

The best way to test an idea is to just do it :-) I just uploaded a branch tk/parametric and it works like this:

julia> @trait Fly

julia> @implement FlyTrait by fly(_, speed::Float64)

julia> struct Duck end

julia> @assign Duck with FlyTrait

julia> @check Duck
┌ Warning: Missing implementation
│   contract = BinaryTraits.AbstractTrait{FlyTrait}: Can{FlyTrait} ⇢ fly(🔹, ::Float64)::Any
└ @ BinaryTraits ~/Library/Mobile Documents/com~apple~CloudDocs/Programming/Julia/BinaryTraits.jl/src/interface.jl:59
❌ Duck is missing these implementations:
1. BinaryTraits.AbstractTrait{FlyTrait}: Can{FlyTrait} ⇢ fly(🔹, ::Float64)::Any

julia> fly(::Duck, speed) = "woo hoo! flying at $speed"
fly (generic function with 1 method)

julia> @check Duck
✅ Duck has implemented:
1. BinaryTraits.AbstractTrait{FlyTrait}: Can{FlyTrait} ⇢ fly(🔹, ::Float64)::Any

Do you guys mind playing with it with some real cases (if any)? Does it feel more natural?

I noticed one difference when I was implementing this proof of concept. The Can/Cannot/Is/Not/Has/Lack types are defined in BinaryTraits, and I have to export them for the sake of support holy traits e.g.

fly(x) = fly(flytrait(x), x)
fly(::Can{FlyTrait}, x) = ...
fly(::Cannot{FlyTrait}, x) = ...

At this point, the only "magical" thing left are:

  1. @trait Fly defines FlyTrait type.
  2. @trait Fly defines a flytrait function that returns Cannot{FlyTrait}().

If we want less magical things thing then we can let the user decide the name of the trait fully i.e. @trait FlyTrait.

KlausC commented 4 years ago

@implement FlyTrait by fly ...

I think that is a step back. We were already able to call @implement CanFly ans well as @implement CannotFly, and that was a big improvement in my eyes.

If we want less magical things thing then we can let the user decide the name of the trait fully i.e. @trait FlyTrait.

I would prefer to remove both magicals. The function flytrait is not needed at all, if we use the generic traitwhatevername(Fly, x) instead of flytrait(x).

tk3369 commented 4 years ago
  1. Regarding @implement FlyTrait by fly ...

I think that is a step back. We were already able to call @implement CanFly ans well as @implement CannotFly, and that was a big improvement in my eyes.

I thought about that a little bit as well. The fact that we can assign contracts to the negative side of trait appears to be a feature. However, I struggled to come up with a use case for that. Can you think of a more practical example.... other than our magical animal kingdom ones 🐦 🐱 🐶 ?

  1. Based upon your comment above, we could remove the last bit of magic with this:
@trait Iterable

is translated to

abstract type Iterable <: Any end          # No more magical Trait suffix
can(::Iterable, x) = Cannot{Iterable}()    # No more magical `<x>trait` function

It's slightly awkward with Holy Traits pattern with an additional argument... although it doesn't bother me too much (yet).

traverse(x) = traverse(can(Iterable,x), x) 
KlausC commented 4 years ago

Can you think of a more practical example

Let's take an example from linear algebra; I am personally interested in this one. (I am using the syntax in master here, not the proposed parameterised form)

@trait Sparse prefix Is,IsNot
@implement IsSparse by sparse_powerto(_, n::Integer)
@implement IsNotSparse by generic_powerto(_, n::Integer)

@assign AbstractSparseMatrix with IsSparse
@assign AbstractDenseMatrix with IsNotSparse

sparse_powerto(A::AbstractSparseMatrix, n::Integer) = ...
# this on this missing! generic_powerto(A::AbstractDenseMatrix, n::Integer) = ...

@check AbstractSparseMatrix   # would be ok
@check AbstractDenseMatrix    # would be not ok - but only if we had the definitions relating IsNotSparse above.
KlausC commented 4 years ago

can(::Iterable, x) = Cannot{Iterable}()

That really looks weird! I would rather then

    # in BinaryTraits.jl
    abstract type Trait{T} end
    struct Can{T} <: Trait{T} end
    struct Cannot{T} <: Trait{T} end
    const Has{T} = Can{T}
    const Is{T} = Can{T}
    const HasNot{T} = Cannot{T}
    const Lack{T} = Cannot{T}
    const IsNot{T} = Cannot{T}

    # generated through @trait Iterable pref=Is,IsNot
    Trait{Iterable}(x::Type)  = IsNot{Iterable}()

    # generated through @assign XXX with Is{Iterable}
    Trait{Iterable}(x::Type{<:XXX}) = Is{Iterable}()
JeffreySarnoff commented 4 years ago

imo there is no reason to prefer keeping an API that is so new and serves developers who for now would have an easy time moving forward with you into a clearer, cleaner, more easily adaptable and robustly sharedable API. For this package to become more widely appreciated, practice some "break it while you make it".

JeffreySarnoff commented 4 years ago

Simply because "Trait" is an oft-used token with Julia's trait-related package devs and appears with regularity in helpful docs, and it would be nice to introduce a most abstract abstract trait type that is easy to remember (like AbstractFloat) ...

abstract type AbstractTrait{T} end
abstract type BinaryTrait{T} <: AbstractTrait{T} end
tk3369 commented 4 years ago

OK, I just updated the branch tk/parametric. Want to try that out? It now works like this:

# The default prefix is now "Positive" and "Negative" (rather than Can/Cannot)
# User is encouraged to specify a more reasonable prefix for the trait.

@trait Fly prefix Can,Cannot
@implement Can{Fly} by fly(_)
@implement Cannot{Fly} by roll(_)

@trait Swim prefix Can,Cannot
@implement Can{Swim} by swim(_)

struct Duck end
@assign Duck with Can{Fly},Can{Swim}

# assigning to a Negative trait explicitly (interface will be checked)
struct Dog end
@assign Dog with Cannot{Fly},Can{Swim}

# Use `trait` function to figure out if an object has or does not have trait
trait(Fly, Duck())   # Positive{Fly}()
trait(Fly, Dog())    # Negative{Fly}()

# Duck - fully implement interface
fly(::Duck) = "woo hoo!"
swim(::Duck) = "splash!"
@check Duck
#=
✅ Duck has implemented:
1. BinaryTraits.AbstractTrait{Swim}: Positive{Swim} ⇢ swim(🔹)::Any
2. BinaryTraits.AbstractTrait{Fly}: Positive{Fly} ⇢ fly(🔹)::Any
=#

# Dog - nothing implemented.  Note negative trait is also checked.
@check Dog
#=
❌ Dog is missing these implementations:
1. BinaryTraits.AbstractTrait{Swim}: Positive{Swim} ⇢ swim(🔹)::Any
2. BinaryTraits.AbstractTrait{Fly}: Negative{Fly} ⇢ roll(🔹)::Any
=#
KlausC commented 4 years ago

I am getting happy with this now. Only small remarks left:

  1. I support @JeffreySarnoff 's proposal

it would be nice to introduce a most abstract abstract trait type that is easy to remember (like AbstractFloat) ...


abstract type AbstractTrait{T} end
abstract type BinaryTrait{T} <: AbstractTrait{T} end

and use BinaryTrait where we are using AbstractTrait now


2. The second argument of `trait` should be a `Type` and not an object, because the trait should be derivable from an object's type alone.

@trait Swim prefix Can, Cannot

trait(::Type{Swim}, ::Type) = Cannot{Swim}()

@assign Waterbird with Can{Swim}

trait(::Type{Swim}, ::Type{S}) where S <: Waterbird = Can{Swim}() # note the <: to cover subtypes

maybe for convenience in traits.jl but I am not sure about that. Typical TH traits

don't and maybe shouldn't rely on object properties.

trait(T::Type, x::X) where {T,X} = trait(T, X)

tk3369 commented 4 years ago

@KlausC and @JeffreySarnoff - Just for the record, can you elaborate why this additional level of abstract type may be useful?

Great catch about the second argument of trait. Will do.

KlausC commented 4 years ago

why this additional level of abstract type may be useful

The word AbstractTrait is so general, that we should not use it exclusively for BinaryTraits. There could be a competitive or complementary package in future, which also wants to implement a kind of AbstractTrait.

JeffreySarnoff commented 4 years ago

Also, one day Base may export AbstractTrait and it would be nice to be pre-then compatible. .. that is more likely to be abstract type AbstractTrait end than abstract type AbstractTrait{T} end .. so maybe

abstract type AbstractTrait end
abstract type BinaryTrait{T} <: AbstractTrait end
tk3369 commented 4 years ago

I'm not sure if I follow. Shouldn't we not define AbstractTrait at all if we feel that one day Base may export its own AbstractTrait type? It does make sense for us to define BinaryTrait rather than AbstractTrait to avoid the possible naming conflict. Then, when Base does export an abstract type traits in the future, we can still subtype from it.

So these three lines:

abstract type AbstractTrait{T} end
struct Positive{T} <: AbstractTrait{T} end
struct Negative{T} <: AbstractTrait{T} end

becomes

abstract type BinaryTrait{T} end
struct Positive{T} <: BinaryTrait{T} end
struct Negative{T} <: BinaryTrait{T} end
KlausC commented 4 years ago

did you already see this: https://github.com/thautwarm/CanonicalTraits.jl - maybe a source of inspiration...

tk3369 commented 4 years ago

Looks pretty cool. I've seen that before but haven't really tried it. I can add it to README's related projects section. We're definitely not in any shortage of traits packages these days 😄

tk3369 commented 4 years ago

PR is ready. We are going forward with this 😄