src/yaml/from_yaml.cr: I include some benchmarks at the end.

I couldn't find any of the benchmarks in that file or as part of the commit message. Can you add that to the PR?

Thank you.

@luislavena I hit "submit" before time. I'm still editing the description :)

I updated the description. This is now ready to review 😄

Great job @asterite. The code reorganization looks good. More than anything, I am just happy to see these fixes get in. Was support added for !!set and !!omap from the YAML spec too? Also, I think we should refer to the spec by it's version rather than Core. As far as I can tell, this adheres to the 1.1 spec, but we could add the 1.2 spec at a later date, so that naming may make more sense.

@jwaldrip Do you know what's the difference between the YAML 1.1 and 1.2 specs? Maybe the regexes for the core type?

I can see here that booleans are just variants of true and false, there's no yes nor no. Is that the difference? Also base 8 starts with 0o (zero o) instead of 0 (zero).

I think I like version 1.2 better because it's simpler. It's a bit strange to deserialize "yes" as true. In fact Ruby deserializes "y" to "y", while the 1.1 spec says it should be true.

But I don't know if that's the difference. Is there anything else?

(in any case, you are totally right that we should mention which version we support)

About the tags:

• !!omap: the default YAML mapping has no order guaranteed, while !!omap should deserialize to something that preserves insertion order. But in Crystal Hash already preserves order so I don't think there's something special we need to do here.
• !!set: should we deserialize this to a Set? Seems pretty easy to implement. However, in Ruby I can see it deserializes to a Hash with nil values. The spec suggests that's also possible. I don't understand why Ruby chose a Hash when there's Set in the standard library. Do you know the reason? In any case I can push an update with !!set support and use Set.

To a bit a more to the above: I'd really prefer to implement 1.2, making "y", "yes", "on", etc., be booleans is super strange, given that JSON is so popular and true and false are ubiquitous.

However, YAML 1.2 doesn't support timestamp, right? So we shouldn't deserialize Time at all?

Actually, we are using libyaml and that's a 1.1 parser, so we'll stick with 1.1 for now.

I'll soon add support for all types defines here.

I also checked pyyaml (which uses libyaml) and they also implicitly parse times like in this PR and like in Ruby's Psych implementation, so I guess it's safe if we do the same thing.

Do you know the reason? In any case I can push an update with !!set support and use Set.

@asterite, Even that Set is present in stdlib of Ruby, is not part of Core. If not mistaken, the decision was to avoid automatically require a data type (Set) that might not be used by developers and instead map to core values.

Cheers.

If I can chime in, I would definitely like !!set to be mapped to a proper Set instance instead of hash-for-everything approach of Ruby 😁

Thank you for working on this! ❤️ ❤️ ❤️

Also !!pairs should probably be Array(NamedTuple(key: value)) and !!omap should just force ordering on the keys.

I actually tried to make !!pairs be Array(Tuple(Type, Type)) but it complicates things a lot because now we have arrays that either have other yaml types or a tuple (or named tuple in your case). I decided to parse it to an array of single-element hashes, like in Ruby. I also don't think !!pairs is common enough (and neither all of the other tags, I've never seen one in my life in real world code).

@asterite, array of single element hashes makes sense. Did you happen to implement !!binary? In where !!binary should base64 encode into Bytes.

@jwaldrip Yes, it decodes to Bytes :-)

Hmmm... I managed to implement everything (not pushed yet) except merge. Well, merge actually works with YAML.parse but it doesn't work with YAML.mapping, and it would be good if it worked.

I'll explain the difficulty. For example this:

development: &development
  host: foo.com
  port: 1234

test:
  <<: *development
  port: 4567

With this:

require "yaml"

class HTTPSetting
  YAML.mapping(name: String, port: Int32)
end

We want to Array(HTTPSetting).from_yaml.

The way it currently works is that when &development is found, just after initializing an HTTPSetting for it we associate it with the anchor "development". Then when *development is found we would retrieve it... but then? We'd somehow have to traverse its instance vars and pull from that instead of from a pull parser. It gets a bit worse if *development doesn't even point to another instance of HTTPSetting (could be another type with less properties).

So I'm thinking the only solution to this is to parse YAML first to an intermediate structure, and YAML.mapping would work on that structure. So that intermediate structure will have a YAML tree associated to &development, and when we find <<: *development we would get that tree and traverse its key/values as usual (the main issue of doing it with a pull parser is that we lost the information of what was under &development, we only have the Crystal deserialization).

This implies a medium-sized refactor, and of course it's a breaking change. It will also slow down parsing a bit because of the intermediate structure, but I don't think it will get that bad. But it's better to be feature complete than fast but incomplete.

That is, unless someone has a better idea of how to solve this issue :-)

@asterite surely we only have to store the YAML structures which are tagged?

So... this PR is now really big, but all the changes are needed to correctly
implement the full YAML 1.1 spec with the custom types, include merge ("<<").

(Conclusion: YAML is hard, but also fun ^_^)

At this point, I think it's better to browse the implementation, or the specs,
to see that everything works as expected. I did my best to document most
of the public API and some internal details and algorithms.

Parsing and dumping YAML is now done in a completely different way.

For parsing (with from_yaml), we first parse from a String or IO
into an in-memory data structure. We then convert this data structure
to the appropriate Crystal types, with these types implementing a new
method that accept two arguments: a YAML::ParseContext and a
YAML::Nodes::Node that is the node to deserialize. The parse context
is used to store and retrieve references for anchors and aliases.

For parsing with YAML.parse we can avoid this intermediate data structure
and directly parse it into the final structure because it's much simpler.

For dumping, we have objects implement to_yaml with an argument of
YAML::Nodes::Builder. This builder, instead of directly outputting stuff
to an IO, will create an in-memory tree. This tree can be linked with anchors
and aliases. That way to_yaml is invoked just once on every object. Then,
we turn that in-memory tree into proper YAML.

By the way, this is also how Ruby does it, probably because of the same
reasons.

This should be slightly slower than before, and a bit more memory will be
allocated to create the intermediate data structure, but it's worth it
because many more features can be implemented, and the implementation turns
out to be simpler like this.

One of the features this refactor supports is merging with "<<". For example,
this now works:

require "yaml"

class Setting
  YAML.mapping(host: String, port: Int32)
end

yaml = <<-YAML
development: &development
  host: foo.com
  port: 1234

test:
  <<: *development
  port: 4567
YAML

settings = Hash(String, Setting).from_yaml(yaml)
p settings
# => 
# {"development" => #<Setting:0x106f7fd80 @host="foo.com", @port=1234>,
#  "test" => #<Setting:0x106f7fd20 @host="foo.com", @port=4567>}

This works because when the implementation of YAML.mapping finds
a << key, it can solve the alias to a node and traverse that
node's keys and values to continue mapping from there (recursively!).
I believe this is something that must be implemented as this usage
of the merge key is pretty common to override settings from a common
setting pool.

Another advantage is that when deserializing a union of types there's
no need to use PullParser#read_raw and build a value from it and
parse it multiple times, retrying on failure: we can just parse
from the already parsed in-memory node. So this should actually be
faster than before.

Finally, here's the updated benchmark result:

Time to dump 1_000 times: 00:00:00.0395070
Time to parse 10_000 times: 00:00:00.5813970

The previous one was:

Time to dump 1_000 times: 00:00:00.0341950
Time to parse 10_000 times: 00:00:00.5267270

We can see it's a bit slower, but still around 9~10 times faster than
Ruby, so more than good enough :-)

Oh, I almost forgot: all YAML types are now implemented, for example binary to encode and decode Bytes.

Now fixes #2873 too :-)

@asterite Other ones that could be related: #4929, #3101 & #4668

I decided to remove YAML::PullParser#read_raw because it's not needed anymore to correctly parse a Union. That means #4929 is trivially fixed.

LGTM

LGTM

Looks really great, thanks @asterite and @jwaldrip for the hard work!

Just as a note: We should consider applying an independent YAML test suite like yaml/yaml-test-suite (though it seems to be incomplete) to make sure that the implementation meets the standard. This doesn't need to be in this PR, obviously.

🕚 to 🚢

@asterite @bcardiff @mverzilli merging this before the weekend will be a nice way to end the week! 😄

The build failed: https://travis-ci.org/crystal-lang/crystal/jobs/282301194

Maybe some conflict between this PR and #5022?