ref struct ASN.1 Decoder type generation#125389
Conversation
This reverts commit 5ac23c6f5c1bb7c8613f85bb1a3dde364e64f61b.
Extend asn.xslt to generate decode-only ref partial struct variants (Value* prefix) for Sequence and Choice types, working on ReadOnlySpan<byte> via ValueAsnReader instead of ReadOnlyMemory<byte>. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ards Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tructs Generate duck-typed ref struct enumerables for collections that opt in via valueName attribute. Test with CertificationRequestInfoAsn Attributes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Support emitType='struct' (default), 'ref', or 'both' on Sequence and Choice XML elements. Existing XML files without the attribute emit only the struct, preserving pre-branch behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR is a proof-of-concept update to the ASN.1 asnxml/XSLT code generator to optionally emit span-based ref struct decoders (Value* types) alongside the existing struct decoders, and it migrates PublicKey and CertificateRequest parsing to use the new Value* decoders to avoid PointerMemoryManager.
Changes:
- Extend
asn.xslt+ schema annotations (emitType,valueTypeName,valueName) to generateValue*ref structdecoders and lazy collection enumerators. - Refactor generated “default DER init” constants into
file static class Shared*helpers so both struct andref structdecoders can share defaults. - Switch
PublicKeyandCertificateRequest.Loaddecoding paths to useValueSubjectPublicKeyInfoAsn,ValueCertificationRequestInfoAsn, etc.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/PublicKey.cs | Switch SPKI decode to ValueSubjectPublicKeyInfoAsn and remove pointer-based memory manager usage. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificateRequest.Load.cs | Switch PKCS#10 parsing to Value* decoders and lazy attribute/value enumeration. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/Asn1/TbsCertificateAsn.xml.cs | Move default DER init to SharedTbsCertificateAsn. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/Asn1/CertificationRequestInfoAsn.xml.cs | Add ValueCertificationRequestInfoAsn with span fields + lazy GetAttributes. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/Asn1/CertificationRequestInfoAsn.xml | Enable emitType="both" and map nested types via valueTypeName / valueName. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/Asn1/BasicConstraintsAsn.xml.cs | Move default DER init to SharedBasicConstraintsAsn. |
| src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/Rfc3161TstInfo.xml.cs | Move defaults to SharedRfc3161TstInfo and add ValueRfc3161TstInfo. |
| src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/Rfc3161TstInfo.xml | Enable emitType="both" and map MessageImprint via valueTypeName. |
| src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/Rfc3161TimeStampReq.xml.cs | Move default DER init to SharedRfc3161TimeStampReq. |
| src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/MessageImprint.xml.cs | Add ValueMessageImprint span-based decoder. |
| src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/MessageImprint.xml | Enable emitType="both". |
| src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/EssCertIdV2.xml.cs | Move default DER init to SharedEssCertIdV2. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/asn.xslt | Core generator changes: emitType, Value* decoding, Shared defaults, lazy collection enumerators. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/X509ExtensionAsn.xml.cs | Move defaults to SharedX509ExtensionAsn and add ValueX509ExtensionAsn. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/X509ExtensionAsn.xml | Enable emitType="both". |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/SubjectPublicKeyInfoAsn.xml.cs | Add ValueSubjectPublicKeyInfoAsn. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/SubjectPublicKeyInfoAsn.xml | Enable emitType="both" and map Algorithm via valueTypeName. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/PssParamsAsn.xml.cs | Move defaults to SharedPssParamsAsn and add ValuePssParamsAsn. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/PssParamsAsn.xml | Enable emitType="both" and map nested alg IDs via valueTypeName. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/PssParamsAsn.manual.cs | Add ValuePssParamsAsn.GetSignaturePadding helper. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/Pkcs12/MacData.xml.cs | Move default DER init to SharedMacData. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/Pbkdf2Params.xml.cs | Move default DER init to SharedPbkdf2Params. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/OaepParamsAsn.xml.cs | Move default DER init to SharedOaepParamsAsn. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/GeneralSubtreeAsn.xml.cs | Move default DER init to SharedGeneralSubtreeAsn. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/AttributeAsn.xml.cs | Add ValueAttributeAsn span-based decoder + lazy GetAttrValues. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/AttributeAsn.xml | Enable emitType="both" and name the value enumerator via valueName. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/AlgorithmIdentifierAsn.xml.cs | Add ValueAlgorithmIdentifierAsn span-based decoder. |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/AlgorithmIdentifierAsn.xml | Enable emitType="both". |
| src/libraries/Common/src/System/Security/Cryptography/Asn1/AlgorithmIdentifierAsn.manual.cs | Add ValueAlgorithmIdentifierAsn helpers (Equals, null-parameter checks). |
Comments suppressed due to low confidence (1)
src/libraries/Common/src/System/Security/Cryptography/Asn1/asn.xslt:1519
- Same as the typed enumerator above: this
Enumeratorconstructor only assigns_readerwhenencodedis non-empty. Initialize_readeron all paths (e.g.,_reader = default;before theif) to avoid a definite-assignment compile error in generated code.
internal ref struct Enumerator
{
private ValueAsnReader _reader;
private ReadOnlySpan<byte> _current;
internal Enumerator(ReadOnlySpan<byte> encoded, AsnEncodingRules ruleSet)
{
if (!encoded.IsEmpty)
{
ValueAsnReader outerReader = new ValueAsnReader(encoded, ruleSet);
_reader = outerReader.Read<xsl:value-of select="$collNoun"/>(<xsl:call-template name="MaybeImplicitCall0"/>);
}
_current = default;
}
You can also share your feedback on Copilot code review. Take the survey.
...ty.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificateRequest.Load.cs
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/Asn1/AlgorithmIdentifierAsn.manual.cs
Show resolved
Hide resolved
...hy/src/System/Security/Cryptography/X509Certificates/Asn1/CertificationRequestInfoAsn.xml.cs
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/Asn1/AttributeAsn.xml.cs
Show resolved
Hide resolved
.../System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/PublicKey.cs
Outdated
Show resolved
Hide resolved
...ty.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificateRequest.Load.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/Asn1/asn.xslt
Outdated
Show resolved
Hide resolved
…/Cryptography/X509Certificates/PublicKey.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/libraries/Common/src/System/Security/Cryptography/Asn1/asn.xslt:1516
- Same definite-assignment issue here:
_readeris only assigned inside theif (!encoded.IsEmpty)block. Ensure_readeris assigned on all paths (e.g.,_reader = defaultat the start of the constructor).
internal Enumerator(ReadOnlySpan<byte> encoded, AsnEncodingRules ruleSet)
{
if (!encoded.IsEmpty)
{
ValueAsnReader outerReader = new ValueAsnReader(encoded, ruleSet);
_reader = outerReader.Read<xsl:value-of select="$collNoun"/>(<xsl:call-template name="MaybeImplicitCall0"/>);
}
You can also share your feedback on Copilot code review. Take the survey.
.../System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/PublicKey.cs
Outdated
Show resolved
Hide resolved
...ty.Cryptography/src/System/Security/Cryptography/X509Certificates/CertificateRequest.Load.cs
Show resolved
Hide resolved
bartonjs
left a comment
There was a problem hiding this comment.
Nice. Your treating it as a separate mode and just duplicating everything has drawbacks (more xslt to maintain), but overcame a lot of the limitations I ran into when doing this previously (and trying to just be one or the other).
CertificateRequest.Load.cs and PublicKey.cs both generally look good.
OTOH, the work CertificateRequest does before enumerating represents more throwaway allocatey work... on the other, you've gotten rid of allocating the lists on success. So, seems like a net positive. (Slightly sad to lose Length, but, oh well...)
src/libraries/Common/src/System/Security/Cryptography/Asn1/asn.xslt
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/Asn1/asn.xslt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
src/libraries/Common/src/System/Security/Cryptography/Asn1/AlgorithmIdentifierAsn.manual.cs
Show resolved
Hide resolved
...hy/src/System/Security/Cryptography/X509Certificates/Asn1/CertificationRequestInfoAsn.xml.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
Some explanation to the changes
The XSLT now has an
emitTypeto define whether astruct,ref struct, or both are emitted. The default is just the struct as it is today.The "default DER init" moved to its own
file-scoped type in the generated file so that both theref structand thestructcan share the default init fields. This keeps source file and assembly size down.The
struct-based emit is largely unchanged, except for using thefiledefault DER init.The
ref structimplementation basically does what you would think. Since it is a ref struct, it prefers to work over spans, and allows us to expose things likeReadOnlySpan<byte>of contents.To support ref structs referring to ref structs, there is a new field called
valueTypeNamethat can be applied toasn:AsnTypewhich the ref struct emitter will use. If it is not present, then it is decoded as raw bytes.Since
ReadOnlySpan<byte>cannot beNullable<T>, optional types have a correspondingHas-prefixed boolean field. LikeHasParametersfor a field calledParameters.The trickiest part was how to handle collections. We can't have arrays, lists, etc. of
ReadOnlySpan<byte>. Instead, the new emitter creates aGet<Name>method that returns a ref struct enumerator that on-the-fly decodes. This is a slight behavior deviation from thestructbased one that eagerly decodes things in to an array. So exceptions may not happen during the call toDecodeand will be deferred to the enumeration.To demonstrate the utility of this, I changed two existing places to use the new
ref structapproach.PublicKey, which is aSubjectPublicKeyInfoAsntoValueSubjectPublicKeyInfoAsnCertificateRequest, and all of its supporting types. This one is more complex because it involves collections, etc.These changes were largely successful, but unfortunately resulted in some duplication of "helpers" for now which may or may not go away if we move more things to the ref struct.
The overall result is positive: No more use of
PointerMemoryManagerin these two locations.Support for
Encodeis not yet implemented. That can be a follow up if it is deemed worthwhile.