Skip to content

feat(mocks): experimental compile-time internals access (#6514 Tier 2) - #6520

Merged
thomhurst merged 10 commits into
mainfrom
feat/mocks-internals-access-experimental
Jul 30, 2026
Merged

feat(mocks): experimental compile-time internals access (#6514 Tier 2)#6520
thomhurst merged 10 commits into
mainfrom
feat/mocks-internals-access-experimental

Conversation

@thomhurst

@thomhurst thomhurst commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Summary

The "Tier 2" follow-up to #6519's runtime auto-stubs for #6514, now fully productionized. Behind TUnitMocksExperimentalInternalsAccess, internal types of selected referenced assemblies become first-class mockable — nameable in test code, source-generated typed mocks, setups, matchers, verification — with zero InternalsVisibleTo required from the target assembly. That exceeds runtime-proxy libraries: NSubstitute/Moq can auto-substitute such types (when the SDK grants IVT to Castle) but can never let the test configure one, because the test cannot write the type's name.

<PropertyGroup>
  <TUnitMocksExperimentalInternalsAccess>true</TUnitMocksExperimentalInternalsAccess>
</PropertyGroup>
<ItemGroup>
  <TUnitMocksInternalsAccess Include="Microsoft.Azure.Functions.Worker.Core" />
</ItemGroup>
var bindings = IFunctionBindingsFeature.Mock();          // internal to the SDK
features.Get<IFunctionBindingsFeature>().Returns(bindings.Object);
features.Get<IFunctionBindingsFeature>().WasCalled(Times.Once);

Mechanism

The established publicizer pattern (Krafs.Publicizer / IgnoresAccessChecksToGenerator), wired into the TUnit.Mocks package:

  1. PublicizeAssemblyReferences (Mono.Cecil MSBuild task) rewrites each selected reference so internals are public, preserving assembly identity (name/version/public key). The implementation assembly is the rewrite source — Roslyn ref assemblies strip internal members when no IVT exists (internal ctors vanish), which surfaced as TM006 on internal classes during test hardening.
  2. The swap targets ReferencePathWithRefAssemblies — the compiler's view and nothing else. ReferencePath, copy-local, and deps.json keep the originals; the runtime loads the real assembly, where the task-emitted IgnoresAccessChecksTo makes the compiled IL (generated mock classes implementing internal interfaces included) valid.
  3. Zero source-generator changes — through the publicized reference the types simply look public.

Productionization

  • Packaging: task multi-targets net472 (VS MSBuild) + net8.0 (dotnet msbuild), selected via $(MSBuildRuntimeType); ships with Mono.Cecil under tasks/<tfm>/; TUnit.Mocks.InternalsAccess.targets packed to buildTransitive/<tfm>/ and imported by TUnit.Mocks.targets — consumers need only the opt-in. Fully inert otherwise (full TUnit.Mocks.Tests suite unaffected: 1186/1186).
  • Tests (10/10, Debug and Release/NoBuild CI-mode): end-to-end pipeline against a strong-named stand-in SDK with zero IVT grants — internal interface, closed-generic internal interface, internal class partial mock (internal ctor), typed setups/matchers/verification of calls made inside the SDK, manual implementations (type-load proof) — plus unit tests of the task itself: publicizing + identity preservation down to the public-key token, incrementality, generated-source content, TUMIA001 for unresolved names.
  • Package-level verification: packed the real nupkg, extracted it, and consumed the buildTransitive targets from a standalone console implementing an internal interface — works, including under PublishTrimmed (full trim mode).
  • CI: projects in TUnit.CI.slnx; RunMockInternalsAccessTestsModule runs the suite.
  • Docs: new Internals Access (experimental) section in the mocking docs.

Remaining caveats

  • IgnoresAccessChecksToAttribute is CoreCLR-honored but not a documented contract (foundation of several long-lived OSS packages; low-but-nonzero risk).
  • .NET Framework test targets: inert with warning TUMIA002 (the runtime doesn't honor the attribute).
  • Native AOT unverified (no MSVC linker on the dev machine); trimmed publish verified.
  • Publicizer scope: types + methods (fields untouched; explicit interface impls stay private as IL requires).

Runtime auto-stubs (#6519) satisfy SDK-internal generic requests with
functional defaults, but the test can never configure or verify such a
type — it cannot write the type's name. This prototype removes that
constraint: behind TUnitMocksExperimentalInternalsAccess, internal
types of selected referenced assemblies become first-class mockable —
nameable in test code, source-generated typed mocks, setups, matchers,
verification — with zero InternalsVisibleTo required from the target
assembly.

Mechanism (the established publicizer pattern, wired for TUnit.Mocks):
a Mono.Cecil MSBuild task rewrites the selected references so internals
are public, preserving assembly identity, and swaps them into
ReferencePathWithRefAssemblies only — the compiler's view. deps.json
and copy-local keep the originals, so the runtime loads the real
assembly, where the task-emitted IgnoresAccessChecksTo attribute makes
the compiled IL (including generated mock classes implementing internal
interfaces) valid. The mocks source generator needs no changes: through
the publicized reference the types simply look public.

Validated end to end against a fake-SDK library that grants no IVT to
anyone (the exact case runtime stubs cannot reach): naming, mocking,
typed setup/matcher/verification of an internal interface requested by
SDK-internal code, and a hand-written implementation proving the
runtime honors the attribute at type load. Incremental builds stay
no-op; cold builds order correctly via project references.

Experimental caveats documented in the project README: undocumented
(but ecosystem-established) runtime attribute, inert on .NET Framework,
AOT unverified, packaging not yet done.
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review: Tier 2 internals-access prototype (#6514)

Solid prototype — the ReferencePathWithRefAssemblies-only swap (leaving ReferencePath/deps.json untouched) is the right insight, and the README is unusually good at documenting the reasoning and known gaps up front. A few things worth addressing before this graduates out of experimental:

1. No exception handling around the Cecil rewrite (PublicizeAssemblyReferences.cs:131-164)
Execute()/Publicize() call ModuleDefinition.ReadModule/Write with nothing catching failures. A locked file (AV scan, another build node), a mixed-mode/obfuscated assembly, or any Cecil parse failure will surface as a raw unhandled-exception stack trace from the task rather than a clean MSBuild diagnostic. Worth wrapping the body in try/catch and reporting via Log.LogErrorFromException(ex, showStackTrace: true) so failures are actionable instead of crashing the build host.

2. Reimplementing an established pattern instead of depending on it
The code and README both cite Krafs.Publicizer / IgnoresAccessChecksToGenerator as the prior art this mirrors. Hand-rolling the Cecil rewrite (and pulling Mono.Cecil into Directory.Packages.props for the whole solution) means edge cases those packages already solved — fields (explicitly left untouched here), generic variance, custom attributes whose arguments reference internal types, multi-TFM ref-assembly quirks — have to be rediscovered independently as this hardens. If this is meant to graduate past "experimental," it's worth a deliberate note on why a custom task was chosen over depending on (or vendoring) one of the existing packages, since that would shrink the surface this project has to maintain long-term.

3. Duplicate-attribute collision risk
WriteIgnoresAccessChecksToSource unconditionally emits an IgnoresAccessChecksToAttribute definition into every consuming project's compilation. This is a well-known enough trick that a test project could plausibly already reference another package defining the same type (the real IgnoresAccessChecksToGenerator, Fody, etc.) — that's a duplicate-type compile error, not a graceful fallback. Worth guarding generation on whether the compilation already declares the type before this leaves prototype status.

4. Silent first-match on simple-name resolution (PublicizeAssemblyReferences.cs:94-95)
ReferencePaths.FirstOrDefault(...) picks the first reference matching the requested simple name. In a diamond-dependency scenario with two same-named assemblies at different paths/versions on the resolved reference list, this silently publicizes whichever sorts first rather than flagging the ambiguity — could produce a confusing "it compiled against the wrong version" failure mode that's hard to trace back to this task.

Nothing here blocks the prototype's purpose (proving the mechanism), and the test coverage (zero-IVT target lib + 4 tests covering naming/mocking/generic dispatch/matchers/manual impl) is a good validation story for the concept. These are mainly hardening items for the productionization pass already called out in the PR description.

… task, tests, CI, docs

Graduates the #6514 Tier 2 prototype into the TUnit.Mocks package:

- The publicizer task multi-targets net472 (Visual Studio's .NET
  Framework MSBuild) and net8.0 (dotnet msbuild), selected via
  $(MSBuildRuntimeType); MSBuild references are compile-time only and
  pinned to a version shipping both assets. Task and Mono.Cecil ship
  under tasks/<tfm>/ in the nupkg (NU5100 suppressed — task assemblies
  are exactly the thing that must not live in lib/).
- TUnit.Mocks.InternalsAccess.targets moved into src/TUnit.Mocks, packed
  to buildTransitive/<tfm>/ and build/, and imported by
  TUnit.Mocks.targets — consumers need only the opt-in property and
  item. Fully inert otherwise (verified: full TUnit.Mocks.Tests suite
  unaffected). Dual-path task resolution covers packaged and repo-local
  layouts.
- Correctness fix surfaced by new coverage: publicize the
  IMPLEMENTATION assembly (%(OriginalPath)), not the reference assembly
  — Roslyn ref assemblies strip internal members when no IVT exists, so
  internal constructors vanished and internal classes emitted TM006.
  The compiler-facing item (the ref assembly) is still what gets
  swapped out.
- Coverage expanded: internal class partial mocks (internal ctor),
  closed-generic internal interfaces, strong-named target assembly
  (identity preservation asserted down to the public key token), and
  unit tests of the task itself — publicizing, incrementality, the
  generated IgnoresAccessChecksTo source, and TUMIA001 for unresolved
  names. Build-order edges use SetTargetFramework per task TFM
  (SkipGetTargetFrameworkProperties alone leaks the referencing
  TargetFramework and breaks the outer build).
- Verified against the produced nupkg: extracted-package buildTransitive
  import compiles and runs a consumer implementing an internal
  interface, including under PublishTrimmed (full trim mode). Native
  AOT remains unverified.
- CI: projects added to TUnit.CI.slnx and a RunMockInternalsAccessTests
  pipeline module runs the suite; docs gain an "Internals Access
  (experimental)" section.
@thomhurst
thomhurst marked this pull request as ready for review July 30, 2026 01:49
Comment thread src/TUnit.Mocks/TUnit.Mocks.InternalsAccess.targets Outdated
Comment thread src/TUnit.Mocks.InternalsAccess.Tasks/PublicizeAssemblyReferences.cs Outdated
@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown

Greptile Summary

Introduces experimental compile-time access to internal types for selected assemblies.

  • Adds a Mono.Cecil MSBuild task that publicizes compiler-only reference copies while preserving runtime assembly identity.
  • Integrates the task through transitive TUnit.Mocks build targets with opt-in configuration and runtime access-check attributes.
  • Adds strong-name, identity-resolution, metadata-preservation, incrementality, packaging, and end-to-end tests.
  • Adds CI coverage and documentation for configuration, behavior, and platform caveats.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/TUnit.Mocks.InternalsAccess.Tasks/PublicizeAssemblyReferences.cs Implements identity-aware reference selection, content-hash incrementality, metadata-preserving publicization, and runtime-implementation resolution; the displayed prior defects are addressed.
src/TUnit.Mocks/TUnit.Mocks.InternalsAccess.targets Adds the opt-in MSBuild pipeline and limits rewritten references to the compiler-facing item group.
src/TUnit.Mocks/TUnit.Mocks.csproj Packages the runtime-specific task binaries, dependencies, and transitive targets.
tests/TUnit.Mocks.InternalsAccess.Tests/PublicizeAssemblyReferencesTaskTests.cs Covers duplicate requests, compiler metadata, multiple identities, implementation selection, strong names, and stale-cache invalidation.
tests/TUnit.Mocks.InternalsAccess.Tests/InternalsAccessTests.cs Exercises typed mocking and runtime access to internal interfaces, generics, and classes.
docs/docs/writing-tests/mocking/advanced.md Documents setup, mechanics, compatibility limitations, and experimental status.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Selected assembly reference] --> B[Resolve matching implementation identity]
    B --> C[Create publicized compiler-only copy]
    C --> D[Replace compiler reference]
    B --> E[Keep original runtime asset]
    F[Generated IgnoresAccessChecksTo attribute] --> G[Test assembly]
    D --> G
    G --> E
Loading

Reviews (9): Last reviewed commit: "fix(mocks): identity-first reference loo..." | Re-trigger Greptile

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b3b321588a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +81 to +82
var originalPath = reference.GetMetadata("OriginalPath");
var source = string.IsNullOrEmpty(originalPath) ? reference.ItemSpec : originalPath;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Resolve NuGet ref assets to implementation assemblies

When a PackageReference supplies ref/<tfm>/Foo.dll directly as its compile asset, OriginalPath is not populated because MSBuild did not substitute that reference in this target. This fallback therefore publicizes the reference assembly itself, where the internal members this feature needs have already been stripped, so opting into internals access still leaves those APIs unavailable. Resolve the corresponding runtime/implementation asset instead of treating a missing OriginalPath as proof that ItemSpec is an implementation assembly.

Useful? React with 👍 / 👎.

Log.LogMessage(MessageImportance.Low, $"TUnitMocksInternalsAccess: '{destination}' is up to date.");
}

var item = new TaskItem(destination);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve compiler metadata on replacement references

When the selected reference has compiler-significant metadata such as Aliases or EmbedInteropTypes, constructing a fresh TaskItem discards it before the target replaces the original item. For example, an assembly referenced only through an extern alias becomes an ordinary global reference and the existing extern alias no longer resolves. Copy the original reference metadata onto the replacement before overriding the copy-local fields.

Useful? React with 👍 / 👎.

var source = string.IsNullOrEmpty(originalPath) ? reference.ItemSpec : originalPath;
var destination = Path.Combine(OutputDirectory, Path.GetFileName(source));

if (!File.Exists(destination) || File.GetLastWriteTimeUtc(destination) < File.GetLastWriteTimeUtc(source))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Invalidate publicized copies by content rather than timestamp

When a dependency is downgraded or replaced by a different assembly whose timestamp is equal to or older than the existing output, this check reuses the previous publicized DLL. The compiler can then compile against the prior assembly's API while copy-local output and runtime resolution use the new original, causing missing-member or type-load failures until obj is cleaned. Track a content hash or assembly identity rather than assuming newer timestamps imply identical input.

Useful? React with 👍 / 👎.

- Preserve original reference metadata (Aliases, EmbedInteropTypes) on the
  swapped compiler reference via CopyMetadataTo
- Content-hash (.sig) incrementality instead of timestamp comparison, so
  downgrades/equal-timestamp replacements re-publicize; sig files in FileWrites
- Resolve implementation assembly from runtime/copy-local assets when the
  compile asset is a ref assembly with no OriginalPath (TUMIA003 when absent)
- Warn TUMIA004 on ambiguous simple-name matches instead of silent first-pick
- Wrap resolve/publicize in try/catch -> TUMIA005 error instead of unhandled
  Cecil exception crashing the build host
- TUnitMocksInternalsAccessEmitAttributeDefinition=false suppresses the
  IgnoresAccessChecksToAttribute definition when another package injects it
- README: diagnostics table + rationale for custom task vs Krafs.Publicizer
@thomhurst

Copy link
Copy Markdown
Owner Author

All review findings addressed in 5ea2559:

Metadata drop on reference swap (Greptile P1 / Codex P2) — valid, fixed. The replacement item now carries the original reference's metadata via CopyMetadataTo (then re-overrides Private/CopyLocal), so Aliases/EmbedInteropTypes survive the swap. Test asserts both round-trip.

Timestamp-only cache (Greptile P2 / Codex P2) — valid, fixed. Incrementality is now content-based: a .sig sidecar records the resolved source path + SHA256; any content change (downgrade, equal-or-older timestamp replacement) re-publicizes. Sig files added to FileWrites. Test manipulates content with a backdated timestamp.

Ref assembly as compile asset with no OriginalPath (Codex P1) — valid, fixed. The task now takes the runtime/copy-local assets (RuntimeCopyLocalItems + ReferenceCopyLocalPaths); when the chosen source carries ReferenceAssemblyAttribute it publicizes the implementation asset instead, and warns TUMIA003 when none exists. Both paths tested (Cecil-forged ref assembly).

Cecil exception handling (Claude #1) — fixed. Resolve+publicize wrapped per assembly; failures surface as TUMIA005 with the exception detail at low verbosity instead of an unhandled stack trace. Tested against a non-PE file.

Duplicate attribute definition (Claude #3) — fixed. TUnitMocksInternalsAccessEmitAttributeDefinition=false suppresses the generated attribute class (the [assembly:...] applications remain). Documented in README + docs.

Silent first-match ambiguity (Claude #4) — fixed. Multiple distinct paths matching one simple name now warn TUMIA004 listing all candidates; first still wins deterministically.

Custom task vs Krafs.Publicizer dependency (Claude #2) — README now carries the rationale: those packages are consumer-facing MSBuild wiring, not library APIs; TUnit needs the step riding transparently inside its own package (single opt-in, buildTransitive, dual MSBuild hosts, RPWRA-only swap). Revisit if scope grows.

Suite: 17/17 including e2e targets pipeline.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ea2559eae

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

continue;
}

method.Attributes = (method.Attributes & ~MethodAttributes.MemberAccessMask) | MethodAttributes.Public;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep private methods out of the publicized API

When a selected assembly contains a public type with a private overload, this unconditional rewrite exposes that overload too, even though the feature promises access to internals. For example, adding a private M(string) beside public M(object) causes an existing consumer call such as M(null) to bind to the formerly private overload whenever internals access is enabled, silently changing behavior in unrelated code. Promote only assembly-visible members (and the constructors/accessors required for internal types), while preserving genuinely private members.

Useful? React with 👍 / 👎.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Code review

Followed up on the prior review round (5ea2559) — verified each of the six previously-flagged fixes against the current code, plus a fresh pass for new issues and CLAUDE.md compliance.

CLAUDE.md compliance: No violations found. This PR is isolated MSBuild tooling (no core engine metadata collection, no source-gen output changes, no VSTest, no blocking-on-async, tests/CI/docs updated alongside the source change).

Previously-flagged issues: Confirmed fixed — metadata preservation (CopyMetadataTo), content-hash .sig incrementality, ref-assembly/implementation-assembly resolution via RuntimeCopyLocalItems/ReferenceCopyLocalPaths, Cecil try/catch → TUMIA005, and the EmitAttributeDefinition opt-out all check out.

One fix looks incomplete — ambiguous simple-name match (TUMIA004)

$"{string.Join(", ", matches.Select(m => m.ItemSpec))}. Using the first; " +
"if that is the wrong one, resolve the version conflict in the project.");
}
var reference = matches[0];
string source;

When two resolved references share a simple name, the task now warns (TUMIA004) but still only records matches[0] as Original (L100, L143). In the targets file, Remove only strips that one item:

over. -->
<ReferencePathWithRefAssemblies Remove="@(_TUnitMocksPublicizedReference->'%(Original)')" />
<ReferencePathWithRefAssemblies Include="@(_TUnitMocksPublicizedReference)" />

So the second matching reference is never removed from ReferencePathWithRefAssemblies — the compiler ends up seeing both the untouched duplicate and the publicized copy (same assembly identity, two paths), which is the classic CS1703 "assembly already imported" scenario rather than a clean "first wins" resolution. The task-level unit test (Ambiguous_Simple_Name_Warns_With_TUMIA004) only asserts the task's own output, so it doesn't catch this — it never exercises the targets-file integration. Suggest also removing the non-selected matches (or filtering ReferencePathWithRefAssemblies to just the winner) so the ambiguous case degrades to "picks one, cleanly" instead of a build break.

Minor/lower-confidence: IgnoresAccessChecksTo is emitted using the requested simple name rather than the true assembly identity read via Cecil (module.Assembly.Name.Name) — fine whenever the file name matches the assembly name (the common case), but would silently fail to suppress the access check if they ever diverge. Not blocking, just worth a note given the task already has the real identity in hand.

…ll ambiguous matches

Review round 3:
- TUMIA004 now supersedes ALL same-simple-name matches, not just the winner —
  a leftover duplicate shared the publicized copy's assembly identity (CS1703).
  New SupersededReferences task output feeds the targets Remove.
- Publicize promotes only internal/protected internal/private protected;
  private and protected members are untouched, so a formerly-private overload
  can no longer capture existing calls in consuming code.
- IgnoresAccessChecksTo is emitted with the assembly's real identity read from
  the resolved file, not the file-derived requested name.
@thomhurst

Copy link
Copy Markdown
Owner Author

Round 3 addressed in 55e5049:

Ambiguous-match removal incomplete (CS1703) — valid, fixed. The task now outputs SupersededReferences — every same-simple-name match, winner included — and the targets removes all of them from ReferencePathWithRefAssemblies before adding the publicized copy. The ambiguous case now degrades to "first wins, cleanly". Test asserts both duplicate paths are superseded.

Publicizer promoted private members (Codex P2) — valid, fixed. Promotion is now scoped to assembly-visible surface only: internal / protected internal / private protected types and methods become public; private and protected stay untouched. A formerly-private Describe(string) beside a public Describe(object) can no longer capture Describe(null) in consuming code. Covered by a fixture class exercising all five visibilities plus nested types.

IgnoresAccessChecksTo identity (minor) — fixed. The attribute is emitted with the assembly identity read from the resolved file (AssemblyName.GetAssemblyName), not the file-derived requested name. Test renames the file and asserts the real identity is used.

Suite: 19/19 including e2e targets pipeline.

Comment thread src/TUnit.Mocks.InternalsAccess.Tasks/PublicizeAssemblyReferences.cs Outdated
Comment thread src/TUnit.Mocks.InternalsAccess.Tasks/PublicizeAssemblyReferences.cs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55e5049ab0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +126 to +129
var signaturePath = destination + ".sig";
var signature = source + "\n" + HashFile(source);

if (!File.Exists(destination) || !File.Exists(signaturePath) || File.ReadAllText(signaturePath) != signature)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Include the publicizer version in the cache signature

When TUnit.Mocks is upgraded while the selected dependency remains unchanged, this signature still matches because it contains only the dependency path and content hash. Any fixes to the rewriting rules—such as changing which members are promoted—will therefore continue using the copy produced by the older task until the consumer cleans obj, so the compiler can see a stale publicized API. Include a transformation/task version in the signature to invalidate outputs when the publicizer changes.

Useful? React with 👍 / 👎.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Code review

Follow-up on the prior two review rounds — checked whether the issues flagged after 5ea2559e were addressed in the latest commit (55e5049a), plus a fresh pass over the delta.

Previously-flagged issue: confirmed fixed, with solid test coverage

The ambiguous simple-name match / CS1703 risk from the last round is now properly closed:

/// <summary>
/// Every resolved reference superseded by a publicized copy — the selected match plus any
/// same-simple-name duplicates. The targets file Removes all of these so an ambiguous match
/// degrades to "first wins, cleanly" instead of the compiler seeing the same assembly
/// identity twice (CS1703).
/// </summary>
[Output]
public ITaskItem[] SupersededReferences { get; set; } = [];

SupersededReferences now carries every same-simple-name match (not just the winner), and the targets file removes all of them from ReferencePathWithRefAssemblies instead of only the selected one:

</PublicizeAssemblyReferences>
<ItemGroup>
<!-- Swap ONLY the compiler's view (ReferencePathWithRefAssemblies feeds Csc and nothing
else). ReferencePath stays untouched, so copy-local outputs and deps.json keep the
original assembly and the runtime binds to it — where IgnoresAccessChecksTo takes
over. Superseded = the selected reference plus any same-simple-name duplicates; a
leftover duplicate would carry the publicized copy's assembly identity (CS1703). -->
<ReferencePathWithRefAssemblies Remove="@(_TUnitMocksSupersededReference)" />
<ReferencePathWithRefAssemblies Include="@(_TUnitMocksPublicizedReference)" />
<Compile Include="$(_TUnitMocksIactSourceFile)" />
<FileWrites Include="$(_TUnitMocksIactSourceFile)" />

This closes the gap the previous review pointed out (the task-level test only asserted the task's own output, not the targets-file integration) — the new Ambiguous_Simple_Name_Warns_With_TUMIA004 assertions now check SupersededReferences directly, which is what actually feeds the Remove.

The "minor/lower-confidence" note from the last round (identity emitted from the requested name rather than the real assembly identity) is also fixed — assemblyName is now read via AssemblyName.GetAssemblyName(source).Name, with a dedicated regression test (Generated_Source_Uses_Real_Assembly_Identity_When_File_Name_Differs).

Bonus hardening beyond what was asked: member promotion is now scoped to internal/protected internal/private protected only (IsAssemblyVisibleType/IsAssemblyVisibleMethod), so a formerly-private overload can no longer sneak into overload resolution for consuming code. Well covered by Publicize_Promotes_Only_Assembly_Visible_Members. README's diagnostics table was updated to match.

CLAUDE.md compliance: No violations — still isolated MSBuild tooling, no core engine/source-gen/VSTest/async-blocking concerns.

Two non-blocking edge cases worth a look before this leaves experimental (both narrow enough that they don't meet the bar for a blocking issue, but touch the exact code path this round just changed, so flagging for visibility):

  1. Metadata loss on the non-winning ambiguous match (PublicizeAssemblyReferences.cs:154) — CopyMetadataTo only copies matches[0]'s metadata, but all matches are now removed from the compiler's reference list. If the discarded match carried different compiler-significant metadata (e.g. Aliases) than the winner, that's now silently dropped rather than just "wrong version picked." Given TUMIA004 already tells the user this is an unresolved conflict they should fix in their project, this is edge-of-an-edge-case — but worth a one-line comment/README caveat if not worth handling.

  2. Duplicate AssembliesToPublicize entries (PublicizeAssemblyReferences.cs:84) — if the same simple name appears twice in @(TUnitMocksInternalsAccess), the loop runs twice and appends two TaskItems with the identical destination path to PublicizedReferences. Whether that round-trips through Csc cleanly isn't verified either way here. A cheap, architecturally cleaner fix if this is worth guarding: dedupe AssembliesToPublicize by simple name once at the top of Execute(), rather than handling it ad hoc later — same fix shape as the simple-name matching already used for matches.

Neither blocks this PR; both are quick to close out in a follow-up if you want zero remaining rough edges before promoting past experimental.

…etadata, version the cache signature

- Duplicate TUnitMocksInternalsAccess items (same simple name, any case) now
  produce a single publicized reference instead of handing Csc the same
  rewritten assembly twice.
- When multiple resolved references share the requested simple name, extern
  aliases are unioned and EmbedInteropTypes kept if any match set it, so
  compiler-significant metadata carried only by a non-selected match survives
  the swap.
- The incrementality signature now includes the task version, so a TUnit.Mocks
  upgrade re-publicizes instead of reusing a copy produced by older rewrite
  rules.
@thomhurst

Copy link
Copy Markdown
Owner Author

Round 4 addressed in 05cdf1e:

Duplicate opt-in items duplicate compiler references (Greptile P1) — valid, fixed. Requested names are now deduped (OrdinalIgnoreCase) at the top of Execute(), so the same TUnitMocksInternalsAccess item included twice — multi-imported props included — yields exactly one publicized reference and one IgnoresAccessChecksTo application. Test: Duplicate_Requests_Produce_Single_Publicized_Reference.

Non-winning ambiguous match loses compiler metadata (Greptile P1 / Claude edge case 1) — valid, fixed. Since every same-simple-name match leaves ReferencePathWithRefAssemblies, metadata carried only by a non-selected match is now merged onto the replacement item: extern aliases are unioned (global visibility preserved when any match had no alias) and EmbedInteropTypes kept if any match set it. TUMIA004 text and README updated. Test: Ambiguous_Matches_Union_Compiler_Metadata — alias and interop flag live only on the loser, asserts global,sdkalias + EmbedInteropTypes=true on the publicized item.

Cache signature missing publicizer version (Codex P2) — valid, fixed. The .sig now leads with the task's informational version, so upgrading TUnit.Mocks re-publicizes even when the referenced assembly is byte-identical — rewrite-rule fixes are never masked by a stale obj copy. Test: Task_Version_Change_Invalidates_Publicized_Copy (tampered version line + backdated output → re-publicized).

Suite: 22/22 (task-level + e2e targets pipeline).

This was referenced Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant