colon can be in a plain scalar #28

cloudwu · 2016-12-03T12:42:11Z

No description provided.

dbeer1 · 2016-12-06T02:00:24Z

👍 This exact issue is preventing us from adopting libyaml, we have lots of documents with urls in them. I see this was discussed in #21, any ETA for merging? Thanks!

ingydotnet · 2016-12-07T19:08:22Z

@dbeer1 are you saying you have lots of docs like:

url: http://example.com

or

- {url: http://example.com}

libyaml is perfectly fine with the former. I haven't seen much of the latter (urls in flow collections) in the wild.

dbeer1 · 2016-12-07T19:13:35Z

Yeah, we have a lot of lines like this (in basically all our files because most of them were created from a template):

Urls:  [ http://127.0.0.1:1234, http://127.0.0.1:2345 ]

We are currently using yaml-cpp to parse, which can correctly read this document. Looking to switch to libyaml as it has been significantly faster in our tests and works on all of our platforms, but it chokes on documents like that.

dbeer1 · 2016-12-07T19:40:23Z

I've read the various discussions on this:

It seems like this scanner error was introduced as a placeholder while the syntax of mappings was being discussed. Is this reason still valid? This error is somewhat problematic as other YAML parser and emitter implementations follow the spec and will create documents that cannot interoperate with libyaml. Searching the internet, I've seen many complaints about this behavior and talk of people patching it locally.

ingydotnet · 2016-12-07T22:16:32Z

Reviewing the spec, it seems to be clear that a space must follow the colon, if the colon is to be considered a mapping separator.

I completely concur with that logic. Therefore I think we can move forward with this.

But...

I now have a working test suite and libyaml test harness here: https://github.com/ingydotnet/yaml-dev-kit/
If you clone that and run make test it will fetch and build the latest libyaml and run 160 tests. Someone should give me a PR for some tests for this. (Or identify if one exists). Or I'll do it after my bike ride.

One edge case is this: {foo:}. It fails but {foo} and {foo } work. I think they should all parse the same.

dbeer1 · 2016-12-08T16:12:12Z

I agree that {foo:} should be equivalent to { foo : } and not { "foo:" : null }, but that does conflict with the spec as written right? On the other hand, its easy enough to code and does match the current pyyaml behavior (but not libyaml behavior!), which allows one of ',[]{}' to follow a colon in a flow context.

Compare https://github.com/yaml/pyyaml/blob/master/lib/yaml/scanner.py#L1301 vs https://github.com/yaml/libyaml/blob/master/src/scanner.c#L3438

ingydotnet · 2016-12-08T18:52:15Z

@dbeer, right, these should all become valid:

- {foo:}
- {foo:{}}
- {foo:[]}
- {foo:,bar}
- [foo:]

I'll add tests for them to the suite. BTW, I just moved the test suite to
https://github.com/yaml/yaml-test-suite

ingydotnet · 2016-12-08T19:05:54Z

Here's an existing test that is affected by this:

https://github.com/yaml/yaml-test-suite/blob/master/test/4ABK.tml

As you can see it is marked to be skipped by libyaml testing.

dbeer · 2016-12-08T20:38:09Z

I think you probably meant to address @dbeer1

dbeer1 · 2016-12-08T21:15:17Z

I restored the empty value behavior in pyyaml in yaml/pyyaml#45. I guess we should do the same here with libyaml?

Now it processes your test as follows:

>>> import yaml
>>> print yaml.load("""
... - {foo:}
... - {foo:{}}
... - {foo:[]}
... - {foo:,bar}
... - [foo:]
... """)
[{'foo': None}, {'foo': {}}, {'foo': []}, {'foo': None, 'bar': None}, [{'foo': None}]]
>>>

The case with omitted keys seems to be significantly more complicated as the grammers in the parser need to be reworked.

dbeer1 · 2017-02-08T20:27:43Z

The pyyaml pull request has been merged and I think the logic here should be updated to match. That is, a ':' should terminate a scalar in the flow context if it is followed not just by a space but also by ',[]{}'. For example: {a:} -> { "a" : null } instead of {a:} -> { "a:" : null }

ingydotnet · 2017-02-09T00:14:14Z

Thanks @dbeer1. I'll add a test to the YAML Test Suite for this.

jstoiko · 2017-12-22T21:45:58Z

any plans to merge this?

perlpunk · 2017-12-23T00:21:43Z

I added a test for a plain url in a flow mapping: yaml/yaml-test-suite@1eacbab

I also updated tests/run-test-suite/test/libyaml-parser.list (not yet in master)
The PR looks good to me, all tests are still passing with the updated test list, and the new test is passing.
@ingydotnet @sigmavirus24 should I merge this?

sigmavirus24

Go ahead and merge @perlpunk

ingydotnet

Seems ok to me.

perlpunk · 2017-12-28T20:07:32Z

There's one thing that should be noted, though.
Previously this was invalid:

- {foo:}
- {foo:,bar}
- [foo:]

Now it is parsed valid, but the result is wrong, as the colon becomes part of the content:

- {"foo:": }
- {"foo:": ,bar}
- ["foo:": ]

Given that this is the wrong behaviour (as mentioned previously in this thread), the question is, does it make sense to add this fix, making something become valid, but wrong?

Cc @sigmavirus24 @ingydotnet

perlpunk · 2017-12-28T20:11:50Z

I also updated tests/run-test-suite/test/libyaml-parser.list (not yet in master)
The PR looks good to me, all tests are still passing with the updated test list, and the new test is passing.

I also wanted to add a script for testing error cases (#81) before merging this, however, @ingydotnet requested changes for which I don't have time now, so this has to go without the additional tests.

perlpunk · 2017-12-28T20:52:59Z

So it seems we are not sure what the correct behaviour is.
Here's a gist of what popular processors do:
https://gist.github.com/anonymous/c3e98f57c0cc9cdc71fd28b7681e280b

ingydotnet · 2017-12-28T20:55:50Z

I think that {foo:} is probably the same semantic as {"foo:": }. I also find it an unfortunate edge case. But also it's a rare edge case.

I think we can shore this up in 1.3, and that applying this patch is correct for now.

I'd like to hold off on applying this until we hear from @flyx.

jstoiko · 2018-01-12T19:34:02Z

I couldn't find tests for those edge cases in yaml-test-suite.

@perlpunk: where does that gist come from? I couldn't find it anywhere in the test suite.

perlpunk · 2018-01-12T20:25:30Z

@jstoiko yeah, these cases are not yet in the test suite because we still have to figure out the correct results :-/
I created that gist with https://github.com/yaml/yaml-editor

flyx · 2018-01-29T15:22:01Z

{foo:} is perfectly valid as per YAML 1.2. However, in YAML 1.1, the colon indeed belongs to the scalar following the productions in the spec. 1.1 requires a s-separate(n,c) (coming from here) after the colon and } is no separation character.

Allowing this basically makes libyaml implement neither 1.1 nor 1.2 (since there are more 1.2 changes it is not aware of). However, it takes its liberties anyway. I have no strong opinion about whether it is a good idea to allow it.

asomov · 2018-01-31T17:55:03Z

If this is accepted is makes other parsers inconsistent.
For instance SnakeYAML implements 1.1. If this change is accepted users of SnakeYAML will complain that the very same YAML document is parsed with this parser, but cannot be parsed with SnakeYAML.

This is a followup to yaml#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: - `[:foo]` - `[foo:]`

perlpunk · 2018-02-02T20:19:07Z

I created PR #104
It will only support things like [http://example], but not the other cases mentioned in #28 (comment)

This is a followup to yaml#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: - `[:foo]` - `[foo:]`

perlpunk · 2018-02-02T20:44:56Z

@asomov Allowing [a:b] conforms to the 1.1 spec (and IMHO makes sense), so supporting this should be ok, and SnakeYAML should probably allow it, too. See also my comments in #104

perlpunk · 2018-02-02T21:23:36Z

@asomov and, btw, YAML parsers are already pretty inconsistent: http://matrix.yaml.io/ ;-)

jstoiko · 2018-02-02T23:23:08Z

FYI, go-yaml has already addressed the issue: go-yaml/yaml#295

asomov · 2018-02-03T10:51:40Z

@perlpunk : the page you show is inconsistent, not the parsers. Why they try to test 1.2 spec against the parsers which explicitly do not support it !
PyYAML, libyaml, SnakeYAML are consistent - they succeed or fail together. If libyaml decides to accepts this (inconsistent) patch then it fall out of the group.

asomov · 2018-02-03T10:55:12Z

@jstoiko : why do you propose this change to all the parsers WITHOUT proposal for the corner cases ?

perlpunk · 2018-02-03T11:51:51Z

@asomov

PyYAML, libyaml, SnakeYAML are consistent - they succeed or fail together.

That's not true. PyYAML accepts [ http://example.org ]. Have a closer look at the test cases to see that there are some differences.

Why they try to test 1.2 spec against the parsers which explicitly do not support it !

Well, sure, they should be tested against 1.1 test cases. But none of us has time to write a test suite for 1.1. We already have enough to do with 1.2.
The only solution would be to remove a lot of parsers from the matrix, but I don't want that. I think it's useful to see how they behave against 1.2 test cases, even if they implement 1.1 only. To me, YAML 1.2 is the successor of 1.1 which fixes a number of things, and ideally parsers should have adopted it, but I can understand why that didn't really happen for existing 1.1 parsers. That's a reason why @ingydotnet started the https://github.com/yaml/yaml-test-suite. There should have been a test suite from the beginning.

I could try to make it more clear on the matrix page which parsers implement 1.1/1.0. What do you think, would that help? Maybe create an issue for the https://github.com/perlpunk/yaml-test-matrix, and then we can discuss it there, since it's probably off topic here.

asomov · 2018-02-03T13:03:29Z

@perlpunk : PyYAML 3.12 does exactly what it should - rejects to accept inconsistent colon

`
python3
Python 3.5.2 (default, Nov 23 2017, 16:37:01)
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.

import yaml
from yaml import load, dump
from yaml import Loader, Dumper
data = load(' [ http://example.org ]', Loader=Loader)
Traceback (most recent call last):
File "", line 1, in
File "/usr/local/lib/python3.5/dist-packages/yaml/init.py", line 72, in load
return loader.get_single_data()
File "/usr/local/lib/python3.5/dist-packages/yaml/constructor.py", line 35, in get_single_data
node = self.get_single_node()
File "/usr/local/lib/python3.5/dist-packages/yaml/composer.py", line 35, in get_single_node
if not self.check_event(StreamEndEvent):
File "/usr/local/lib/python3.5/dist-packages/yaml/parser.py", line 98, in check_event
self.current_event = self.state()
File "/usr/local/lib/python3.5/dist-packages/yaml/parser.py", line 143, in parse_implicit_document_start
StreamEndToken):
File "/usr/local/lib/python3.5/dist-packages/yaml/scanner.py", line 116, in check_token
self.fetch_more_tokens()
File "/usr/local/lib/python3.5/dist-packages/yaml/scanner.py", line 252, in fetch_more_tokens
return self.fetch_plain()
File "/usr/local/lib/python3.5/dist-packages/yaml/scanner.py", line 676, in fetch_plain
self.tokens.append(self.scan_plain())
File "/usr/local/lib/python3.5/dist-packages/yaml/scanner.py", line 1299, in scan_plain
"Please check http://pyyaml.org/wiki/YAMLColonInFlowContext for details.")
yaml.scanner.ScannerError: while scanning a plain scalar
in "", line 1, column 4:
[ http://example.org ]
^
found unexpected ':'
in "", line 1, column 8:
[ http://example.org ]
^
Please check http://pyyaml.org/wiki/YAMLColonInFlowContext for details.

`

perlpunk · 2018-02-03T13:26:14Z

@asomov ok, I can confirm that when testing that locally. The PyYAML in https://github.com/yaml/yaml-editor, however, accepts it. That's because the PyYAML in yaml-editor uses PyYAML master branch and this already has the fix from yaml/pyyaml#45.
But looking at the Spec and example "Example 9.12. Plain Characters", it's clear to me that the intent was to allow colons (even at the beginning):

# Outside flow collection:
- ::std::vector
- Up, up and away!
- -123
# Inside flow collection:
- [ ::std::vector,
  "Up, up and away!",
  -123 ]

Only that production number 154/155 seem to have a mistake, allowing also {}[], after the colon, which doesn't make sense (to me), because:

Inside flow collections, plain scalars are further restricted to avoid
containing the “[”, “]”, “{”, “}” and “,” characters [...]

http://yaml.org/spec/1.1/#id907281 "9.1.3. Plain"
http://yaml.org/spec/1.1/#nb-plain-char-in
What do you think?

asomov · 2018-02-03T13:35:56Z

@perlpunk (as I already said many times)

YAML spec 1.1 is inconsistent.
Any attempt to "fix" 1.1 parsers is inconsistent (see p 1)
Instead of waisting time to mess with parsers come with a proposal for YAML 1.3 or YAML 2.
Spend your time implementing the new spec
Enjoy.

flyx · 2018-02-03T13:37:58Z

I would say that it makes perfect sense to allow [ http://example.org ] since this is valid YAML 1.1. Is there any reason to challenge that? There is nothing in the spec's productions that forbid that, so a conforming parser has to parse it.

However, {foo:} is invalid as discussed above and in this case, I agree with @asomov that „fixing“ this might not be desirable because it leads to further inconsistencies between implementations. People wanting that to be fixed should go with a YAML 1.2 parser, since 1.2 is the revision that fixes this in the spec.

perlpunk · 2018-02-03T13:49:48Z

@asomov

Instead of waisting time to mess with parsers come with a proposal for YAML 1.3 or YAML 2.

We are working on YAML 1.3: https://github.com/yaml/summit.yaml.io/wiki/YAML-RFC-Index

Spend your time implementing the new spec

I'm implementing a Processor in Perl for 1.2, and I'm trying to design it in a way that it can also support 1.3 in the future.

I'm contributing to yaml-test-suite and yaml-editor, and I created yaml-test-matrix to avoid a mess in the future.

perlpunk · 2018-02-03T13:51:04Z

@flyx so you would support #104?

perlpunk · 2018-02-03T19:08:07Z

to make it clear: I'd vote for closing this and instead use #104

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: - `[:foo]` - `[foo:]`

This is a followup to yaml#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: - `[:foo]` - `[foo:]`

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: - `[:foo]` - `[foo:]` See https://gist.github.com/perlpunk/de2c631a7b0e9002de351a680eadb573 for all the relevant cases where this change applies.

perlpunk · 2018-06-30T23:04:04Z

#104 was merged

cloudwu force-pushed the colonfix branch from 163a495 to dff0be8 Compare December 3, 2016 12:43

colon can be in plain scalar, see PR yaml#21

2a00a38

cloudwu force-pushed the colonfix branch from dff0be8 to 2a00a38 Compare December 3, 2016 12:44

dbeer1 mentioned this pull request Dec 7, 2016

Allow colon in a plain scalar in a flow context yaml/pyyaml#45

Merged

sigmavirus24 pushed a commit to sigmavirus24/libyaml that referenced this pull request Apr 6, 2017

Update new_library.md (yaml#28)

4a630a8

perlpunk requested review from ingydotnet and sigmavirus24 December 23, 2017 00:22

sigmavirus24 approved these changes Dec 23, 2017

View reviewed changes

jstoiko mentioned this pull request Dec 23, 2017

Plain scalars with colons inside flow mappings & sequences are valid YAML go-yaml/yaml#295

Closed

ingydotnet approved these changes Dec 23, 2017

View reviewed changes

ingydotnet mentioned this pull request Jan 7, 2018

Release YAML 0.1.8. #90

Open

perlpunk mentioned this pull request Feb 2, 2018

Allow colons in plain scalars inside flow collections #104

Closed

perlpunk closed this Sep 8, 2018

kddejong mentioned this pull request Apr 19, 2021

Issue parsing colons on Linux aws-cloudformation/cfn-lint#1984

Closed

colon can be in a plain scalar #28

colon can be in a plain scalar #28

Conversation

cloudwu commented Dec 3, 2016

dbeer1 commented Dec 6, 2016 • edited Loading

ingydotnet commented Dec 7, 2016

dbeer1 commented Dec 7, 2016

dbeer1 commented Dec 7, 2016

ingydotnet commented Dec 7, 2016

dbeer1 commented Dec 8, 2016 • edited Loading

ingydotnet commented Dec 8, 2016

ingydotnet commented Dec 8, 2016

dbeer commented Dec 8, 2016

dbeer1 commented Dec 8, 2016

dbeer1 commented Feb 8, 2017

ingydotnet commented Feb 9, 2017

jstoiko commented Dec 22, 2017

perlpunk commented Dec 23, 2017

sigmavirus24 left a comment

Choose a reason for hiding this comment

ingydotnet left a comment

Choose a reason for hiding this comment

perlpunk commented Dec 28, 2017 • edited Loading

perlpunk commented Dec 28, 2017

perlpunk commented Dec 28, 2017

ingydotnet commented Dec 28, 2017

jstoiko commented Jan 12, 2018

perlpunk commented Jan 12, 2018

flyx commented Jan 29, 2018

asomov commented Jan 31, 2018

perlpunk commented Feb 2, 2018

perlpunk commented Feb 2, 2018

perlpunk commented Feb 2, 2018

jstoiko commented Feb 2, 2018

asomov commented Feb 3, 2018

asomov commented Feb 3, 2018

perlpunk commented Feb 3, 2018 • edited Loading

asomov commented Feb 3, 2018

perlpunk commented Feb 3, 2018 • edited Loading

asomov commented Feb 3, 2018 • edited Loading

flyx commented Feb 3, 2018

perlpunk commented Feb 3, 2018 • edited Loading

perlpunk commented Feb 3, 2018

perlpunk commented Feb 3, 2018

perlpunk commented Jun 30, 2018

dbeer1 commented Dec 6, 2016 •

edited

Loading

dbeer1 commented Dec 8, 2016 •

edited

Loading

perlpunk commented Dec 28, 2017 •

edited

Loading

perlpunk commented Feb 3, 2018 •

edited

Loading

perlpunk commented Feb 3, 2018 •

edited

Loading

asomov commented Feb 3, 2018 •

edited

Loading

perlpunk commented Feb 3, 2018 •

edited

Loading