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

Interface Checking Broken on 1.10.0 #62

Open SBuercklin opened 8 months ago

SBuercklin commented 8 months ago

MWE

using BinaryTraits
using BinaryTraits: Prefix

julia> @trait Foo

julia> @implement Prefix.Is{Foo} by f(_, x)

julia> struct Bar end

julia> f(::Bar, x) = 1

julia> @assign Bar with Prefix.Is{Foo}

julia> @check Bar
ERROR: Tuple field type cannot be Union{}

The issue seems to be covered by this issue on the Julia repo. If that problem is resolved such that we can use Tuple{Union{}} again, I suspect this will go back to working

SBuercklin commented 8 months ago

Stopgap solution: if you add type annotations to your @implement line, even trivial ones like Any, the checker works


julia> @implement Prefix.Is{Foo} by f(_, x::Any)

julia> @check Bar
✅ Bar has implemented:
1. BinaryTrait{Foo}: Positive{Foo} ⇢ f(🔹, ::Any)
kylebeggs commented 8 months ago

Huge help, @SBuercklin.

kylebeggs commented 8 months ago

Temporary solution still breaks if you type annotate the implementation:

julia> using BinaryTraits

julia> using BinaryTraits: Prefix

julia> @trait Foo

julia> @implement Prefix.Is{Foo} by f(_, x::Any)

julia> struct Bar end

julia> f(::Bar, x::Float64) = 1
f (generic function with 1 method)

julia> @assign Bar with Prefix.Is{Foo}

julia> @check Bar
┌ Warning: Missing implementation
│   contract = BinaryTrait{Foo}: Positive{Foo} ⇢ f(🔹, ::Any)
└ @ BinaryTraits ~/.julia/packages/BinaryTraits/Hl5cc/src/interface.jl:62
❌ Bar is missing these implementations:
1. BinaryTrait{Foo}: Positive{Foo} ⇢ f(🔹, ::Any) (Missing implementation)
SBuercklin commented 8 months ago

That's an expected failure because the implemented method does not satisfy the interface.

kylebeggs commented 8 months ago

Maybe I don't fully understand how the package works then - I would expect a subtype to satisfy? What am I missing? Nvm - I had them reversed

kylebeggs commented 8 months ago

This is the actual MWE which I am struggling with (which works with Julia 1.9):

using BinaryTraits
using BinaryTraits: Prefix

@trait Foo
@trait Faz

abstract type AbstractBar end
abstract type AbstractBoo end
struct Bar end
struct Boo end

@implement Prefix.Is{Foo} by f(_, x)
@implement Prefix.Is{Faz} by f(x, _)

f(::Bar, x::Boo) = 1

@assign Bar with Prefix.Is{Foo}
@assign Boo with Prefix.Is{Faz}

@check Bar
@check Boo

I don't think you can make the stopgap solution work for this scenario?

SBuercklin commented 8 months ago

f does not satisfy the interface for the same reasons above.

kylebeggs commented 8 months ago

Yeah that makes sense, so this passing in Julia <1.9 was a bug really

SBuercklin commented 8 months ago

No, the interfaces defined by

@implement Is{Foo} by f(_, x)

vs

@implement Is{Foo} by f(_, ::Any)

are distinct.

If you need a more strict interface than ::Any, you need to specify a more strict type. The inclusion of ::Any matches the method signature implemented to satisfy the interface in the "stopgap" solution. If you aren't implementing for ::Any, you can't specify ::Any and expect it to work. The original bug is related to how BinaryTraits.jl handles the first case, which doesn't map onto replacing x by x::Any.

kylebeggs commented 8 months ago

Ok, I suppose the lapse in my understanding then is how exactly BinaryTraits.jl handles @implement Is{Foo} by f(_, x)? Its confusing to me because by leaving out the type you are essentially saying its ::Any, right?

tk3369 commented 2 months ago

Sorry for being late to the party :-) I have been too busy at work and not able to spend time on open source lately.

I have looked into this issue. It appears that the problem will be fixed by Julia 1.12 according to the latest comment from this issue https://github.com/JuliaLang/julia/issues/52385

Why this was broken

As a trait package, BinaryTraits provide a way to suers to define interfaces and validate structs against pre-defined interfaces. When an interface contract says that a function must work with Integer, then any function defined with any supertype of Integer would satisfy the interface. In order to express a contract that does not care about the type, we use Base.Bottom type to be the anchor point of the validation.

This diagram illustrate how it works https://excalidraw.com/#json=t7I0lkZbLr9zGZJRFfWia,2FVa-UCV3MxUMmTozR3ojA image

The primary facility that it checks for implementation is to use hasmethod function, which takes a Tuple type. But Bottom (or Union{}) can no longer be used due to the Julia issue.

julia> f(::Integer) = 1

julia> hasmethod(f, Tuple{Union{}})
ERROR: Tuple field type cannot be Union{}

Workaround

The stop gap solution suggested above works, but I want to point out that it gives you different semantic. If you replace x with x::Any then you are literally requiring the function to be defined with x or x::Any (as opposed to allowing any type). It might work in your use case if that's the intention.

SBuercklin commented 2 months ago

Thanks for checking in :)

I'm happy to see #52385 is getting resolved, having a bottom type and being able to make Tuple types with it is quite useful. Unfortunate that it's not going to make it into 1.11, but I'm glad it's getting addressed nonetheless