Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v3] Improve comment handling #684

Closed
wants to merge 9 commits into from
Closed

[v3] Improve comment handling #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
52a3704
Add idempotency tests.
felixfontein Dec 29, 2020
d43cefb
Add tests that fail.
felixfontein Dec 29, 2020
05a4f38
Dump tree to stdout to ease debugging.
felixfontein Dec 29, 2020
6abbb6e
Fix moving comment.
felixfontein Dec 30, 2020
d2b2a0b
Improve empty document handling.
felixfontein Dec 30, 2020
7b36ba5
Prevent comment document crawling.
felixfontein Dec 30, 2020
46fa0f1
Document with only comments should not have newline following.
felixfontein Dec 30, 2020
193064d
Apply some review suggestions.
felixfontein Jan 5, 2021
02f3190
Add single-document tests from yaml_test to node_test.
felixfontein Jan 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions decode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ func (p *parser) anchor(n *Node, anchor []byte) {
}
}

func (p *parser) parse() *Node {
func (p *parser) parse(toplevel bool) *Node {
p.init()
switch p.peek() {
case yaml_SCALAR_EVENT:
Expand All @@ -156,6 +156,11 @@ func (p *parser) parse() *Node {
return p.document()
case yaml_STREAM_END_EVENT:
// Happens when attempting to decode an empty buffer.
if toplevel && len(p.event.head_comment) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either the pre-existing comment above is incorrect, or we don't need the new variable. Also, it sounds like we should have enough state available in the parser to tell whether this is a top-level value or not. Can you provide some more details about what's going on here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know / understand the parser well enough to be sure whether the comment is correct or not. That's why I added the parameter toplevel, to avoid accidentally introducing a bug here. If you prefer I can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's okay, but we cannot have additional logic in here unless we understand that well, otherwise we may be surprised in other situations, or may have misleading logic that makes it sound like this code executes at times it doesn't. Right now this is labeled as a "stream end event". Either that's true and the new logic is wrong, or it's incorrect and the old comment is wrong.

n := p.node(DocumentNode, "", "", "")
p.event.head_comment = nil
return n
}
return nil
case yaml_TAIL_COMMENT_EVENT:
panic("internal error: unexpected tail comment event (please report)")
Expand Down Expand Up @@ -191,14 +196,21 @@ func (p *parser) node(kind Kind, defaultTag, tag, value string) *Node {
}

func (p *parser) parseChild(parent *Node) *Node {
child := p.parse()
child := p.parse(false)
parent.Content = append(parent.Content, child)
return child
}

func (p *parser) document() *Node {
n := p.node(DocumentNode, "", "", "")
p.doc = n
if !p.event.implicit && len(p.event.head_comment) > 0 {
// This comment belongs to **before** the document event
n.Line = 1
n.Column = 1
p.event.head_comment = nil
return n
}
p.expect(yaml_DOCUMENT_START_EVENT)
p.parseChild(n)
if p.peek() == yaml_DOCUMENT_END_EVENT {
Expand Down
6 changes: 5 additions & 1 deletion emitterc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ func yaml_emitter_emit_stream_start(emitter *yaml_emitter_t, event *yaml_event_t
func yaml_emitter_emit_document_start(emitter *yaml_emitter_t, event *yaml_event_t, first bool) bool {

if event.typ == yaml_DOCUMENT_START_EVENT {
isEmpty := emitter.events[emitter.events_head + 1].typ == yaml_DOCUMENT_END_EVENT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we know here that the +1 is safe?


if event.version_directive != nil {
if !yaml_emitter_analyze_version_directive(emitter, event.version_directive) {
Expand Down Expand Up @@ -450,12 +451,15 @@ func yaml_emitter_emit_document_start(emitter *yaml_emitter_t, event *yaml_event
if !yaml_emitter_process_head_comment(emitter) {
return false
}
if !put_break(emitter) {
if !isEmpty && !put_break(emitter) {
return false
}
}

emitter.state = yaml_EMIT_DOCUMENT_CONTENT_STATE
if isEmpty {
emitter.state = yaml_EMIT_DOCUMENT_END_STATE
}
return true
}

Expand Down
266 changes: 264 additions & 2 deletions node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,41 @@ var nodeTests = []struct {
}},
},
}, {
"[encode]null\n",
yaml.Node{},
"null\n",
yaml.Node{
Kind: yaml.DocumentNode,
Line: 1,
Column: 1,
Content: []*yaml.Node{{
Kind: yaml.ScalarNode,
Tag: "!!null",
Value: "null",
Line: 1,
Column: 1,
}},
},
}, {
"[decode]\n",
yaml.Node{
Kind: 0,
Line: 0,
Column: 0,
Content: []*yaml.Node(nil),
},
}, {
"[decode]---\n",
yaml.Node{
Kind: yaml.DocumentNode,
Line: 1,
Column: 1,
Content: []*yaml.Node{{
Kind: yaml.ScalarNode,
Tag: "!!null",
Value: "",
Line: 2,
Column: 1,
}},
},
}, {
"foo\n",
yaml.Node{
Expand Down Expand Up @@ -2339,6 +2372,235 @@ var nodeTests = []struct {
},
}},
},
}, {
"# foo\n",
yaml.Node{
Kind: yaml.DocumentNode,
Line: 2,
Column: 1,
HeadComment: "# foo",
Content: []*yaml.Node(nil),
},
}, {
"# beginning\na:\n ## foo\n ##\n b:\n",
yaml.Node{
Kind: yaml.DocumentNode,
Line: 2,
Column: 1,
Content: []*yaml.Node{{
Kind: yaml.MappingNode,
Tag: "!!map",
Line: 2,
Column: 1,
Content: []*yaml.Node{
{
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 2,
Column: 1,
Value: "a",
HeadComment: "# beginning",
}, {
Kind: yaml.MappingNode,
Tag: "!!map",
Line: 5,
Column: 3,
Content: []*yaml.Node{
{
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 5,
Column: 3,
Value: "b",
HeadComment: "## foo\n##",
}, {
Kind: yaml.ScalarNode,
Tag: "!!null",
Line: 5,
Column: 5,
},
},
},
},
}},
},
}, {
// When re-encoding, the newline is gone. Without the newline, the bug
// which moves the comment to the wrong place was not happening.
"[decode]a:\n b:\n # comment followed by newline\n\n c: d\n",
yaml.Node{
Kind: yaml.DocumentNode,
Line: 1,
Column: 1,
Content: []*yaml.Node{{
Kind: yaml.MappingNode,
Tag: "!!map",
Line: 1,
Column: 1,
Content: []*yaml.Node{
{
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 1,
Column: 1,
Value: "a",
}, {
Kind: yaml.MappingNode,
Tag: "!!map",
Line: 2,
Column: 3,
Content: []*yaml.Node{
{
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 2,
Column: 3,
Value: "b",
}, {
Kind: yaml.MappingNode,
Tag: "!!map",
Line: 5,
Column: 5,
Content: []*yaml.Node{
{
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 5,
Column: 5,
Value: "c",
HeadComment: "# comment followed by newline",
}, {
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 5,
Column: 8,
Value: "d",
},
},
},
},
},
},
}},
},
}, {
"# begin\na:\n # foo\n # bar\n b:\n # baz\n c:\n foo: bar\n # asdf\n # bang\n" +
"d:\n # a\n # b\n - 1\n # c\n - - 123\n # f\n # d\n - 2\n # e\n",
yaml.Node{
Kind: yaml.DocumentNode,
Line: 2,
Column: 1,
Content: []*yaml.Node{{
Kind: yaml.MappingNode,
Tag: "!!map",
Line: 2,
Column: 1,
Content: []*yaml.Node{
{
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 2,
Column: 1,
Value: "a",
HeadComment: "# begin",
}, {
Kind: yaml.MappingNode,
Tag: "!!map",
Line: 5,
Column: 3,
Content: []*yaml.Node{
{
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 5,
Column: 3,
Value: "b",
HeadComment: "# foo\n# bar",
}, {
Kind: yaml.ScalarNode,
Tag: "!!null",
Line: 5,
Column: 5,
}, {
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 7,
Column: 3,
Value: "c",
HeadComment: "# baz",
FootComment: "# bang",
}, {
Kind: yaml.MappingNode,
Tag: "!!map",
Line: 8,
Column: 5,
Content: []*yaml.Node{
{
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 8,
Column: 5,
Value: "foo",
FootComment: "# asdf",
}, {
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 8,
Column: 10,
Value: "bar",
},
},
},
},
}, {
Kind: yaml.ScalarNode,
Tag: "!!str",
Line: 11,
Column: 1,
Value: "d",
}, {
Kind: yaml.SequenceNode,
Tag: "!!seq",
Line: 14,
Column: 3,
Content: []*yaml.Node{
{
Kind: yaml.ScalarNode,
Tag: "!!int",
Line: 14,
Column: 5,
Value: "1",
HeadComment: "# a\n# b",
}, {
Kind: yaml.SequenceNode,
Tag: "!!seq",
Line: 16,
Column: 5,
HeadComment: "# c",
Content: []*yaml.Node{
{
Kind: yaml.ScalarNode,
Tag: "!!int",
Line: 16,
Column: 7,
Value: "123",
FootComment: "# f",
},
},
}, {
Kind: yaml.ScalarNode,
Tag: "!!int",
Line: 19,
Column: 5,
Value: "2",
HeadComment: "# d",
FootComment: "# e",
},
},
},
},
}},
},
},
}

Expand Down
21 changes: 21 additions & 0 deletions parserc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ func yaml_parser_parse_document_start(parser *yaml_parser_t, event *yaml_event_t
typ: yaml_DOCUMENT_START_EVENT,
start_mark: token.start_mark,
end_mark: token.end_mark,
implicit: true,

head_comment: head_comment,
}
Expand Down Expand Up @@ -338,6 +339,7 @@ func yaml_parser_parse_document_start(parser *yaml_parser_t, event *yaml_event_t
tag_directives: tag_directives,
implicit: false,
}
yaml_parser_set_event_comments(parser, event)
skip_token(parser)

} else {
Expand All @@ -348,6 +350,7 @@ func yaml_parser_parse_document_start(parser *yaml_parser_t, event *yaml_event_t
start_mark: token.start_mark,
end_mark: token.end_mark,
}
yaml_parser_set_event_comments(parser, event)
skip_token(parser)
}

Expand Down Expand Up @@ -890,6 +893,24 @@ func yaml_parser_parse_block_mapping_value(parser *yaml_parser_t, event *yaml_ev
if token.typ == yaml_VALUE_TOKEN {
mark := token.end_mark
skip_token(parser)
// Move foot comment to head comment. This prevents that in some cases, comments
// in maps are moved to the wrong place. In the following map:
//
// a:
// b:
// # comment followed by newline
//
// c: d
//
// (the newline between the comment and the next line is essential!)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is stated more succinctly in the body of the yaml right above, so no need to repeat here.

Otherwise, thanks for the comment. It provides clear rationale for the non-obvious logic.

// the comment ends up as the foot comment of `c`, which is obviously wrong.
if len(parser.foot_comment) > 0 {
if len(parser.head_comment) > 0 {
parser.head_comment = append(parser.head_comment, '\n')
}
parser.head_comment = append(parser.head_comment, parser.foot_comment...)
parser.foot_comment = nil
}
token = peek_token(parser)
if token == nil {
return false
Expand Down
Loading