Decode unknown-tag content according to the immutable flag

[theeldermillenial]

PR 2 — Decode unknown-tag content according to the immutable flag

Branch: fix/tag-content-immutable

Problem

The content of a semantic tag with no designated decoder is always decoded as immutable — tuple for arrays, frozendict for maps — regardless of the decoder's immutable flag.

Every other container honours immutable: top-level arrays/maps, nested arrays/maps, and even the tag-258 (set) decoder all thread the flag through. Only the generic-tag arm in decode_semantic hardcodes true:

return Ok(BeginFrame(
    callback,
    true,            // <-- ignores `immutable`
    Some(container),
    DisplayName::SemanticTag(tagnum),
));

So cbor2.loads(cbor2.dumps(CBORTag(6000, [1, 2, 3]))).value is a tuple, even though the caller asked for (default) mutable decoding.

This:

• surprises callers who decode with immutable=False and then try to mutate a tag's array/map value;
• breaks downstream libraries that assume cbor2.loads(tagged).value is a list — the behavior of cbor2 ≤ 5.x. (Concretely, this is what breaks pycose's CoseMessage.decode, which does isinstance(value, list) and value.pop(0).)

It is a further instance of the mutability bug fixed in 6.0.1 (#295, "values being decoded as immutable in unexpected places").

Solution

Thread the immutable flag through the generic-tag BeginFrame so tag content honours it like every other container (true → immutable).

Changes

• rust/decoder.rs: one-line fix in decode_semantic's generic-tag arm.
• tests/test_decoder.py: test_unknown_tag_value_respects_immutable — array content is list by default / tuple under immutable=True; map content is dict / non-dict mapping respectively.
• changelog entry.

Verification

Full test suite passes (442); no existing test depended on tag content being immutable.

[coveralls]

coverage: 94.682%. remained the same — theeldermillenial:fix/tag-content-immutable into agronholm:master

[theeldermillenial]

@agronholm I see you've been merging in other PRs.

Is there something wrong with this and #311 that I should fix? Or is the content just something you need to think through?

[agronholm]

Those were straightforward bug fixes. This changes the decoding semantics by changing what type gets passed to the decoder hook. This is a potentially backwards incompatible change.

[theeldermillenial]

I understand that, but can you suggest a path forward? Are you considering merging this? Is there something I can do that would be better?

At the end of the day the current encoding/decoding loop isn't idempotent. Roundtrip serialization can produce different results, and that's a problem when you're trying to hash encoded cbor to check for similarity.

I'm very dedicated to trying to make this work. Currently we forked the old repo because there were too many issues, but I'm trying to bring us back into your new Rust version. I'd prefer to stay on your version rather than create a new fork so that we have a bigger community and we can all work together on this.

I would like to have some kind of solution in the next few weeks, and I have time to dedicate to implementing this however we need to in order to move forward. As far as I can tell, this is going to require some kind of breaking change.

[agronholm]

#296 reported the same issue, yes? If so, that would probably be enough to convince me to make the backwards incompatible change. It will require a major version bump but that's okay.

[theeldermillenial]

Yes, that works for me!

We originally forked and modified, but I never like doing that. It splits work and I think makes a less useful tool in the long run.

I also see in that conversation your frustration about not checking out the release candidate. I apologize I did not see that release candidate. I know I did open a PR when you were doing your Rust rewrite that got closed, but I never came back to check it out.

Let me know how to proceed.

[theeldermillenial]

@agronholm What are the next steps for this?

[agronholm]

I will handle all available PRs, bake them into a new 6.x release and then merge yours.

[agronholm]

I'd like some wording changes in the changelog.

[agronholm]

Okay, this will be in the 7.0 release, whenever it's ready. I'm looking into also including the streaming decoder in that one.