more complete ambiguity detection #16255
Conversation
|
Given the current state of subtyping, ambiguity detection isn't going to be perfect. I don't think it's good to sometimes warn at definition time and sometimes not. That just adds indecision and unclarity on top of imperfection. It seems to me specificity should still be transitive. Partial orders still obey transitivity. |
So you've mentioned, but also doesn't come up here
Sounds great. When did I contradict this? If they don't get printed at definition time in the WIP, bootstrap fails without a warning for this WIP. But good luck working out why
possibly not the exact right concept, but this is the closest description I could muster to the problem evidenced by your example in #16220 (comment). I think an unambiguous partial order obeys transitivity. However, the detection here is for whether the ordering is ambiguous (by verifying that the partial order doesn't require an extra intersection signature method). |
|
Is this what you need, Jameson? baremodule Mod
const Base = Main.Base
for n in Base.names(Base, true, true)
Base.isdefined(Base, n) || continue
try
for m in Base.methods(Base.getfield(Base, n))
eval(Mod, :(($n)($(Base.map(x->:(::$x), Base.getindex(m.sig.parameters,Base.colon(2, Base.length(m.sig.parameters))))...)) = 1))
end
catch
Base.println("skipping $n")
end
end
end
julia> Test.detect_ambiguities(Mod)
74-element Array{Tuple{Method,Method},1}:
# …full list. There's a terrifying number of |
|
yep, that looks like the same list (https://github.com/JuliaLang/julia/pull/16255/files#diff-080ddbc6b9941ca45cdaa2622943f143R1) |
vtjnash
changed the title
| @@ -117,7 +117,7 @@ end | |||
|
|
|||
| ## Constructors ## | |||
|
|
|||
| similar(a::Array, T, dims::Dims) = Array(T, dims) | |||
| similar(a::Array, T::Type, dims::Dims) = Array(T, dims) | |||
previously we stopped checking for ambiguities as soon as we reached the definition this assumed that jl_args_morespecific was transitive, but in actuality it is only a partial sort and we build the typemap assuming no ambiguities, which sometimes made it even less accurate at finding them
| @@ -340,6 +340,9 @@ function round{T}(::Type{T}, x::Rational{Bool}) | |||
| convert(T, x) | |||
| end | |||
|
|
|||
| round{T}(::Type{T}, x::Rational{Bool}, ::RoundingMode{:Nearest}) = round(T, x) | |||
| round{T}(::Type{T}, x::Rational{Bool}, ::RoundingMode{:NearestTiesAway}) = round(T, x) | |||
| round{T}(::Type{T}, x::Rational{Bool}, ::RoundingMode{:NearestTiesUp}) = round(T, x) | |||
There was a problem hiding this comment.
they resolve the ambiguity of the method that follows
There was a problem hiding this comment.
what's the ambiguity though, aren't these 3 subtypes of the 4th? nevermind I see it higher up in the file now
|
Do you need help anymore fixing the ambiguities? I just checked out this branch and built with If you already got them all, thanks! And sorry to be late to the party. Mergeable, right? |
|
Thanks for the offer. It turned out most of them were simple method typos (repeats of #16254 for other AbstractArrays), not anything complicated so they're all fixed now. This is simply waiting for someone to hit the merge button. |
Well, I can handle that. Thanks for doing this! |
previously we stopped checking for ambiguities as soon as we reached the definition. this assumed that jl_args_morespecific was transitive, but in actuality it is only a partial sort and we build the typemap assuming no ambiguities, which sometimes made it even less accurate at finding them.
note that I needed to partially revert to printing ambiguities or this doesn't make it very far in bootstrap before throwing a method ambiguity error. the RFH (request for help) is to crowd-source resolving these ambiguities (or determining which aren't applicable due to that argument combination not being meaningful).ref #16252
ref #16220 (comment)
edit: additionally, due to some recent change, we seem to be unable to detect the ambiguities with Varargs previously computed by https://travis-ci.org/JuliaLang/julia/jobs/126188063edit2: I mistake in that PR was making it possible to compute that ambiguity (while breaking everything else), it's not a recent regression to be unable to compute that.