Generalize IsBitwiseEquatable to field-wise IEquatable value types

[tannergooding]

Generalizes RuntimeHelpers.IsBitwiseEquatable<T>() so that an unmanaged value type which implements IEquatable<T> of self is reported bitwise-equatable when its Equals is provably a plain field-wise comparison (i.e. equivalent to memcmp), rather than only recognizing a hardcoded set of types.

This picks up the thread from #75642 (closed on the premise that we could/should do an IL pattern match instead) and #130644 (which special-cased Guid/Int128). Instead of maintaining a list, the VM now scans the IEquatable<T>.Equals body and accepts it when every instance field is compared exactly once and the results are ANDed, which is what makes the whole comparison a memcmp. A single forward through a field-wise op_Equality (the very common Equals(T other) => this == other) is followed as well.

The implementation lives in the CoreCLR VM (getILIntrinsicImplementationForRuntimeHelpers continues to fold the result to a JIT-time constant) and is mirrored in the ILC/NativeAOT ComparerIntrinsics so both report the same answer. IsBitwiseEquatable is deliberately conservative for this first pass -- the type must be unmanaged (IsReferenceOrContainsReferences is false), non-generic, tightly packed, and not an inline array.

Fields accepted by the scan:

• an integer-like primitive compared with == or its own Equals (both lower to a bit-for-bit compare)
• a nested value-type field compared through its own IEquatable<F>.Equals, recursing per level so a struct-of-structs is handled when each level is itself field-wise

float/double are excluded -- neither == nor Equals is a memcmp for them (NaN and signed-zero handling differs).

---

Fixing this exposed a latent MethodTable::IsNotTightlyPacked() bug. InitializeFieldDescs accumulated only 1 << dwLog2FieldSize (with dwLog2FieldSize forced to 0) for by-value instance fields, so any struct containing a multi-byte value-type field was always flagged NotTightlyPacked, needlessly pushing ValueType.Equals/GetHashCode onto the reflection slow path. It now accumulates the real GetNumInstanceFieldBytes(), and the flag is transitive so nested padding (e.g. struct S { S2 value; } where S2 has trailing padding) is reflected at every level.

---

Out of scope / deferred:

• InlineArray of a compatible T (could be handled later)
• fancier Equals shapes (SequenceEqual over a span, SIMD, etc.)

Test.CoreLib's primitives are bare structs with no IEquatable<T>, so the ILC unit tests can only exercise the == and nested-Equals forms; the primitive-.Equals path is validated in the VM against the real CoreLib. No src/coreclr/jit files changed, so clrjit.dll is unchanged and SuperPMI asmdiffs would report nothing by construction; codegen impact only shows up against a re-collected MCH.

Marking as draft for early feedback on the approach and the scan's shape.

Note

This PR description and portions of the code/comments were drafted with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[EgorBo]

just a note: the actual reason I didn't try to do this is the fact the API is not public, so it has very few public-facing ways to hit it, might not worth the complexity

[tannergooding]

CC. @jkotas, @EgorBo

This would be a pattern matching approach. Since we cache the tightly packed and equatable checks the perf shouldn't be "too bad" and we'll bail out early on anything that trivially fails it, otherwise we'll check per field as part of building a types own check.

It then essentially just checks for field1.Equals(other.field2) && ... being done for each field. It does notably allow field1 == other.field2 for primitives (but not for other types) and handles the common case of Equals(other) => this == other.

I didn't want to complicate it too much and I think we can push users towards this shape with an analyzer rather than expanding on it. But should generally be good for most types and for what C# emits for records. (needs to recognize EqualityComparer<T>.Default.Equals for struct records)

It doesn't handle inline arrays (that one is already weird because Equals does the wrong thing) as we'd need to match SequenceEquals (doable, but not as important), skips unions, anything with transitive padding, etc.

[tannergooding]

just a note: the actual reason I didn't try to do this is the fact the API is not public, so it has very few public-facing ways to hit it, might not worth the complexity

I think now that its "useful" we could consider making it public.

