fix: resolve inherited instance data source members for MethodDataSource

[thomhurst]

Summary

Fixes #6162 — [MethodDataSource] targeting an instance member, combined with [InheritsTests] and a class-level DependencyInjectionDataSourceAttribute, failed with:

Cannot dynamically create an instance of type '...'. Reason: No parameterless constructor defined.
   at TUnit.Core.MethodDataSourceAttribute.GetOrCreateInstanceAsync(...)

Root cause

Both modes failed to recognise the data source as instance-targeting, so the engine never pre-created a properly-constructed test class instance (via the class-level data source) and MethodDataSourceAttribute fell back to Activator.CreateInstance:

• Source-gen mode: GenerateMethodDataSourceAttribute resolved the data source member with GetMembers on the test class symbol only — Roslyn's GetMembers does not include inherited members. With [InheritsTests] the member lives on a base class, so the lookup missed, and the generator emitted a plain MethodDataSourceAttribute without a Factory and without the InstanceMethodDataSourceAttribute conversion that triggers early instance creation.
• Reflection mode: extraction returned the raw attribute instance, which never gets upgraded to InstanceMethodDataSourceAttribute (the source generator normally does this at compile time), so the same Activator.CreateInstance fallback was hit — independent of [InheritsTests].

Changes

• TestMetadataGenerator.cs: walk the base-type chain (most-derived first) when resolving the data source member.
• ReflectionAttributeExtractor.cs: new ExtractMethodDataSources upgrades plain MethodDataSourceAttributes targeting an instance member to InstanceMethodDataSourceAttribute, mirroring the source generator's compile-time conversion. Method-level only — class-level data sources keep their existing circular-dependency guard.
• ReflectionTestDataCollector.cs: use the new extraction for method-level data sources (single choke point — all reflection paths that build method-level data sources route through BuildTestMetadata).
• TUnit.TestProject/Bugs/6162/Tests.cs: regression test — abstract base with an instance property data source, inherited by a sealed class constructed via a DI data source.

Verification

• Repro test: failed in both modes before, passes in both modes after
• *MethodDataSource* sweep (85 tests): identical results to main in both modes (1 pre-existing Bug3266 failure, stash-verified)
• *Inherit* sweep: 36/36 in both modes
• Source generator snapshot tests: 118 passed, no .received.txt produced
• TUnit.Engine and TUnit.Core.SourceGenerator build clean across all TFMs, no new IL trim warnings

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 critical

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
Security	1 critical

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

This is a solid, well-scoped fix for #6162. The root cause analysis in the PR description is accurate, and both failure modes (source-gen and reflection) are addressed. A few things worth discussing:

---

Source generator fix (TestMetadataGenerator.cs) ✅

Walking the base-type chain with searchType.BaseType is the correct approach here — Roslyn's GetMembers intentionally doesn't include inherited members, so this is the right fix. No issues.

---

Reflection fix — fragile property copying

The conversion from MethodDataSourceAttribute to InstanceMethodDataSourceAttribute manually copies two properties:

converted.Arguments = methodDataSource.Arguments;
converted.SkipIfEmpty = methodDataSource.SkipIfEmpty;

