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: add support for invalidating invoke edges #48954

Merged
merged 1 commit into from
Mar 9, 2023
Merged

gf: add support for invalidating invoke edges #48954

merged 1 commit into from
Mar 9, 2023

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Mar 8, 2023

Apparently we never actually implemented support for invalidation detection on invoke edges, and furthermore, it had erased part of the support for regular edges.

Generalize our code for detecting replacement of a method, to be used when computing replacement of an invoke edge.

Fix #48802

(if you squint, you will notice this PR is mostly about merging several code paths, and not actually quite as involved as it looks at first glance)

Apparently we never actually implemented support for invalidation
detection on invoke edges, and furthermore, it had erased part of the
support for regular edges.

Generalize our code for detecting replacement of a method, to be used
when computing replacement of an invoke edge.

Fix #48802
@vtjnash vtjnash added types and dispatch Types, subtyping and method dispatch backport 1.9 Change should be backported to release-1.9 labels Mar 8, 2023
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this. My only comment of consequence is that this would benefit from a test.

@@ -1817,6 +1817,36 @@ static int jl_type_intersection2(jl_value_t *t1, jl_value_t *t2, jl_value_t **is
return 1;
}

enum morespec_options {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if morespec -> morespecific would be worth the extra Chars. Also, what about morespecifc_true/morespecific_false/morespecific_unknown?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really want to deal with the code churn to rename an existing flag now. It is only used locally anyways.

@@ -1935,6 +1961,11 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
continue;
isect3 = jl_type_intersection(m->sig, (jl_value_t*)mi->specTypes);
if (jl_type_intersection2(type, isect3, &isect, &isect2)) {
// TODO: this only checks pair-wise for ambiguities, but the ambiguities could arise from the interaction of multiple methods
// and thus might miss a case where we introduce an ambiguity between two existing methods
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't doubt this is true, but is there a simple example of how this could happen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple? Probably not haha. The current canonical example in one argument looks something like this, where 1 and 2 are ambiguous, and 3 is ambiguous with 1+2 but neither 1 nor 2:

module Ambig8
# complex / unsorted(-able) ambiguities
f(::Union{typeof(pi), Integer}) =  1
f(::Union{AbstractIrrational, Int}) =  2
f(::Signed) = 3
end

This is only ambiguous over f(::Int), but it causes the sort order for f(::Signed) to be somewhat ambiguous too (since f(::Int) is one subtype of that). This is a simple case though, since it is mostly just subtyping relations, and subtyping is transitive.

A more complicated case (just invented here, never seen before!), might look like the below. Note how no 2 methods are ambiguous (there always exists a parameter of the union that distinguishes them), but that among the 3 of them, there exists a circularity of specificity such that none is uniformly dominant for f(::Int) over the other 2.

julia> f(::Union{Int, UInt, AbstractIrrational}) = 1
f (generic function with 1 method)

julia> f(::Union{Int, Float64, Unsigned}) = 2
f (generic function with 2 methods)

julia> f(::Union{Int, typeof(pi), AbstractFloat}) = 3
f (generic function with 3 methods)

julia> Base.isambiguous(methods(f)[1], methods(f)[2])
false

julia> Base.isambiguous(methods(f)[1], methods(f)[3])
false

julia> Base.isambiguous(methods(f)[2], methods(f)[3])
false

julia> f(1)
ERROR: MethodError: f(::Int64) is ambiguous.

Candidates:
  f(::Union{Float64, Int64, Unsigned})
    @ Main REPL[2]:1
  f(::Union{Irrational{:π}, Int64, AbstractFloat})
    @ Main REPL[3]:1
  f(::Union{Int64, UInt64, AbstractIrrational})
    @ Main REPL[1]:1

Possible fix, define
  f(::Int64)

Stacktrace:
 [1] top-level scope
   @ REPL[7]:1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Genius. And sad that it's so complicated (but no one said subtyping was easy).

@vtjnash vtjnash merged commit 386b1b6 into master Mar 9, 2023
@vtjnash vtjnash deleted the jn/48802 branch March 9, 2023 17:17
timholy added a commit that referenced this pull request Mar 12, 2023
timholy added a commit that referenced this pull request Mar 14, 2023
timholy added a commit that referenced this pull request Mar 18, 2023
To interpret causes of invalidation, it's helpful to understand what
signature got invalidated. #48954 inadvertently dropped this info
from the logging stream; this commit restores it.
vtjnash pushed a commit that referenced this pull request Mar 20, 2023
To interpret causes of invalidation, it's helpful to understand what
signature got invalidated. #48954 inadvertently dropped this info
from the logging stream; this commit restores it.
KristofferC pushed a commit that referenced this pull request Mar 24, 2023
Apparently we never actually implemented support for invalidation
detection on invoke edges, and furthermore, it had erased part of the
support for regular edges.

Generalize our code for detecting replacement of a method, to be used
when computing replacement of an invoke edge.

Fix #48802

(cherry picked from commit 386b1b6)
KristofferC pushed a commit that referenced this pull request Mar 24, 2023
To interpret causes of invalidation, it's helpful to understand what
signature got invalidated. #48954 inadvertently dropped this info
from the logging stream; this commit restores it.

(cherry picked from commit 99fce41)
@KristofferC KristofferC mentioned this pull request Mar 24, 2023
52 tasks
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Mar 31, 2023
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
Apparently we never actually implemented support for invalidation
detection on invoke edges, and furthermore, it had erased part of the
support for regular edges.

Generalize our code for detecting replacement of a method, to be used
when computing replacement of an invoke edge.

Fix JuliaLang#48802
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
To interpret causes of invalidation, it's helpful to understand what
signature got invalidated. JuliaLang#48954 inadvertently dropped this info
from the logging stream; this commit restores it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buggy interaction between @eval and delete_method
3 participants