yaml / yaml-test-suite

Comprehensive, language independent Test Suite for YAML
MIT License
172 stars 58 forks source link

Tests 9KBC and CXX2 can't both be right #29

Closed eemeli closed 6 years ago

eemeli commented 6 years ago

These tests have a map start on the same line with the directives-end marker. In 9KBC, this is expected to result in an error, whereas in CXX2 the collection should be rendered without a problem. The only difference between these is that that the latter has only one key, while the former has two.

Furthermore, I at least can't find a clear explanation in the spec about what content is or is not allowed on the directives-end line. The spec does include some examples that have at least plain and block scalars on the directives-end line, as well as tags for collections that start on the following line. So where does it say that collections can't start there either?

perlpunk commented 6 years ago

Yes, that's really an edge-case. We came to the conclusion that a mapping starting on the --- line is actually allowed, that's why CXX2 is valid:

--- &anchor a: b

The question, though, is, what's the correct indendation for the following lines? As far as I know that's hard (or impossible?) to tell from the spec. There are three possibilities:

--- a: 1
b: 2

--- a: 1
 b: 2

--- a: 1
    b: 2

One could argue that the document-start has a "logical" indendation of -1, so the mapping would have an indendation of 0, so the first example seems valid. While the third example looks nicer, because the indendation matches.

Our opinion is that, because of this, a parser doesn't have to allow (or even should not allow) these kind of mappings (or sequences), but we still included the test case with one line because that's clearly valid from the spec. We also would like to forbid it completely for YAML 1.3.

Some existing parsers don't allow it (like libyaml), some allow the first example (libsyck), some allow the third (js-yaml).

I agree that these kind of test cases should have explanations.

@flyx, @ingydotnet any more thoughts?

flyx commented 6 years ago

Definitely disallowed in YAML 1.3. The 1.2 Spec is a bloody mess when it comes to directive end markers. For example, this is a valid document according to the productions:

---- a
- b

And it is equivalent to

- a
- b

However, I don't think any implementation supports that and I don't think this was intended. I would advice to agree that these edge cases are not properly covered by the spec and therefore we should drop both test cases, assuming that what is supported and what isn't is implementation-defined. This conforms to the current situation.

eemeli commented 6 years ago

Actually, the spec does imply that the indentation should be

--- a: 1
b: 2

That's based on Example 9.5, which contains this bit:

--- |
%!PS-Adobe-2.0

which is considered equivalent to:

---
!!str "%!PS-Adobe-2.0\n"

That would only be valid if ---[ \t]* would not be considered when determining the indentation, and if it's true for block scalars, it ought to be true for block collections.

Regarding dropping the tests, I agree that at least 9KBC could be dropped due to being considered implementation-specific behaviour, but I think CXX2 is actually fine as it is; by keeping the contents to a single line, the uncertainty about the indentation can be ignored.

perlpunk commented 6 years ago

@eemeli yeah: "Document nodes are indented as if they have a parent indented at -1 spaces. Since a node must be more indented than its parent node, this allows the document’s node to be indented at zero or more spaces." http://yaml.org/spec/1.2/spec.html#id2800967 9.1.3. Bare Documents

ingydotnet commented 6 years ago

Here's the meta take on this:

Regardless of how, the spec is interpreted, 9KBC and CXX2 were not meant to be valid in 1.2. They are failures in the spec. The correct thing to do here is to tag them as being such.

We have been thinking about tags for these situations. Ideas are: 1.2-unknown, 1.2-bad-spec, 1.2-pedantic.

eemeli commented 6 years ago

This was fixed by a2f4449.