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 #29936, precompile should not assume UnionAlls have stable addresses #31047

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

JeffBezanson
Copy link
Member

typejoin can re-form a type like Val{x} where x, re-using Val.var and Val.body but allocating a new UnionAll. However, jl_serialize_datatype compares the address of the inner type (Val.body) while the code for serializing UnionAll compares the address of the UnionAll itself, leading to a possible inconsistency. This makes them both use the inner type, which has a stable address.

Backportable like whoa.

@JeffBezanson JeffBezanson added compiler:precompilation Precompilation of modules bugfix This change fixes an existing bug backport 1.0 labels Feb 12, 2019
@JeffBezanson JeffBezanson merged commit 2dd48b9 into master Feb 12, 2019
@JeffBezanson JeffBezanson deleted the jb/fix29936 branch February 12, 2019 20:46
@vtjnash
Copy link
Member

vtjnash commented Feb 13, 2019

This appears to assert that unwrap(a) === unwrap(b) implies that a === b. That seems odd, but I guess does happen to be true most of the time. But that seems a bit strange because this PR then seems to be explicitly falsifiable per #29936 demonstrating that that assumed implication is bad.

@JeffBezanson
Copy link
Member Author

The problem here is that there is a special serialization for unwrap(typename.wrapper), which jl_serialize_datatype detects by comparing the address of the DataType. So we serialized Val{x} where x such that the inner Val{x} got that special representation, but the UnionAll didn't match so it and its TypeVar were saved separately. So the deserializer constructed Val{x} where x, where the inner and outer x are different TypeVar objects, which is a malformed type. The intent of this change is to remove that discrepancy.

Another way to look at it is that if a::UnionAll === b::UnionAll is true, that does not imply that the addresses of a and b are the same, so comparing the addresses in C doesn't quite do what we want.

@vtjnash
Copy link
Member

vtjnash commented Feb 13, 2019

Right, I understand that's why this hides the bug. But just pointing out that it seems to potentially indicate a deeper issue, where we assume that TypeVar identity will be correctly maintained, but doesn't.

@vtjnash
Copy link
Member

vtjnash commented Mar 13, 2019

This is an example of what this PR could cause to fail (if it appeared in user code):

diff --git a/test/precompile.jl b/test/precompile.jl
index ed93b60bfd..d3417bfc8f 100644
--- a/test/precompile.jl
+++ b/test/precompile.jl
@@ -135,6 +135,8 @@ try
 
               const x28297 = Result(missing)
 
+              const d28297a = UnionAll(Dict.var, UnionAll(Dict.body.var, Dict.body.body)) # === Dict
+              const d28297b = UnionAll(Dict.body.var, UnionAll(Dict.var, Dict.body.body)) # === Dict{K,V} where K where V !== Dict
 
               # issue #28998
               const x28998 = [missing, 2, missing, 6, missing,
@@ -186,6 +188,8 @@ try
         @test Foo.abigint_x::BigInt + 1 == big"125"
 
         @test Foo.x28297.result === missing
+        @show Foo.d28297a # === Dict
+        @show Foo.d28297b # === Dict
 
         @test Foo.x28998[end] == 6
     end

KristofferC pushed a commit that referenced this pull request Apr 15, 2019
@KristofferC KristofferC mentioned this pull request Apr 15, 2019
39 tasks
KristofferC pushed a commit that referenced this pull request Apr 17, 2019
@KristofferC KristofferC mentioned this pull request Apr 19, 2019
39 tasks
KristofferC pushed a commit that referenced this pull request Apr 20, 2019
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:precompilation Precompilation of modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants