Skip to content

Conversation

@facutuesca
Copy link
Contributor

@facutuesca facutuesca commented Jan 24, 2026

Following the discussion in #14183, here's an alternative implementation for CHOICE fields.
If one of the types inside the union is a asn1.Variant, all the other types should be too.

# all types in the union are asn1.Variant
@asn1.sequence
class Example:
    foo: (
        Annotated[Variant[int, typing.Literal["Tag1"]], asn1.Implicit(0)]
        | Annotated[Variant[int, typing.Literal["Tag2"]], asn1.Implicit(1)]
        | Annotated[Variant[str, typing.Literal["Tag3"]], asn1.Implicit(2)]
        | Annotated[Variant[bool, typing.Literal["Tag4"]], asn1.Implicit(3)]
    )

obj = Example(foo=Variant(42, "Tag1"))
encoded = encode_der(obj)

decoded = decode(Example, encoded)
assert isinstance(decoded.foo.value, int)
assert decoded.foo.tag == "Tag1"
assert decoded.foo.value == 42


# none of the types in the union are asn1.Variant
@asn1.sequence
class Example:
    foo: (
        | Annotated[str, asn1.Implicit(0)]
        | Annotated[bool, asn1.Implicit(1)]
    )

Part of #12283

@facutuesca facutuesca marked this pull request as ready for review January 24, 2026 16:05
@alex
Copy link
Member

alex commented Jan 25, 2026

I haven't reviewed the impl closely yet, but this is probably my favorite of the various APIs we've brainstormed. @reaperhulk wdyt?

@reaperhulk
Copy link
Member

Hmm, what does this look like when applied to the basic real world example of a generalname? (I would actually do this myself but am currently mobile)

@facutuesca
Copy link
Contributor Author

Hmm, what does this look like when applied to the basic real world example of a generalname? (I would actually do this myself but am currently mobile)

Something like this (assuming the non-trivial types are defined elsewhere):

#   GeneralName ::= CHOICE {
#        otherName                       [0]     OtherName,
#        rfc822Name                      [1]     IA5String,
#        dNSName                         [2]     IA5String,
#        x400Address                     [3]     ORAddress,
#        directoryName                   [4]     Name,
#        ediPartyName                    [5]     EDIPartyName,
#        uniformResourceIdentifier       [6]     IA5String,
#        iPAddress                       [7]     OCTET STRING,
#        registeredID                    [8]     OBJECT IDENTIFIER }

GeneralName: typing.TypeAlias = (
    Annotated[OtherName, asn1.Implicit(0)]
    | Annotated[
        asn1.Variant[asn1.IA5String, typing.Literal["rfc822Name"]],
        asn1.Implicit(1),
    ]
    | Annotated[
        asn1.Variant[asn1.IA5String, typing.Literal["dNSName"]],
        asn1.Implicit(2),
    ]
    | Annotated[ORAddress, asn1.Implicit(3)]
    | Annotated[Name, asn1.Implicit(4)]
    | Annotated[EDIPartyName, asn1.Implicit(5)]
    | Annotated[
        asn1.Variant[
            asn1.IA5String, typing.Literal["uniformResourceIdentifier"]
        ],
        asn1.Implicit(6),
    ]
    | Annotated[bytes, asn1.Implicit(7)]
    | Annotated[x509.ObjectIdentifier, asn1.Implicit(8)]
)

@asn1.sequence
class Example:
    name: GeneralName

obj = Example(name=asn1.Variant(asn1.IA5String("abcd"), "dNSName"))
encoded = encode_der(obj)

decoded = decode(Example, encoded)
assert isinstance(decoded.name.value, asn1.IA5String)
assert decoded.name.tag == "dNSName"
assert decoded.name.value.as_str() == "abcd"

@facutuesca
Copy link
Contributor Author

facutuesca commented Jan 25, 2026

I wonder if maybe we should require the use of Variant with a tag for all the possible types? Makes the API more uniform, both when declaring the types but also when the user needs to get the type and value from a decoded variant. Downside is that it's more verbose.

# current API, this would be the `iPAddress [7] OCTET STRING` variant
obj = Example(name=b"\x01\x02")
encoded = encode_der(obj)

decoded = decode(Example, encoded)
assert isinstance(decoded.name, bytes)
assert decoded.name == b"\x01\x02"


# API where every variant is defined with a `Variant(type, tag)` annotation
GeneralName: typing.TypeAlias = (
#...
    | Annotated[
        asn1.Variant[bytes, typing.Literal["iPAddress"]],
        asn1.Implicit(7),
    ]
# ...

obj = Example(name=asn1.Variant(b"\x01\x02", "iPAddress"))
encoded = encode_der(obj)

decoded = decode(Example, encoded)

assert isinstance(decoded.name.value, bytes)
assert decoded.name.tag == "ipAddress"
assert decoded.name.value == b"\x01\x02"

@alex
Copy link
Member

alex commented Jan 25, 2026

Either all variant or no-variant perhaps.

@facutuesca
Copy link
Contributor Author

Either all variant or no-variant perhaps.

how would no-variant work?

@alex
Copy link
Member

alex commented Jan 25, 2026

Would be fine for cases where all the variants have different types, and therefore don't need anything to disambiguate them.

@facutuesca
Copy link
Contributor Author

Would be fine for cases where all the variants have different types, and therefore don't need anything to disambiguate them.

Done, I also updated the PR description with the changes.

Comment on lines 44 to 67
Tag = typing.TypeVar("Tag")


@dataclasses.dataclass(frozen=True)
class Variant(typing.Generic[U, Tag]):
"""
A tagged variant for CHOICE fields with the same underlying type.

Use this when you have multiple CHOICE alternatives with the same type
and need to distinguish between them:

foo: (
Annotated[Variant[int, "IntA"], Implicit(0)]
| Annotated[Variant[int, "IntB"], Implicit(1)]
)

Usage:
example = Example(foo=Variant(5, "IntA"))
decoded.foo.value # The int value
decoded.foo.tag # "IntA" or "IntB"
"""

value: U
tag: str
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Tag either need to be annotated to indicate its a Literal[str], or should we just allow arbitrary tag types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the docstring is wrong, fixed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question was more about the Tag TypeVar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you annotate it? Literal[str] is not a valid expression

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure! The answer is maybe that this is all correct -- but I was wondering if the Tag typevar should have a bound of some sort on it?

Copy link
Contributor Author

@facutuesca facutuesca Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add a bound of LiteralString. It's not perfect, str and any subclass of str will pass the type check if passed as a Tag, but we can catch those at runtime.

So something like:

Tag = typing.TypeVar("Tag", bound=typing.LiteralString)

@dataclasses.dataclass(frozen=True)
class Variant(typing.Generic[U, Tag]):

and we keep the runtime check for Tag being a typing.Literal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit with this ^ change

Comment on lines 216 to 219
let expected_tags = expected_tags_for_type(py, inner, encoding);
match parser.peek_tag() {
Some(next_tag) if expected_tags.contains(&next_tag) => (),
_ => return Ok(default.clone_ref(py).into_bound(py)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems bad, in that it's going to result in tons of allocations, can we instead have an is_tag_valid_for_type or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah good catch, fixed

Comment on lines 610 to 612
assert_roundtrips([(Example(foo=9), b"\x30\x03\x02\x01\x09")])
assert_roundtrips([(Example(foo=True), b"\x30\x03\x01\x01\xff")])
assert_roundtrips([(Example(foo="a"), b"\x30\x03\x0c\x01a")])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why three cals to assert_roundtrips, rather than passing a list to the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

facutuesca and others added 6 commits January 27, 2026 19:25
Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
Comment on lines 44 to 67
Tag = typing.TypeVar("Tag")


@dataclasses.dataclass(frozen=True)
class Variant(typing.Generic[U, Tag]):
"""
A tagged variant for CHOICE fields with the same underlying type.

Use this when you have multiple CHOICE alternatives with the same type
and need to distinguish between them:

foo: (
Annotated[Variant[int, "IntA"], Implicit(0)]
| Annotated[Variant[int, "IntB"], Implicit(1)]
)

Usage:
example = Example(foo=Variant(5, "IntA"))
decoded.foo.value # The int value
decoded.foo.tag # "IntA" or "IntB"
"""

value: U
tag: str
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question was more about the Tag TypeVar.

};
// Parse the TLV data (which contains the field without the EXPLICIT
// wrapper)
asn1::parse(tlv.full_data(), |d| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tlv.parse()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tlv.parse() requires knowing the type at compile time:

    pub fn parse<T: Asn1Readable<'a>>(&self) -> ParseResult<T> {

mod tests {
use crate::declarative_asn1::types::{AnnotatedType, Annotation, Encoding, Type, Variant};
#[test]
fn test_decode_implicit_choice() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, why do we need a rust test case for this? Not reachable from the PYthon API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we fail first at the Python level if someone annotates a CHOICE with IMPLICIT


match encoding {
// Utility function to see if a tag matches an unnanotated variant.
pub(crate) fn is_tag_valid_for_variant(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you refactor this out into a seperate PR for ease of review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, see #14231

Signed-off-by: Facundo Tuesca <[email protected]>
@facutuesca facutuesca changed the title asn1: Add support for CHOICE fields (alternative impl) asn1: Add support for CHOICE fields Jan 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants