Skip to content

Conversation

@thomhurst
Copy link
Owner

No description provided.

Copilot AI review requested due to automatic review settings December 22, 2025 20:09
@thomhurst thomhurst enabled auto-merge (squash) December 22, 2025 20:10
@claude
Copy link
Contributor

claude bot commented Dec 22, 2025

Pull Request Review: Fix MatrixMethod attribute ignored for enum parameter type

Summary

This PR fixes a bug where MatrixMethodAttribute was being ignored for enum parameter types in the source-generated mode. The fix introduces a new marker interface IDataSourceMemberAttribute to distinguish parameter-level data source attributes from test-level ones, ensuring they get properly cached by the source generator for AOT compatibility.


Strengths

  1. Well-Scoped Fix - The solution correctly identifies the root cause
  2. AOT-Compatible - Aligns with TUnit AOT/trimming requirements
  3. Snapshot Tests Updated - All snapshot tests properly updated as .verified.txt files
  4. Regression Test Added - New test case provides clear validation
  5. Public API Snapshots Updated - All target framework snapshots updated

Concerns and Recommendations

CRITICAL: Verify Dual-Mode Compatibility

Per Rule 1 in CLAUDE.md: ALL changes must work identically in both execution modes.

Verification Needed: Confirm that MatrixAttribute implements or inherits from IDataSourceAttribute so the filtering logic in CombinedDataSourcesAttribute.cs:129-131 works correctly.

The cached attributes are now Attribute[] but then filtered with .OfType IDataSourceAttribute. This will only work if MatrixAttribute and MatrixMethodAttribute implement IDataSourceAttribute.

Action: Please verify that the reflection mode also works correctly by running tests with reflection-based discovery enabled.


Code Quality Assessment

  • Code Style: Follows modern C# conventions
  • Performance: No performance impact; improves AOT scenarios
  • Security: No security concerns
  • Breaking Changes: Minor API change but marked EditorBrowsable Never for internal use

Recommendation

Status: Approve with verification requested

Required Verification:

  1. Confirm MatrixAttribute implements IDataSourceAttribute
  2. Verify the test runs in both execution modes successfully

Suggested Improvements:

  • Add documentation explaining the Attribute[] to IDataSourceAttribute[] filtering pattern
  • Link IDataSourceMemberAttribute docs to MatrixAttribute as example

This is a well-crafted fix addressing the root cause. Great work!

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

TUnit.Core.SourceGenerator/CodeGenerationHelpers.cs:76

  • The method GenerateParameterMetadataArray includes logic to generate CachedDataSourceAttributes for both IDataSourceAttribute and IDataSourceMemberAttribute implementations (lines 51-71). However, this method appears to be unused in the codebase - there are no call sites for it.

The actual parameter metadata generation uses MetadataGenerationHelper.WriteParameterMetadata, which does NOT include any logic to populate CachedDataSourceAttributes. This means that the AOT-compatible caching mechanism described in the comments is not actually being utilized.

Either:

  1. WriteParameterMetadata should be updated to include the same CachedDataSourceAttributes generation logic, or
  2. The code should be refactored to use GenerateParameterMetadataArray instead of WriteParameterMetadata, or
  3. If GenerateParameterMetadataArray is truly unused/dead code, it should be removed to avoid confusion.

