Repository navigation
fix(sourcegen): generate a data source factory for [InstanceMethodDataSource] (#7225) - #7348
Conversation
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 55.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: generating a data source factory for InstanceMethodDataSource. Linked Issues check Direct issue [ #7225] requires a generated factory for[InstanceMethodDataSource]on an instance field, using the test-class instance for Native AOT. The reviewed changes route `InstanceMethodDataSou…Out of Scope Changes check The changes remain connected to [ #7225] and its shared data-source factory path. The object, task, value-task, async-enumerable, inaccessible-method, null-source, and tuple-handling changes preserve r…Full details: Linked Issues check
-
Explanation
Direct issue [
#7225] requires a generated factory for[InstanceMethodDataSource]on an instance field, using the test-class instance for Native AOT. The reviewed changes routeInstanceMethodDataSourceAttributethroughGenerateMethodDataSourceAttribute. Generated code reads instance fields throughFromInstanceRuntimeValueand keeps the instance attribute type. The_7225fixture and generator snapshots cover the instance field. The supplied test evidence reports passing source-generation, reflection-mode, and earlier Native AOT runs. The latest Native AOT run was not repeated, but the relevant routing and generated-code behavior are present.
Full details: Out of Scope Changes check
-
Explanation
The changes remain connected to [
#7225] and its shared data-source factory path. The object, task, value-task, async-enumerable, inaccessible-method, null-source, and tuple-handling changes preserve reflection-mode behavior after members use generated factories. The same shared-path fixes also support[MethodDataSource], which the description identifies as affected by the changed generator and helper code. The added public overloads support generated consumer code, and API snapshots record them. The supplied summary shows focused generator and_7225tests. No unrelated change is demonstrated.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 55.00% which is insufficient. The required threshold is 80.00%. 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 20 functions across 5 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
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 the rows at dawn
Tuples keep their shape as they hop on
Fields and properties join the queue
Null sources leave no rows to view
Task-shaped data finds its way
The test runs cleanly through the day
Soft paws stamp the source-generated hay
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ReviewThe fix is small and targeted.
Looks good to me. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b07a2a60f
ℹ️ 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".
 [Medium impact] The PR appears safe to merge; no outstanding blocking issue was found. SummaryGenerates factories for
Reviews (9) · Last reviewed commit: "refactor(sourcegen): build complete refl..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…aSource] (#7225) [InstanceMethodDataSource] went through the generic attribute writer and got no Factory, so Native AOT fell back to a reflection member lookup that found no trimmed field metadata. Route it through GenerateMethodDataSourceAttribute and keep the declared attribute type so the engine still sees IAccessesInstanceData.
0b07a2a to
88c694f
Compare
Review of #7348Verdict: LGTM. The fix is small and sits in the right place. What I checked
Optional, non-blocking
I did not run the tests myself. This review is from reading the diff and the surrounding generator code. |
…nstanceMethodDataSource] (#7225) - An object-typed property or method named by [InstanceMethodDataSource] decides its rows from the runtime value, as the attribute's reflection lookup did before it had a generated factory. - A parameterless private or protected data source method is read through a constant-name typeof(T).GetMethod lookup that Native AOT keeps, like private fields and properties, instead of a direct call the generated class cannot compile. - Share the InstanceMethodDataSourceAttribute name as a constant.
Review: #7348 – generate a factory for
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de2b2a1665
ℹ️ 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".
…ce for [InstanceMethodDataSource] (#7225) A private parameterless data source method beside a parameterless generic overload of the same name made the generated GetMethod lookup ambiguous; the factory now filters out generic method definitions instead. A null value from an [InstanceMethodDataSource] member became one null row, so SkipIfEmpty no longer skipped the test as the reflection path does. The generated factory now reads null as an empty source.
|
@codex review |
Review of #7348The approach is sound. Routing I checked the diff and found no blocking bugs. The snapshot and fixture coverage is good, including the Things worth a second look, none blocking:
Overall this looks good to merge once CI is green. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bedcaba9a3
ℹ️ 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".
…7225) FromTask and FromAsyncEnumerable (and their instance variants) awaited or enumerated a null value and threw. The attribute's reflection path reads a null source as one with no rows, so [InstanceMethodDataSource(..., SkipIfEmpty = true)] on such a member now skips in source-gen mode as it does in reflection mode.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 388bbb0719
ℹ️ 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".
ReviewThe change looks correct and well scoped. I reviewed the diff only; I did not build it or run any tests. What it does
Observations (non-blocking)
No blocking issues found. |
… reflection fallback (#7225) TUnit0011 only analyzes [MethodDataSource], so a void method named by [InstanceMethodDataSource] reached the generated factory and failed with CS0030.
|
Dispositions for the non-blocking points in the Claude review of bedcaba:
|
|
@codex review |
Review of #7348I found no blocking issues. The fix is correct and matches the project's AOT goals. What I checked
Suggestions (non-blocking)
Overall this looks good to merge once CI is green. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…d row elements (#7225) Generated data source factories unwrap a reference Tuple row into its elements, as they do a value tuple, but the generated invoker only rebuilt value tuples, so a test taking one Tuple<...> parameter failed with 'Expected exactly 1 argument'. Reflection mode keeps the row whole. This affected [MethodDataSource] already and [InstanceMethodDataSource] once it got a generated factory.
Summary
[InstanceMethodDataSource]went through the generic attribute writer inTestMetadataGenerator.GenerateDataSourceAttribute, so it never got a generatedFactory. At run timeMethodDataSourceAttribute.GetDataRowsAsyncthen looked the member up by reflection, which under Native AOT finds no metadata for a field and throwsMethod, property, or field 'Rows' not found.InstanceMethodDataSourceAttributethroughGenerateMethodDataSourceAttribute, the same path[MethodDataSource]uses.InstanceMethodDataSourceAttribute(notMethodDataSourceAttribute) so the engine still seesIAccessesInstanceData, as it does in reflection mode.[InstanceMethodDataSource]had when it read members by reflection. Anobject-typed property or method goes through the runtime-value helper, soobject Rows => new[] { 1, 2 }still gives two rows. A parameterless private or protected method is read through a constant-nametypeof(T).GetMethod(...)lookup that Native AOT keeps, like private fields and properties already are, instead of a direct call that does not compile. Other inaccessible methods keep the attribute's reflection fallback. A null value from an[InstanceMethodDataSource]member is an empty source, soSkipIfEmptystill skips the test. The method change also applies to[MethodDataSource], where such a method previously produced uncompilable generated code.Tests
tests/TUnit.TestProject/Bugs/_7225(instance field, instance property, static method via[InstanceMethodDataSource]),[EngineTest(ExpectedResult.Pass)].Tests7225asserting a factory is generated for each member;Tests1821snapshot updated (its instance method now gets a factory too).Bugs/_7225/InstanceMethodDataSourceMemberShapeTests(object property, object method, single value-tuple and reference-tuple parameters, private method, a private method beside a generic overload of the same name, a null source withSkipIfEmpty) and snapshotTests7225.MemberShapes.Validation
On this follow-up (non-generic task factory inference):
TaskandValueTaskmembers useFromRuntimeValue/FromInstanceRuntimeValue, so no generic type argument needs to be inferred. This also applies to static[MethodDataSource]members. The member is still read by the generated factory, without the attribute's reflection member lookup.Taskis awaited; non-genericValueTaskis passed as a scalar. Changing resultless-task semantics in both modes is tracked in Non-generic ValueTask data sources reach the test without being awaited #7411._7225tests pass in source-gen and reflection modes (28 passed, 3 intended skips each). Generator snapshots reviewed and updated; full generator suite: 217 passed, 1 skipped (net10.0). No public API change. Native AOT publishing was not re-run for this follow-up.On c4c00e4 (Codex and Greptile follow-up: a one-element
System.Tuplerow holding null):Tuple.Create<string?>(null)row into[null], so a test takingTuple<string?>got null. The invoker cannot tell that apart from[Arguments(null)]. When the test takes a single tuple, theFromInstance*helpers and new public overloadsDataSourceFactories.From{Value,Enumerable,Task,ValueTask,AsyncEnumerable}(DataGeneratorMetadata, ...)now keep a tuple row whole, as reflection mode'sToObjectArrayWithTypesdoes.[InstanceMethodDataSource]passes the metadata to its static helpers too. Instance[MethodDataSource]members get the same behavior throughFromInstance*. Static[MethodDataSource]members are unchanged (Source-gen: static [MethodDataSource] row Tuple.Create<string?>(null) reaches a single Tuple<string?> parameter as null #7402).One_Element_Reference_Tuple_Holding_Null_Stays_A_TupleandStatic_One_Element_Reference_Tuple_Holding_Null_Stays_A_Tuple. Both fail without the fix.TUnit.Core.SourceGenerator.Tests(net10.0): 217 passed, 1 skipped (Tests7225.TestandTests7225.MemberShapessnapshots updated).TUnit.PublicAPI: 74 passed (additive snapshot update).TUnit.TestProjectnet10.0 Release, source-gen and--reflection:Bugs._7225gives 24 passed and 3 skipped (the intendedSkipIfEmptyskips).Bugs._1821,Bugs._3992,Bugs._7157,TupleDataSourceDrivenTests,ClassTupleDataSourceDrivenTests,NestedTupleDataSourceTests,TupleUnwrappingTests,EnumerableTupleDataSourceDrivenTests,ClassDataSourceTupleTests,SimpleTupleTestandGenericInstanceMethodDataSourceTestspass in both modes.Type.IsAssignableTo(typeof(ITuple))on the parameter metadata, with no reflection lookup.ValueTuple<T>parameter generates code that does not compile (Source-gen: a single one-element ValueTuple<T> test parameter generates code that does not compile #7401).On e57d6aa (Codex and Greptile follow-up: null and omitted
System.Tuplearguments):Tuple<T>was wrapped intonew Tuple<T>(null), and[Arguments]with an optionalTuple<...>? value = nullthrew.One_Element_Reference_Tuple_Null_Argument_Stays_NullandOptional_Reference_Tuple_Gets_Its_Default.TUnit.Core.SourceGenerator.Tests(net10.0): 217 passed, 1 skipped (Tests7225.MemberShapessnapshot updated).TUnit.TestProjectnet10.0 Release, source-gen and--reflection:Bugs._7225gives 22 passed and 3 skipped (the intendedSkipIfEmptyskips).Bugs._1821,TupleDataSourceDrivenTestsandClassTupleDataSourceDrivenTestspass in both modes.Also in scope, beyond the factory routing (each is needed for
[InstanceMethodDataSource]to keep its reflection-mode behavior, and each also applies to[MethodDataSource]):System.Tuple<T1..T7>parameter from unwrapped row elements.DataSourceFactories.FromTask/FromAsyncEnumerable(and their instance variants) treat a null value as an empty source.On 90a9df5 (Codex follow-up: a reference tuple row for a single
Tuple<...>parameter):System.Tuplerow into its elements, as they do a value tuple, but the generated invoker rebuilt only value tuples. A test with oneTuple<int, int>parameter failed in source-gen mode withExpected exactly 1 argument, but got 2. This already happened with[MethodDataSource]onmain. The invoker now rebuildsSystem.Tuple<T1..T7>as well. With one element, it first checks whether the argument is already the tuple.TUnit.Core.SourceGenerator.Tests(net10.0): 217 passed, 1 skipped (Tests7225.MemberShapessnapshot updated).TUnit.TestProjectnet10.0 Release, source-gen and--reflection:Bugs._7225gives 20 passed and 3 skipped (the intendedSkipIfEmptyskips).Bugs._1821,_7092,_7173,_7212,NestedTupleDataSourceTests,SimpleTupleTest,TupleDataSourceDrivenTests,EnumerableTupleDataSourceDrivenTests,TupleUnwrappingTests,ClassTupleDataSourceDrivenTestsandClassDataSourceTupleTestsall pass in both modes.new Tuple<...>(...)and uses no reflection.On 02b0d66 (Claude review follow-up: a
voidmethod). TUnit0011 analyzes only[MethodDataSource], so avoidmethod named by[InstanceMethodDataSource]reached the factory and the generated code failed withCS0030: Cannot convert type 'void' to 'object'. Such a method now keeps the attribute's reflection fallback. Checked with a throwaway fixture:TUnit.TestProjectfailed to build without the guard and built with it. Re-ranTUnit.Core.SourceGenerator.Tests(217 passed, 1 skipped) andBugs._7225in both modes (16 passed, 3 skipped).On 388bbb0 (Codex follow-up: a null
TaskorIAsyncEnumerablemember is an empty source):DataSourceFactories.FromTask/FromAsyncEnumerableand their instance variantsyield breakon a null value, as reflection mode does, instead of throwing. This also applies to[MethodDataSource], where such a member used to throw aNullReferenceException.TUnit.Core.SourceGenerator.Tests(net10.0): 217 passed, 1 skipped after updating theTests7225.MemberShapessnapshot (two new fixture members).TUnit.TestProjectnet10.0 Release, source-gen and--reflection:Bugs._7225gives 16 passed and 3 skipped (the intendedSkipIfEmptyskips). Source-gen:Bugs._1821,Bugs._3992,Bugs._7092,Bugs._7157andGenericInstanceMethodDataSourceTestspass.On bedcaba (Codex follow-up: a generic overload next to a private method, and a null source with
SkipIfEmpty):TUnit.Core.SourceGenerator.Tests(net10.0): 217 passed, 1 skipped.Tests1821,Tests7225.TestandTests7225.MemberShapessnapshots updated for the null-as-empty guard.TUnit.TestProjectnet10.0 Release, source-gen and--reflection:Bugs._7225gives 16 passed and 1 skipped (the intendedSkipIfEmptyskip).Bugs._1821,Bugs._3992,Bugs._3997,Bugs._7092,Bugs._7157andGenericInstanceMethodDataSourceTestsall pass.Bugs._7225gives 16 passed and 1 skipped.Bugs._1821gives 2 passed.SkipIfEmpty, a null source still behaves differently in the two modes. That difference already exists for[MethodDataSource]; it is tracked in Null-valued MethodDataSource/InstanceMethodDataSource member gives different tests in reflection and source-gen modes #7388.On de2b2a1:
TUnit.Core.SourceGenerator.Tests(net10.0): 217 passed, 1 skipped.TUnit.TestProjectnet10.0 Release, source-gen and--reflection:Bugs._7225(14),Bugs._1821,Bugs._3992,Bugs._3997,Bugs._7157,GenericInstanceMethodDataSourceTestsall pass.Bugs._7108passes except its by-design non-[EngineTest]fixtures.Bugs._722514/14,Bugs._1821,Bugs._3992,Bugs._7157pass.Bugs._710890 pass, plus its 3 by-design failures.On the original head:
TUnit.Core.SourceGenerator.Tests(net10.0): 216 passed, 1 skipped.TUnit.TestProjectnet10.0 Release, source-gen and--reflection, all passing in both modes:Bugs._7225(6),Bugs._1821,Bugs._3992,Bugs._3997,Bugs._6361,Bugs._7157,GenericInt,GenericInstanceMethodDataSourceTests.Issue2862BugReproductionhas its usual intentionalSevenIsSevenfailure, identical in both modes.dotnet publish -c Release -f net10.0 -r win-x64 -p:Aot=trueofTUnit.TestProject, then ran the published exe:Bugs._72256/6,Bugs._18212/2,Bugs._39926/6,Bugs._715713/13 pass.Bugs._7108has its 3 intentional-failure fixtures (PrivateRowTypeDataSourceTests,NoMatchingOverloadDataSourceTests, documented as non-[EngineTest]by design) and 90 passes.Closes #7225
Summary by CodeRabbit