Allow colons in plain scalars inside flow collections

[perlpunk]

This is a followup to #28

See http://yaml.org/spec/1.1/#nb-plain-char(c) and the following
productions.

This commit will allow

[http://example]

but still fail for the following:

[:foo]

This will be a bit more complicated to allow.

It also still fails for this:

[foo:]

This shouldn't be allowed. After discussing with @flyx my understanding is
that the spec has a mistake here:

[153] nb-plain-char(c) ::= c = flow-out ⇒ nb-plain-char-out
                           c = flow-in  ⇒ nb-plain-char-in
                           c = flow-key ⇒ nb-plain-char-in
[154] nb-plain-char-out ::=   ( nb-char - “:” - “#” - #x9 /*TAB*/ )
                            | ( ns-plain-char(flow-out) “#” )
                            | ( “:” ns-plain-char(flow-out) )
[155] nb-plain-char-in ::= nb-plain-char-out - “,” - “[” - “]” - “{” - “}”
[156] ns-plain-char(c) ::= nb-plain-char(c) - #x20 /*SP*/

Because nb-plain-char-out points to ns-plain-char(flow-out), so
theoretically this would be allowed:

{key:}}
# equal to
{"key:}": }

I think allowing colons in the middle of a flow style scalar is a good thing,
while leaving the other cases alone for now, because they are rare and you
can then always use quotes.

Edit:
With this patch the following will be allowed, too:

{foo::: bar}

While this is valid for 1.2, I'm not sure about 1.1. (Edit: I think it's valid in 1.1 too.)

[flyx]

I see no reason to refuse [:foo]. It is valid YAML 1.1 meaning [ ":foo" ]. My proposed change would make it to be successfully parsed. It would also cause [foo:] to be parsed as [ "foo:]" (because that's what the spec defines). It could be valid if another ] follows.

[flyx]

This change is undesirable. Regardless of how strange it is, [foo:]] is valid YAML 1.1 meaning [ "foo:]" ]. Moreover, ? certainly is not a flow indicator, so I am unsure how it found its way in here.

I think the correct fix would be to remove this if statement altogether (frankly, I have no idea why it is there in the first place; x:x is completely valid in a plain scalar inside flow content). : with succeeding space is handled later on. Note the comment about example 8.13 in the spec. That example is wrong! It shows this YAML:

{
  ? foo :°,
  ?° : bar,
}

° meaning empty scalar. This is not valid according to the productions, because there must be space after the : before the ,.

However, if I understand the rest of the code correctly, removing the if statement would cause [foo:] be parsed as [ "foo:" ] because the code is not designed to eat the character after the : as it should according to the spec. My proposed solution would be having a local ate_colon and then instead of the if:

ate_colon = CHECK(parser->buffer, ':');

and then at the end of the loop:

/* Copy the character. */

if (!READ(parser, string)) goto error;
if (ate_colon) if (!READ(parser, string)) goto error; /* <<< add this line */

end_mark = parser->mark;

if (!CACHE(parser, 2)) goto error;

[perlpunk]

Moreover, ? certainly is not a flow indicator, so I am unsure how it found its way in here.

I added this for consistency, because it's also in the next if-statement. Maybe I should take it out in both statements. Then [foo:?bar] would be allowed.

[perlpunk]

This change is undesirable. Regardless of how strange it is, [foo:]] is valid YAML 1.1

The point is that this currently is not allowed in libyaml, and I just keep it like that with this change. The only thing which changes is to allow [foo:bar].
ns-plain-char - ,[]{} characters after the colon. So I don't know why this change would be undesirable.

Could you clarify what you mean?

The goal of this PR is to accept things that make sense and are valid, but leaving the other cases alone for now, as I wrote above.

[perlpunk]

I see no reason to refuse [:foo]

The problem is, if I allow that, then other cases break.
Currently libyaml accepts { "foo":bar }, although this is only valid in YAML 1.2

If I change the code to allow [:foo], { "foo":bar } breaks. This would be acceptable since it's not valid YAML 1.1, however, forbidding something that has been accepted might not be desirable.

The code can probably be fixed allowing [:foo] while still accepting { "foo":bar }, but that's a more complicated change, I think. You would have to find out if the previous scalar token was a quoted one.

[perlpunk]

Regardless of how strange it is, [foo:]] is valid YAML 1.1 meaning [ "foo:]" ].

I agree that the 1.1 spec production says that, but the sentence before production 154 says something different. http://yaml.org/spec/1.1/#id907281
I think it was a mistake and doesn't make sense. In such cases it is ok IMHO to not allow such cases and let them be invalid.

[perlpunk]

@flyx I removed the ? in both if statements.

However, this is a change on its own, allowing [foo?bar] and even [foo ? bar]. It's valid according to the spec as far as I can see, but PyYAML and SnakeYAML don't accept it. Maybe it should be an extra PR.

Edit: I reverted this change

[perlpunk]

I created PR #105 to allow question marks.
I'm not sure if this change is as important as adding the colon, though.
Together with #104 this would allow things like [http://example/query?a=b]

[asomov]

@perlpunk : what is https://github.com/yaml/yaml-test-suite ?
How can I run it with SnakeYAML ? How do I know which tests work and which do not ?
The documentation did not help. I could not even find the test cases in the project....

[perlpunk]

@asomov It's a collection of test cases for YAML 1.2.
The test cases are written in TestML format, each test has one .tml file (they are in the test directory).
For convenience, there is a data branch, which contains the data for each test in seperate files, generated from the TestML. I agree the documentation is pretty sparse so far.
Also have a look at the wiki, there's a documentation for the test.event file, but not complete yet: https://github.com/yaml/yaml-test-suite/wiki/test-event

You take the in.yaml, parse it with your parser and compare the events you get with the test.event file.

Edit: Actually, you should get the data from the releases: https://github.com/yaml/yaml-test-suite/releases
As the data branch is for development, and not all commits will be kept. This is a recent change and not yet in the README.

[asomov]

@perlpunk cool ! But I do not understand how you run the test suite against SnakeYAML.

[perlpunk]

@asomov for generating http://matrix.yaml.io/ we are using the programs in https://github.com/yaml/yaml-editor.
Scripts that don't need to be compiled are in https://github.com/yaml/yaml-editor/tree/master/sbin

Here are the ones for SnakeYAML: https://github.com/yaml/yaml-editor/tree/master/docker/src/java/src/main/java/org/yaml/editor
Feel free to use this for integrating it into SnakeYAML.

(Getting a bit off topic here, for specific questions maybe create an issue or join #yaml on freenode)

[perlpunk]

I compared several cases with various parsers and libyaml master and this PR applied:
https://gist.github.com/perlpunk/de2c631a7b0e9002de351a680eadb573

[perlpunk]

Rebased to current master and squashed the two commits

[ingydotnet]

Applied. Pushed. Thanks!