Preserve parsed key order in maps

[yjbanov]

Preserve parsed key order in YamlMap.

Fixes dart-lang/tools#1909

[kevmoo]

We should add test(s) to make sure the new behavior is maintained!

[yjbanov]

@kevmoo any ideas how to test this? A test would have to rely on implementation details of HashMap and LinkedHashMap. Coincidentally, in many cases the two behave identically, which is why current tests do not fail. In fact, until I tried processing a large pubspec.yaml file I never noticed a difference.

[rakudrama]

To test order, I would create a collections of maps with all permutations of 4 or 5 simple keys, and check they are invariant through a round-trip.

[yjbanov]

@rakudrama that triggered a failure pretty quickly. Thanks for the suggestion. Implemented.

[yjbanov]

Oops! I did something wrong to the whitespace in that file. Fixing.

[yjbanov]

Fixed.

[kevmoo]

Maybe flag this as "possibly" breaking? It's not likely to affect most folks, right?

[yjbanov]

It is only mildly breaking in the sense that if anything is broken the fix should be trivial. However, strictly speaking it could break everyone. For example, I found that it breaks some test in the analyzer, which everyone depends on. I expect that the change will propagate gradually with minimal disruption.

IOW, done :)

[vsmenon]

We can't land things like this that just break our own code. It'll prevent internal rolls.

Is someone working with the analyzer team to fix? That need to happen before landing PRs like this.

fyi - @natebosch

[yjbanov]

I ran internal global presubmit for this change before landing and only found a couple of analyzer test failures that looked easily fixable.

[vsmenon]

Thanks for running the global, but someone needs to fix those before/as we land these.

[yjbanov]

Is there a document listing packages that follow this rule? This seems to pop up randomly. About a year ago package:quiver broke package:build. There doesn't seem to be a clear way for package authors to be proactive about it (e.g. we didn't even know quiver was a dependency of build).

[yjbanov]

@kevmoo can you please push a pub release?

[natebosch]

Apparently this wasn't a good idea. The yaml spec explicitly forbids attaching meaning to the order of keys:

https://yaml.org/spec/1.2/spec.html#id2765608

[yjbanov]

Apparently this wasn't a good idea. The yaml spec explicitly forbids attaching meaning to the order of keys

Attaching meaning to key order ≠ preserving key order in the parser. Remember that the spec also says "YAML is easily readable by humans" (https://yaml.org/spec/1.2/spec.html#id2708649). This means that if we allow programs to output YAML, then it must also be "readable by humans". Now what happens if the input to such program is another YAML document? The order of keys in the input document was specifically chosen by a human such that it looks readable to them. If the program ingests this YAML and arbitrarily reorders the key, then the output will violate the spec too by being non-human readable.

Now, if you are parsing YAML for purposes other than outputting it back to YAML, such as when you read a pubspec.yaml to upload a package to pub, then yes, please do not rely on the order of keys. However, for a YAML parser to mess up keys upfront seems very counter to the spirit of the YAML spec.

[kevmoo]

Agreed w/ Yegor!

…

On Thu, Oct 24, 2019 at 10:31 PM Yegor ***@***.***> wrote: Apparently this wasn't a good idea. The yaml spec explicitly forbids attaching meaning to the order of keys Attaching meaning to key order ≠ preserving key order in the parser. Remember that the spec also says "YAML is easily readable by humans" ( https://yaml.org/spec/1.2/spec.html#id2708649). This means that if we allow programs to output YAML, then it must also be "readable by humans". Now what happens if the input to such program is another YAML document? The order of keys in the input document was specifically chosen by a human such that it looks readable to them. If the program ingests this YAML and arbitrarily reorders the key, then the output will violate the spec too by output non-human readable YAML. Now, if you are parsing YAML for purposes *other* than outputting it back to YAML, such as when you read a pubspec.yaml to upload a package to pub, then yes, please do not rely on the order of keys. However, for a YAML parser to mess up keys upfront seems very counter to the spirit of the YAML spec. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#57>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEFCW6XBBI2DYLFGZG5GLQQKABJANCNFSM4IXYW3FQ> .