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 lookup with Tuple{DataType,Any} regression #23024

Closed
yuyichao opened this issue Jul 29, 2017 · 4 comments · Fixed by #23058
Closed

Method lookup with Tuple{DataType,Any} regression #23024

yuyichao opened this issue Jul 29, 2017 · 4 comments · Fixed by #23058
Labels
bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version types and dispatch Types, subtyping and method dispatch

Comments

@yuyichao
Copy link
Contributor

Reduced from OpenCL.jl test failure.

function f2(::Type{T}, ::Int) where T
    1 + 1
end
@show code_typed(f2, Tuple{DataType,Any})

v = 1
T = UInt8
g() = f2(T::DataType, v)

@code_warntype g()
g()

Output:

code_typed(f2, Tuple{DataType, Any}) = 0-element Array{Any,1}
Variables:
  #self#::#g

Body:
  begin
      return (Main.f2)((Core.typeassert)(Main.T, Main.DataType)::DataType, Main.v)::Union{}
  end::Union{}
Unreachable reached at 0x7fb886a9814e

Doesn't happen on 0.6.

@yuyichao yuyichao added bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version types and dispatch Types, subtyping and method dispatch labels Jul 29, 2017
@yuyichao
Copy link
Contributor Author

Seems the method lookup didn't regress (0.6 also return empty lookup result) but now type inference is exploiding that....

@vtjnash
Copy link
Member

vtjnash commented Jul 29, 2017

Looks like this case is wrong:

    frame #0: 0x00000001000ed473 libjulia.0.7.0.dylib`jl_type_intersection_env_s(a=<unavailable>, b=0x0000000109d180f0, penv=<unavailable>, issubty=<unavailable>) at subtype.c:2153 [opt]
   2150	                    if (jl_is_leaf_type(*ans)) {
   2151	                        for(i=0; i < sz; i++) {
   2152	                            if (jl_is_typevar(env[i])) {
-> 2153	                                *ans = jl_bottom_type; goto bot;
   2154	                            }
   2155	                        }
   2156	                    }

@vtjnash
Copy link
Member

vtjnash commented Jul 29, 2017

@JeffBezanson Why does this filter code exist? I don't see any tests fail if I remove it (although I only tested it on my branch where I also have #21026 fixed):

diff --git a/src/subtype.c b/src/subtype.c
index ac6bc07c01..95d91d8b4c 100644
--- a/src/subtype.c
+++ b/src/subtype.c
@@ -2142,20 +2142,9 @@ jl_value_t *jl_type_intersection_env_s(jl_value_t *a, jl_value_t *b, jl_svec_t *
         else {
             sz = szb;
             // TODO: compute better `env` directly during intersection.
-            // we assume that if the intersection is a leaf type, we have
-            // full information in `env`. however the intersection algorithm
-            // does not yet provide that in all cases so use subtype.
+            // for now, we attempt to compute env by using subtype on the intersection result
             if (szb > 0 && !jl_types_equal(b, (jl_value_t*)jl_type_type)) {
-                if (jl_subtype_env(*ans, b, env, szb)) {
-                    if (jl_is_leaf_type(*ans)) {
-                        for(i=0; i < sz; i++) {
-                            if (jl_is_typevar(env[i])) {
-                                *ans = jl_bottom_type; goto bot;
-                            }
-                        }
-                    }
-                }
-                else {
+                if (!jl_subtype_env(*ans, b, env, szb)) {
                     sz = 0;
                 }
             }

@JeffBezanson
Copy link
Member

If anything, it would probably be an inference precision issue, where it's harder to get failures. It also might only be needed because of #21026.

vtjnash added a commit that referenced this issue Jul 31, 2017
fix #23024 and improve subtype test macro to always check for this
vtjnash added a commit that referenced this issue Jul 31, 2017
fix #23024 and improve subtype test macro to always check for this
ararslan pushed a commit that referenced this issue Sep 13, 2017
fix #23024 and improve subtype test macro to always check for this

Ref #23058
(cherry picked from commit fef5313)
vtjnash added a commit that referenced this issue Sep 14, 2017
fix #23024 and improve subtype test macro to always check for this

Ref #23058
(cherry picked from commit fef5313)
ararslan pushed a commit that referenced this issue Sep 15, 2017
fix #23024 and improve subtype test macro to always check for this

Ref #23058
(cherry picked from commit fef5313)
ararslan pushed a commit that referenced this issue Sep 16, 2017
fix #23024 and improve subtype test macro to always check for this

Ref #23058
(cherry picked from commit fef5313)
ararslan pushed a commit that referenced this issue Sep 18, 2017
fix #23024 and improve subtype test macro to always check for this

Ref #23058
(cherry picked from commit fef5313)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants