Add support for ExpandValueTuple in SymbolDisplayFormat for Roslyn 4.7+

[thomhurst]

Fixes #2799

[claude]

Claude finished @thomhurst's task —— View job

---

Pull Request Review ✅

Reviewing PR #2827 that adds ValueTuple support for Roslyn 4.7+.

Review Tasks:

• Analyze DisplayFormats.cs changes for code quality and implementation
• Review test coverage and test implementation
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Provide overall feedback and recommendations

Code Quality Analysis

DisplayFormats.cs Changes: ✅ Good Implementation

• Correctly uses conditional compilation with #if ROSLYN4_7_OR_GREATER
• Consistently applies ExpandValueTuple flag to all relevant display formats
• Clean, readable code with consistent formatting
• Follows existing patterns in the codebase

Test Coverage Analysis

Tests.cs: ✅ Adequate Coverage

• Minimal but focused test case that reproduces the issue
• Tests the specific scenario with ValueTuple implicit operator conversion
• Uses proper test structure with [Test] and [MethodDataSource] attributes
• Test will verify the fix works end-to-end

Technical Review

Potential Issues: ✅ No Issues Found

• No bugs detected in the implementation
• Conditional compilation guard prevents issues with older Roslyn versions
• All 5 display format constants are consistently updated

Performance: ✅ No Impact

• ExpandValueTuple is a compile-time flag that affects source generation
• No runtime performance implications
• Minimal memory footprint change

Security: ✅ No Concerns

• Changes are purely cosmetic/formatting related
• No security implications from expanding tuple display format
• No exposure of sensitive data

Detailed Analysis

The PR addresses an issue where ValueTuple types weren't properly expanded in generated code for Roslyn 4.7+. The fix:

1. Root Cause: Without ExpandValueTuple, the source generator would emit ValueTuple<int, int> instead of the more readable (int, int) syntax
2. Solution: Adding SymbolDisplayMiscellaneousOptions.ExpandValueTuple to all display formats when Roslyn 4.7+ is available
3. Backward Compatibility: Uses conditional compilation to ensure compatibility with older Roslyn versions

The test case in TUnit.TestProject/Bugs/2798/Tests.cs demonstrates the scenario where this matters - a record with an implicit operator from a ValueTuple that needs proper code generation.

Recommendations

✅ Approve: This is a well-implemented fix that:

• Solves a specific code generation issue with ValueTuples
• Maintains backward compatibility
• Includes appropriate test coverage
• Follows project conventions and best practices
• Has no negative side effects

Minor Suggestions:

• Consider adding a comment in DisplayFormats.cs explaining why ExpandValueTuple is needed for Roslyn 4.7+
• The test could potentially include assertions to verify the generated code format, though the current approach is sufficient

Overall, this is a solid fix that improves the developer experience by generating more readable tuple syntax in source-generated code.

[thomhurst]

That looks like it worked a treat. Thanks @Brokolis !

        if (value is global::System.ValueTuple<int, int> typedValue)
        {
            return (global::TUnit.TestProject.Bugs._2798.Foo)typedValue;
        }

[claude]

Claude finished @thomhurst's task —— View job

---

Pull Request Review ✅

Reviewing PR #2827 that adds ValueTuple support for Roslyn 4.7+.

Review Tasks:

• Read and analyze DisplayFormats.cs changes
• Review new test file and test coverage
• Check verified test output changes
• Analyze for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Provide overall feedback and recommendations

Code Quality Analysis

DisplayFormats.cs Changes: ✅ Excellent Implementation

• Correctly uses conditional compilation with #if ROSLYN4_7_OR_GREATER
• Consistently applies ExpandValueTuple flag to all 5 relevant display formats
• Clean, readable code with consistent formatting
• Follows existing patterns in the codebase
• Proper backward compatibility preservation

Test Coverage Analysis

Tests.cs: ✅ Targeted and Effective

• Minimal but focused test case that reproduces the exact issue from Fix tuple pattern matching in AotConverterGenerator to use ValueTuple types #2799
• Tests the specific scenario with ValueTuple implicit operator conversion
• Uses proper TUnit test attributes ([Test], [MethodDataSource])
• Test will verify the fix works end-to-end through source generation

Verified Output Changes: ✅ Confirms Fix Works

• Shows transition from compact tuple syntax (int, string, bool) to expanded global::System.ValueTuple<int, string, bool>
• Demonstrates the fix applies consistently across different tuple scenarios
• Generated code maintains proper type safety and functionality

Technical Review

Potential Issues: ✅ No Issues Found

• No bugs detected in the implementation
• Conditional compilation guard prevents issues with older Roslyn versions
• All display format constants are consistently updated
• No breaking changes to existing functionality

Performance: ✅ No Negative Impact

• ExpandValueTuple is a compile-time flag that only affects source generation
• No runtime performance implications
• Minimal memory footprint change during compilation

Security: ✅ No Concerns

• Changes are purely cosmetic/formatting related to code generation
• No security implications from expanding tuple display format
• No exposure of sensitive data or attack vectors

Detailed Technical Analysis

The PR addresses issue #2799 where ValueTuple types weren't properly expanded in generated code for Roslyn 4.7+.

Root Cause: Without ExpandValueTuple, the source generator would emit ValueTuple<int, int> instead of the more readable (int, int) syntax, but this actually caused issues with implicit operators that expected the compact syntax.

Solution: Adding SymbolDisplayMiscellaneousOptions.ExpandValueTuple to all display formats when Roslyn 4.7+ is available ensures consistent type representation.

Backward Compatibility: Uses conditional compilation to ensure compatibility with older Roslyn versions that don't support this flag.

The test case in TUnit.TestProject/Bugs/2798/Tests.cs demonstrates the scenario where this matters - a record with an implicit operator from a ValueTuple that needs proper code generation.

Recommendations

✅ Approve: This is a well-implemented fix that:

• Solves a specific source generation issue with ValueTuples
• Maintains perfect backward compatibility
• Includes appropriate test coverage
• Follows project conventions and best practices
• Has no negative side effects
• Provides clear evidence it works (author confirmed with generated code example)

Optional Enhancement: Consider adding a comment in DisplayFormats.cs explaining why ExpandValueTuple is needed for Roslyn 4.7+, though the conditional compilation makes the intent fairly clear.

Overall Assessment: Excellent implementation that resolves the reported issue cleanly and safely. The generated code will now properly handle ValueTuple scenarios as demonstrated by the author's confirmation that it produces the expected global::System.ValueTuple<int, int> syntax.

---