[tannergooding]

I'll kick off mihubot for diffs after CI comes back clean and any early feedback is addressed, if any.

[jkotas]

Does it work for the code auto-generated by Roslyn for records?

[Copilot]

Pull request overview

This PR enhances RuntimeHelpers.IsBitwiseEquatable<T>() so it can treat unmanaged, tightly-packed value types that implement IEquatable<T> as bitwise-equatable when their Equals(T) is provably a straightforward field-by-field comparison (memcmp-equivalent), rather than relying primarily on a hardcoded allowlist. It also fixes how the VM determines “tightly packed” value types by correctly accounting for by-value field sizes and propagating padding information transitively through nested structs.

Changes:

• CoreCLR: add an IL pattern scanner for IEquatable<T>.Equals (including common forwarding to op_Equality) and use it to decide bitwise-equatability for IEquatable<T> value types.
• CoreCLR: fix and make transitive the “NotTightlyPacked” flag computation by summing true by-value instance sizes and propagating nested padding.
• NativeAOT/ILC: mirror the VM logic in ComparerIntrinsics and add targeted unit tests + test assets.

Show a summary per file

File	Description
src/coreclr/vm/methodtablebuilder.cpp	Fixes declared-size accumulation for by-value fields and propagates “not tightly packed” transitively through nested value-type fields.
src/coreclr/vm/jitinterface.cpp	Implements VM-side IL scanning for field-wise IEquatable<T>.Equals and integrates it into IsBitwiseEquatable.
src/coreclr/vm/corelib.h	Adds binder metadata for IEquatable<T>.Equals.
src/coreclr/tools/Common/TypeSystem/IL/Stubs/RuntimeHelpersIntrinsics.cs	Updates ILC intrinsic expansion to use the new field-wise IEquatable<T> scan.
src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs	Adds the ILC-side field-wise Equals scanner and a transitive tightly-packed layout check.
src/coreclr/tools/aot/ILCompiler.Compiler.Tests/ILCompiler.Compiler.Tests.csproj	Wires in new unit test source + test asset project.
src/coreclr/tools/aot/ILCompiler.Compiler.Tests/EquatableAssets/EquatableAssets.csproj	New test-asset assembly compiled with optimizations to stabilize the expected IL shapes.
src/coreclr/tools/aot/ILCompiler.Compiler.Tests/EquatableAssets/BitwiseEquatable.cs	Adds positive/negative struct patterns to exercise the scanner (including nested cases and padding rejection).
src/coreclr/tools/aot/ILCompiler.Compiler.Tests/BitwiseEquatableTests.cs	Adds tests validating expected acceptance/rejection of field-wise IEquatable<T>.Equals patterns.
src/coreclr/nativeaot/Test.CoreLib/src/Test.CoreLib.csproj	Includes System/IEquatable.cs in Test.CoreLib to support the new test asset assembly.
src/coreclr/nativeaot/Test.CoreLib/src/System/IEquatable.cs	Introduces IEquatable<T> definition for the NativeAOT test CoreLib surface.

Copilot's findings

• Files reviewed: 11/11 changed files
• Comments generated: 2

[tannergooding]

Does it work for the code auto-generated by Roslyn for records?

I think that would need a tweak, because records currently emit: EqualityComparer<T>.Default.Equals(field1, other.field1)

That shouldn't be terribly hard to match, but it'd be slightly nicer if they reused Equals directly

[Copilot]

Copilot's findings

• Files reviewed: 12/12 changed files
• Comments generated: 3

[tannergooding]

Pushed a few follow-ups:

