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

fix #44705, fix tuple_tfunc on Union containing Type{...} #44725

Merged
merged 1 commit into from
Mar 30, 2022
Merged

Conversation

aviatesk
Copy link
Sponsor Member

No description provided.

@aviatesk aviatesk changed the title fix #44705 fix #44705, fix tuple_tfunc on Union containing Type{...} Mar 24, 2022
# https://github.com/JuliaLang/julia/issues/44705
import Core.Compiler: tuple_tfunc, widenconst
@test widenconst(tuple_tfunc(Any[Type{Int}])) === Tuple{DataType}
@test tuple_tfunc(Any[Union{Type{Int32},Type{Int64}}]) === Tuple{Type}
Copy link
Member

Choose a reason for hiding this comment

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

Why not Tuple{DataType}?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We would need a more precise idea of widenconst that does widenconcrete, and we don't have that right now

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved
@aviatesk
Copy link
Sponsor Member Author

buildbot/package_xxx builds started to fail somehow, although I can't really reproduce it locally..

@aviatesk aviatesk force-pushed the avi/44705 branch 2 times, most recently from 0f0cfb1 to 8e61753 Compare March 24, 2022 08:41
@@ -1554,6 +1558,8 @@ function tuple_tfunc(argtypes::Vector{Any})
else
params[i] = Type
end
elseif !isvarargtype(x) && hasintersect(x, Type)
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I encountered a weird behavior here -- without this !isvarargtype(x) check buildbot/xxx builds fail with the following error:

Assertion failed: (!jl_is_vararg(x) && !jl_is_vararg(y)), function subtype, file /Users/julia/buildbot/worker/package_macos64/build/src/subtype.c, line 1261.

(e.g. https://build.julialang.org/#/builders/34/builds/4697)
while the same commit successfully builds w/o any problem on my machine (after make cleanall or even a fresh clone).
Maybe buildbot uses a different method to build something, and in that case the bootstrap makes more use of interpreter or something?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Turned out that I needed to enable "assert build".

@aviatesk
Copy link
Sponsor Member Author

This causes SquadGames.jl test suite to fail.
Here is the reproducer:

function assertx(xs::Vector{T}) where T
    t = (xs[1], xs[2])
    t::Tuple{T,T}
end

@code_typed optimize=false assertx(Type{<:Number}[Int,Int])
@code_typed optimize=true assertx(Type{<:Number}[Int,Int])
diff --git a/_master.jl b/_pr.jl
index 03768efc773..54ccceaa2cc 100644
--- a/_master.jl
+++ b/_pr.jl
@@ -2,8 +2,8 @@ julia> @code_typed optimize=false assertx(Type{<:Number}[Int,Int])
 CodeInfo(
 1 ─ %1 = Base.getindex(xs, 1)::Type{<:Number}
 │   %2 = Base.getindex(xs, 2)::Type{<:Number}
-│        (t = Core.tuple(%1, %2))::Tuple{Type{<:Number}, Type{<:Number}}
-│   %4 = t::Tuple{Type{<:Number}, Type{<:Number}}
+│        (t = Core.tuple(%1, %2))::Tuple{Type, Type}
+│   %4 = t::Tuple{Type, Type}
 │   %5 = Core.apply_type(Main.Tuple, $(Expr(:static_parameter, 1)), $(Expr(:static_parameter, 1)))::Core.Const(Tuple{Type{<:Number}, Type{<:Number}})
 │   %6 = Core.typeassert(%4, %5)::Tuple{Type{<:Number}, Type{<:Number}}
 └──      return %6
@@ -13,6 +13,8 @@ julia> @code_typed optimize=true assertx(Type{<:Number}[Int,Int])
 CodeInfo(
 1 ─ %1 = Base.arrayref(true, xs, 1)::Type{<:Number}
 │   %2 = Base.arrayref(true, xs, 2)::Type{<:Number}
-│   %3 = Core.tuple(%1, %2)::Tuple{Type{<:Number}, Type{<:Number}}
-└──      return %3
+│   %3 = Core.tuple(%1, %2)::Tuple{Type, Type}
+│        Core.typeassert(%3, Tuple{Type{<:Number}, Type{<:Number}})::Tuple{Type{<:Number}, Type{<:Number}}
+│   %5 = π (%3, Tuple{Type{<:Number}, Type{<:Number}})
+└──      return %5
 ) => Tuple{Type{<:Number}, Type{<:Number}}

So it seems like this assertion error was wrongly erased before? I wonder if we want to disable constructing a container whose element type is Tuple{Type{...}}.

@aviatesk
Copy link
Sponsor Member Author

We discussed this behavior of Tuple type is expected and that typeassertion should not be erased. We may want to fix our isa-implementation in the future so that isa can work with Tuple whose parameter is Type though. For now, I'd like to just proceed this PR with fixing SquadGames separately.

@aviatesk aviatesk added the status:merge me PR is reviewed. Merge when all tests are passing label Mar 29, 2022
aviatesk added a commit to aviatesk/SquidGame.jl that referenced this pull request Mar 29, 2022
This commit adapts to the incoming upstream change of the Julia compiler.
More specifically this change is necessary after JuliaLang/julia#44725,
which fixes an internal bug of the compiler, which in turn makes the test
suite of SquidGame fail without this change (if you're interested, please
see <JuliaLang/julia#44725 (comment)>).
@aviatesk aviatesk merged commit 4115686 into master Mar 30, 2022
@aviatesk aviatesk deleted the avi/44705 branch March 30, 2022 00:27
@giordano giordano removed the status:merge me PR is reviewed. Merge when all tests are passing label Apr 3, 2022
aviatesk added a commit that referenced this pull request Apr 7, 2022
aviatesk added a commit to ianatol/julia that referenced this pull request Apr 7, 2022
aviatesk added a commit that referenced this pull request Apr 9, 2022
aviatesk added a commit that referenced this pull request Apr 10, 2022
aviatesk added a commit that referenced this pull request Apr 10, 2022
aviatesk added a commit that referenced this pull request Apr 11, 2022
@vtjnash vtjnash added the backport 1.8 Change should be backported to release-1.8 label Jun 23, 2022
KristofferC pushed a commit that referenced this pull request Jul 4, 2022
fix #44705

(cherry picked from commit 4115686)
KristofferC pushed a commit that referenced this pull request Jul 4, 2022
KristofferC pushed a commit that referenced this pull request Jul 4, 2022
fix #44705

(cherry picked from commit 4115686)
KristofferC pushed a commit that referenced this pull request Jul 4, 2022
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants