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

Method inheritance #30408

Closed
bramtayl opened this issue Dec 16, 2018 · 21 comments
Closed

Method inheritance #30408

bramtayl opened this issue Dec 16, 2018 · 21 comments

Comments

@bramtayl
Copy link
Contributor

bramtayl commented Dec 16, 2018

If these were inferable, defining interfaces could be quite a bit robust. For example, you could define is_abstract_array(x) = hasmethod(eltype, Tuple{typeof(x)}) && hasmethod(getindex, Tuple{typeof(x), Int}).

Edit: constant propagatable hasproperty would also be cool

@bramtayl
Copy link
Contributor Author

And in fact, a lot of previously defined traits could disappear. HasLength, HasSize, HasEltype, etc.

@bramtayl
Copy link
Contributor Author

After thinking about this for an hour I don't think this would be useful. For example, if you defined:

length(x::MyType) = rand(Bool) ? 1 : error() then even though a method for length is defined, length isn't really defined... I'm sure there are complicated trait-based systems that can handle this better.

@bramtayl bramtayl changed the title Constant inferrable has_method or applicable? Required method Dec 28, 2018
@bramtayl
Copy link
Contributor Author

Ok, here's a better system that I think is actually workable. Instead of relying on traits like HasEltype, instead use:

@inline required_method(::typeof(eltype), a::AbstractArray) = true
@inline required_method(::typeof(eltype), anything) = false

and then instead of dispatching based on AbstractArray, do

function collect(x)
    if required_method(eltype, x) && required_method(axes, x) && required_method(getindex, x, 1)
        # abstract array assumptions
    else
        # general iterator assumptions
    end
end

@bramtayl bramtayl reopened this Dec 28, 2018
@bramtayl
Copy link
Contributor Author

Ok so this is actually kinda a cool idea right?

@StefanKarpinski
Copy link
Member

Perhaps? I'm a little unclear on what you're proposing / requesting here.

@bramtayl
Copy link
Contributor Author

Instead of relying on duck-typing, we could do compile-tile math to check for the existence of required methods.

@bramtayl
Copy link
Contributor Author

Does that makes sense? Or am I crazy

@favba
Copy link
Contributor

favba commented Jan 18, 2019

I felt the need for a similar feature when I was writing my own ReinterpretArray.
I was trying to define a struct that is a wrapper around an AbstractArray.
If the array is a DenseArray so my array wrapper should be and all defined methods for DenseArray should just work (BLAS routines calls, FFTW).
But currently, with type dispatch system instead of trait dispatch, I wasn't able to do that.
So I had to define two struct that behaves exactly the same in order for that to work.

@bramtayl
Copy link
Contributor Author

bramtayl commented Mar 28, 2019

So here's a more detailed example:

to_type(x) = typeof(x)
to_type(x::Type) = x

"""
    works(f_type::Type, in_types::Type...)

Check whether a function of type `f_type` is expected to work on arguments of types `in_types`.

    works(f, ins...) = works(typeof(f), typeof.(ins)...)

Provided for convenience; define new `works` methods `Type` arguments.
"""
works(ins...) = works(to_type.(ins)...)
works(::Vararg{Type}) = false
works(::Type{F}, ::Type{A}) where {F <: typeof(size), A <: AbstractArray} = true
works(::Type{F}, ::Type{A}, ::Vararg{Type{Int}, N}) where {N, F <: typeof(getindex), A <: AbstractArray{T, N} where {T}} = true
works(::Type{F}, ::Type{A}, ::Vararg{Type{Int}, N}) where {N, F <: typeof(setindex!), A <: AbstractArray{T, N} where {T}} = true

using Test: @test
@test works(size, [1])
@test works(getindex, [1], 1)
@test works(setindex!, [1], 1)

has_linear_indices(x) = works(size, x) & works(getindex, x, 1) & works(setindex!, x, 1)
@test has_linear_indices([1])
@code_warntype has_linear_indices([1])
# return true

What do people think?

@bramtayl
Copy link
Contributor Author

@StefanKarpinski does that clear things up?

@StefanKarpinski
Copy link
Member

Doesn't applicable do this already?

@bramtayl
Copy link
Contributor Author

Sort of, but applicable doesn't constant infer (so you can't use it for dispatch), and the semantics are slightly difference. applicable checks whether a method is defined, whereas works would check whether a method is expected as part of an interface.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Mar 28, 2019

This issue is titled "Required method" and continues with "If these were inferable, defining interfaces could be quite a bit robust." I still have no idea what this means. What is a "required method"? Is this a language feature you are proposing adding to the language? Are you proposing a generic API for asking what a type expects people to implement? What does it have to do with inference and constant propagation? That seems totally unrelated...

@bramtayl
Copy link
Contributor Author

:( I’m always so sad when things make perfect sense in my head but I can’t communicate effectively. Let me try again? Say you have something like mappedarray. Its not an abstract array because it doesn’t have an eltype, and you can’t setindex!. But you can get its size and getindex into it. So you have a function (like sum), which would theoretically work on both abstractarrays and mapped arrays. If you don’t want to have to write the same method twice, you could use works to check whether or not an input supports getindex.

@timholy
Copy link
Member

timholy commented Mar 28, 2019

How does this improve on traits?

Hmm, maybe I should add a GSOC project: make Traitor work. I'm pretty sure we have everything we need now, see andyferris/Traitor.jl#8.

@StefanKarpinski
Copy link
Member

How would one declare this sort of thing? Using method definitions like in your post above?

@bramtayl
Copy link
Contributor Author

bramtayl commented Mar 28, 2019

I mean, I guess I'm basically talking about traits, but I'm being a bit more specific by organizing them around methods, and you can combine them in various logical ways because the Bools will constant propagate. So here's maybe how you could use it in practice:

collect(it) =
    if works(size, it)
        if works(eltype, it)
            collect_size_eltype(it)
        else
            collect_size_no_eltype(it)
        end
    elseif works(length, it)
        if works(eltype, it)
            collect_length_eltype(it)
        else
            collect_length_no_eltype(it)
        end
    else
        if works(eltype, it)
            collect_no_length_eltype(it)
        else
            collect_no_length_no_eltype(it)
        end
    end

@timholy
Copy link
Member

timholy commented Mar 28, 2019

Compare with traits:

@traitor collect(it::::Tuple{HasShape,HasEltype}) = collect_size_eltype(it)
@traitor collect(it::::Tuple{HasShape,EltypeUnknown}) = collect_size_no_eltype(it)
@traitor collect(it::::Tuple{HasLength,HasEltype}) = collect_length_eltype(it)
@traitor collect(it::::Tuple{HasLength,EltypeUnknown}) = collect_length_no_eltype(it)
@traitor collect(it::::Tuple{SizeUnknown,HasEltype}) = collect_no_length_eltype(it)
@traitor collect(it::::Tuple{SizeUnknown,EltypeUnknown}) = collect_no_length_no_eltype(it)

How different is that, really? I agree that not needing to know the trait name has a certain appeal; OTOH, your definition is "closed" whereas someone can still specialize this further.

@bramtayl
Copy link
Contributor Author

Eh I guess that's the same with nicer syntax. Well except that you could do any operation that works with Bools (|, xor, any, etc.) if traits were based on them.

@bramtayl bramtayl changed the title Required method Method inheritance Feb 24, 2020
@bramtayl
Copy link
Contributor Author

Further thoughts in light of #34196: maybe a broader idea is a system of method inheritance. A method might only be defined on a type if other methods are also defined on that type.

@bramtayl
Copy link
Contributor Author

bramtayl commented May 9, 2020

Closed in favor of #35095

@bramtayl bramtayl closed this as completed May 9, 2020
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

4 participants