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

Disallow usage of alias before anchor declaration #491

Conversation

am11
Copy link
Contributor

@am11 am11 commented May 10, 2020

According to YAML 1.1 §8.4 and YAML 1.2 §7.1:

It is an error to have an alias node use an anchor that does not previously occur in the document.

Currently we emit a: test0 for this YAML:

a: *anchor1
b: &anchor1 test0

PR aligns it with spec and changes it to throw AnchorNotFoundException instead.

Adjusted some tests which were asserting that forward alias usage is allowed.

Also added a test for duplicate anchors case, explicitly asserting what values are expected, per YAML 1.2 §3.2.2.2: yaml/yaml-spec#48.

@am11 am11 force-pushed the feature/conformance/emitter/usage-of-alias-before-declaration branch 3 times, most recently from 6b02621 to 9c769a6 Compare May 10, 2020 02:47
@am11 am11 force-pushed the feature/conformance/emitter/usage-of-alias-before-declaration branch from 9c769a6 to 75d3a1f Compare May 10, 2020 19:55
@aaubry
Copy link
Owner

aaubry commented May 11, 2020

I know that this is against the spec. When I implemented support for forward references, I hadn't understood correctly that part of the spec and assumed that the anchor could appear after it was referenced.
The problem is that discontinuing this support would be a breaking change. I don't know if anyone is relying on this, and I agree that we should do it, but that will need to be included in a major release.
Also, there's more code that should be removed in the deserialization pipeline, namely everything that is related to IValuePromise. That's the abstraction that allows to deserialize an object and later set the properties that were resolved later.

@am11
Copy link
Contributor Author

am11 commented May 11, 2020

I agree that it would be a breaking change among other required breakages which we will need to fully implement #484 (both serialization and deserialization conforming to spec v1.1 and v1.2). Most of the emitter spec conformance will take place step by step due to the nature of changes required to fit into the current API design. IMO, other than the required cleanups along the way, the rest of the cleanups, refactorings and performance improvements can be done once the correctness (standard conformance) is completed.

Is it a good time to target the next release as 9.0 and wait for all the changes we need (as long as it takes to accomplish spec v1.2 feat)? Otherwise, if there is a plan to ship some important fixes in v8.x, I think we can branch out v9.0 and push these changes there.

@aaubry
Copy link
Owner

aaubry commented May 28, 2020

Sorry, I forgot to follow up on this conversation. I think there's no problem in merging this now. There's no need to cleanup the IValuePromise stuff now as I have already discontinued that in the schemas branch.
Regarding the releases, I don't really have a plan. I just increase the version numbers according the changes that were performed.

@aaubry aaubry merged commit bd37fab into aaubry:master May 28, 2020
@am11 am11 deleted the feature/conformance/emitter/usage-of-alias-before-declaration branch May 28, 2020 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants