fix: respect user-set TUnitImplicitUsings from Directory.Build.props

[thomhurst]

Summary

Fixes #5277 and #5208.

When a user set <TUnitImplicitUsings>false</TUnitImplicitUsings> in Directory.Build.props, TUnit's props files unconditionally overwrote it back to true, causing ambiguous reference errors like CS0104: 'Test' is an ambiguous reference between 'TUnit.Core.TestAttribute' and 'NUnit.Framework.TestAttribute'.

Root causes:

1. TUnitImplicitUsings was unconditionally set to true in TUnit.Core.props, ignoring user-set values from Directory.Build.props
2. There was a dead xunit detection mechanism (XunitPackageReferenceFound) that was supposed to skip implicit usings when xunit was present, but it never worked — the Target runs during execution phase while the PropertyGroup is evaluated during the static evaluation phase

Changes:

• TUnit.Core.props — Remove dead XunitPackageReferenceFound property + target. Simplify to Condition="'$(TUnitImplicitUsings)' == ''" guard so user-set values are respected
• TUnit.Assertions.props — Same cleanup: remove dead xunit detection, keep the existing '$(TUnitAssertionsImplicitUsings)' == '' guard

Implicit usings still default to true and still flow transitively — users just need to set <TUnitImplicitUsings>false</TUnitImplicitUsings> in their Directory.Build.props to opt out, and it will now actually be respected.

Test plan

• CI modularpipeline jobs pass on all 3 OS
• Setting <TUnitImplicitUsings>false</TUnitImplicitUsings> in Directory.Build.props is respected
• Default behavior unchanged — projects that don't set the property still get implicit usings

[claude]

Code Review

This is a well-targeted fix for a genuine and annoying NuGet transitive contamination bug. The root cause diagnosis is correct and the general approach (separate build/ vs buildTransitive/ content) is the proper NuGet solution.

---

Issue: Duplicate Target Definitions for Direct Consumers

This is the main concern. According to NuGet's design, buildTransitive/ files are imported for both direct and transitive consumers. So a project that directly references TUnit.Core will import:

1. build/TUnit.Core.props - defines DetectPolyfillReference target
2. buildTransitive/TUnit.Core.props - also defines DetectPolyfillReference target

Same applies to _TUnitEnsurePolyfill in the .targets files. MSBuild silently lets the last import win on duplicate targets, so this won't break the build - but it creates redundant work and is a latent maintenance hazard (some tooling configurations surface this as a warning).

Suggested architectural fix:

Since buildTransitive/ already applies to direct consumers, the Polyfill logic only needs to live in the transitive file. The direct build/TUnit.Core.targets can be stripped down to just what's unique to direct consumers (implicit usings, source gen settings):

<!-- build/TUnit.Core.targets - only what direct consumers need beyond what transitive provides -->
<Project>
    <ItemGroup Condition="'$(TUnitImplicitUsings)' == 'true'">
        <Using Include="TUnit.Core.HookType" Static="True" />
        <Using Include="TUnit.Core" />
    </ItemGroup>
    <!-- Polyfill logic lives exclusively in buildTransitive/TUnit.Core.targets -->
</Project>

This eliminates the duplicate target definitions and the version duplication with no behavioral change for direct consumers.

---

Minor: Polyfill Version Hardcoded in Two Places

_TUnitPolyfillVersion (9.23.0) now appears in both TUnit.Core.targets and TUnit.Core.buildTransitive.targets. A version bump requires updating two files - easy to miss. The architectural fix above resolves this naturally.

---

What's Done Well

• The separation of concerns is correct: implicit usings, source generators, and test project settings should never flow transitively, and this fix cleanly enforces that boundary.
• The naming convention (TUnit.Core.buildTransitive.props packed as buildTransitive/TUnit.Core.props) is correct NuGet packaging idiom.
• The '$(TUnitImplicitUsings)' == '' guard fix on TUnit.Core.props is a good defensive fix that matches what TUnit.Assertions.props already did.
• The transitive TUnit.Assertions and TUnit.Engine stubs being intentionally empty is clearly documented - right call given they have nothing legitimate to propagate transitively.
• The DetectPolyfillReference target uses BeforeTargets="PrepareForBuild" for correct evaluation order.

The fix correctly addresses the reported issue (#5277). The duplicate target concern is worth resolving before merge to avoid future confusion, but it won't cause any breakage as-is.

[claude]

Follow-up Review

The previous review raised two concerns. Checking the current state of the PR (still a single commit, no updates since the last review):

---

Previous Issue 1: Duplicate Target Definitions — Still Present

Both TUnit.Core.props (build/) and TUnit.Core.buildTransitive.props (buildTransitive/) define the DetectPolyfillReference target. Both TUnit.Core.targets (build/) and TUnit.Core.buildTransitive.targets (buildTransitive/) define _TUnitEnsurePolyfill. Since NuGet imports buildTransitive/ for both direct and transitive consumers, a direct consumer of TUnit.Core ends up with both targets imported — effectively defining each target twice.

MSBuild silently uses the last definition, so there's no build failure, but it is redundant work and a footgun: any future change made to the build/ version but not the buildTransitive/ version (or vice versa) will produce subtly different behavior between direct and transitive consumers for the Polyfill logic, which is supposed to be identical.

The clean resolution is to strip the Polyfill logic out of TUnit.Core.props/TUnit.Core.targets entirely and let the buildTransitive/ counterparts serve as the single source of truth for that logic (since they apply to both direct and transitive consumers). The build/ files would then only contain what's unique to direct consumers: the SetXunitPackageReferenceFound/TUnitImplicitUsings logic, source generation settings, and CompilerVisibleProperty exports.

---

Previous Issue 2: Polyfill Version Hardcoded in Two Places — Still Present

<_TUnitPolyfillVersion>9.23.0</_TUnitPolyfillVersion> appears in both TUnit.Core.targets and TUnit.Core.buildTransitive.targets. A version bump requires touching two files and it's easy to get them out of sync. The fix above (Polyfill logic only in buildTransitive/) eliminates this naturally — the version would live in exactly one place.

---

Summary

The core fix (preventing implicit usings, source generators, and test project settings from flowing to transitive consumers) is correct and the approach is right. These two concerns are the remaining items to clean up before merge. Neither will cause a regression in the reported scenario, but the duplicate target definitions are a maintenance hazard worth addressing now while the files are being restructured anyway.

[claude]

Follow-up Review (Second Commit)

The second commit ("refactor: deduplicate Polyfill logic per review feedback") has addressed both issues raised in the previous reviews.

Previous Issue 1: Duplicate Target Definitions — Resolved

DetectPolyfillReference and _TUnitEnsurePolyfill have been removed from TUnit.Core.props and TUnit.Core.targets respectively. The Polyfill logic now lives exclusively in TUnit.Core.buildTransitive.props and TUnit.Core.buildTransitive.targets. Since NuGet imports buildTransitive/ for both direct and transitive consumers, direct consumers get the Polyfill logic exactly once — no duplication.

Previous Issue 2: Polyfill Version Hardcoded in Two Places — Resolved

_TUnitPolyfillVersion (9.23.0) now appears only in TUnit.Core.buildTransitive.targets. There is a single source of truth for the version.

---

Final State Assessment

The PR is now clean. The complete set of changes:

• TUnit.Core: build/ contains only direct-consumer concerns (xunit detection, TUnitImplicitUsings, source gen settings, CompilerVisibleProperty exports, implicit usings injection). buildTransitive/ contains only the Polyfill logic, which applies correctly to both direct and transitive consumers.
• TUnit.Assertions: build/ keeps implicit usings and xunit detection. buildTransitive/ is intentionally empty — nothing from assertions should propagate.
• TUnit.Engine: build/ keeps test project settings (IsTestProject, OutputType=Exe, etc.). buildTransitive/ is intentionally empty — these settings are destructive if applied to non-test projects.
• TUnit.Core.props guard fix: The '$(TUnitImplicitUsings)' == '' condition is now present, matching the existing behavior of TUnit.Assertions.props and properly respecting user opt-outs.

The root cause of #5277 (transitive consumers getting implicit usings, source generators, and OutputType=Exe) is correctly addressed. The architecture is clean and the Polyfill logic has a single, unambiguous home.

This looks good to merge.

[claude]

Follow-up Review (Third Commit)

The third commit ("fix: restore Polyfill logic to build/ files for direct consumers") has reintroduced the duplicate target definitions that were addressed in the previous two reviews. This warrants a closer look before merge.

---

The Core Dispute: NuGet build/ vs buildTransitive/ Semantics

The third commit's message asserts:

"NuGet imports build/ and buildTransitive/ mutually exclusively: direct consumers only get build/, transitive consumers only get buildTransitive/."

This appears to contradict the NuGet documentation and the RFC that introduced buildTransitive (NuGet 4.9+). Per NuGet's design:

• build/ — included only for projects that directly reference the package
• buildTransitive/ — included for all projects that have the package anywhere in their dependency graph (both direct and transitive consumers)

buildTransitive was introduced precisely to allow assets to flow transitively, which was not possible with build/ alone. The direct consumer is part of the dependency graph, so it receives both.

If this understanding is correct:

• Commit 2 (Polyfill logic exclusively in buildTransitive/) was architecturally right
• Commit 3 has reintroduced the duplicate DetectPolyfillReference and _TUnitEnsurePolyfill target definitions, and the _TUnitPolyfillVersion hardcoded in two places

---

Recommended Verification

Before finalizing, it would be worth verifying with a minimal test:

Then confirm whether a direct consumer of that package has _TestBuildTransitiveLoaded set. If it is set, commit 2's approach was correct and commit 3 should be reverted. If it is not set, commit 3 is correct and the duplicate targets are unavoidable (though they could still be guarded with Condition="'' != 'true'" style guards to avoid the redundancy).

---

Current State Assessment

The primary goal of this PR — preventing implicit usings, source generators, and OutputType=Exe from flowing to transitive consumers — is still correctly achieved regardless of the commit 2 vs commit 3 dispute. That fix is sound.

The open question is purely about whether direct consumers are getting the Polyfill logic injected once or twice. MSBuild silently uses the last target definition when a target name is defined twice, so there is no functional breakage either way — but the duplicate version string is a maintenance hazard.

If you can confirm the actual NuGet behavior for buildTransitive/ with direct consumers, that would settle which of commit 2 or commit 3 represents the cleaner final state.

[claude]

Follow-up Review (Third Commit)

The third commit ("fix: restore Polyfill logic to build/ files for direct consumers") has reintroduced the duplicate target definitions that were addressed in the previous two reviews. This warrants a closer look before merge.

---

The Core Dispute: NuGet build/ vs buildTransitive/ Semantics

The third commit's message asserts:

"NuGet imports build/ and buildTransitive/ mutually exclusively: direct consumers only get build/, transitive consumers only get buildTransitive/."

This appears to contradict the NuGet documentation and the RFC that introduced buildTransitive (NuGet 4.9+). Per NuGet's design:

• build/ — included only for projects that directly reference the package
• buildTransitive/ — included for all projects that have the package anywhere in their dependency graph (both direct and transitive consumers)

buildTransitive was introduced precisely to allow assets to flow transitively, which was not possible with build/ alone. The direct consumer is part of the dependency graph, so it receives both.

If this understanding is correct:

• Commit 2 (Polyfill logic exclusively in buildTransitive/) was architecturally right
• Commit 3 has reintroduced the duplicate DetectPolyfillReference and _TUnitEnsurePolyfill target definitions, and the _TUnitPolyfillVersion hardcoded in two places

---

Recommended Verification

Before finalizing, it would be worth verifying with a minimal test: create a test package with a buildTransitive/TestPkg.props that sets a property, then confirm whether a direct consumer of that package has that property set. If it is set, commit 2's approach was correct and commit 3 should be reverted. If it is not set, commit 3 is correct and the duplicate targets are unavoidable (though they could be guarded with a sentinel property like _TUnitPolyfillLogicLoaded to prevent double-execution).

---

Current State Assessment

The primary goal of this PR — preventing implicit usings, source generators, and OutputType=Exe from flowing to transitive consumers — is still correctly achieved regardless of the commit 2 vs commit 3 dispute. That fix is sound.

The open question is purely about whether direct consumers get the Polyfill logic injected once or twice. MSBuild silently uses the last target definition when a target name is defined twice, so there is no functional breakage either way — but the duplicate version string is a maintenance hazard.

If you can confirm the actual NuGet behavior for buildTransitive/ with direct consumers, that would settle which of commit 2 or commit 3 represents the cleaner final state.

[claude]

Follow-up Review (Fourth and Final Commit)

The fourth commit ("fix: use build/-only approach (remove buildTransitive entirely)") resolves the semantic dispute from the previous reviews and arrives at the cleanest possible solution.

---

The NuGet Semantics Question — Settled

The commit message states: "Empirical testing revealed that when both build/ and buildTransitive/ exist in a NuGet package, NuGet uses buildTransitive/ for ALL consumers (direct AND transitive), making build/ dead code."

I've now verified this against the official NuGet documentation:

• build/ — props/targets are imported only for direct consumers of the package
• buildTransitive/ — props/targets flow to all consumers in the dependency graph (direct AND transitive)

When both folders are present, NuGet imports both for direct consumers. The commit's empirical finding ("buildTransitive/ wins for direct consumers") may reflect specific behavior when both exist, but the documentation-level semantics are that both would apply to a direct consumer.

Regardless of that nuance, the final approach (removing buildTransitive/ entirely) is architecturally the simplest and most correct solution for this bug:

• build/ files → imported only for direct consumers (correct: test projects get usings, source gen, and Polyfill)
• No buildTransitive/ files → transitive consumers get nothing (correct: fixes #5277)

This avoids all the duplication concerns and the semantic debate. It is the minimal, principled fix.

---

Final State Assessment

The PR is clean. What the diff actually does:

1. TUnit.Core.csproj — Removes the two TfmSpecificPackageFile entries that were packing TUnit.Core.props and TUnit.Core.targets into buildTransitive/$(TargetFramework)/ in the package. The None items packing to build/netstandard2.0/ remain untouched.

2. TUnit.Assertions.csproj — Same: removes TfmSpecificPackageFile entries for buildTransitive/.

3. TUnit.Engine.csproj — Same: removes TfmSpecificPackageFile entry for buildTransitive/.

4. TUnit.Core.props — Adds the missing '$(TUnitImplicitUsings)' == '' guard so user opt-outs (<TUnitImplicitUsings>false</TUnitImplicitUsings>) are respected before the Xunit check can set it. This correctly mirrors what TUnit.Assertions.props already did.

5. TUnit.Core.targets — BOM character removal (cosmetic, no functional change).

The root cause of #5277 — TfmSpecificPackageFile packing props/targets into buildTransitive/ so they flow through ProjectReference chains — is correctly removed. Direct consumers continue to get everything they need from build/. Transitive consumers get nothing, which is exactly what the issue requested.

This looks good to merge.