Skip to content

[cdac] Convert TypeHandle to the ITypeHandle interface#129800

Merged
max-charlamb merged 23 commits into
mainfrom
dev/max-charlamb/rts-synthetic-typehandles
Jul 21, 2026
Merged

[cdac] Convert TypeHandle to the ITypeHandle interface#129800
max-charlamb merged 23 commits into
mainfrom
dev/max-charlamb/rts-synthetic-typehandles

Conversation

@max-charlamb

@max-charlamb max-charlamb commented Jun 24, 2026

Copy link
Copy Markdown
Member

Summary

Converts the cDAC TypeHandle value type into an ITypeHandle interface with a canonical, target-backed implementation (TargetTypeHandle). This is a foundational refactor; synthetic (reader-fabricated) type handles for unloaded constructed types are deferred to a follow-up PR.

Absent or unresolved types use standard C# nullable-reference semantics (ITypeHandle? / null). Non-null handles are canonical identities produced and interned by RuntimeTypeSystem within a target cache epoch.

Motivation

This change is primarily enabling groundwork. Turning TypeHandle into an interface lets the Runtime Type System (RTS) contract grow in two directions that the current value-type shape cannot accommodate:

  • Versioning -- different contract versions can supply their own ITypeHandle implementations without changing every consumer when the representation evolves.
  • More expansive RTS support -- most immediately, synthetic handles for constructed types (Ptr/Byref/SzArray/Array, and later generic instantiations) that are not loaded in the target. A polymorphic ITypeHandle lets RTS fabricate a queryable handle instead of relying on ad-hoc side channels.

Behavior note

The old struct compared by address value. The new handles use canonical reference identity: repeated GetTypeHandle calls for the same address return the same object until Target.Flush invalidates the cache. Resolving the same address after a flush may return a different object reference. This identity change is intentional and prevents future handle implementations from having to coordinate cross-implementation value equality.

Changes

  • Rename TypeHandle -> ITypeHandle across cDAC type references (members named TypeHandle remain unchanged).
  • Add the ITypeHandle interface in Abstractions (Address).
  • Add the internal TargetTypeHandle implementation for real target-backed handles.
  • Intern target handles per address so repeated GetTypeHandle calls return the same canonical instance within a cache epoch.
  • Use reference identity for constructed-type cache keys.
  • Represent absent/unresolved handles with nullable references and annotate the affected contracts and consumers.
  • Preserve the existing ReadOnlySpan collection API shapes.
  • Update the authoritative data-contract documentation.

Why split it this way

The original draft also added synthetic handles and reworked calling-convention side channels in one change. Splitting out the interface conversion keeps this PR focused; synthetic-handle behavior and its downstream simplifications can build on the stable ITypeHandle abstraction in a follow-up.

Testing

  • Contracts and Legacy build cleanly.
  • All 2733 cDAC unit tests pass (16 skipped).
  • All 34 DataGenerator tests pass.
  • DumpTests and StressTests compile.

Note

This PR description and the changes were produced with the assistance of GitHub Copilot.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors cDAC’s type identity from a concrete TypeHandle struct to an ITypeHandle abstraction, enabling “synthetic” (reader-fabricated) handles for unloaded constructed types (Ptr/Byref/SZArray/Array) and updating the contracts/legacy layers/tests accordingly.

Changes:

  • Introduces ITypeHandle (+ null/synthetic/target-backed implementations) and updates contracts to traffic in ITypeHandle.
  • Updates RuntimeTypeSystem_1.GetConstructedType to synthesize constructed types on miss for Ptr/Byref/SZArray/Array.
  • Updates legacy SOS/DacDbi code and cDAC unit/dump tests to consume the new handle abstraction.

Reviewed changes

Copilot reviewed 52 out of 52 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/native/managed/cdac/tests/UnitTests/TypeDescTests.cs Updates unit tests to use ITypeHandle and array-returning APIs.
src/native/managed/cdac/tests/UnitTests/SOSDacInterface5Tests.cs Switches test setup to TargetTypeHandle.
src/native/managed/cdac/tests/UnitTests/RuntimeMutableTypeSystemTests.cs Updates Moq setups and APIs to ITypeHandle.
src/native/managed/cdac/tests/UnitTests/ObjectTests.cs Updates object contract tests to use TargetTypeHandle.
src/native/managed/cdac/tests/UnitTests/MethodTableTests.cs Replaces TypeHandle usage with ITypeHandle across MT tests.
src/native/managed/cdac/tests/UnitTests/MethodDescTests.cs Updates generic-instantiation test assertions to use ITypeHandle[].
src/native/managed/cdac/tests/UnitTests/ExceptionTests.cs Updates exception contract tests to use TargetTypeHandle.
src/native/managed/cdac/tests/UnitTests/DacDbiImplTests.cs Updates DacDbi tests to use TargetTypeHandle and new APIs.
src/native/managed/cdac/tests/UnitTests/CodeVersionsTests.cs Updates code versions test helpers to use ITypeHandle.
src/native/managed/cdac/tests/DumpTests/RuntimeTypeSystemDumpTests.cs Updates dump tests to the ITypeHandle API surface.
src/native/managed/cdac/tests/DumpTests/ObjectiveCMarshalDumpTests.cs Updates dump test to ITypeHandle.
src/native/managed/cdac/tests/DumpTests/IXCLRDataValueDumpTests.cs Updates commentary/type references for ITypeHandle.
src/native/managed/cdac/tests/DumpTests/IXCLRDataMethodDefinitionDumpTests.cs Updates dump test to ITypeHandle.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiObjectDumpTests.cs Updates DacDbi dump tests and helpers to ITypeHandle.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs Updates DacDbi loader dump test to ITypeHandle.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiExactTypeHandleDumpTests.cs Updates exact-typehandle roundtrip dump test to ITypeHandle.
src/native/managed/cdac/tests/DumpTests/CCWDumpTests.cs Updates CCW dump test wording and ITypeHandle usage.
src/native/managed/cdac/tests/DumpTests/AsyncContinuationDumpTests.cs Updates continuation dump tests to ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/TypeNameBuilder.cs Updates type/method name formatting to consume ITypeHandle and array-returning instantiation APIs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs Updates IXCLRDataProcess implementation to ITypeHandle and array-returning APIs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs Updates SOS DAC implementation to ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SigFormat.cs Updates signature formatting helpers to use ITypeHandle and array-returning APIs.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/IDacDbiInterface.cs Updates interop struct comments to refer to ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/Helpers/HeapWalk.cs Updates heap-walk helper to use ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs Updates DacDbi implementation to use ITypeHandle and constructed-type synthesis.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodInstance.cs Updates method instance implementation to use ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs Updates generic-instantiation checks to ITypeHandle[].
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataFrame.cs Updates frame/type resolution paths to use ITypeHandle and array-returning instantiations.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/ExtensionMethods.cs Adapts type-handle helper extensions to operate on ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/GC/GcSignatureTypeProvider.cs Updates GC signature decoding context and lookups to ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/FrameHandling/FrameHelpers.cs Updates frame helper logic to use ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Signature/SignatureTypeProvider.cs Updates signature type provider to produce/consume ITypeHandle and constructed types.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Signature/Signature_1.cs Updates signature contract implementation to decode to ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Signature/IRuntimeSignatureTypeProvider.cs Updates documentation for “internal type” handling to ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem/TypeHandleImplementations.cs Adds TargetTypeHandle and SyntheticTypeHandle implementations of ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs Core RuntimeTypeSystem changes: ITypeHandle plumbing + synthetic constructed-type behavior.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeMutableTypeSystem_1.cs Updates mutable type system APIs to accept ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Object_1.cs Updates object sizing/array logic to use ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ManagedTypeSource_1.cs Updates managed type resolution/caching to ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerCore.cs Updates exception-clause reading path to use ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Exception_1.cs Updates exception contract implementation to use ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ConditionalWeakTable_1.cs Updates CWT contract implementation to use ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ComWrappers_1.cs Updates RCW detection to use the renamed generated type-handle accessor.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CodeVersions_1.cs Updates code-versions contract to use ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ISignature.cs Changes signature decoding contract to return ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs Introduces ITypeHandle/NullTypeHandle and updates the RuntimeTypeSystem contract signatures.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeMutableTypeSystem.cs Updates mutable-type-system contract signature to use ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IManagedTypeSource.cs Updates managed type source contract to use ITypeHandle.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/CdacAttributes.cs Updates generator-related docs to refer to ITypeHandle.
src/native/managed/cdac/gen/TypeNameResolverSource.cs Updates generated helper to return ITypeHandle.
src/native/managed/cdac/gen/Emitter.cs Updates generated output to reference ITypeHandle.
src/native/managed/cdac/gen/CdacGenerator.cs Updates generator docs to refer to ITypeHandle.

