internal/encoding/yaml: encode YAML anchors as CUE definitions

[OmriSteiner]

Fixes #3818

There's one small issue open to handle. Tests are currently failing with:

--- FAIL: TestUnmarshalErrors (0.00s)
    --- FAIL: TestUnmarshalErrors/test_7:_"a:_&a\n__b:_*a\n" (0.00s)
        decode_test.go:971:   yaml:
            a: &a
              b: *a
        decode_test.go:977: got <nil>; want test.yaml:2: anchor "a" value contains itself; (value #a: {
                b: #a
            }
            a: #a)
--- FAIL: TestDecoderErrors (0.00s)
    --- FAIL: TestDecoderErrors/test_7:_"a:_&a\n__b:_*a\n" (0.00s)
        decode_test.go:986:   yaml:
            a: &a
              b: *a
        decode_test.go:988: got <nil>; want test.yaml:2: anchor "a" value contains itself

I could change the code to match the old behavior of rejecting such YAML documents, since the resulting CUE document is invalid.
However, I think the original reason we error out for such document was that it would lead to endless recursion in the parser. This is no longer the case, so I'm debating whether the correct fix would be to error out, or change the test.

[mvdan]

If recursive anchors like that no longer fail, what CUE do they result in?

[OmriSteiner]

If recursive anchors like that no longer fail, what CUE do they result in?

You can see in the error above:

#a: {
    b: #a
}
a: #a

Still recursive, so it's invalid. But the encoder doesn't recurse infinitely.

[mvdan]

Ah I see. That's fine; I don't imagine these cases will actually show up in practice often. Indeed I think we avoided cycles because of endless recursion.

[OmriSteiner]

On the one hand it makes no sense to emit an invalid CUE file.
But on the other hand - perhaps there's a use-case I'm missing here?

[OmriSteiner]

Ah I see. That's fine; I don't imagine these cases will actually show up in practice often. Indeed I think we avoided cycles because of endless recursion.

Fine, then I'll edit the test.

[OmriSteiner]

I pushed some changes to make the tests pass.
I also found an additional edge case I'll have to fix, so currently working on that.

[OmriSteiner]

I pushed some changes to make the tests pass. I also found an additional edge case I'll have to fix, so currently working on that.

Alright, that should be sorted. Had a positioning issue where I accidentally called d.pos for the anchor reference after extracting the node itself.

[mvdan]

@OmriSteiner the tests are failing, not sure if you saw that?

[OmriSteiner]

@OmriSteiner the tests are failing, not sure if you saw that?

Yes, I did.
As I wrote:

Anyway, I pushed the patch. But I think it's unwise to merge it like this.
If you have any suggestions for a cleaner implementation I'm up for it.
But in the current form, I think keeping things simple is more important than being marginally better.

I only pushed the additional commit for you to look at. If you think this is worth the tradeoff with implementation complexity, then I can fix the tests. Otherwise I think it's better if I remove the last commit here (and then the tests pass).

[mvdan]

Ah, I had misunderstood. And apologies for the slowness. Yes, I think your latest commit is a perfectly fine approach. If you can fix the tests, I'll do a final review :)

[mvdan]

Marking as "request changes" as a reminder to myself that this needs CI to be fixed before I review again.

[mvdan]

Thanks!

[mvdan]

importing into Gerrit here: https://cue.gerrithub.io/c/cue-lang/cue/+/1223900

I tweaked the commit message a bit as well, assuming that's okay with you.

[mvdan]

@rogpeppe correctly spotted that the use of definitions breaks valid use cases today, so I need to take a closer look. See the Gerrit patch discussion.