Without the caching working properly, the framework will fall back to reflection at runtime, which defeats the purpose of the AOT compatibility improvements.

    public static string GenerateParameterMetadataArray(IMethodSymbol method)
    {
        if (method.Parameters.Length == 0)
        {
            return "System.Array.Empty<global::TUnit.Core.ParameterMetadata>()";
        }

        using var writer = new CodeWriter("", includeHeader: false);
        writer.SetIndentLevel(2);
        using (writer.BeginArrayInitializer("new global::TUnit.Core.ParameterMetadata[]"))
        {
            foreach (var param in method.Parameters)
            {
                var parameterIndex = method.Parameters.IndexOf(param);
                var containsTypeParam = ContainsTypeParameter(param.Type);
                var typeForConstructor = containsTypeParam ? "object" : param.Type.GloballyQualified();

                using (writer.BeginObjectInitializer($"new global::TUnit.Core.ParameterMetadata(typeof({typeForConstructor}))", ","))
                {
                    writer.AppendLine($"Name = \"{param.Name}\",");
                    writer.AppendLine($"TypeInfo = {GenerateTypeInfo(param.Type)},");
                    writer.AppendLine($"IsNullable = {param.Type.IsNullable().ToString().ToLowerInvariant()},");
                    var paramTypesArray = GenerateParameterTypesArray(method);
                    if (paramTypesArray == "null")
                    {
                        writer.AppendLine($"ReflectionInfo = typeof({method.ContainingType.GloballyQualified()}).GetMethods(global::System.Reflection.BindingFlags.Public | global::System.Reflection.BindingFlags.NonPublic | global::System.Reflection.BindingFlags.Instance | global::System.Reflection.BindingFlags.Static).FirstOrDefault(m => m.Name == \"{method.Name}\" && m.GetParameters().Length == {method.Parameters.Length})?.GetParameters()[{parameterIndex}],");
                    }
                    else
                    {
                        writer.AppendLine($"ReflectionInfo = typeof({method.ContainingType.GloballyQualified()}).GetMethod(\"{method.Name}\", global::System.Reflection.BindingFlags.Public | global::System.Reflection.BindingFlags.NonPublic | global::System.Reflection.BindingFlags.Instance | global::System.Reflection.BindingFlags.Static, null, {paramTypesArray}, null)!.GetParameters()[{parameterIndex}],");
                    }

                    // Generate cached data source attributes for AOT compatibility
                    // Include both IDataSourceAttribute and IDataSourceMemberAttribute implementations
                    var dataSourceAttributes = param.GetAttributes()
                        .Where(attr => attr.AttributeClass != null &&
                               attr.AttributeClass.AllInterfaces.Any(i =>
                                   i.Name == "IDataSourceAttribute" || i.Name == "IDataSourceMemberAttribute"))
                        .ToArray();

                    if (dataSourceAttributes.Length > 0)
                    {
                        writer.AppendLine($"CachedDataSourceAttributes = new global::System.Attribute[]");
                        writer.AppendLine("{");
                        writer.SetIndentLevel(3);
                        foreach (var attr in dataSourceAttributes)
                        {
                            var attrCode = GenerateAttributeInstantiation(attr, method.Parameters);
                            writer.AppendLine($"{attrCode},");
                        }
                        writer.SetIndentLevel(2);
                        writer.Append("}");
                    }
                }
            }
        }
        return writer.ToString().TrimEnd(); // Trim trailing newline for inline use
    }

@claude
Copy link
Contributor

claude bot commented Dec 22, 2025

This PR fixes a bug where MatrixMethodAttribute was being ignored during source generation for enum parameters. The fix introduces a new marker interface IDataSourceMemberAttribute to ensure these attributes are properly cached by the source generator for AOT-compatible runtime access.

Key Changes:

  • Added IDataSourceMemberAttribute marker interface
  • MatrixAttribute now implements IDataSourceMemberAttribute
  • Source generator caches both IDataSourceAttribute and IDataSourceMemberAttribute
  • Updated ParameterMetadata.CachedDataSourceAttributes type to Attribute[] to support both interfaces
  • CombinedDataSourcesAttribute filters cached attributes properly

Compliance with CLAUDE.md:
✅ Dual-Mode Implementation - Both source-gen and reflection modes work correctly
✅ Snapshot Testing - All .verified.txt files updated and committed
✅ Performance - No hot-path impact (compile-time only)
✅ AOT Compatibility - Improves AOT support by caching more attributes
✅ Test Coverage - Adds regression test, updates expected counts (+4 test cases)
✅ Code Style - Modern C# syntax, proper conventions

Test Math: New test generates 4 combinations (2 bool × 2 enum values), matching the increase from 133→137 (net framework) and 271→275 (other platforms).

LGTM ✅ - Clean, well-implemented fix that follows TUnit architectural patterns and maintains backward compatibility. Ready to merge.

This was referenced Dec 23, 2025
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.

2 participants