Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 6, 2025

The source generated code in *.verified.txt files had inconsistent indentation, particularly for complex object initializations like MethodMetadata. The generated code showed improper alignment where opening braces and content were not indented correctly relative to their containing context.

Problem

Looking at the generated code, the indentation issue was evident in patterns like this:

// Incorrectly indented (before fix)
            MethodMetadata = new global::TUnit.Core.MethodMetadata
{
Type = typeof(global::TUnit.TestProject.NameOfArgumentTests),
Parameters = new global::TUnit.Core.ParameterMetadata[]
{
new global::TUnit.Core.ParameterMetadata(typeof(string))
{
    Name = "name",
},
},
}

Root Causes

  1. CodeWriter Initialization: When creating new CodeWriter instances with includeHeader: false, the _isNewLine field wasn't properly initialized to true, preventing proper indentation on the first line.

  2. Context Loss: Inner code generation methods in MetadataGenerationHelper created new CodeWriter instances without inheriting the current indentation context from the parent writer.

  3. Multi-line Assignment Handling: Complex object assignments needed special handling where the first line continues the assignment and subsequent lines maintain proper relative indentation.

Solution

1. Fixed CodeWriter Initialization

  • Added explicit _isNewLine = true initialization when includeHeader = false to ensure proper indentation behavior.

2. Added Indentation Context Propagation

  • Updated all MetadataGenerationHelper methods to accept currentIndentLevel parameter:
    • GenerateMethodMetadata
    • GenerateClassMetadataGetOrAdd
    • GenerateParameterMetadataArrayForMethod
    • GenerateParameterMetadataArrayForConstructor
    • GeneratePropertyMetadataArray
  • Updated all callers to pass the current indentation level.

3. Implemented Proper Multi-line Object Handling

  • Enhanced SourceInformationWriter.GenerateMethodInformation with sophisticated multi-line handling logic
  • Applied similar fixes to GenerateClassInformation and TestInformationGenerator.GenerateTestInformation
  • The logic:
    • First line continues the current assignment (no extra indentation)
    • Subsequent lines preserve relative indentation structure
    • Calculates base indentation from the opening brace line
    • Maintains hierarchical indentation for nested objects

Result

The generated code now has proper, consistent indentation:

// Correctly indented (after fix)
            MethodMetadata = new global::TUnit.Core.MethodMetadata
            {
                Type = typeof(global::TUnit.TestProject.NameOfArgumentTests),
                Parameters = new global::TUnit.Core.ParameterMetadata[]
                {
                    new global::TUnit.Core.ParameterMetadata(typeof(string))
                    {
                        Name = "name",
                    },
                },
            }

This fix ensures that all source generated code follows consistent indentation patterns, making the codebase more maintainable and readable.

Fixes #2806.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 4 commits August 6, 2025 23:05
…lization and MetadataGenerationHelper indentation levels

Co-authored-by: thomhurst <[email protected]>
…of multi-line object initialization

Co-authored-by: thomhurst <[email protected]>
…ine object initialization in source generated code

Co-authored-by: thomhurst <[email protected]>
Copilot AI changed the title [WIP] Source generated code not always indented properly Fix source generated code indentation by properly handling CodeWriter initialization and multi-line object assignments Aug 6, 2025
Copilot AI requested a review from thomhurst August 6, 2025 23:26
Copilot finished work on behalf of thomhurst August 6, 2025 23:26
- Refactored metadata generation methods to use CodeWriter directly instead of returning strings
- Fixed indentation by using SetIndentLevel instead of Indent/Unindent to avoid unwanted EnsureNewLine calls
- All nested object initializer properties now have correct indentation levels
- ClassMetadata lambda bodies now properly indent their content
@claude
Copy link
Contributor

claude bot commented Aug 7, 2025

Claude finished @Copilot's task —— View job


Code Review for PR #2807 - Fix Source Generated Code Indentation

I've thoroughly reviewed the pull request that fixes indentation issues in source generated code. Here's my comprehensive analysis:

Todo List:

  • Examine the core source generator changes
  • Review CodeWriter initialization fix
  • Analyze indentation context propagation changes
  • Review multi-line object handling logic
  • Check for potential bugs or edge cases
  • Assess performance considerations
  • Evaluate test coverage and approach
  • Provide comprehensive feedback

Overall Assessment: Excellent Fix

This PR addresses a critical maintainability issue with generated code formatting. The solution is well-architected and tackles the root causes systematically.


🔧 Code Quality & Best Practices

Strengths:

  • Root Cause Analysis: Correctly identified the three core issues: CodeWriter initialization, context loss, and multi-line handling
  • Surgical Precision: Changes are focused and minimal - only 4 core files modified, with the bulk being expected test file updates
  • Consistent Patterns: The multi-line handling logic is consistently applied across SourceInformationWriter.cs:17-49, TestInformationGenerator.cs:17-49 - good code reuse

Architectural Decisions:

  • Dual API Design: Smart choice maintaining both WriteXxx() methods (for direct writing) and GenerateXxx() methods (for string generation) in MetadataGenerationHelper.cs - ensures backward compatibility
  • Manual Indent Management: The explicit SetIndentLevel() calls are appropriate for this use case where precise control is needed

🐛 Potential Issues Found

Critical Issue - CodeWriter.cs:29:

_isNewLine = true; // Fix: Always start at new line state for proper indentation

Good fix, but the comment could be clearer about why this is needed.

Minor Concerns:

  1. Magic Number in Indentation Logic (Multiple files):

    var extraIndentLevels = relativeIndent / 4; // Line 38 in multiple files

    The hardcoded 4 assumes 4-space indentation. Consider using _indentString.Length or making this configurable.

  2. Repetitive Multi-line Logic:
    The exact same 30+ line logic block appears in both SourceInformationWriter.GenerateClassInformation() and TestInformationGenerator.GenerateTestInformation(). Consider extracting to a shared helper method.

  3. Empty Line Handling Edge Case:

    if (!string.IsNullOrWhiteSpace(lines[i]) || i < lines.Length - 1)

    This condition may skip the final empty line when it should be preserved for proper code structure.


Performance Considerations

Generally Good:

  • String splitting is done once per metadata block - acceptable overhead
  • Manual indent level tracking avoids repeated string allocations
  • SetIndentLevel() is O(1) operation

Minor Optimization Opportunity:
The multi-line processing creates temporary strings and performs multiple string operations. For heavy source generation scenarios, consider using Span<char> or StringBuilder optimizations.


🔒 Security Assessment

No Security Concerns Identified

  • All changes are in code generation logic
  • No user input handling
  • No reflection security issues introduced

🧪 Test Coverage

Excellent Test Coverage:

  • 96 verified test files updated - comprehensive regression testing
  • All major test scenarios covered: basic tests, generic types, data-driven tests, inheritance, etc.
  • The .verified.txt pattern ensures any formatting regressions will be caught immediately

Test Quality:
Looking at the generated output in files like NameOfArgumentTests.Test.verified.txt:46-76, the indentation is now properly structured:

MethodMetadata = new global::TUnit.Core.MethodMetadata
{
    Type = typeof(global::TUnit.TestProject.NameOfArgumentTests), // ✅ Properly indented
    Parameters = new global::TUnit.Core.ParameterMetadata[]       // ✅ Proper alignment
    {
        new global::TUnit.Core.ParameterMetadata(typeof(string))  // ✅ Nested indentation correct
        {
            Name = "name",                                        // ✅ Object initializer aligned
        },
    },
}

📈 Recommendations

High Priority:

  1. Extract the duplicate multi-line handling logic into a shared WriteMultiLineWithIndentation() helper method
  2. Replace hardcoded 4 with _indentString.Length for indent calculation

Medium Priority:
3. Add XML documentation to the new WriteIndentedString() method explaining its purpose
4. Consider adding a unit test specifically for the _isNewLine = true fix in CodeWriter

Low Priority:
5. Consider performance optimizations for high-volume source generation scenarios


🎯 Final Verdict

✅ Ready to Merge - This is a solid fix that addresses a real maintainability problem. The approach is sound, the implementation is correct, and the test coverage is comprehensive. The minor issues identified are opportunities for future refinement rather than blockers.

The generated code will now be much more readable and maintainable, which is crucial for debugging source generator issues.


@thomhurst thomhurst marked this pull request as ready for review August 7, 2025 00:35
@claude
Copy link
Contributor

claude bot commented Aug 7, 2025

Claude finished @Copilot's task —— View job


Code Review for PR #2807 - Fix Source Generated Code Indentation

Todo List:

  • Examine the core source generator changes
  • Review CodeWriter initialization fix
  • Analyze indentation context propagation changes
  • Review multi-line object handling logic
  • Check for potential bugs or edge cases
  • Assess performance considerations
  • Evaluate test coverage and approach
  • Provide comprehensive feedback

Overall Assessment: Excellent Fix

This PR addresses a critical maintainability issue with generated code formatting. The solution is well-architected and tackles the root causes systematically.


🔧 Code Quality & Best Practices

Strengths:

  • Root Cause Analysis: Correctly identified the three core issues: CodeWriter initialization, context loss, and multi-line handling
  • Surgical Precision: Changes are focused and minimal - only 4 core files modified, with the bulk being expected test file updates
  • Consistent Patterns: The multi-line handling logic is consistently applied across SourceInformationWriter.cs:17-49, TestInformationGenerator.cs:17-49 - good code reuse

Architectural Decisions:

  • Dual API Design: Smart choice maintaining both WriteXxx() methods (for direct writing) and GenerateXxx() methods (for string generation) in MetadataGenerationHelper.cs - ensures backward compatibility
  • Manual Indent Management: The explicit SetIndentLevel() calls are appropriate for this use case where precise control is needed

🐛 Potential Issues Found

Critical Issue - CodeWriter.cs:29:

_isNewLine = true; // Fix: Always start at new line state for proper indentation

Good fix, but the comment could be clearer about why this is needed.

Minor Concerns:

  1. Magic Number in Indentation Logic (Multiple files):

    var extraIndentLevels = relativeIndent / 4; // Line 38 in multiple files

    The hardcoded 4 assumes 4-space indentation. Consider using _indentString.Length or making this configurable.

  2. Repetitive Multi-line Logic:
    The exact same 30+ line logic block appears in both SourceInformationWriter.GenerateClassInformation() and TestInformationGenerator.GenerateTestInformation(). Consider extracting to a shared helper method.

  3. Empty Line Handling Edge Case:

    if (!string.IsNullOrWhiteSpace(lines[i]) || i < lines.Length - 1)

    This condition may skip the final empty line when it should be preserved for proper code structure.


Performance Considerations

Generally Good:

  • String splitting is done once per metadata block - acceptable overhead
  • Manual indent level tracking avoids repeated string allocations
  • SetIndentLevel() is O(1) operation

Minor Optimization Opportunity:
The multi-line processing creates temporary strings and performs multiple string operations. For heavy source generation scenarios, consider using Span<char> or StringBuilder optimizations.


🔒 Security Assessment

No Security Concerns Identified

  • All changes are in code generation logic
  • No user input handling
  • No reflection security issues introduced

🧪 Test Coverage

Excellent Test Coverage:

  • 96 verified test files updated - comprehensive regression testing
  • All major test scenarios covered: basic tests, generic types, data-driven tests, inheritance, etc.
  • The .verified.txt pattern ensures any formatting regressions will be caught immediately

Test Quality:
Looking at the generated output in files like NameOfArgumentTests.Test.verified.txt:46-76, the indentation is now properly structured:

MethodMetadata = new global::TUnit.Core.MethodMetadata
{
    Type = typeof(global::TUnit.TestProject.NameOfArgumentTests), // ✅ Properly indented
    Parameters = new global::TUnit.Core.ParameterMetadata[]       // ✅ Proper alignment
    {
        new global::TUnit.Core.ParameterMetadata(typeof(string))  // ✅ Nested indentation correct
        {
            Name = "name",                                        // ✅ Object initializer aligned
        },
    },
}

📈 Recommendations

High Priority:

  1. Extract the duplicate multi-line handling logic into a shared WriteMultiLineWithIndentation() helper method
  2. Replace hardcoded 4 with _indentString.Length for indent calculation

Medium Priority:
3. Add XML documentation to the new WriteIndentedString() method explaining its purpose
4. Consider adding a unit test specifically for the _isNewLine = true fix in CodeWriter

Low Priority:
5. Consider performance optimizations for high-volume source generation scenarios


🎯 Final Verdict

✅ Ready to Merge - This is a solid fix that addresses a real maintainability problem. The approach is sound, the implementation is correct, and the test coverage is comprehensive. The minor issues identified are opportunities for future refinement rather than blockers.

The generated code will now be much more readable and maintainable, which is crucial for debugging source generator issues.


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.

Source generated code not always indented properly

2 participants