• Records: now recognizes the EqualityComparer<F>.Default.Equals(this.F, other.F) shape Roslyn emits for record structs, accepted only when F is itself bitwise-equatable (same recursion/float-exclusion as the .Equals call form). Handled in both the VM and ILC scanners.
• CI fix: the Loader/classloader/InlineArray failure was a real regression from the IsNotTightlyPacked change. Inline arrays throw NotSupportedException from ValueType.Equals/GetHashCode only via the QCALL, and only while the type's CanCompareBits flag is uncached. Once a struct wrapping an inline array was correctly reported tightly-packed, CanCompareBitsOrUseFastGetHashCode began recursing into the inline-array field and caching its flag as false, permanently suppressing the throw. Fix is to return false for inline arrays without caching, so the managed fast path keeps routing through the throwing QCALL -- this also removes a pre-existing ordering dependency where hashing a wrapper first would poison the inner type.
• Addressed the review nits (unsigned shift in ReadILToken, stale comment).

Generic value types are a noted follow-up: the blocker isn't layout/caching (that's per-instantiation and correct) but that the scanner resolves IL tokens against the open definition, so a T-typed field/EqualityComparer<!0> comes back as the formal variable rather than the concrete type. Making it work needs instantiation-aware token resolution. InlineArray-of-compatible-T is likewise deferred.

Note

This comment was drafted by Copilot (AI-generated) on @tannergooding's behalf.

[tannergooding]

Addressed the latest review feedback and pushed:

• ReadILToken now casts each byte to uint32_t before shifting (well-defined token decode).
• ILC comment updated to reference IsIEquatableEqualsFieldwise/IsTightlyPacked.
• ILC field-wise scan guards PeekILOpcode() with a HasNext check so a truncated IL stream returns false conservatively.
• Dropped the unnecessary CS660/CS661 suppressions from the ILC test asset (kept CS0649).
• Added CoreCLR test coverage under src/tests/Loader/classloader/BitwiseEquatable that calls the internal IsBitwiseEquatable<T>() via reflection and covers the positive/negative cases (field-wise IEquatable<T>, ==-forwarding, nested structs, primitive .Equals, records, and float/padding/ignored-field rejections) plus a tight-packing behavioral check.

All open review threads are resolved.

Note

This comment was drafted by Copilot.

[Copilot]

Copilot's findings

• Files reviewed: 14/14 changed files
• Comments generated: 2

[Copilot]

Copilot's findings

• Files reviewed: 15/15 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

• Files reviewed: 15/15 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

• Files reviewed: 15/15 changed files
• Comments generated: 0 new

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "1df9e78dcb8301fe0b6585e29d771333b9fe694c",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "5d9e18e67268382799e981ea3792e830a00a56db",
  "last_reviewed_commit": "1df9e78dcb8301fe0b6585e29d771333b9fe694c",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "5d9e18e67268382799e981ea3792e830a00a56db",
  "last_recorded_worker_run_id": "29761347936",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "89c6fc406f62c68cb29ebc72ffb6707eb353c198",
      "review_id": 4730937254
    },
    {
      "commit": "1df9e78dcb8301fe0b6585e29d771333b9fe694c",
      "review_id": 4737167594
    }
  ]
}

[github-actions]

Holistic Review

Motivation: The problem is real and well-justified. The prior mechanism hardcoded a list of bitwise-equatable types (Guid, Int128, Rune, etc.), which is unmaintainable and misses user types. Generalizing IsBitwiseEquatable to any unmanaged, tightly-packed value type whose IEquatable<T>.Equals is provably field-wise is a clean improvement, and the incidentally-discovered IsNotTightlyPacked bug (accumulating 1 << dwLog2FieldSize with dwLog2FieldSize forced to 0 for by-value fields) is a genuine latent defect worth fixing on its own.

Approach: IL pattern-matching against the exact shapes Roslyn lowers f0 == o.f0 && ... to. The design is correctness-conservative: every unrecognized shape, resolution failure, or thrown exception folds to false, so the only risk is a lost optimization, never a wrong answer — provided no accepted shape is actually non-memcmp-equivalent. The guards (unmanaged, non-generic, tightly-packed transitively, not an inline array, float/double excluded, each field compared exactly once and ANDed) make a false-positive hard to construct, and the test matrix exercises the boundary cases (IgnoresField, CustomLogic, CallsHelper, WithPadding, HasFloat, record structs, enums, nested/padded wrappers) thoroughly.

