Refactor TestClassModelBuilder.BuildAttributes to a LINQ pipeline#8576
Conversation
Address code-quality bot feedback from PR #8574: replace the manual foreach + ImmutableArray.Builder loop in TestClassModelBuilder.BuildAttributes with an explicit Select / Where / Select / ToEquatableArray pipeline. See #8574 (comment). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
| # | 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 aWhereNotNull()extension — a pattern already established in the siblingMSTest.SourceGenerationproject 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.allowedlist 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
Address review feedback on PR #8576: add a WhereNotNull<T>() extension to EquatableArrayExtensions (mirroring the pattern already used in MSTest.SourceGeneration's EnumerableExtensions) and simplify TestClassModelBuilder.BuildAttributes from a 3-stage Select/Where/Select! pipeline to a 2-stage Select/WhereNotNull pipeline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address code-quality bot feedback on PR #8574: replace the manual
foreach+ImmutableArray<AttributeApplicationModel>.Builderloop inTestClassModelBuilder.BuildAttributeswith an explicitSelect/Where/Select/ToEquatableArrayLINQ pipeline.See discussion: #8574 (comment).
No behavior change; no new imports needed (
System.Linqis already imported andToEquatableArrayacceptsIEnumerable<T>).