This will silently drop any new settable property added to MethodDataSourceAttribute in the future (e.g., an Order, Timeout, or SharedType equivalent). The Factory property is intentionally skipped here (it's only set by the source generator, so it's always null in reflection mode — that's correct), but the pattern is still brittle.

Suggested improvement: Add a protected or internal copy/clone method on MethodDataSourceAttribute, or a constructor overload that accepts the source attribute, so the upgrade path is self-contained and doesn't require callers to know which properties matter:

// Option A: virtual factory method on MethodDataSourceAttribute
public virtual MethodDataSourceAttribute AsInstanceVariant()
{
    var attr = ClassProvidingDataSource is { } t
        ? new InstanceMethodDataSourceAttribute(t, MethodNameProvidingDataSource)
        : new InstanceMethodDataSourceAttribute(MethodNameProvidingDataSource);
    attr.Arguments = Arguments;
    attr.SkipIfEmpty = SkipIfEmpty;
    return attr;
}

This way, when new properties are added to the base attribute they only need to be handled in one place. The current code scatters the copy logic in ReflectionAttributeExtractor, which is a poor home for it.

---

AmbiguousMatchException handling — misleading comment

catch (AmbiguousMatchException)
{
    // Ambiguous overloads - leave the attribute as-is and let runtime resolution handle it
    return false;
}

The comment says "let runtime resolution handle it" — but returning false skips the InstanceMethodDataSourceAttribute conversion, so if the overloaded method is an instance member, the runtime will hit the exact Activator.CreateInstance failure this PR was written to fix. The comment creates false confidence that the skip is safe.

Possible options:

1. If overloaded instance data source methods are not a supported use case, change the comment to say so explicitly.
2. If they should work, iterate over targetType.GetMethods(flags).Where(m => m.Name == memberName) and return true if any overload is non-static (conservative: if any overload is instance, treat it as instance).

At minimum the comment should be updated to: // Overloaded name — cannot determine statically; may fail at runtime if the member is an instance method.

---

Duplicated BindingFlags constant

The comment // Must stay in sync with MethodDataSourceAttribute.BindingFlags highlights a maintenance risk. Since MethodDataSourceAttribute.BindingFlags is private, the extractor has to duplicate it. If they ever diverge, the pre-check in TargetsInstanceMember would disagree with what GetDataRowsAsync resolves at runtime.

Suggested improvement: Promote the constant to internal in MethodDataSourceAttribute and reference it directly from ReflectionAttributeExtractor, eliminating the sync requirement.

---

Test coverage ✅

The regression test in TUnit.TestProject/Bugs/6162/Tests.cs accurately reproduces the reported scenario (abstract base with instance property data source, [InheritsTests], DI-constructed derived class with no parameterless constructor). It's well-structured and clearly documented.

---

Summary

The fix is correct and the overall approach is right — mirroring the source generator's compile-time conversion in reflection mode at the single choke point in BuildTestMetadata is a clean design. The three issues above are worth addressing, with the property-copy fragility being the most architecturally significant. Happy to approve once the AmbiguousMatchException comment is at least updated to be accurate.

[thomhurst]

Addressed all three review items in 8240009:

1. Property-copy fragility: conversion now lives on MethodDataSourceAttribute as an internal ToInstanceVariant() method — new settable properties only need handling in one place, next to the type that owns them. Factory remains intentionally uncopied (generator-only, always null in reflection mode; documented on the method).

2. AmbiguousMatchException handling: replaced GetMethod + catch with Type.GetMember enumeration, which never throws for overloaded names. Any instance overload now conservatively triggers the conversion instead of silently skipping it (the previous behavior would have re-exposed the Activator crash for overloaded instance data sources). The misleading comment is gone with the try/catch.

3. Duplicated BindingFlags: promoted MethodDataSourceAttribute.BindingFlags to internal (TUnit.Core already has InternalsVisibleTo TUnit.Engine) and the extractor references it directly — no sync comment needed.

Public API snapshots updated: the only diff is the compiler-generated async state machine name (<GetDataRowsAsync>d__21 → d__22) shifting due to the new internal method — no public surface change.

Re-verified: repro passes both modes, reflection-mode *MethodDataSource* sweep unchanged (84/85, 1 pre-existing Bug3266 failure also present on main), public API tests green on all four TFMs.

[claude]

Code Review: fix/6162-inherited-instance-method-data-source

This is a clean, well-targeted bug fix with accurate root-cause analysis and good test coverage. The dual-mode fix is correctly structured — source-gen and reflection paths are each fixed independently, which is the right approach.

Source Generator Fix (TestMetadataGenerator.cs)

The change from GetMembers to a base-type walk is correct — Roslyn's ITypeSymbol.GetMembers(name) is declared-members-only, so the inherited member was invisible before. The fix is minimal and focused. Taking members[0] is consistent with the pre-existing FirstOrDefault() behaviour.

One minor point: if someone has overloaded methods with the same name where some are static and some instance (contrived, but possible), only members[0] is checked for the isStatic decision. This was already the case before this PR, and the source generator later emits both the type and method name into the Factory lambda, so the right overload is ultimately resolved. Not a regression, just worth a note.

Reflection Fix (ReflectionAttributeExtractor.cs)

The TargetsInstanceMember design is sound. Using Type.GetMember with BindingFlags.Instance | FlattenHierarchy correctly traverses the full inheritance chain for instance members — instance members are already traversed by the Instance flag, and FlattenHierarchy additionally pulls in inherited static members. The conservative approach (any matching instance member → treat as instance-targeting) is the right call for overload ambiguity.

The and not InstanceMethodDataSourceAttribute guard in ExtractMethodDataSources correctly skips already-upgraded attributes, and since ExtractDataSources returns a new array, the in-place mutation is safe for concurrency.

ToInstanceVariant()

All user-settable state (Arguments, SkipIfEmpty) is copied. Factory is correctly excluded — it is always null in reflection mode since it is source-generator-populated. The internal visibility is appropriate.

Public API Snapshots

The d__21 → d__22 state-machine number bump is expected: the new ToInstanceVariant method shifts the compiler-generated async state machine number for GetDataRowsAsync. The snapshot updates are correct.

Regression Test

The repro in TUnit.TestProject/Bugs/6162/Tests.cs precisely targets the reported failure scenario (abstract base with an instance property data source, sealed subclass constructed via DI with no parameterless constructor). [EngineTest(ExpectedResult.Pass)] ensures it runs in both modes.

---

Overall, this is a solid, well-reasoned fix. No issues found. Good work.

[claude]

Code Review — Round 3

Both items raised in previous reviews (property-copy fragility, AmbiguousMatchException risk) have been addressed cleanly in the follow-up refactor commit.

Previous feedback — confirmed addressed

Property-copy fragility → ToInstanceVariant() on the attribute itself
Moving the conversion onto MethodDataSourceAttribute is the right call. New settable properties only need handling in one place, next to the type that owns them. The explicit Factory exclusion with inline documentation is clear and correct — it is always null in reflection mode.

AmbiguousMatchException risk → Type.GetMember enumeration
GetMember with the shared BindingFlags constant handles overloads without throwing. The conservative "any instance member → treat as instance-targeting" approach is appropriate. The BindingFlags constant is correctly promoted to internal and referenced from both sites rather than duplicated.

Current state

Everything looks good:

• The base-type walk in the source-gen path uses Roslyn's declared-members-only GetMembers, which is the correct API to iterate explicitly.
• ExtractMethodDataSources guards with and not InstanceMethodDataSourceAttribute before upgrading — no double-conversion possible.
• Arguments in ToInstanceVariant() is a reference copy rather than a clone, but attribute argument arrays are set once at construction and never mutated in this codebase, so this is acceptable.
• The public API snapshot changes (d__21 → d__22) are an expected compiler artefact from the new internal method shifting the async state machine ordinal — not a public surface change.
• Regression test precisely targets the reported failure path (abstract base + instance property data source + sealed derived class with no parameterless constructor + DI data source).

No further concerns. Solid fix.