Multiple Inheritance #43
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
===========================================
- Coverage 85.11% 74.23% -10.89%
===========================================
Files 5 5
Lines 215 260 +45
===========================================
+ Hits 183 193 +10
- Misses 32 67 +35 ☔ View full report in Codecov by Sentry. |
|
One caveat here I havent really thought through is that any kind of inheritance necessitates that tests of all inherited interfaces accept the same objects Mostly I think that will be fine, but may hit problems where mulitiple arguments are used |
|
I'll check this out! |
|
Sorry I did not take the time, will make a pass this week |
|
No worries! |
There was a problem hiding this comment.
I don't feel like I can review the internals, but I like the basic mechanism of flattening inheritance, and the way that the inheritance is specified.
Do we have problems if we try to dispatch on several interfaces at once?
For instance if I define a function f(x, y) and I want the first argument to implement ArrayInterface and the second DictInterface. I know it's been a problem for traits packages too (see https://github.com/mauro3/SimpleTraits.jl?tab=readme-ov-file#details-of-method-dispatch for example)
| @test supertype(ArrayInterface) <: Interfaces.Interface{<:Any,IterationInterface{(:reverse, :indexing)}} | ||
| @test supertype(DictInterface) <: Interfaces.Interface{<:Any,IterationInterface{(:reverse,)}} |
There was a problem hiding this comment.
| @test supertype(ArrayInterface) <: Interfaces.Interface{<:Any,IterationInterface{(:reverse, :indexing)}} | |
| @test supertype(DictInterface) <: Interfaces.Interface{<:Any,IterationInterface{(:reverse,)}} | |
| @test supertype(ArrayInterface) <: Interfaces.Interface{<:Any,>:IterationInterface{(:reverse, :indexing)}} | |
| @test supertype(DictInterface) <: Interfaces.Interface{<:Any,>:IterationInterface{(:reverse,)}} |
There was a problem hiding this comment.
in case we define more interfaces in the future and the second parameter is a union, this seems more robust?
There was a problem hiding this comment.
also why not just test ArrayInterface <: ...?
| @@ -201,4 +201,4 @@ array_components = (; | |||
|
|
|||
| _wrappertype(A) = Base.typename(typeof(A)).wrapper | |||
|
|
|||
| @interface ArrayInterface AbstractArray array_components "Base Julia AbstractArray interface" | |||
| @interface ArrayInterface <: IterationInterface{(:reverse,:indexing)} AbstractArray array_components "Base Julia AbstractArray interface" | |||
There was a problem hiding this comment.
Interface definitions are becoming fairly long to put everything on a single line.
I remember we had discussed defining the documentation somewhere else and just giving the macro a String variable, but for some reason it didn't work?
| implements(::Type{<:Interface}, obj::Type) = false | ||
| implements(T::Type{<:Interface}, obj::Type) = inherits(T, obj) | ||
|
|
||
| function inherits end |
There was a problem hiding this comment.
I'm not qualified to review the exact implem but there are lots of codecov misses here
| @@ -21,6 +21,24 @@ optional_keys(T::Type{<:Interface}) = keys(components(T).optional) | |||
|
|
|||
| mandatory_keys(T::Type{<:Interface}, args...) = keys(components(T).mandatory) | |||
|
|
|||
| function _flatten_inheritance(::Type{T}) where T | |||
There was a problem hiding this comment.
Same here for the codecov misses, is it due to macros?
| end | ||
| _check_no_options(T::Type) = T | ||
| _check_no_options(::Type{<:Interface{Keys}}) where Keys = | ||
| throw(ArgumentError("Interface options not accepted for more than one implementation")) |
| @@ -141,7 +144,7 @@ function test(::Type{T}; show=true, kw...) where T | |||
| results = map(methodlist) do m | |||
| t = m.sig.parameters[2].var.ub | |||
| t isa UnionAll || return true | |||
| interface = t.body.name.wrapper | |||
| interface = t.body.body.name.wrapper | |||
There was a problem hiding this comment.
I recently discovered nameof, which might simplify this kind of stuff?
This PR implements multiple inheritance for
Interfacetypes storing the inherited types in a secondInheritstype parameter of the supertypeInterface, and adding dispatch on the second type parameter in the@implementsmacro so that inheriting types will have a fallback to the inherited types dispatch.The
Inheritsparameter can be aUnion, so dispatch will work for multiple inherited interfaces. We flatten the inheritance when interfaces are chained (likeMyArrayInterfaceinheritsArrayInterfaceinheritsIterationInterface) so everything is in oneUnionrather than nested. So theInheritsparameter will beUnion{ArrayInterface,IterationInterface}orUnion{ArrayInterface{(:some, :options)},IterationInterface}. But the options are stripped byinherited_basetypewhenInheritsis used for dispatch.Needs docs and reorganisation/renaming of all the helper methods, it was hard to write so I imagine also hard to understand.
@gdalle if you have time for some feedback, that could help guiding how to document and explain this.
Closes #6