You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Match method data source attributes by fully qualified identity - #7649
Generic test inference could treat unrelated attributes named MethodDataSourceAttribute or InstanceMethodDataSourceAttribute as TUnit data sources, introducing type instantiations from unrelated metadata. Classification, duplicate-row grouping, deferred sources, and class-source inference now check the fully qualified TUnit identity consistently.
Regression fixtures put both lookalike attributes beside genuine static and instance sources. The reviewed generator snapshot emits only the intended int instantiations, and engine integration coverage checks exactly two passing rows in both discovery modes.
TestProject net10.0 build passed; targeted fixture passed exactly 2 tests in generated mode and exactly 2 in reflection mode.
Engine integration suite left to CI per repository instructions. Other target frameworks and Native AOT publishing were not run locally; no reflection execution paths changed.
Tests using method-based data sources now generate and run with the intended values, including generic static and instance sources. Unrelated data-source attributes no longer contribute extra test rows, helping ensure test counts and results reflect the applicable sources.
No actionable comments were generated in the recent review.
Walkthrough
The source generator now identifies method data-source attributes by fully qualified, non-generic names in inference, filtering, and bucketing paths. Regression tests combine TUnit sources with unrelated attributes that share their simple names and verify generated and runtime test results.
Inference, filtering, and bucketing paths use qualified attribute identities for method data sources. A shared predicate recognizes the static and instance attributes. The [Arguments] bucket now matches the exact qualified ArgumentsAttribute name.
Static and instance source fixtures tests/TUnit.TestProject/Bugs/_7648/MethodSourceIdentityTests.cs
The fixture combines unrelated same-name attributes with TUnit static and instance sources. Both tests use integer source values; the fixture also defines an unrelated string source.
Generated output and engine assertions tests/TUnit.Core.SourceGenerator.Tests/Bugs/7648/Tests7648.cs, tests/TUnit.Core.SourceGenerator.Tests/Tests7648.Test.verified.txt, tests/TUnit.Engine.Tests/Issue7648Tests.cs
The generator test checks that the int source identities are present and string variants are absent. The engine test checks for two passed tests and zero failed tests.
The change appears ready to merge after normal checks; no concrete regression remains identified.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title "Match method data source attributes by fully qualified identity" matches the main change. The raw summary says the generator now identifies method and instance method data-source attributes…
Linked Issues check
Directly linked issue #7648 requires fully qualified identity checks for method data source classification and filtering. It also requires regression fixtures with unrelated MethodDataSourceAttribute …
Out of Scope Changes check
Changes in the reviewed diff fall into four groups: 1. Method data source identity checks in TestMetadataGenerator.cs. These implement #7648 directly. 2. Fixtures and tests: Bugs/_7648 fixtures, the T…
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Title check
Explanation
The title "Match method data source attributes by fully qualified identity" matches the main change. The raw summary says the generator now identifies method and instance method data-source attributes by their globally qualified, non-generic names instead of short names. The PR objectives describe the same change to classification, duplicate-row grouping, deferred sources, and class-source inference. The title is specific and concise, and it names the primary change.
Full details: Linked Issues check
Explanation
Directly linked issue #7648 requires fully qualified identity checks for method data source classification and filtering. It also requires regression fixtures with unrelated MethodDataSourceAttribute and InstanceMethodDataSourceAttribute types, and matching generated and reflection rows.
Code evidence (merge base to reviewed head, TestMetadataGenerator.cs):
Classification in TryAddInstantiation (formerly simple-name check at about line 1260) now calls IsMethodDataSourceAttribute, which compares GloballyQualifiedNonGeneric() against global::TUnit.Core.MethodDataSourceAttribute or InstanceMethodDataSourceAttributeName.
Duplicate-row grouping (JoinsDuplicate and methodRow join) uses the same helper.
Source filtering (formerly the simple-name check at about line 1469) compares the fully qualified name.
The deferred instance-source join, the bucket switch, the instance-explicit-types helper, the uninferred-failure path, and GetOwnClassMethodDataSourcesForInstantiation all use fully qualified identity.
Test evidence: the PR adds Bugs/_7648 fixtures with lookalike attributes beside genuine static and instance sources, a verified generator snapshot (Tests7648.Test.verified.txt) that emits only int instantiations, and Issue7648Tests that checks two passing rows in both TestMode values. The test results are author-reported. I did not run them. The snapshot and fixture contents were taken from the PR summary, not from direct file reads.
The shared identity helper suggested in the issue is present. No unmet coding requirement was found in the reviewed changes.
Full details: Out of Scope Changes check
Explanation
Changes in the reviewed diff fall into four groups:
Method data source identity checks in TestMetadataGenerator.cs. These implement #7648 directly.
Fixtures and tests: Bugs/_7648 fixtures, the Tests7648 generator test, the verified snapshot, and Issue7648Tests. These support #7648 directly.
An ArgumentsAttribute bucket change. The switch case changed from a simple name plus IsOrInherits check to an exact match on global::TUnit.Core.ArgumentsAttribute. This is not a method data source attribute. It uses the same identity-matching pattern in the same switch, and it removes simple-name matching of lookalike ArgumentsAttribute types. The PR description does not name this change. The PR objectives say adjacent ArgumentsAttribute simple-name checks are tracked in #7650. The connection to #7648 is by mechanism, not by requirement. This change is minor and does not add new behavior for method data sources.
The non-null guard on methodRow, added to remove a nullable warning. This is a supporting change.
No demonstrated unrelated change was found.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 4 files. (1 skipped: 1 unsupported.)
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each source by name, No stray twin joins the test-run game. The static rows bring integers through, The instance rows bring integers too. Two tests pass beneath the moon, While string-shaped rows leave the room.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 924b89ab-16be-44ec-adbe-d3c15feff231
Commits
Reviewing files that changed from the base of the PR and between b9d47de and d47986c.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.
The change is correct and well scoped. Name-only matching (Name is "MethodDataSourceAttribute") is replaced with fully qualified identity checks. I checked that GloballyQualifiedNonGeneric exists in the generator's TypeExtensions. The new IsMethodDataSourceAttribute helper and the InstanceMethodDataSourceAttributeName constant remove most of the duplicated string literals.
The regression fixtures put lookalike attributes beside the real ones. The generator snapshot and the engine test cover both discovery modes.
Non-blocking suggestions:
A few spots still repeat the literal "global::TUnit.Core.MethodDataSourceAttribute" and "global::TUnit.Core.ArgumentsAttribute". These are the bucket switch, the ArgumentsAttribute checks in TryAddInstantiation, and the GetOwnClassMethodDataSources… check. Add constants for them next to InstanceMethodDataSourceAttributeName. Better still, give ArgumentsAttribute its own helper like the one for method data sources, so the two identity checks cannot drift apart.
specificAttr.AttributeClass?.Name == "ArgumentsAttribute" (around line 1050) and methodRow?.AttributeClass?.Name == "ArgumentsAttribute" (around line 1060) still match by name only. This is the same class of bug the PR fixes, so a lookalike ArgumentsAttribute would be treated as a literal row. Please convert these too.
StaticPropertyInitializationGenerator.cs:322 still uses attributeClassName == "MethodDataSourceAttribute". It may be out of scope, but it looks like the same hazard. Please check it or file a follow-up issue.
In the typedDataSourceBucket loop, GloballyQualifiedNonGeneric() is called twice per attribute. Hoist it into a local.
Overall this is good to merge once the ArgumentsAttribute name checks are addressed or deliberately deferred.
Reviewed every conversation comment, review, inline comment and review thread on e032133. Codex reports completed with no findings; Greptile has no actionable findings; there are no inline threads.
Disposition of Claude's non-blocking suggestions: the remaining ArgumentsAttribute simple-name checks extend beyond the two duplicate-row checks (class-row filtering also uses short names), and the static-property parser is a separate path. Confirmed both on current main and tracked their audit and regression coverage in #7650. Deliberately defer those existing adjacent paths to that follow-up so this PR retains #7648's method-source inference scope. The constants/helper consolidation and repeated display conversion are also recorded there as non-blocking cleanup.
Claude's verdict permits merge once the ArgumentsAttribute checks are addressed or deliberately deferred; the follow-up above supplies that disposition. Promoting this unchanged, reviewed head to ready starts the full CI checks and a fresh Claude review. Auto-merge will be armed only after that Claude run passes and its comment is checked.
The change is correct and focused. It replaces name-only checks (Name is "MethodDataSourceAttribute") with fully qualified identity checks through a shared IsMethodDataSourceAttribute helper and a const for the instance variant. The lookalike-attribute fixtures cover both the generator snapshot and the engine run in both modes.
Notes:
Leftover name-only check:CodeGenerators/StaticPropertyInitializationGenerator.cs:322 still compares attributeClassName == "MethodDataSourceAttribute". It is outside the inference path this PR fixes, but it is the same class of bug. Either fix it here or file a follow-up issue.
Repeated calls:GloballyQualifiedNonGeneric() is called twice in a row in the typed-source bucket loop (around line 1468). Hoist it into a local. The generator runs incrementally, so cheap string work matters.
Constants:global::TUnit.Core.MethodDataSourceAttribute and global::TUnit.Core.ArgumentsAttribute are inline string literals, while the instance variant has a const. Making all three consts would be more consistent.
Symbol comparison: if a well-known type lookup is available, SymbolEqualityComparer on the symbol avoids string allocation. That would be a broader refactor, not needed for this PR.
None of these block merging. Nice regression coverage.
Resolved the new conflict with main in d47986c. Run 42037264658 completed successfully; it is no longer the blocker. Rebased onto b9d47de, retaining main’s qualified ArgumentsAttribute checks and this PR’s qualified method-source checks at the shared insertion points. An explicit non-null row guard also removes the nullable warning introduced by the helper call.
Validation: generator snapshots passed (353 passed, 1 skipped), with no snapshot changes. TestProject net10.0 single-worker build passed; the targeted MethodSourceIdentityTests fixture passed exactly 2 tests in generated mode and exactly 2 in reflection mode. The initial parallel build hit the existing MSB4166 problem tracked by #7468. Engine integration remains for CI per repository instructions.
Returned to draft for fresh Claude and Codex review of the rebased head. Prior non-blocking cleanup remains tracked in #7650, which now has PR #7651; this PR does not depend on that follow-up.
The change looks correct and well scoped. Every bare Name comparison for MethodDataSourceAttribute and InstanceMethodDataSourceAttribute in TestMetadataGenerator.cs now uses the fully qualified TUnit identity. That covers classification, duplicate-row joining, deferred sources, the uninferred-source bucket and the class-level source scan.
Checked:
InstanceMethodDataSourceAttributeName is defined at line 27. The new IsMethodDataSourceAttribute helper removes the repeated Name is ... or ... checks.
The methodRow is not null guard avoids passing a null row to AddRow. The old code relied on null-conditional behaviour there.
The regression fixtures put lookalike attributes next to the genuine ones. The snapshot and the exact-count engine test, run in both modes, would catch a regression.
Non-blocking suggestions:
src/TUnit.Core.SourceGenerator/CodeGenerators/StaticPropertyInitializationGenerator.cs:322 still uses attributeClassName == "MethodDataSourceAttribute". It may have the same lookalike problem for static properties. Please check whether it needs the same fix, or open a follow-up issue.
Consider moving IsMethodDataSourceAttribute and the name constants into a shared helper such as DataSourceAttributeHelper. Other generators could then reuse them, which avoids this kind of drift.
GloballyQualifiedNonGeneric() builds a string on each call. That is fine for these paths, but a SymbolEqualityComparer check against the compilation's well-known type would avoid the allocation in hot loops.
I did not run the tests. The PR description says the engine integration suite is left to CI, so that result still needs to come from CI.
Run 42037264658 succeeded and is no longer a blocker. Rechecked all conversation comments, reviews, inline comments and review threads: Claude and Codex both approve current head d47986c, with no unresolved threads. Claude’s non-blocking static-property audit and shared-helper/allocation suggestions remain covered by #7650 and its open PR #7651; neither is a prerequisite for this scoped fix.
Promoted the unchanged head to ready. Full CI is now running, and promotion started Claude run 42048096489. Auto-merge must wait until that run passes and its new review comment is dispositioned. No code changes or local validation were needed in this unit.
The PR replaces the simple-name checks (Name is "MethodDataSourceAttribute" ...) with fully qualified identity checks. That closes the lookalike-attribute leak from #7648. I checked the diff and found no correctness issues.
The checks are consistent across classification, duplicate-row grouping, deferred sources, constraint-violation diagnostics and class-source inference. A shared IsMethodDataSourceAttribute helper and an InstanceMethodDataSourceAttributeName constant replace the repeated string literals.
The bucket switch now matches on the qualified name. This is equivalent to the old ArgumentsAttribute when IsOrInherits check, because the old Name match only accepted that exact name anyway.
The methodRow is not null guard in the duplicate-join branch is a small null-safety improvement.
The regression fixtures put lookalike attributes beside real static and instance sources. The snapshot and the DoesNotContain assertions confirm only int instantiations are emitted. The engine test covers both discovery modes.
Optional follow-up: "global::TUnit.Core.MethodDataSourceAttribute" is still a literal in a few places, while the instance variant has a constant. A matching MethodDataSourceAttributeName constant would make the pair symmetric.
Run 42048096489 completed successfully on d47986c; the earlier wait is cleared. Read every review, inline comment, review thread and PR conversation comment again. Claude’s post-promotion review is LGTM, Codex’s completed summary names this exact head and has no findings, and CodeRabbit and Greptile report no actionable findings. No inline comments or review threads exist.
The optional constants/helper/allocation cleanup remains dispositioned in #7650 and its open PR #7651; these are follow-ups rather than prerequisites. The PR body already closes #7648. All current CI checks, including both Engine tests and AOT platforms, passed. Current main protection requires those checks and resolved conversations, with no formal approval count. Proceeding through the repository merge gate for the unchanged head.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub Actions
Inactive
Generic test inference could treat unrelated attributes named
MethodDataSourceAttributeorInstanceMethodDataSourceAttributeas TUnit data sources, introducing type instantiations from unrelated metadata. Classification, duplicate-row grouping, deferred sources, and class-source inference now check the fully qualified TUnit identity consistently.Regression fixtures put both lookalike attributes beside genuine static and instance sources. The reviewed generator snapshot emits only the intended
intinstantiations, and engine integration coverage checks exactly two passing rows in both discovery modes.Validation:
Closes #7648
Summary by CodeRabbit