@max-charlamb
max-charlamb force-pushed the dev/max-charlamb/rts-synthetic-typehandles branch from 1c85305 to 2050b89 Compare June 24, 2026 14:50
@rcj1

rcj1 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

ad-hoc side-channels

Am I missing something, or where are you removing said side channels with this abstraction? If this is a helper for a PR that you have in progress, what does it look like before and after?

@max-charlamb

Copy link
Copy Markdown
Member Author

ad-hoc side-channels

Am I missing something, or where are you removing said side channels with this abstraction? If this is a helper for a PR that you have in progress, what does it look like before and after?

I needed extra data for implementing the ArgIterator: https://github.com/dotnet/runtime/pull/129769/changes#r3468318817

I believe adding synthetic typehandles aligns with our long-term goal to not have to load things in the runtime just for diagnostics.

@rcj1

rcj1 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

ad-hoc side-channels

Am I missing something, or where are you removing said side channels with this abstraction? If this is a helper for a PR that you have in progress, what does it look like before and after?

I needed extra data for implementing the ArgIterator: https://github.com/dotnet/runtime/pull/129769/changes#r3468318817

I believe adding synthetic typehandles aligns with our long-term goal to not have to load things in the runtime just for diagnostics.

Could we accomplish this by adding (optional) fields and properties to the TypeHandle struct?

@max-charlamb

Copy link
Copy Markdown
Member Author

ad-hoc side-channels

Am I missing something, or where are you removing said side channels with this abstraction? If this is a helper for a PR that you have in progress, what does it look like before and after?

I needed extra data for implementing the ArgIterator: https://github.com/dotnet/runtime/pull/129769/changes#r3468318817
I believe adding synthetic typehandles aligns with our long-term goal to not have to load things in the runtime just for diagnostics.

Could we accomplish this by adding (optional) fields and properties to the TypeHandle struct?

The TypeHandle was always meant to be an opaque handle, moving to an interface better accomplishes that and matches the pattern in other contracts.

// an opaque handle to a type handle. See IMetadata.GetMethodTableData
public readonly struct TypeHandle
{
// TODO-Layering: These members should be accessible only to contract implementations.
public TypeHandle(TargetPointer address)
{
Address = address;
}
public TargetPointer Address { get; }
public bool IsNull => Address == 0;
}

Copilot AI review requested due to automatic review settings June 24, 2026 16:03
@max-charlamb
max-charlamb force-pushed the dev/max-charlamb/rts-synthetic-typehandles branch from 2050b89 to 6522cf8 Compare June 24, 2026 16:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 54 out of 54 changed files in this pull request and generated 6 comments.

@max-charlamb
max-charlamb force-pushed the dev/max-charlamb/rts-synthetic-typehandles branch from 6522cf8 to 791163a Compare June 24, 2026 19:40
@rcj1

rcj1 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

The TypeHandle was always meant to be an opaque handle

Ok, sounds good

Copilot AI review requested due to automatic review settings July 1, 2026 17:17
@max-charlamb
max-charlamb force-pushed the dev/max-charlamb/rts-synthetic-typehandles branch from 791163a to ee5fa4e Compare July 1, 2026 17:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 60 out of 60 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CallingConvention/CallingConvention_1.cs:245

  • Using Address==TargetPointer.Null to detect “null handle” is no longer equivalent now that synthetic handles have Address==0 but IsNull==false. This should use IsNull (and preferably avoid Address comparisons for ITypeHandle entirely) to prevent misclassifying synthetic handles as missing/uncached handles.
                    ITypeHandle probe = methodSig.ParameterTypes[argIndex];
                    if (probe.Address == TargetPointer.Null)
                        probe = paramInfo[argIndex].OpenGenericType;
                    if (probe.Address != TargetPointer.Null)
                    {

Comment thread src/native/managed/cdac/tests/DumpTests/ExceptionHandlingInfoDumpTests.cs Outdated
Comment thread src/native/managed/cdac/gen/Emitter.cs
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copilot AI review requested due to automatic review settings July 1, 2026 19:26
@max-charlamb
max-charlamb force-pushed the dev/max-charlamb/rts-synthetic-typehandles branch from c6446df to 5c47052 Compare July 1, 2026 19:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 60 out of 60 changed files in this pull request and generated 6 comments.

Comment thread src/native/managed/cdac/tests/DumpTests/ExceptionHandlingInfoDumpTests.cs Outdated
Comment thread src/native/managed/cdac/gen/Emitter.cs
Max Charlamb and others added 10 commits July 20, 2026 13:18
- Describe ELEMENT_TYPE_INTERNAL payloads as native runtime TypeHandles,
  which are resolved to cDAC ITypeHandles by the reader.
- Restore native VMPTR_TypeHandle names in the DacDbi wire-layout comments.
- Use explicit ImmutableArray<ITypeHandle> locals instead of var.
- Remove redundant Contracts. qualifiers where the Contracts namespace is
  already imported.
- Fix remaining "a ITypeHandle" grammar.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c
Keep the RuntimeTypeSystem collection API shape unchanged by using
ReadOnlySpan<ITypeHandle> over cached ITypeHandle arrays. This removes the
unnecessary ImmutableArray API churn and builder allocations from the
TypeHandle-to-ITypeHandle conversion.

Clear ManagedTypeSource's cached ITypeHandle values on every flush because
RuntimeTypeSystem invalidates its canonical handle instances. Continue to
retain immutable CoreLib type layouts and field descriptors across forward
execution. Add dump-test coverage for canonical handle resolution after a
forward flush.

Also correct the remaining ITypeHandle documentation and native TypeHandle
terminology.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c
- Default ArgumentLocation handle properties to ITypeHandle.Null so omitted
  object-initializer members preserve the old zero-handle semantics.
- Compare TargetTypeHandle addresses with TargetPointer.Null.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c
Use the explicit TargetPointer null sentinel for type-handle address checks
and the TypeDescAddress failure result.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c
Remove the stale System.Collections.Immutable using left after restoring
ReadOnlySpan<ITypeHandle> in TypeDescTests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c
Use nullable backing fields with null-coalescing getters for TypeHandle and
OpenGenericType. This preserves the old zero-valued TypeHandle behavior for
new and default(ArgumentLocation), including zero-initialized arrays, without
requiring an explicit parameterless struct constructor.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c
Keep ArgumentLocation as a plain readonly struct; its control-flow-specific
consumers do not read omitted handle properties. Replace the remaining
PR-added var declarations with explicit ITypeHandle/span types.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c
Use an explicit struct constructor with ITypeHandle.Null property initializers
so object initializers that omit TypeHandle or OpenGenericType preserve the old
zero-valued TypeHandle semantics.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c
Document that ELEMENT_TYPE_INTERNAL and ELEMENT_TYPE_CMOD_INTERNAL contain
raw runtime TypeHandle pointers that the provider resolves to cDAC types.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated 1 comment.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holistic Review

Motivation: Unchanged from the initial review. The PR replaces the cDAC TypeHandle value struct with an ITypeHandle interface backed by TargetTypeHandle (an address-backed handle) and a NullTypeHandle singleton exposed via ITypeHandle.Null, laying groundwork for modeling handles not backed by a target address. The single new commit only refines a doc comment and does not alter the motivation.

Approach: Unchanged. IRuntimeTypeSystem and consumers switch every TypeHandle to ITypeHandle; RuntimeTypeSystem_1 interns TargetTypeHandle per address and moves TypeKey to reference identity; ManagedTypeSource_1 clears its handle cache on every flush; struct holders (ArgumentLocation, CdacTypeHandle) now hold reference fields. The new commit (#17, 2e2d66c) edits only two XML doc comments in IRuntimeSignatureTypeProvider.cs, clarifying that ELEMENT_TYPE_INTERNAL/ELEMENT_TYPE_CMOD_INTERNAL carry raw runtime TypeHandle pointers that the provider resolves to cDAC types. This is a documentation-only clarification with no behavioral or API effect.

Summary: ⚠️ Needs Human Review (unchanged). The incremental change since the prior review is a pure doc-comment edit and introduces no new findings. The two previously identified semantic subtleties from the value→reference conversion — the reference-identity TypeKey interning invariant and the latent NRE risk for a default-constructed CdacTypeHandle — remain the reasons this PR warrants a maintainer's confirmation. Those live in code unchanged by the new commit, so per re-review scope they are not re-raised as new actionable findings here; see the initial review for their details.


Assessment History

  • review 4730716240 reviewed commit 07d5884 with verdict Needs Human Review. Current verdict: Needs Human Review. Unchanged — the only new commit (2e2d66c) is a documentation-only clarification of two doc comments in IRuntimeSignatureTypeProvider.cs, leaving motivation, approach, and risk identical.

Detailed Findings

  • No new actionable findings. The incremental diff (07d5884..2e2d66c) touches only XML documentation comments and the wording is accurate: these element types do embed raw runtime TypeHandle pointers that the type provider resolves. No correctness, performance, or API concerns arise from the change.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 52.4 AIC · ⌖ 10.7 AIC · ⊞ 10K

Document that unsupported type parameters return ITypeHandle.Null rather than
a null reference.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CallingConvention/CallingConvention_1.cs:464

  • ParamMetadataProvider produces TrackedType values where OpenGeneric is left unset for most element types (Wrap / byref / ptr / array). Since OpenGeneric is an ITypeHandle reference, that means it can be a null reference at runtime. DecodeParamTypeInfo then copies this into ParamTypeInfo.OpenGenericType, and GetArgumentLayout assigns it into ArgumentLocation.OpenGenericType, overwriting the = ITypeHandle.Null default on ArgumentLocation. This reintroduces the risk of NullReferenceException if any later code reads arg.OpenGenericType.Address on an entry where the closed handle is null/uncached.

To preserve the old default(TypeHandle) semantics and the intended non-nullable contract, ensure TrackedType.OpenGeneric is always at least ITypeHandle.Null and propagate it through wrappers.

        private static TrackedType Wrap(ITypeHandle th)
            => new() { Underlying = th };

        public TrackedType GetByReferenceType(TrackedType elementType)
            => new() { Underlying = elementType.Underlying, IsByRef = true,
                       OutermostKind = CdacCorElementType.Byref };

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holistic Review

Motivation: Unchanged from prior reviews. The PR replaces the cDAC TypeHandle value struct with an ITypeHandle interface backed by TargetTypeHandle (an address-backed handle) and a NullTypeHandle singleton exposed via ITypeHandle.Null, laying groundwork for modeling handles not backed by a target address. The single new commit only refines a code comment and does not alter the motivation.

Approach: Unchanged. IRuntimeTypeSystem and consumers switch every TypeHandle to ITypeHandle; RuntimeTypeSystem_1 interns TargetTypeHandle per address and moves TypeKey to reference identity; ManagedTypeSource_1 clears its handle cache on every flush; struct holders (ArgumentLocation, CdacTypeHandle) now hold reference fields. The new commit (#18, b67eb50) edits a single trailing comment on the ITypeHandle inspection region in IRuntimeTypeSystem.cs, changing "Returns null if the ITypeHandle is not a class/struct/generic variable" to "Returns ITypeHandle.Null if the handle is not a class, struct, or generic variable." This is a documentation-only clarification with no behavioral or API effect.

Summary: ⚠️ Needs Human Review (unchanged). The incremental change since the prior review is a pure comment edit and introduces no new findings. It is also accurate and consistent with the refactor: the relevant inspection APIs (e.g. GetTypeParam) now return ITypeHandle.Null rather than a null reference, so the comment now correctly names the sentinel. The two previously identified semantic subtleties from the value→reference conversion — the reference-identity TypeKey interning invariant and the latent NRE risk for a default-constructed CdacTypeHandle — remain the reasons this PR warrants a maintainer's confirmation. Those live in code unchanged by the new commit, so per re-review scope they are not re-raised as new actionable findings here; see the initial review for their details.


Assessment History

  • review 4730716240 reviewed commit 07d5884 with verdict Needs Human Review. Current verdict: Needs Human Review. Unchanged — the new commit is a documentation-only comment clarification, leaving motivation, approach, and risk identical.
  • review 4737835307 reviewed commit 2e2d66c with verdict Needs Human Review. Current verdict: Needs Human Review. Unchanged — the only new commit (b67eb50) edits one comment in IRuntimeTypeSystem.cs to name the ITypeHandle.Null sentinel, with no behavioral, API, or risk change.

Detailed Findings

  • No new actionable findings. The incremental diff (2e2d66c..b67eb50) touches only a single source comment on the ITypeHandle inspection region, and the revised wording accurately reflects the refactor's null-sentinel semantics. No correctness, performance, or API concerns arise from the change.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 60.7 AIC · ⌖ 16 AIC · ⊞ 10K

@noahfalk noahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think its fine to check this in and unblock yourself, but I think its worth considering if using reference null to implement a null type would be better than a non-null sentinel object.

Comment thread docs/design/datacontracts/ManagedTypeSource.md Outdated
Comment thread docs/design/datacontracts/RuntimeTypeSystem.md Outdated
Comment thread docs/design/datacontracts/RuntimeTypeSystem.md Outdated
Comment thread docs/design/datacontracts/Signature.md Outdated
max-charlamb and others added 2 commits July 20, 2026 22:52
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CallingConvention/CallingConvention_1.cs:463

  • TrackedType.OpenGeneric/ParamTypeInfo.OpenGenericType are non-nullable, but most TrackedType factories (e.g., Wrap, GetByReferenceType, GetPointerType, array helpers) never set OpenGeneric. Because ITypeHandle is a reference type, that means OpenGeneric can be a null reference at runtime (unlike the old TypeHandle struct default), which then flows into ParamTypeInfo.OpenGenericType and ArgumentLocation.OpenGenericType and can cause NullReferenceException if any code reads .Address/.IsNull on it. Initialize OpenGeneric to ITypeHandle.Null by default and propagate it through wrapper constructors.
        private static TrackedType Wrap(ITypeHandle th)
            => new() { Underlying = th };

        public TrackedType GetByReferenceType(TrackedType elementType)
            => new() { Underlying = elementType.Underlying, IsByRef = true,

Comment thread docs/design/datacontracts/Signature.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CallingConvention/CallingConvention_1.cs:464

  • TrackedType.OpenGeneric is a non-nullable ITypeHandle, but most TrackedType values are created via Wrap(...) / wrapper helpers without setting it. That leaves OpenGeneric as a null reference, which then flows into ParamTypeInfo.OpenGenericType and overrides ArgumentLocation.OpenGenericType (which is intentionally defaulted to ITypeHandle.Null). Later code reads .Address on these values (e.g. probe = arg.OpenGenericType; if (probe.Address == ...)), so a null here can surface as a NullReferenceException.

Fix by ensuring Wrap initializes OpenGeneric to ITypeHandle.Null and that wrapper helpers propagate elementType.OpenGeneric so the value is never null (and generic-instantiation metadata isn’t lost through wrappers).

        private static TrackedType Wrap(ITypeHandle th)
            => new() { Underlying = th };

        public TrackedType GetByReferenceType(TrackedType elementType)
            => new() { Underlying = elementType.Underlying, IsByRef = true,
                       OutermostKind = CdacCorElementType.Byref };

docs/design/datacontracts/Signature.md:57

  • Grammar: use “an” before “ITypeHandle”.
Decoding a signature follows the ECMA-335 §II.23.2 grammar. For all standard element types, decoding behaves identically to `System.Reflection.Metadata.SignatureDecoder<TType, TGenericContext>`. When the decoder encounters one of the runtime-internal tags above, it reads the target-sized pointer to a runtime `TypeHandle` (and optional `required` byte for `ELEMENT_TYPE_CMOD_INTERNAL`) from the signature blob and resolves it to a `ITypeHandle`.

Represent the absence of a type with C# null rather than a non-null sentinel.
This removes ITypeHandle.Null, NullTypeHandle, and ITypeHandle.IsNull, and uses
nullable annotations to document the APIs and fields where type resolution may
fail.

- Make constructed/approximate/signature resolution results nullable.
- Annotate ManagedTypeSource TryGetTypeHandle with NotNullWhen(true).
- Propagate nullable signature types through providers and calling-convention
  metadata while keeping loaded runtime instantiations non-null.
- Update DacDbi/Legacy consumers and dump tests to use is null/is not null.
- Update authoritative contract documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated 1 comment.

Document that canonical reference identity is scoped to a target cache epoch,
fix nullable ITypeHandle grammar, and replace an implicit null-forgiving
invariant with an actionable runtime guard.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated 4 comments.

Comment thread docs/design/datacontracts/ManagedTypeSource.md Outdated
Comment thread docs/design/datacontracts/ManagedTypeSource.md Outdated
Update the authoritative pseudocode to use nullable ITypeHandle and
MetadataReader outputs with NotNullWhen(true), matching the contract and
implementation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3508d1b8-e4de-4d3e-981f-79fe92c12f2c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 63 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:2466

  • IRuntimeTypeSystem.GetTypeHandle now throws for a 0/invalid handle, so EnumerateClassFields will start returning E_INVALIDARG instead of the native CORDBG_E_CLASS_NOT_LOADED when thExact == 0 (previously handled via TypeHandle.IsNull). Consider explicitly mapping the null-handle sentinel before calling into the contract to preserve the legacy HRESULT semantics.
    src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:2520
  • EnumerateInstantiationFields previously treated a null/zero vmThApprox as CORDBG_E_CLASS_NOT_LOADED (TypeHandle.IsNull check). With ITypeHandle, GetTypeHandle(0) throws and the catch will surface its HResult (likely E_INVALIDARG) instead. Add an explicit vmThApprox == 0 guard to preserve the intended HRESULT behavior for unloaded/unknown types.

@max-charlamb

Copy link
Copy Markdown
Member Author

/ba-g build analysis stuck

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants