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

Adding a method for Tuple adds two methods to Tuple #55231

Closed
Drvi opened this issue Jul 24, 2024 · 7 comments · Fixed by #55365
Closed

Adding a method for Tuple adds two methods to Tuple #55231

Drvi opened this issue Jul 24, 2024 · 7 comments · Fixed by #55365
Labels
types and dispatch Types, subtyping and method dispatch

Comments

@Drvi
Copy link
Contributor

Drvi commented Jul 24, 2024

I'm not sure if this is a problem, but we were surprised by it over at oxinabox/Tricks.jl#42. The following demonstrates the unexpected behavior:

julia> methods(Tuple)
# 6 methods for type constructor:
 [1] Tuple(index::CartesianIndex)
     @ Base.IteratorsMD multidimensional.jl:103
 [2] Tuple(nt::NamedTuple)
     @ Base namedtuple.jl:197
 [3] Tuple(x::Array{T, 0}) where T
     @ Base tuple.jl:389
 [4] Tuple(x::Ref)
     @ Base tuple.jl:388
 [5] (::Type{T})(x::Tuple) where T<:Tuple
     @ Base tuple.jl:386
 [6] (::Type{T})(itr) where T<:Tuple
     @ Base tuple.jl:391

julia> Base.Tuple(a,b,c,d,e,f) = 1

julia> methods(Tuple)
# 8 methods for type constructor:
 [1] Tuple(index::CartesianIndex)
     @ Base.IteratorsMD multidimensional.jl:103
 [2] Tuple(x::Array{T, 0}) where T
     @ Base tuple.jl:389
 [3] Tuple(x::Ref)
     @ Base tuple.jl:388
 [4] (::Type{T})(x::Tuple) where T<:Tuple
     @ Base tuple.jl:386
 [5] Tuple(nt::NamedTuple)
     @ Base namedtuple.jl:197
 [6] (::Type{T})(nt::NamedTuple) where T<:Tuple
     @ Base namedtuple.jl:198
 [7] Tuple(a, b, c, d, e, f)
     @ Main REPL[2]:1
 [8] (::Type{T})(itr) where T<:Tuple
     @ Base tuple.jl:391

EDIT: This is on Julia 1.10.4

@nsajko nsajko added types and dispatch Types, subtyping and method dispatch bug Indicates an unexpected problem or unintended behavior correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing labels Jul 24, 2024
@nsajko
Copy link
Contributor

nsajko commented Jul 24, 2024

Not specific to tuples:

struct U{P} end; struct V{P} end
U(::V) = nothing
(::Type{T})(::V) where {T<:U} = nothing
length(methods(U))  # 1
U(a, b) = nothing
length(methods(U))  # 2
U(a, b, c) = nothing
length(methods(U))  # 3
U(a, b, c, d) = nothing
length(methods(U))  # 4
U(a, b, c, d, e) = nothing
length(methods(U))  # 5
U(a, b, c, d, e, f) = nothing
length(methods(U))  # 7
julia> versioninfo()
Julia Version 1.12.0-DEV.896
Commit 24cfe6e2cc7 (2024-07-24 06:36 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × AMD Ryzen 3 5300U with Radeon Graphics
  WORD_SIZE: 64
  LLVM: libLLVM-17.0.6 (ORCJIT, znver2)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)

@adienes
Copy link
Contributor

adienes commented Jul 24, 2024

why did you add correctness bug ? in what situations is this "likely to lead to incorrect results in user code" ?

it seems the added method is (::Type{T})(nt::NamedTuple) where T<:Tuple, (respectively, (::Type{T})(::V) where T<:U )

but these have no subtypes anyway

@gbaraldi gbaraldi removed the correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing label Jul 24, 2024
@JeffBezanson
Copy link
Member

Looks like a bug in the query used by methods.

@vtjnash vtjnash removed the bug Indicates an unexpected problem or unintended behavior label Jul 24, 2024
@vtjnash
Copy link
Member

vtjnash commented Jul 24, 2024

It isn't really a bug, but it is suboptimal. It is using the test jl_subtype((jl_value_t*)ti, m2->sig) as an approximation of whether the result U(::V) is ambiguous with (::Type{T})(::V) where T<:U since they had the same intersection using the approximation methods algorithm. We may just need to be a bit stricter about exactly how we apply that test

vtjnash added a commit that referenced this issue Jul 24, 2024
@vtjnash
Copy link
Member

vtjnash commented Jul 24, 2024

Here's a fix, though currently without a test: https://github.com/JuliaLang/julia/compare/jn/55231

@ararslan
Copy link
Member

Could you use @nsajko's example above as a test for your change? Something like

diff --git a/test/ambiguous.jl b/test/ambiguous.jl
index d6f69f21bc..4c323d7aee 100644
--- a/test/ambiguous.jl
+++ b/test/ambiguous.jl
@@ -447,4 +447,19 @@ cc46601(::Type{T}, x::Int) where {T<:AbstractString} = 7
 @test length(methods(cc46601, Tuple{Type{<:Integer}, Integer})) == 2
 @test length(Base.methods_including_ambiguous(cc46601, Tuple{Type{<:Integer}, Integer})) == 7

+# Issue #55231
+module M55231
+    using Test
+    struct U{P} end
+    struct V{P} end
+    U(::V) = nothing
+    (::Type{T})(::V) where {T<:U} = nothing
+    U(a, b) = nothing
+    U(a, b, c) = nothing
+    U(a, b, c, d) = nothing
+    U(a, b, c, d, e) = nothing
+    U(a, b, c, d, e, f) = nothing
+    @test length(methods(U)) == 6
+end
+
 nothing

@vtjnash
Copy link
Member

vtjnash commented Jul 25, 2024

You just need the first two and make sure neither is filtered out of this query, regardless of which order you define them in

vtjnash added a commit that referenced this issue Aug 3, 2024
Some methods were filtered out based simply on visit order, which was
not intentional, with the lim==-1 weak-edges mode.

Fix #55231
vtjnash added a commit that referenced this issue Aug 7, 2024
Some methods were filtered out based simply on visit order, which was
not intentional, with the lim==-1 weak-edges mode.

Fix #55231
KristofferC pushed a commit that referenced this issue Aug 13, 2024
Some methods were filtered out based simply on visit order, which was
not intentional, with the lim==-1 weak-edges mode.

Fix #55231

(cherry picked from commit 1db5cf7)
lazarusA pushed a commit to lazarusA/julia that referenced this issue Aug 17, 2024
Some methods were filtered out based simply on visit order, which was
not intentional, with the lim==-1 weak-edges mode.

Fix JuliaLang#55231
KristofferC pushed a commit that referenced this issue Sep 12, 2024
Some methods were filtered out based simply on visit order, which was
not intentional, with the lim==-1 weak-edges mode.

Fix #55231

(cherry picked from commit 1db5cf7)
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 a pull request may close this issue.

7 participants