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

gf: support more dispatch on abstract types #31916

Merged
merged 1 commit into from May 15, 2019
Merged

gf: support more dispatch on abstract types #31916

merged 1 commit into from May 15, 2019

Conversation

@vtjnash
Copy link
Member

vtjnash commented May 3, 2019

This removes the restriction on defining dispatch over user-defined abstract types. This fixes #14919 for 99% of cases.

abstract type A14919; end
struct B14919 <: A14919; end
struct C14919 <: A14919; end
struct D14919 <: Function; end
(::A14919)() = "It's a brand new world"
(::Union{C14919,D14919})() = "Boo."
@test B14919()() == "It's a brand new world"
@test C14919()() == D14919()() == "Boo."

The "cannot add methods to an abstract type" error is now only applicable to a couple types (Any, Function, and functions), and instead now gives a "not implemented yet" message (that's perhaps not entirely right, since in several instances of this, I think they would be ambiguous with calling a Builtin function—and we don't need to ever enable that particular foot-gun).

This PR actually hilariously similar to my branch #14919 (comment) from 3 years ago, but adds a few more error checks so that performance is unaffected.

@vtjnash vtjnash force-pushed the jn/14919 branch from 84e00cb to 2821e9b May 3, 2019
@JeffBezanson

This comment has been minimized.

Copy link
Member

JeffBezanson commented May 5, 2019

I think not being able to define methods for Any is definitely a feature :)

@vtjnash vtjnash force-pushed the jn/14919 branch from 2821e9b to 033d9d4 May 7, 2019
@JeffreySarnoff

This comment has been minimized.

Copy link
Contributor

JeffreySarnoff commented May 9, 2019

ya' know the Community will be dancing while this becomes second nature 🌞

This removes the restriction on defining dispatch over user-defined abstract types.

The "cannot add methods to an abstract type" error is now only
applicable to a couple types (`Any`, `Function`, and functions),
and instead now gives a "not implemented yet" message.

fixes #14919 for 99% of cases
@vtjnash vtjnash force-pushed the jn/14919 branch from 033d9d4 to 7b057bd May 9, 2019
@vtjnash

This comment has been minimized.

Copy link
Member Author

vtjnash commented May 10, 2019

@nanosoldier runbenchmarks(ALL, vs=":master")

@JeffBezanson

This comment has been minimized.

Copy link
Member

JeffBezanson commented May 10, 2019

This benchmark from #21760 goes from .034 sec to .042 sec on this branch:

struct f{t}
end
(::f{t})(x) where {t} = t
@time for i=1:1000; f{i}()(1); end
@nanosoldier

This comment has been minimized.

Copy link
Collaborator

nanosoldier commented May 10, 2019

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@vtjnash

This comment has been minimized.

Copy link
Member Author

vtjnash commented May 14, 2019

@JeffBezanson I ran that code locally, and saw no performance difference. Probably just noise or a different random seed.

@vtjnash vtjnash merged commit 99d2406 into master May 15, 2019
13 of 15 checks passed
13 of 15 checks passed
Nanosoldier possible performance regressions were detected
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
buildbot/package_freebsd64 Run complete
Details
buildbot/package_linux32 Run complete
Details
buildbot/package_linux64 Run complete
Details
buildbot/package_macos64 Run complete
Details
buildbot/package_win32 Run complete
Details
buildbot/package_win64 Run complete
Details
buildbot/tester_freebsd64 Run complete
Details
buildbot/tester_linux32 Run complete
Details
buildbot/tester_linux64 Run complete
Details
buildbot/tester_macos64 Run complete
Details
buildbot/tester_win32 Run complete
Details
buildbot/tester_win64 Run complete
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@vtjnash vtjnash deleted the jn/14919 branch May 15, 2019
else {
tn = jl_new_typename_in((jl_sym_t*)name, module);
if (super == jl_function_type || super == jl_builtin_type || jl_symbol_name(name)[0] == '#') {
// Callable objects (including compiler-generated closures) get independent method tables

This comment has been minimized.

Copy link
@JeffBezanson

JeffBezanson May 15, 2019

Member

This will not catch the case of defining struct Foo <: Function, which is probably fine, just want to check that that's the intent.

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 15, 2019

Author Member

Correct, that was my intention (it's even part of my examples at the top). Eventually the goal is probably just to merge all of the tables, but for now, I just merge the subset that I expect will be small.

//#else
//#define write_ulong(s, x) (write_uint32((s), (x)))
//#define read_ulong(s, x) (read_uint32((s), (x)))
//#endif

This comment has been minimized.

Copy link
@JeffBezanson
@vchuravy

This comment has been minimized.

Copy link
Member

vchuravy commented May 18, 2019

Running into a segmentation fault that bisect blames on this, JuliaGPU/CUDAnative.jl#407

@JeffBezanson JeffBezanson mentioned this pull request May 20, 2019
vtjnash added a commit that referenced this pull request Jun 5, 2019
vtjnash added a commit that referenced this pull request Jun 7, 2019
vtjnash added a commit that referenced this pull request Jun 7, 2019
vtjnash added a commit that referenced this pull request Jun 11, 2019
@MasonProtter

This comment has been minimized.

Copy link
Contributor

MasonProtter commented Jun 13, 2019

The "cannot add methods to an abstract type" error is now only applicable to a couple types (Any, Function, and functions), and instead now gives a "not implemented yet" message (that's perhaps not entirely right, since in several instances of this, I think they would be ambiguous with calling a Builtin function—and we don't need to ever enable that particular foot-gun).

What if we were to allow this, but only in cases where a method error would be thrown? Ie. make (::Function)(x::MyType) = ... the lowest possible priority such that it always loses priority to standard definitions?

vtjnash added a commit that referenced this pull request Jun 14, 2019
vtjnash added a commit that referenced this pull request Jun 18, 2019
@oxinabox oxinabox mentioned this pull request Jul 9, 2019
2 of 2 tasks complete
@caseykneale

This comment has been minimized.

Copy link

caseykneale commented Aug 18, 2019

Awesome I've been waiting for this: https://github.com/caseykneale/ChemometricsTools.jl/blob/master/src/RegressionModels.jl

How can developers maintain backward compatibility with previous julia versions with the implementation of this(once its streamlined)? Maybe the correct solution is to tell users to use the latest version of julia.

@StefanKarpinski

This comment has been minimized.

Copy link
Member

StefanKarpinski commented Aug 18, 2019

This is a language feature that cannot be implemented in a package, so the only option of one needs this is to use a newer Julia version.

kernelmethod added a commit to kernelmethod/LSH.jl that referenced this pull request Nov 27, 2019
…is is valid Julia syntax, see JuliaLang/julia#31916).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.