Refactor TestClassModelBuilder.BuildAttributes to a LINQ pipeline

[Evangelink]

Address code-quality bot feedback on PR #8574: replace the manual foreach + ImmutableArray<AttributeApplicationModel>.Builder loop in TestClassModelBuilder.BuildAttributes with an explicit Select / Where / Select / ToEquatableArray LINQ pipeline.

See discussion: #8574 (comment).

No behavior change; no new imports needed (System.Linq is already imported and ToEquatableArray accepts IEnumerable<T>).

[Copilot]

Pull request overview

Refactors MSTest.AotReflection.SourceGeneration’s TestClassModelBuilder.BuildAttributes to use a LINQ pipeline rather than an explicit foreach loop + ImmutableArray<T>.Builder, in response to code-quality feedback.

Changes:

• Replaced the manual attribute loop with Select/Where/Select/ToEquatableArray() to filter out null models and materialize the result.

Show a summary per file

File	Description
src/Analyzers/MSTest.AotReflection.SourceGeneration/Generators/TestClassModelBuilder.cs	Rewrites BuildAttributes to a LINQ-based pipeline for building EquatableArray<AttributeApplicationModel>.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[Evangelink]

Addressed in 70649ab by switching to the WhereNotNull() extension (see code-structure reply for details). The 3-stage Select/Where/Select! is now a 2-stage Select/WhereNotNull, which removes one iterator allocation. I kept the LINQ form so #8574 reviewers still get the readability win; the residual iterator overhead is the same shape as the established MSTest.SourceGeneration codepaths.

[Evangelink]

#	Dimension	Verdict
5	Performance & Allocations	🟡 1 MODERATE
15	Code Structure & Simplification	🟡 1 MODERATE

✅ 19/21 dimensions clean.

Summary:

This refactoring replaces an imperative loop with a LINQ pipeline, which is a reasonable goal. However, two concerns need addressing:

• Performance: The LINQ pipeline loses the pre-allocation optimization (builder sized to attributes.Length). While this is compile-time code (not runtime-critical), source generators must be efficient — they impact build times and IDE responsiveness. The ~2x allocation overhead compounds across thousands of symbols.

• Code consistency: The three-stage LINQ pattern (Select → Where → Select!) can be simplified using a WhereNotNull() extension — a pattern already established in the sibling MSTest.SourceGeneration project for identical scenarios. Adding this helper would align with existing conventions and eliminate the redundant third stage.

Recommended path forward:

Consider reverting to the original builder pattern for performance, OR if LINQ is preferred, add the WhereNotNull() extension to improve consistency and simplify the pipeline to two stages. See inline comments for detailed analysis and implementation guidance.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• builds.dotnet.microsoft.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "builds.dotnet.microsoft.com"

See Network Configuration for more information.

Generated by Expert Code Review (on open) for issue #8576 · ● 4.5M

[Evangelink]

[MODERATE] Code Structure & Simplification

The three-stage LINQ pipeline (Select → Where → Select with !) can be simplified to two stages using a WhereNotNull() extension method — a pattern already established in the sibling MSTest.SourceGeneration project.

Current code:

.Select(BuildAttribute)
.Where(static model => model is not null)
.Select(static model => model!)

Established pattern in MSTest.SourceGeneration:

// MSTest.SourceGeneration/ObjectModels/TestTypeInfo.cs:107
.Select(method => TestMethodInfo.TryBuild(method, namedTypeSymbol, wellKnownTypes))
.WhereNotNull()
.ToImmutableArray();

// MSTest.SourceGeneration/Helpers/EnumerableExtensions.cs
public static IEnumerable<T> WhereNotNull<T>(this IEnumerable<T?> source)
    where T : class
    => source.Where((Func<T?, bool>)NotNullTest)!;

Recommendation:

Add WhereNotNull() to MSTest.AotReflection.SourceGeneration/Model/TestClassModel.cs (where ToEquatableArray is already defined):

internal static class EquatableArrayExtensions
{
    private static readonly Func<object?, bool> NotNullTest = x => x != null;

    public static EquatableArray<T> ToEquatableArray<T>(this IEnumerable<T> source)
        where T : IEquatable<T>
        => new(source.ToImmutableArray());

    public static IEnumerable<T> WhereNotNull<T>(this IEnumerable<T?> source)
        where T : class
        => source.Where((Func<T?, bool>)NotNullTest)!;
}

Then simplify this code to:

return attributes
    .Select(BuildAttribute)
    .WhereNotNull()
    .ToEquatableArray();

Benefits:

• ✅ 2 lines shorter (eliminates redundant third stage)
• ✅ More declarative (expresses intent: "where not null")
• ✅ Consistent with established codebase patterns
• ✅ Type-safe (no null-forgiving operator needed)
• ✅ Reusable for future nullable filtering scenarios

[Evangelink]

Done in 70649ab — added WhereNotNull<T>() to EquatableArrayExtensions in Model/TestClassModel.cs (right next to ToEquatableArray) following the exact same shape as MSTest.SourceGeneration/Helpers/EnumerableExtensions.cs, and simplified BuildAttributes to the suggested 2-stage pipeline:

return attributes
.Select(BuildAttribute)
.WhereNotNull()
.ToEquatableArray();

[Evangelink]

[MODERATE] Performance & Allocations

The LINQ pipeline replaces a pre-allocated builder with chained iterators that cannot pre-size the backing array.

Allocation analysis:

• Original: ~3 allocations (builder + temp array[N] + final array[M])
  • Pre-allocates with exact capacity (attributes.Length)
  • Avoids reallocation during Add() operations
• LINQ: ~5-6 allocations (3 iterator objects + builder + 1-2 arrays)
  • Select creates iterator (boxed struct)
  • Where creates iterator (class instance)
  • Select creates iterator (class instance)
  • ToImmutableArray() cannot pre-size (iterators don't expose Count)

Concrete scenario:
For a test class with 10 attributes where 2 return null:

• Original: Allocate array[10], populate 8 slots, ToImmutable() copies to array[8]
• LINQ: 3 iterator objects + default-sized builder (grows dynamically) → array[8]

Context & Impact:
Source generators run at compile-time (IDE IntelliSense, builds). This method is called once per type/method/property during generation. In a large codebase with hundreds of test classes × dozens of attributes, the ~2x allocation overhead compounds, contributing to build time and IDE responsiveness.

Recommendation:
For source generator code, the pre-allocated builder pattern is preferred practice. The original imperative loop is objectively more efficient for this use case. While the performance difference is moderate (not runtime-critical), the allocation overhead is measurable and compounds across thousands of symbols.

Consider reverting to the original pattern or using the optimization suggested in the simplification finding below.

[Evangelink]

Addressed in 70649ab. I chose the LINQ-with-WhereNotNull path from your "OR" recommendation rather than reverting, for two reasons:

1. It keeps the readability win from the original Unify placeholder support across MTP --*-filename CLI options #8574 bot feedback.
2. It matches the established pattern in the sibling MSTest.SourceGeneration project (TestTypeInfo.cs, TestNodesGenerator.cs), so the AotReflection generator is now consistent with it.

The pipeline drops from 3 iterators to 2 (Select + WhereNotNull instead of Select + Where + Select!). Pre-sizing the builder via attributes.Length would over-allocate by the number of nulls anyway, so we're trading one short-lived iterator for the upfront-sizing — acceptable, and consistent with the rest of the codebase. Happy to revert to the explicit builder if you'd rather optimize allocations further.