Preserve formatting in DependsOn code fix

[thomhurst]

Closes #3510

Summary

• stop normalizing the entire compilation unit when adding DependsOn
• move existing declaration trivia onto the inserted attribute while preserving indentation and trailing trivia
• update expectations and add byte-sensitive coverage for blank lines, token spacing, and comments

Validation

• guarded Release build: ModularPipelines.Analyzers.sln (0 warnings, 0 errors)
• ModularPipelinesAnalyzersUnitTests: 10/10 passed

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b5a5ba6d62

ℹ️ 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".

[claude]

Review

Nice fix overall — replacing the whole-compilation-unit NormalizeWhitespace() with targeted trivia surgery is the right direction, and the new CodeFix_Preserves_Unrelated_Formatting test does a good job asserting blank lines, odd token spacing, and trailing comments survive. I checked out the branch and ran the full analyzer test suite locally (ModularPipelines.Analyzers.Test): 148/148 passed.

Bug: new attribute isn't indented when the type already has an attribute list

AddAttribute (MissingDependsOnAttributeCodeFixProvider.cs:87-97) only moves/derives leading indentation trivia inside the typeDecl.AttributeLists.Count == 0 branch:

if (typeDecl.AttributeLists.Count == 0)
{
    var leadingTrivia = typeDecl.GetLeadingTrivia();
    var indentation = SyntaxFactory.TriviaList(
        leadingTrivia.Reverse().TakeWhile(...).Reverse());
    attribute = attribute.WithLeadingTrivia(leadingTrivia);
    typeDecl = typeDecl.WithLeadingTrivia(indentation);
}

var attributes = typeDecl.AttributeLists.Add(attribute);

When the count is > 0 (the type already carries at least one attribute — e.g. [Obsolete], or a previously-added [DependsOn<X>]), the new attribute list is appended with no leading trivia at all, so it lands flush at column 0 regardless of the declaration's actual indentation.

I reproduced this against the exact transformation logic in a standalone Roslyn repro. Input (nested/indented class, mirroring how the existing GeneratedAccessorSource test fixture uses block-scoped namespaces):

namespace Example
{
    [Obsolete]
    public class Consumer
    {
    }
}

Output after adding [DependsOn<Dependency>]:

namespace Example
{
    [Obsolete]
[DependsOn<Dependency>]
    public class Consumer
    {
    }
}

The new attribute is misaligned with its sibling and with public class Consumer. This is exactly the class of bug the PR sets out to fix, just in the other branch — and it's realistic for this codebase specifically: [DependsOn<X>] is conventionally stacked as one attribute list per dependency (see UploadPackagesToLocalNuGetModule.cs, CreateReleaseModule.cs), so hitting the analyzer a second time on a module that already has one DependsOn (or any other attribute) is a normal flow, not a corner case. It happens to be invisible in this repo's own modules only because they're always declared at column 0 in file-scoped namespaces — but it will bite any nested type, block-scoped namespace, or indented style.

None of the new/existing tests exercise AttributeLists.Count > 0 (CodeFixWorks, OptionalCodeFixWorks, and the new CodeFix_Preserves_Unrelated_Formatting all start from a type with zero attributes), so this gap has no coverage.

Suggested fix: don't special-case on Count == 0. Compute the "indentation to use for a newly-inserted attribute" the same way regardless of whether it's the first attribute list or an additional one:

• If Count == 0: derive indentation from typeDecl's leading trivia (current behavior), and move the rest of that leading trivia onto the new attribute, same as today.
• If Count > 0: derive indentation from the last existing attribute list's leading trivia (same "trailing whitespace run" extraction logic already written for the other branch) and apply it as the new attribute's leading trivia.

That collapses two asymmetric code paths into one consistent rule ("match the indentation of whatever token used to start this line") instead of only handling the case that happens to be exercised by the current tests.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 36d9ed93b0

ℹ️ 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".

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Previous feedback: The indentation bug I flagged earlier (new attribute not indented when the type already had an attribute list, in MissingDependsOnAttributeCodeFixProvider.cs) has been fixed in 36d9ed93 — indentation is now derived from the last existing attribute list's leading trivia when AttributeLists.Count > 0, with a new regression test (CodeFix_Indents_Attribute_Appended_To_Existing_List) covering it.

This pass: Two independent reviewers checked the trivia-manipulation logic (elastic-trivia formatting via AddUsing/NormalizeWhitespace ordering, the Count == 0 vs Count > 0 branches, block vs file-scoped namespaces, comments/blank-lines/directives in leading trivia) against the diff and ran the analyzer test suite (149/149 passing). No compile errors, logic errors, or CLAUDE.md violations found. Two reviewers independently noted the same minor cosmetic edge case (an attribute list on the same line as the type declaration, e.g. [Obsolete] public class Foo, loses indentation on the class line after the fix is applied) — this is an unusual/unrealistic input, produces valid compiling code, and doesn't rise to a reportable issue.

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Previous feedback: Both earlier rounds of feedback have been addressed — the attribute-indentation bug (fixed in 36d9ed93) and this pass's target, the missing-newline-separator bug for inserted using directives (fixed in 931f185f).

This pass (reviewing 931f185f, the only commit since the last review): AddUsing in SyntaxNodeExtensions.cs previously constructed the new UsingDirectiveSyntax with no trailing trivia, so when appended via AddUsings/Insert it could run directly into whatever followed (e.g. using ModularPipelines.Attributes;#nullable enable when the source had no pre-existing usings). The fix derives the file's actual line-ending style from the first EndOfLineTrivia found in the document (falling back to Environment.NewLine only if the file has no newlines at all) and attaches it as trailing trivia on the new using directive. I traced this against both insertion paths — the empty-Usings-list case and the conditionalMatch insert-before-existing-conditional-using case — and it's applied consistently to both, so it fixes the class of bug rather than one instance of it. The new test CodeFix_Separates_Inserted_Using_When_Source_Has_No_Usings exercises the exact scenario (file starting with #nullable enable, no existing usings) and its expected output matches what the trivia surgery actually produces.

No new issues found in this commit.