Summary: ⚠️ Needs Human Review. I found no correctness defect and the safety posture is sound, but the approach has two structural properties a maintainer should consciously accept: (1) the VM C++ scanner and the ILC C# scanner are two independent reimplementations of the same IL grammar that must stay behaviorally identical or CoreCLR and NativeAOT will disagree on IsBitwiseEquatable; and (2) IL-shape matching is inherently coupled to Roslyn codegen — future compiler changes silently regress to the conservative path. Both are acceptable given the fail-safe design, but they are the points that warrant human sign-off. The PR is already deeply reviewed by area owners.

---

Detailed Findings

✅ Correctness — Fail-safe folding is sound

The intrinsic must be conservative because its result is folded to a JIT/AOT-time constant. Both implementations honor this: IsBitwiseEquatable in jitinterface.cpp wraps the whole scan in EX_TRY/EX_CATCH returning false (re-throwing terminal exceptions), and ScanFieldwiseEqualsBody in ComparerIntrinsics.cs catches InvalidProgramException. Every unmatched opcode returns false. A false negative only loses an optimization; there is no path where a non-memcmp type is reported bitwise-equatable given the field-coverage + tightly-packed + float-exclusion guards.

✅ IsNotTightlyPacked fix — Correct and transitive

Accumulating GetNumInstanceFieldBytes() for by-value fields instead of 1 << dwLog2FieldSize is the right fix, and propagating the flag from nested value-type fields (the pByValueClassCache[i]->IsNotTightlyPacked() loop in methodtablebuilder.cpp) correctly makes it transitive so callers need not recurse. The CanCompareBitsOrUseFastGetHashCode change to return FALSE for inline arrays without caching is a necessary correctness detail — caching a false primed by an enclosing type's recursion would have let the inline-array Equals/GetHashCode throw be silently skipped.

⚠️ Maintainability — Duplicated scanner grammar (cross-cutting, advisory)

The field-wise IL grammar is implemented twice: ScanFieldwiseEqualsBody/IsFieldwiseEqualsBitwiseEquivalent in src/coreclr/vm/jitinterface.cpp and ScanFieldwiseEqualsBodyCore/IsIEquatableEqualsFieldwise in src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs. They must accept exactly the same shapes or CoreCLR and NativeAOT will produce divergent IsBitwiseEquatable<T>() results for the same type. They are currently in parity (records lead-in, op_Equality forward, primitive/nested/EqualityComparer forms, long/short branch forms, shared false-tail), but there is no shared source of truth or cross-check test asserting equivalence. A maintainer should note that any future extension (e.g. the deferred InlineArray/SequenceEqual shapes) has to be mirrored in both. Not merge-blocking.

⚠️ Robustness — Coupling to Roslyn lowering (advisory)

The scanner recognizes the precise opcode sequences current Roslyn emits (ldarg.0; ldfld F; ldarg.1; ldfld F; bne.un[.s], the record EqualityComparer<F>.Default form, etc.). If a future C# compiler alters lowering (operand order, a beq instead of bne.un, added nops under some config), matching silently regresses to the conservative false. That is safe but invisible — there is no diagnostic when a type that used to be recognized stops being recognized. The <Optimize>true</Optimize> pin in the test csproj mitigates the Debug-vs-Release shape divergence for the test itself, but does not protect real corelib types. Worth a maintainer's awareness rather than a code change.

✅ Corelib refactors — Equality funneled through field-wise Equals

Rewriting Guid.Equals, Int128.Equals, UInt128.Equals, and Rune.Equals to plain field-wise comparisons and routing ==/!= through them is consistent and keeps a single canonical comparison. Losing Guid's explicit Vector128 fast path in EqualsCore is acceptable because the point of the PR is that the JIT now proves Guid bitwise-equatable and lowers equality to a vectorized memcmp anyway; the scalar field-wise body only runs where the intrinsic could not fold. This is a deliberate tradeoff the author and area owners have discussed on the thread.

✅ Tests — Strong boundary coverage

BitwiseEquatable.cs covers the accept and reject sides well: many-field GuidShape (forces long-form branches), ForwardsToOp, nested (Nested/NestedLast/AllNested), padded/partial wrappers, records (RecTwo/RecNested/RecMixed/RecPadded/RecFloat/RecEnum), enum fields, .Equals-instead-of-== primitive forms, and the negative cases (IgnoresField, CustomLogic, CallsHelper, WithPadding, HasFloat, OverriddenOnly). Using UnsafeAccessor to hit the intrinsic expansion path (rather than reflection) is the correct way to validate JIT/AOT folding. Gating off Mono via CLRTestTargetUnsupported and pinning Optimize=true are appropriate. One residual gap the PR description already acknowledges: Test.CoreLib primitives are bare structs, so the ILC unit path cannot exercise the primitive-.Equals form — that is only validated in the VM against real CoreLib.

💡 Minor — Documented undefined-behavior note

The added remark on RuntimeHelpers.IsBitwiseEquatable<T>() about torn reads under unsynchronized concurrent mutation is accurate and appropriately scoped (a data race is already UB; a torn read cannot fabricate an invalid managed reference since the types are unmanaged). No change requested.

Note

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

Generated by Holistic Review · 189 AIC · ⌖ 11.7 AIC · ⊞ 10K

[Copilot]

Copilot's findings

• Files reviewed: 17/17 changed files
• Comments generated: 1

[github-actions]

Holistic Review

Motivation: Unchanged from the prior review. The PR generalizes IsBitwiseEquatable from a hardcoded allow-list (Guid, Int128, Rune, ...) to any unmanaged, tightly-packed value type whose IEquatable<T>.Equals is provably field-wise, and fixes the latent IsNotTightlyPacked byte-accumulation bug. Still real and well-justified.

