Merge pull request #37616 from JuliaLang/jn/36962-again

improve accuracy of ambiguity checking (take 2)

Author: vtjnash

base/reflection.jl

@@ -1363,6 +1363,8 @@ Determine whether two methods `m1` and `m2` may be ambiguous for some call
 signature. This test is performed in the context of other methods of the same
 function; in isolation, `m1` and `m2` might be ambiguous, but if a third method
 resolving the ambiguity has been defined, this returns `false`.
+Alternatively, in isolation `m1` and `m2` might be ordered, but if a third
+method cannot be sorted with them, they may cause an ambiguity together.
 
 For parametric types, the `ambiguous_bottom` keyword argument controls whether
 `Union{}` counts as an ambiguous intersection of type parameters – when `true`,
@@ -1389,27 +1391,87 @@ false
 ```
 """
 function isambiguous(m1::Method, m2::Method; ambiguous_bottom::Bool=false)
-    # TODO: eagerly returning `morespecific` is wrong, and fails to consider
-    # the possibility of an ambiguity caused by a third method:
-    # see the precise algorithm in ml_matches for a more correct computation
-    if m1 === m2 || morespecific(m1.sig, m2.sig) || morespecific(m2.sig, m1.sig)
-        return false
-    end
+    m1 === m2 && return false
     ti = typeintersect(m1.sig, m2.sig)
-    (ti <: m1.sig && ti <: m2.sig) || return false # XXX: completely wrong, obviously
     ti === Bottom && return false
-    if !ambiguous_bottom
-        has_bottom_parameter(ti) && return false
-    end
-    matches = _methods_by_ftype(ti, -1, typemax(UInt))
-    for match in matches
-        m = match.method
-        m === m1 && continue
-        m === m2 && continue
-        if ti <: m.sig && morespecific(m.sig, m1.sig) && morespecific(m.sig, m2.sig)
+    function inner(ti)
+        ti === Bottom && return false
+        if !ambiguous_bottom
+            has_bottom_parameter(ti) && return false
+        end
+        min = UInt[typemin(UInt)]
+        max = UInt[typemax(UInt)]
+        has_ambig = Int32[0]
+        ms = _methods_by_ftype(ti, -1, typemax(UInt), true, min, max, has_ambig)
+        has_ambig[] == 0 && return false
+        if !ambiguous_bottom
+            filter!(ms) do m
+                return !has_bottom_parameter(m.spec_types)
+            end
+        end
+        # if ml-matches reported the existence of an ambiguity over their
+        # intersection, see if both m1 and m2 may be involved in it
+        have_m1 = have_m2 = false
+        for match in ms
+            m = match.method
+            m === m1 && (have_m1 = true)
+            m === m2 && (have_m2 = true)
+        end
+        if !have_m1 || !have_m2
+            # ml-matches did not need both methods to expose the reported ambiguity
+            return false
+        end
+        if !ambiguous_bottom
+            # since we're intentionally ignoring certain ambiguities (via the
+            # filter call above), see if we can now declare the intersection fully
+            # covered even though it is partially ambiguous over Union{} as a type
+            # parameter somewhere
+            minmax = nothing
+            for match in ms
+                m = match.method
+                match.fully_covers || continue
+                if minmax === nothing || morespecific(m.sig, minmax.sig)
+                    minmax = m
+                end
+            end
+            if minmax === nothing
+                return true
+            end
+            for match in ms
+                m = match.method
+                m === minmax && continue
+                if match.fully_covers
+                    if !morespecific(minmax.sig, m.sig)
+                        return true
+                    end
+                else
+                    if morespecific(m.sig, minmax.sig)
+                        return true
+                    end
+                end
+            end
             return false
         end
+        return true
+    end
+    if !(ti <: m1.sig && ti <: m2.sig)
+        # When type-intersection fails, it's often also not commutative. Thus
+        # checking the reverse may allow detecting ambiguity solutions
+        # correctly in more cases (and faster).
+        ti2 = typeintersect(m2.sig, m1.sig)
+        if ti2 <: m1.sig && ti2 <: m2.sig
+            ti = ti2
+        elseif ti != ti2
+            # TODO: this would be the correct way to handle this case, but
+            #       people complained so we don't do it
+            # inner(ti2) || return false
+            return false # report that the type system failed to decide if it was ambiguous by saying they definitely aren't
+        else
+            return false # report that the type system failed to decide if it was ambiguous by saying they definitely aren't
+        end
     end
+    inner(ti) || return false
+    # otherwise type-intersection reported an ambiguity we couldn't solve
     return true
 end
 

src/clangsa/GCChecker.cpp

@@ -1340,16 +1340,16 @@ bool GCChecker::evalCall(const CallExpr *CE,
     return true;
   } else if (name == "JL_GC_PUSH1" || name == "JL_GC_PUSH2" ||
              name == "JL_GC_PUSH3" || name == "JL_GC_PUSH4" ||
-             name == "JL_GC_PUSH5" || name == "JL_GC_PUSH6") {
+             name == "JL_GC_PUSH5" || name == "JL_GC_PUSH6" ||
+             name == "JL_GC_PUSH7") {
     ProgramStateRef State = C.getState();
     // Transform slots to roots, transform values to rooted
     unsigned NumArgs = CE->getNumArgs();
     for (unsigned i = 0; i < NumArgs; ++i) {
       SVal V = C.getSVal(CE->getArg(i));
       auto MRV = V.getAs<loc::MemRegionVal>();
       if (!MRV) {
-        report_error(C,
-                     "JL_GC_PUSH with something other than a local variable");
+        report_error(C, "JL_GC_PUSH with something other than a local variable");
         return true;
       }
       const MemRegion *Region = MRV->getRegion();