Fix JSON, YAML use_*_discriminator for recursive struct types

[HertzDevil]

Fixes #13214. This makes sure the temporary variables are not nilable for properties that aren't nilable and have no default values, since otherwise it seems the deserialization method recursively calls itself and breaks type inference.

This was discovered for JSON but applies to YAML as well.

[z64]

FYI, this code no longer compiles on 1.8 due to this change

require "json"

struct U
  def self.new(pull : JSON::PullParser) : U?
    pull.read_raw
    true ? nil : U.new
  end

  def initialize
  end
end

struct T
  include JSON::Serializable
  getter u : U
end

pp(T.from_json(%({"u": null})))

... and similar code expressed with converters.

It was previously a runtime cast error.

[straight-shoota]

I think the proper action is to raise right away instead of returning nil.

Suppose the deserializer could technically type the variable as typeof(::Union({{value[:type]}}).new(pull)) and verify the actual runtime value at the end. This would preserve this use case while still fixing the invalid nil propagation issue.

[HertzDevil]

It is arguable whether U.new should be able to return something that is not a subtype of U. Is there a snippet that uses converters and doesn't involve .new?

[z64]

In our code, we had a TimeConverter that tried to return a time with a specific timestamp format, and ignored it if the format was invalid.

module TimeConverter
  def self.from_json(pull : JSON::PullParser) : Time?
    TIME_FORMAT.parse(pull.read_string)
  rescue
    nil
  end
end

struct Foo
  @[JSON::Field(converter: TimeConverter)
  getter time : Time # NOT nilable
end

It is not strictly related to new, but I provided that example as it doesn't have to do with converters either.

(In this case, it was a mistake that getter time : Time was not nilable: it should have been)

[straight-shoota]

It's unfortunate to see this previously working code break. But I think it's for good. It only worked due to a bug in the first place and as far as I can tell was never intended nor documented behaviour.

We could re-enable that behaviour by adding in an appropriate cast in the deserialization logic. But I don't like that. It trades away type safety for no real benefit out of itself only to keep existing code that relied on a bug working.
This is a tough call of course. Should we prioritize avoiding to break code that worked previously out of error? We value backwards compatibility, but in this situation it depends on goodwill.

I believe it's best to accept the breaking code. The quality of the library will be better that way. And as the last example shows (#13238 (comment)) the breakage even helps to discover type errors.
That's the point of having type safety. Giving that away opens up other holes.

It's easy to fix the affected code by raising directly in the converter. This is also backwards compatible with older Crystal releases.