Approach: Unchanged in shape (dual VM C++ / ILC C# IL-shape scanners, fail-safe to false). The new commit ac384b3 addresses review feedback: it (1) makes the fold honor profiler ReJIT by recording every scanned Equals body as an inlining dependency of the method the fold is baked into and blanket-disables the fold for EnC-editable modules; (2) renames the VM accessor to a positive IsTightlyPacked (the underlying EEClass flag stays negative) and updates all call sites; (3) mirrors the VM in the ILC by rejecting ContainsGCPointers in the caller rather than inside the tightly-packed predicate; and (4) identifies EqualityComparer1by name, namespace, and module and keys the comparer form offget_Default` presence instead of a "records" framing.

Summary: ⚠️ Needs Human Review — verdict unchanged. The two structural properties from the prior review remain the points warranting human sign-off (the VM and ILC scanners are independent reimplementations of the same IL grammar that must stay behaviorally identical, and IL-shape matching is coupled to Roslyn lowering). This commit does not change either, but it closes a genuine folding-correctness gap: baking a scanned Equals body's IL into a caller without a ReJIT/EnC dependency would have let a profiler-rejitted or EnC-edited Equals silently diverge from the folded result. That is now handled. I found no new correctness defect in the incremental change. The refactor of the ReJIT bookkeeping into a shared TrackInliningForRejit faithfully preserves the original reportInliningDecision logic (lock discipline, race-recovery ReJIT request) and reuses it, which is a net readability win.

Assessment History

• review 4730937254 reviewed commit 89c6fc4 with verdict ⚠️ Needs Human Review. Current verdict is ⚠️ Needs Human Review — unchanged. The incremental commit ac384b3 is a feedback-hardening pass (EnC/ReJIT correctness, VM/ILC parity of the GC-pointer check, positive IsTightlyPacked, stronger EqualityComparer1` identification) that strengthens the correctness posture without altering the motivation, overall approach, or the two structural risks that drove the original "needs human review" call.

Detailed Findings

✅ ReJIT / EnC correctness — folding dependency now tracked

Folding IsBitwiseEquatable<T>() to true effectively inlines the scanned Equals (and any forwarded op_Equality / nested Equals) IL into the compiled caller. IsFieldwiseEqualsBitwiseEquivalent now appends each relied-upon body to scannedMethods, and getILIntrinsicImplementationForRuntimeHelpers calls TrackInliningForRejit(pMethodBeingCompiled, scannedMethod) for each under FEATURE_REJIT. Combined with the new EnC blanket-disable (pEqualsMD->GetModule()->IsEditAndContinueEnabled() → false), this closes the path where a profiler ReJIT or an EnC edit of Equals would leave a stale folded constant. Bodies are appended only on a successful scan, and nothing is recorded on the primitive/element-type fast path (which needs no dependency). Correct.

✅ VM/ILC parity of the GC-pointer check

Moving ContainsGCPointers rejection out of the ILC IsTightlyPacked and into IsIEquatableEqualsFieldwise mirrors the VM (IsFieldwiseEqualsBitwiseEquivalent checks ContainsGCPointers() alongside !IsTightlyPacked()). The nested-field recursion now gates on fieldType.IsValueType && !fieldType.IsPrimitive && !fieldType.IsEnum, so pointers and references are treated as leaves whose element size already accounts for their footprint, and only nested value types recurse. Since a GC-pointer-free top-level type cannot transitively contain a GC-pointer value-type field, dropping the per-level GC check inside IsTightlyPacked does not weaken the guard. NativeAOT has no EnC, so the absence of an ILC EnC equivalent is expected.

✅ IsTightlyPacked accessor flip

Renaming MethodTable::IsNotTightlyPacked() to a positive IsTightlyPacked() (implemented as !GetClass()->IsNotTightlyPacked(), the EEClass flag staying negative) is a mechanical readability change; all three call sites (comutilnative.cpp, jitinterface.cpp, methodtablebuilder.cpp) are updated consistently and preserve the original polarity.

✅ Stronger EqualityComparer1` identification

Replacing the by-name SystemModule.GetType(...) lookup with a check that the resolved owning type definition's Module == SystemModule, Name == "EqualityComparer1", and Namespace == "System.Collections.Generic"(plus the single-instantiation-equals-field-type check) removes a fragile string-type resolution and is more robust. It matches the intent of keying the comparer form offgetDefault` presence.

i️ Structural risks (carried over, advisory, not merge-blocking)

The duplicated VM/ILC scanner grammar and the coupling to Roslyn lowering described in the prior review are unchanged by this commit and remain the reasons for a human sign-off. No shared source of truth or cross-check test asserting VM/ILC equivalence was added; any future shape extension must still be mirrored in both.

Note

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

Generated by Holistic Review · 95.7 AIC · ⌖ 11.4 AIC · ⊞ 10K

[Copilot]

Copilot's findings

• Files reviewed: 17/17 changed files
• Comments generated: 0 new

[github-actions]

Holistic Review

Motivation: Unchanged from the prior reviews. The PR generalizes IsBitwiseEquatable from a hardcoded allow-list (Guid, Int128, Rune, ...) to any unmanaged, tightly-packed value type whose IEquatable<T>.Equals is provably field-wise, and fixes the latent IsNotTightlyPacked byte-accumulation bug. Still real and well-justified.

Approach: Unchanged in shape (dual VM C++ / ILC C# IL-shape scanners, fail-safe to false). The one new commit (1df9e78) lifts the deliberate non-generic restriction so generic value types are now in scope. In the VM, the three raw single-module token resolvers are replaced by context-aware ones (TryResolveFieldToken, TryResolveMethodToken, TryResolveTypeToken) that take a SigTypeContext built from the value type, so TypeSpec/MethodSpec/MemberRef tokens over a type parameter resolve to the concrete argument; field/callee reasoning now runs on the field's exact type via GetExactFieldType, and the field-owner check relaxes from an exact-MT compare to HasSameTypeDefAs. The ILC mirror wraps the typical method's IL in an InstantiatedMethodIL (GetScannableMethodIL) so its token lookups resolve in the instantiation, and drops the HasInstantiation bail-outs. The ldobj operand of the op_Equality forward is now resolved in context instead of compared as a raw token.

Summary: ⚠️ Needs Human Review — verdict unchanged. I found no correctness defect in the incremental generic-support change. The extension is consistent between the VM and ILC scanners and preserves the fail-safe posture: every resolver returns null/TypeHandle() on an unexpected token kind or a caught (non-terminal) exception, and the tightly-packed / ContainsGCPointers / inline-array guards run on the exact instantiation, so a reference-typed instantiation (which carries GC pointers) and a non-memcmp element type (e.g. float) both fold to false. Because value-type generic instantiations are not code-shared, the fold always sees a concrete MethodTable, never __Canon, so per-instantiation correctness holds. The two structural properties that drove the original "needs human review" call still stand and are, if anything, slightly amplified: the VM and ILC remain independent reimplementations of the same IL grammar that must stay behaviorally identical (now including generic token resolution), and the scanner remains coupled to Roslyn lowering (a T field is matched only through the EqualityComparer<T>.Default.Equals shape Roslyn must emit for a type parameter).

Assessment History

• review 4730937254 reviewed commit 89c6fc4 with verdict ⚠️ Needs Human Review. Current verdict is ⚠️ Needs Human Review — unchanged. Motivation, overall approach, and the two structural risks are the same.
• review 4736838070 reviewed commit ac384b3 with verdict ⚠️ Needs Human Review. Current verdict is ⚠️ Needs Human Review — unchanged. The new commit 1df9e78 broadens scope to generic value types by making token resolution instantiation-aware, without altering the motivation, the dual-scanner approach, or the fail-safe design; it widens the token-resolution surface but does not change the risk profile that warrants human sign-off.

Detailed Findings

✅ Generic support — context-aware resolution is sound and conservative

The new SigTypeContext sigTypeContext(valueTypeTh) is threaded into every field, method, and type token lookup, and field reasoning switches to GetExactFieldType(valueTypeTh), so a T field is inspected as its concrete argument. TryResolveFieldToken/TryResolveMethodToken/TryResolveTypeToken restrict accepted token kinds, wrap resolution in EX_TRY/EX_CATCH that folds to null and rethrows terminal exceptions, and the outer IsBitwiseEquatable still traps the whole scan. The field-owner check correctly relaxes to HasSameTypeDefAs because the resolved field's enclosing MT is the open/approx definition. On the ILC side, GetScannableMethodIL wrapping the typical IL in InstantiatedMethodIL is the right mechanism to make GetObject token lookups resolve in the exact instantiation, and the removed HasInstantiation guards are no longer needed. No path reports a non-memcmp instantiation as bitwise-equatable.

✅ VM/ILC parity maintained across the generic extension

Both scanners gained the same capability with equivalent semantics: exact-field-type inspection, instantiation-aware EqualityComparer<F> identification (VM keeps the by-def-name + module check; ILC keeps its definition.Module == SystemModule name/namespace check), and the same op_Equality-forward-in-context handling. The grammars remain in lockstep for the new generic cases.

✅ Test coverage — generic boundary cases exercised

BitwiseEquatable.cs adds the meaningful accept/reject generic cases: GenPair<int>/<Point>/<RecTwo> (accept), GenPair<string> (GC pointers → reject), GenPair<float> (float → reject), GenMixed<int> (inline == for an int field plus EqualityComparer for T), GenForwardsToOp<int> (forwards into a generic op_Equality), and GenPadded<int> (leading byte forces padding → reject). These directly exercise the per-instantiation token threading and the EqualityComparer<T>.Default.Equals path that Roslyn must emit for a type parameter.

i️ Structural risks (carried over, advisory, not merge-blocking)

The duplicated VM/ILC scanner grammar and the coupling to Roslyn lowering remain the reasons for human sign-off, now covering generic token resolution as well. There is still no shared source of truth or cross-check test asserting VM/ILC equivalence, so any future shape extension must continue to be mirrored in both.

Note

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

Generated by Holistic Review · 152.9 AIC · ⌖ 11.5 AIC · ⊞ 10K