Fix JSON::Serializable on certain recursively defined types (#13344)

crystal-lang · Apr 19, 2023 · d3f5d75 · d3f5d75
1 parent b8c514b
commit d3f5d75
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 10 deletions.
diff --git a/spec/std/json/serializable_spec.cr b/spec/std/json/serializable_spec.cr
@@ -465,6 +465,18 @@ module JSONNamespace
   end
 end
 
+class JSONSomething
+  include JSON::Serializable
+
+  property value : JSONAttrValue(Set(JSONSomethingElse)?)?
+end
+
+class JSONSomethingElse
+  include JSON::Serializable
+
+  property value : JSONAttrValue(Set(JSONSomethingElse)?)?
+end
+
 describe "JSON mapping" do
   it "works with record" do
     JSONAttrPoint.new(1, 2).to_json.should eq "{\"x\":1,\"y\":2}"
@@ -1107,4 +1119,9 @@ describe "JSON mapping" do
       request.bar.id.should eq "id:bar"
     end
   end
+
+  it "fixes #13337" do
+    JSONSomething.from_json(%({"value":{}})).value.should_not be_nil
+    JSONSomethingElse.from_json(%({"value":{}})).value.should_not be_nil
+  end
 end
diff --git a/spec/std/yaml/serializable_spec.cr b/spec/std/yaml/serializable_spec.cr
@@ -398,6 +398,18 @@ class YAMLStrictDiscriminatorBar < YAMLStrictDiscriminator
   property y : YAMLStrictDiscriminator
 end
 
+class YAMLSomething
+  include YAML::Serializable
+
+  property value : YAMLAttrValue(Set(YAMLSomethingElse)?)?
+end
+
+class YAMLSomethingElse
+  include YAML::Serializable
+
+  property value : YAMLAttrValue(Set(YAMLSomethingElse)?)?
+end
+
 describe "YAML::Serializable" do
   it "works with record" do
     YAMLAttrPoint.new(1, 2).to_yaml.should eq "---\nx: 1\ny: 2\n"
@@ -1008,4 +1020,9 @@ describe "YAML::Serializable" do
       request.bar.id.should eq "id:bar"
     end
   end
+
+  it "fixes #13337" do
+    YAMLSomething.from_yaml(%({"value":{}})).value.should_not be_nil
+    YAMLSomethingElse.from_yaml(%({"value":{}})).value.should_not be_nil
+  end
 end
diff --git a/src/json/serialization.cr b/src/json/serialization.cr
@@ -186,7 +186,6 @@ module JSON
           {% unless ann && (ann[:ignore] || ann[:ignore_deserialize]) %}
             {%
               properties[ivar.id] = {
-                type:        ivar.type,
                 key:         ((ann && ann[:key]) || ivar).id.stringify,
                 has_default: ivar.has_default_value?,
                 default:     ivar.default_value,
@@ -199,8 +198,14 @@ module JSON
           {% end %}
         {% end %}
 
+        # `%var`'s type must be exact to avoid type inference issues with
+        # recursively defined serializable types
         {% for name, value in properties %}
-          %var{name} = {% if value[:has_default] || value[:nilable] %} nil {% else %} uninitialized ::Union({{value[:type]}}) {% end %}
+          %var{name} = {% if value[:has_default] || value[:nilable] %}
+                         nil.as(::Nil | typeof(@{{name}}))
+                       {% else %}
+                         uninitialized typeof(@{{name}})
+                       {% end %}
           %found{name} = false
         {% end %}
 
@@ -223,7 +228,7 @@ module JSON
                       {% if value[:converter] %}
                         {{value[:converter]}}.from_json(pull)
                       {% else %}
-                        ::Union({{value[:type]}}).new(pull)
+                        typeof(@{{name}}).new(pull)
                       {% end %}
                     end
                 end
@@ -288,7 +293,6 @@ module JSON
           {% unless ann && (ann[:ignore] || ann[:ignore_serialize] == true) %}
             {%
               properties[ivar.id] = {
-                type:             ivar.type,
                 key:              ((ann && ann[:key]) || ivar).id.stringify,
                 root:             ann && ann[:root],
                 converter:        ann && ann[:converter],

diff --git a/src/yaml/serialization.cr b/src/yaml/serialization.cr
@@ -192,7 +192,6 @@ module YAML
           {% unless ann && (ann[:ignore] || ann[:ignore_deserialize]) %}
             {%
               properties[ivar.id] = {
-                type:        ivar.type,
                 key:         ((ann && ann[:key]) || ivar).id.stringify,
                 has_default: ivar.has_default_value?,
                 default:     ivar.default_value,
@@ -204,8 +203,14 @@ module YAML
           {% end %}
         {% end %}
 
+        # `%var`'s type must be exact to avoid type inference issues with
+        # recursively defined serializable types
         {% for name, value in properties %}
-          %var{name} = {% if value[:has_default] || value[:nilable] %} nil {% else %} uninitialized ::Union({{value[:type]}}) {% end %}
+          %var{name} = {% if value[:has_default] || value[:nilable] %}
+                         nil.as(::Nil | typeof(@{{name}}))
+                       {% else %}
+                         uninitialized typeof(@{{name}})
+                       {% end %}
           %found{name} = false
         {% end %}
 
@@ -226,10 +231,8 @@ module YAML
                     %var{name} =
                       {% if value[:converter] %}
                         {{value[:converter]}}.from_yaml(ctx, value_node)
-                      {% elsif value[:type].is_a?(Path) || value[:type].is_a?(Generic) %}
-                        {{value[:type]}}.new(ctx, value_node)
                       {% else %}
-                        ::Union({{value[:type]}}).new(ctx, value_node)
+                        typeof(@{{name}}).new(ctx, value_node)
                       {% end %}
                   end
 
@@ -299,7 +302,6 @@ module YAML
           {% unless ann && (ann[:ignore] || ann[:ignore_serialize]) %}
             {%
               properties[ivar.id] = {
-                type:      ivar.type,
                 key:       ((ann && ann[:key]) || ivar).id.stringify,
                 converter: ann && ann[:converter],
                 emit_null: (ann && (ann[:emit_null] != nil) ? ann[:emit_null] : emit_nulls),