Skip to content
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

Interface Checking Broken on 1.10.0 #62

Open
SBuercklin opened this issue Jan 10, 2024 · 12 comments
Open

Interface Checking Broken on 1.10.0 #62

SBuercklin opened this issue Jan 10, 2024 · 12 comments

Comments

@SBuercklin
Copy link

SBuercklin commented Jan 10, 2024

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
Copy link
Author

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
Copy link

Huge help, @SBuercklin.

@kylebeggs
Copy link

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
Copy link
Author

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

@kylebeggs
Copy link

kylebeggs commented Jan 18, 2024

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
Copy link

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
Copy link
Author

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

@kylebeggs
Copy link

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

@SBuercklin
Copy link
Author

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
Copy link

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
Copy link
Owner

tk3369 commented Jul 20, 2024

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 JuliaLang/julia#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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants