Crystal: #to_json/#to_yaml defined on every object

[straight-shoota]

Since encountering the issue from #3450 again, I am convinced that printing a more helpful error message won't be enough. There needs to be a programmatic way to discover if to_json/to_yaml can be safely called on an object or not.

Right now, a.responds_to? :to_json returns true for any object, because Object#to_json() and Object#to_json(IO) are always defined if you require "json". But calling a.to_json won't compile unless a has a #to_json(JSON::Builder) method.

As far as I can see, possible solutions are:

• A) Remove static definitions of Object#to_json() and Object#to_json(IO) and add these methods only if #to_json(JSON::Builder) is defined. This could be done manually (e.g. by including a module) and would of course be included with JSON.mapping. Or it might even be possible to add automatically if #to_json(JSON::Builder) is defined (for example using finished macro hook - or even method_missing). But the latter might be too much magic happening and there is always an issue documenting that.
• B) Rename to_json(JSON::Builder) so it can be distinguished from the synonymous wrapper methods - for example to to_json_impl(JSON::Builder). Then, a.responds_to? :to_json_impl would show if an object has an actual implementation or not. There might perhaps be Object#to_json(JSON::Builder) as an alias but that will cause issues if people overwrite that alias instead of the implementation method.
• C) Add a module (for example JSON::Serializable) with an abstract def to_json(JSON::Builder) and all classes implementing this method should include the module (automatically included using JSON.mapping). a.is_a? JSON::Serializable would tell if it has an actual to_json implementation or not.
• D) Improve reflection capabilities to be able to query the argument types of a method. For example a.responds_to? :to_json, JSON::Builder would return true only if #to_json(JSON::Builder) is defined.
• E) Define Object#to_json(JSON::Builder) on every object as well and have it emit null. That's probably a reasonable default. I'm not sure about possible side effects, but I don't think this should be a huge issue. The biggest downside is that you can't distinguish if a class has a .

I think I would favour B) because it has the least impact though it's by far not optimal regarding naming.
E) also sounds nice, but I'm not sure if it's feasible and I don't like the fact that you still can't tell if a class has a real implementation or just a dummy method. For most cases, emitting null might be the right way to handle missing serializers, but there might be other strategies to handle objects without serializer as well and there should be a way to determine that.

[asterite]

A) is the way to go. to_json should only be defined in objects that have that method and its implementation.

[straight-shoota]

And how to implement?

A module could look like this:

module JSON::Serializable
  def to_json : String
    String.build do |str|
      to_json str
    end
  end

  def to_json(io : IO)
    JSON.build(io) do |json|
      to_json(json)
    end
  end

  def to_pretty_json(indent : String = "  ") : String
    String.build do |str|
      to_pretty_json str, indent: indent
    end
  end

  def to_pretty_json(io : IO, indent : String = "  ")
    JSON.build(io, indent: indent) do |json|
      to_json(json)
    end
  end

  abstract def to_json(builder : JSON::Builder)
end

And similar for YAML.

[asterite]

Probably, yes, but that will conflict with the idea I had regarding meta attributes.

[straight-shoota]

How so?
.new(JSON::PullParser) and to_json(JSON::Builder) need to be defined somehow anyway... would it be much effort to include a module?

[straight-shoota]

Maybe that module could even define those methods based on instance var reflection. Including classes need to be able to overwrite, of course.

[asterite]

That's the idea. Then you would attach metadata to the instance vars to say how the are serialized.

The problem is, String also has to_json, but defining that method based on instance vars doesn't work. But, well, we can always redefine that in that case.

I think introducing a module with those methods is fine 👍