Prevent sequence & mapping tags from being ignored

[gmorpheme]

So turns out #141 wasn't as effective as I'd hoped. (i.e. at all).

While it certainly allowed tags to be specified against mappings and sequences and passed them into libyaml C, there was an "implicit" parameter was hardcoded to 1 in Libyaml.hs which was causing libyaml C to treat them all as optional and ignore them.

Tried a couple of approaches to fixing this.

First off, we could further overload the Style enums to encode this information as well, like we're already doing in Plain vs PlainNoTag in the scalar style... and then maybe hide away the factoring out of the "implicit" parameter (i.e. this ickiness...

                    let (pi, style) =
                            case style0 of
                                PlainNoTag -> (1, Plain)
                                x -> (0, x)

)

...into a type class to slightly clean up the calling code.

But I think, even if the libyaml C API is a bit wonky here, trying to simplify it by stuffing these concerns together into the same data types is just muddying the waters further. We could end up with other tangles to unpick further down the line.

So I think this PR is the better approach: just expose the "implicit" parameter in the Event data type, even if it does involve changing the Event type once more and add in a field that few people will need.

There are still some awkwardnesses:

• this is really only meaningful for render; on read it defaults to TagOptional just for to be extra safe for compatibility
• I've put some tests in YamlSpec that really belong in a LibyamlSpec - but there isn't one
• feels wrong to not to clean this up in the Scalar case too, but that's rather more confounded (two different "implicit" parameters with tight relationships to the style values) and probably much riskier

I'm only using Libyaml so I haven't given much thought to the Data.Yaml side of things beyond trying to ensure nothing breaks.

In case sequence and mapping tags seem like a bit of an esoteric corner case, they are actually quite heavily used in AWS CloudFormation for example (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-join.html).

[snoyberg]

I'd like to avoid yet another breaking change.

1. Are you certain this change actually addresses your concern?
2. Is there no possibility of a non-breaking change for this?
3. Is it possible to limit the breakage to only libyaml without breaking the yaml API?

[gmorpheme]

Sure - entirely understand.

Are you certain this change actually addresses your concern?

Yes - these tests verify the exact ByteStrings generated.

Is there no possibility of a non-breaking change for this?

There is a non-breaking alternative that I alluded to above. I don't like it very much but I'm happy to run with it if it's what you'd rather see in the library. More detail below.

Is it possible to limit the breakage to only libyaml without breaking the yaml API?

Yes. I hope that is what I have already done. All existing tests pass.

To actually get a mapping or sequence tag output (which has not been possible till now), users of yaml would have to find a way to get the new flag into libyaml somehow and I have not provided any way to do that.

Probably there should be a way to output these tags from yaml but I don't presume to have an answer on how best to do that.

The alternative...

...would only add new constructors to existing sum-types MappingStyle and SequenceStyle - a bit like the PlainNoTag constructor in Style.

PlainNoTag is purely a Libyaml.hs invention that maps onto a combination of parameter values in the libyaml C API. These new constructors would be the same.

However I'm uneasy about the approach.

a) in order to preserve backward compatible meaning for the existing constructors the new constructors would be backwards from the PlainNoTag example in Style:

data SequenceStyle
 = AnySequence
 | AnySequenceWithTag -- new
 | BlockSequence
 | BlockSequenceWithTag -- new
 | FlowSequence
 | FlowSequenceWithTag -- new
 deriving (Show, Eq, Enum, Bounded, Ord, Data, Typeable)

...at which point I think we've just damaged the conceptual integrity of the API in our attempts to mask its complexity.

b) I'm worried we're making slightly brittle attempts to "simplify" an API that really we are interested in exposing - it would be quite easy to end up getting into further muddles by making these second hand fudges over an API designed and maintained elsewhere.

I believe this would be source-compatible with existing code. (It might conceivably cause warnings about incomplete case matches if anyone is matching on these types).

[snoyberg]

Would it be possible to get the result you're looking for with just an additional field in FormatOptions?

[gmorpheme]

I don't think so.

This might work to set a "tags on" setting for the entire event stream.

It would not cleanly allow you to control the visibility of the tag for individual events. It is quite likely you only want to generate a handful of these tags in an otherwise untagged document (the AWS case I mentioned is an example - AWS abuse tags to express functions).

Potentially the NoTag value could be the solution here. If all mappings and sequences by default had NoTag then you could ensure that no tag were generated even when FormatOptions says to produce them. But the tags for mappings and sequences are not NoTag but:

tagToString SeqTag = "tag:yaml.org,2002:seq"
tagToString MapTag = "tag:yaml.org,2002:map"

...which normally you want left implicit, despite the fact they're really there.

[snoyberg]

When generating the EventSequenceStart and EventMappingStart events, can't you set the tag to NoTag?

[gmorpheme]

So I'd be setting a stream-wide setting to force the display of all (mapping & sequence) tags but then setting all normal mapping and sequences to NoTag?

I can live with it.

They'll all be read back with SeqTag and MapTag (I think) but that's probably fine.

I'll try it.

[gmorpheme]

Works out quite easy, now on #165.

[gmorpheme]

Closing this in favour of #165.