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.

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

fix empty yaml unmarshal #1807

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

critterjohnson
Copy link

@critterjohnson critterjohnson commented Jul 22, 2022

Fixes #1790. This assumes that any time an evaluator returns nil, but no error, it is simply evaluating an empty value and returns a Bottom.

Signed-off-by: Christopher Johnson <critterjohnson45@gmail.com>
@mvdan
Copy link
Member

mvdan commented Jul 25, 2022

I think this one is subtle enough that we want @mpvl's eyes on it :)

@mvdan mvdan requested a review from mpvl July 25, 2022 11:05
@mvdan mvdan self-assigned this Apr 13, 2023
@mvdan
Copy link
Member

mvdan commented Apr 19, 2023

We looked at this one with @mpvl; we definitely agree that the bug is valid, but we think the fix probably belongs in the encoding package rather than the evaluator. It's also not a trivial fix, because the YAML spec doesn't clearly define whether the empty input should be invalid, null, or something else. I'll take a deeper look when I can.

@mvdan mvdan added the Discuss Requires maintainer discussion label Apr 19, 2023
@uhthomas
Copy link
Contributor

I don't think there's a good answer for this. The YAML spec is very vague and implementations don't even seem to have agreement.

nodeca/js-yaml#565

Some of the YAML spec seems to suggest that a completely empty document, or even a document just consisting of comments should not be represented at all.

https://yaml.org/spec/1.2.2/#rule-l-bare-document

Then, an explicit document denoted with --- should be represented as null.

https://yaml.org/spec/1.2.2/#915-directives-documents


I guess based on that, a bottom (_|_) value may be appropriate? I guess it then begs the question of what {} would mean and whether it would better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discuss Requires maintainer discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

encoding/yaml: Panic when unmarshalling YAML content that is only comments
3 participants