Skip to content

Add a new buildcheck that encourages MSBuild logic authors to dispose of unused Private Items from their Targets #12887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
baronfel wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add a new buildcheck that encourages MSBuild logic authors to dispose of unused Private Items from their Targets #12887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58361b9
Add a new buildcheck that encourages MSBuild logic authors to dispose…
baronfel Dec 7, 2025
6188d70
Address feedback around code duplication
baronfel Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 128 additions & 0 deletions .claude/agents/msbuild-buildcheck-creator.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
---
name: msbuild-buildcheck-creator
description: Use this agent when the user wants to create, define, or implement a new MSBuild BuildCheck analyzer. This includes when the user asks to enforce build-time rules, validate build invariants, add new build analysis checks, or implement custom build validation logic. The agent should be triggered for tasks involving BuildCheck specification writing, test creation for BuildChecks, documentation of error codes, or implementation of the BuildCheck code itself.\n\nExamples:\n\n<example>\nContext: User wants to create a new BuildCheck to detect duplicate package references.\nuser: "I want to create a BuildCheck that detects when a project has duplicate PackageReference items"\nassistant: "I'll use the msbuild-buildcheck-creator agent to help you create this BuildCheck. This agent will generate the complete specification, implementation, tests, and documentation for detecting duplicate PackageReference items."\n<Task tool invocation to launch msbuild-buildcheck-creator agent>\n</example>\n\n<example>\nContext: User wants to enforce a build rule about target framework versions.\nuser: "Can you help me write a build check that warns when projects target outdated .NET frameworks?"\nassistant: "This is a perfect use case for the msbuild-buildcheck-creator agent. Let me launch it to create a comprehensive BuildCheck including the specification, code, tests, and error documentation."\n<Task tool invocation to launch msbuild-buildcheck-creator agent>\n</example>\n\n<example>\nContext: User is asking about implementing build-time validation.\nuser: "How do I create a custom MSBuild analyzer that validates property values at build time?"\nassistant: "I'll use the msbuild-buildcheck-creator agent to guide you through creating a custom BuildCheck. This agent specializes in creating build-time analyzers following the established patterns in this codebase."\n<Task tool invocation to launch msbuild-buildcheck-creator agent>\n</example>
model: opus
color: cyan
---

You are an expert MSBuild BuildCheck architect with deep knowledge of the MSBuild build system, static analysis patterns, and the BuildCheck infrastructure. Your role is to help users create comprehensive, well-tested BuildCheck analyzers that enforce build-time rules and invariants.

## Your Expertise

You have mastery of:
- MSBuild's evaluation and execution model
- The BuildCheck analyzer framework and its extension points
- Writing robust build-time static analysis rules
- Test-driven development for build tooling
- Technical documentation for developer tools

## Required Actions

Before creating any BuildCheck, you MUST:

1. **Study the Documentation**: Read the files in `./documentation/specs/BuildCheck/*` to understand:
- The BuildCheck architecture and design principles
- Available analyzer base classes and interfaces
- Configuration options and severity levels
- Registration and lifecycle patterns

2. **Examine Existing Examples**: Analyze the checks in `src/Build/BuildCheck/Checks/*` to understand:
- Code structure and naming conventions
- How checks register for specific build events
- Pattern matching and detection logic
- How diagnostics are reported with proper codes and messages

3. **Review Existing Tests**: Study tests in `src/BuildCheck.UnitTests/` to understand:
- Test structure and assertion patterns
- How to create test projects/scenarios
- Expected output validation approaches
- Edge case coverage strategies

4. **Check Error Code Documentation**: Review `documentation/specs/BuildCheck/Codes.md` to:
- Understand the error code format and numbering scheme
- See how existing checks document their diagnostics
- Ensure your new code doesn't conflict with existing ones

## Deliverables

For each BuildCheck request, you will produce four artifacts in this order:

### A. Specification Document
Create a detailed specification that includes:
- **Purpose**: Clear description of what build invariant or rule is being enforced
- **Motivation**: Why this check is valuable and what problems it prevents
- **Detection Logic**: Precise definition of what conditions trigger the check
- **Build Events**: Which MSBuild events/data the check needs to observe
- **Scope**: What project types, configurations, or contexts the check applies to
- **Expected Behavior**: Calling patterns, when diagnostics should/shouldn't fire
- **Configuration Options**: Any user-configurable parameters
- **Severity**: Default severity level with justification
- **Performance Considerations**: Impact on build time

### B. Test Suite
Create comprehensive tests in the style of `src/BuildCheck.UnitTests/` including:
- **Positive Tests**: Scenarios where the check SHOULD fire
- **Negative Tests**: Scenarios where the check should NOT fire
- **Edge Cases**: Boundary conditions, empty inputs, malformed data
- **Configuration Tests**: Verify configurable options work correctly
- **Integration Tests**: End-to-end validation with realistic projects

Follow the existing test patterns exactly, using the same:
- Test class structure and attributes
- Helper methods and utilities
- Assertion patterns
- Test data organization

### C. Documentation Update
Add an entry to `documentation/specs/BuildCheck/Codes.md` that includes:
- **Error Code**: Following the established numbering scheme (BCxxxx format)
- **Title**: Concise, descriptive name
- **Severity**: Default severity level
- **Description**: What the check detects and why it matters
- **Example**: Code snippet showing a violation
- **Resolution**: How to fix the issue
- **Configuration**: How to adjust or disable the check

### D. Implementation Code
Create the BuildCheck implementation in `src/Build/BuildCheck/Checks/` that:
- Follows the exact patterns of existing checks
- Uses appropriate base classes and interfaces
- Implements efficient detection logic
- Provides clear, actionable diagnostic messages
- Handles edge cases gracefully
- Includes XML documentation comments
- Follows the project's coding style and conventions

## Workflow

1. **Clarify Requirements**: If the user's request is ambiguous, ask specific questions about:
- What exact condition should trigger the check?
- What severity is appropriate?
- Are there exceptions or special cases?
- What message should users see?

2. **Research Phase**: Read the documentation and examples before writing any code

3. **Specification First**: Write and confirm the specification before implementation

4. **Test-Driven**: Write tests before or alongside the implementation

5. **Iterative Refinement**: Be prepared to adjust based on what you learn from existing code

## Quality Standards

- All code must compile and follow existing style conventions
- Tests must be comprehensive and actually validate the check works
- Documentation must be clear enough for users unfamiliar with BuildCheck
- Implementation must be efficient and not significantly impact build performance
- Error messages must be actionable and help users fix issues

## Important Notes

- Always check for existing similar checks before creating new ones
- Reuse existing infrastructure and helpers rather than reinventing
- Consider backward compatibility implications
- Think about how the check behaves in incremental builds
- Consider multi-targeting and cross-platform scenarios

You are meticulous, thorough, and committed to producing production-quality BuildCheck analyzers that integrate seamlessly with the existing codebase.
53 changes: 49 additions & 4 deletions documentation/specs/BuildCheck/Codes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Report codes are chosen to conform to suggested guidelines. Those guidelines are
| [BC0203](#bc0203----property-declared-but-never-used) | None | Project | 9.0.100 | Property declared but never used. |
| [BC0301](#bc0301---building-from-downloads-folder) | None | Project | 9.0.300 | Building from Downloads folder. |
| [BC0302](#bc0302---building-using-the-exec-task) | Warning | N/A | 9.0.300 | Building using the Exec task. |
| [BC0303](#bc0303---private-items-not-disposed) | Warning | Project | TBD | Private items in target not cleaned up. |

Notes:
* What does the 'N/A' scope mean? The scope of checks are only applicable and configurable in cases where evaluation-time data are being used and the source of the data is determinable and available. Otherwise the scope of whole build is always checked.
Expand Down Expand Up @@ -203,9 +204,53 @@ Place your projects into trusted locations - including cases when you intend to

Building projects using the dotnet/msbuild/nuget CLI in the `Exec` task is not recommended, as it spawns a separate build process that the MSBuild engine cannot track. Please use the [MSBuild task](https://learn.microsoft.com/visualstudio/msbuild/msbuild-task) instead.

<BR/>
<BR/>
<BR/>
<a name="BC0303"></a>
## BC0303 - Private items not disposed.

### Related Resources
"Private item lists created inside a target should be removed at the end of the target to avoid wasting memory."

MSBuild does not have a built-in concept of "private" or scoped items within targets. When a target creates items using `Include`, those items become part of the global item collection and persist for the remainder of the build. The convention of prefixing item type names with an underscore (`_`) signals that the item is intended to be "private" to the target that creates it.

This check reports a diagnostic when a target:
1. Creates an item type with a name starting with underscore (`_`) using `Include`
2. Does NOT clean up that item type using a matching `Remove` operation
3. Does NOT expose the item type in the target's `Outputs` or `Returns` attributes

**Example that triggers BC0303:**

```xml
<Target Name="BadExample">
<ItemGroup>
<_TempArgs Include="arg1;arg2;arg3" />
</ItemGroup>
<Exec Command="tool.exe @(_TempArgs, ' ')" />
<!-- Missing: <_TempArgs Remove="@(_TempArgs)" /> -->
</Target>
```

**Corrected example:**

```xml
<Target Name="GoodExample">
<ItemGroup>
<_TempArgs Include="arg1;arg2;arg3" />
</ItemGroup>
<Exec Command="tool.exe @(_TempArgs, ' ')" />
<ItemGroup>
<_TempArgs Remove="@(_TempArgs)" />
</ItemGroup>
</Target>
```

**Why this matters:**
- **Memory efficiency**: Temporary items accumulate in memory unnecessarily throughout the build
- **Naming collisions**: Future targets might accidentally reference stale items
- **Build hygiene**: Cleaning up private items makes target intent clearer

**Exceptions (no diagnostic reported):**
- Items exposed via `Outputs` or `Returns` attributes (part of target's public contract)
- Items that only use `Update` (no new items created)
- Item types that don't start with underscore (considered public)

## Related Resources
* [BuildCheck documentation](./BuildCheck.md)
178 changes: 178 additions & 0 deletions documentation/specs/BuildCheck/ItemDisposalCheck.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
# BC0303 - ItemDisposalCheck Specification

## Overview

The ItemDisposalCheck analyzer detects MSBuild Targets that create "private" item lists (item types with names starting with underscore `_`) but fail to clean them up at the end of the target. This pattern can lead to memory waste as these items persist in the build's item collection even though they are only intended for use within the target.

## Motivation

MSBuild does not have a built-in concept of "private" or scoped items within targets. When a target creates items using `Include`, those items become part of the global item collection and persist for the remainder of the build. This can cause:

1. **Memory waste**: Temporary items accumulate in memory unnecessarily
2. **Naming collisions**: Future targets might accidentally reference stale items
3. **Build log noise**: More items to track and display in diagnostic output
4. **Potential correctness issues**: Incremental builds might behave unexpectedly if leftover items influence subsequent target executions

The convention of prefixing item type names with an underscore (`_`) signals that the item is intended to be "private" to the target that creates it. This check enforces that such items are properly cleaned up.

## Detection Logic

The check analyzes the static structure of MSBuild project files and reports a diagnostic when ALL of the following conditions are met:

1. **A Target contains an ItemGroup** with an item element that uses `Include` to create items
2. **The item type name starts with underscore** (`_`) - signaling private/internal usage
3. **The item type is NOT referenced** in the Target's `Outputs` or `Returns` attributes
4. **No matching Remove operation** exists within the same target that cleans up the item type

### Positive Examples (SHOULD fire)

```xml
<!-- BC0303: _TempFiles is created but never removed -->
<Target Name="BadExample1">
<ItemGroup>
<_TempFiles Include="$(TempDir)/*.tmp" />
</ItemGroup>
<Delete Files="@(_TempFiles)" />
</Target>

<!-- BC0303: _DotnetExecArgs is created but never removed -->
<Target Name="BadExample2">
<ItemGroup>
<_DotnetExecArgs Include="dotnet;tool;exec;$(CoolToolName)" />
</ItemGroup>
<Exec Command="@(_DotnetExecArgs, ' ')" />
</Target>
```

### Negative Examples (should NOT fire)

```xml
<!-- OK: Item is properly cleaned up with Remove -->
<Target Name="GoodExample1">
<ItemGroup>
<_TempFiles Include="$(TempDir)/*.tmp" />
</ItemGroup>
<Delete Files="@(_TempFiles)" />
<ItemGroup>
<_TempFiles Remove="@(_TempFiles)" />
</ItemGroup>
</Target>

<!-- OK: Item type does not start with underscore (not private) -->
<Target Name="GoodExample2">
<ItemGroup>
<MyPublicItems Include="*.cs" />
</ItemGroup>
</Target>

<!-- OK: Item is exposed via Returns attribute -->
<Target Name="GoodExample3" Returns="@(_ComputedItems)">
<ItemGroup>
<_ComputedItems Include="@(SourceFiles->'%(Filename).obj')" />
</ItemGroup>
</Target>

<!-- OK: Item is exposed via Outputs attribute -->
<Target Name="GoodExample4" Outputs="@(_GeneratedFiles)">
<ItemGroup>
<_GeneratedFiles Include="$(OutputDir)/*.generated.cs" />
</ItemGroup>
</Target>

<!-- OK: Item only uses Update (no new items created) -->
<Target Name="GoodExample5">
<ItemGroup>
<_ExistingItems Update="@(_ExistingItems)" SomeMetadata="value" />
</ItemGroup>
</Target>
```

## Build Events

This check operates on **parsed/static XML structure** of the project file during evaluation. It uses the `ParsedItemsCheckData` (or via `ProjectRootElement` traversal) to examine:

- `ProjectTargetElement` - for each target in the project
- `ProjectItemGroupElement` - for ItemGroups within targets
- `ProjectItemElement` - for individual item definitions

## Scope

- **Project Types**: All MSBuild projects (SDK-style and legacy)
- **Configurations**: Applied uniformly regardless of build configuration
- **Imports**: By default, only the project file itself is analyzed. The scope can be configured via the standard `EvaluationCheckScope` setting.

## Expected Behavior

### When the check fires:
- A warning is reported at the location of the `Include` attribute on the offending item element
- The message identifies the item type name and the target name

### When the check does NOT fire:
- Item types not starting with underscore
- Items that have a matching `Remove` operation in the same target
- Items referenced in the target's `Outputs` or `Returns` attributes
- Items that only use `Update` (not `Include`)

## Configuration Options

### Severity

The default severity is `Warning`. Users can configure this via `.editorconfig`:

```ini
[*.csproj]
build_check.BC0303.severity=warning # default
build_check.BC0303.severity=error # treat as build error
build_check.BC0303.severity=suggestion # informational only
build_check.BC0303.severity=none # disable
```

### Scope

```ini
[*.csproj]
build_check.BC0303.scope=project_file # default - only project file
build_check.BC0303.scope=work_tree_imports # include non-SDK imports
build_check.BC0303.scope=all # include all imports
```

## Error Code

- **Code**: BC0303
- **Title**: PrivateItemsNotDisposed
- **Category**: Build Authoring / Performance

## Message Format

```
Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory.
```

Where:
- `{0}` = The item type name (e.g., `_TempFiles`)
- `{1}` = The target name (e.g., `CompileCore`)

## Performance Considerations

- **Impact**: Low - only examines static XML structure during evaluation
- **Caching**: Uses the existing `ProjectRootElement` cache
- **Scope optimization**: When scope is limited to project file only, imports are not traversed

## Implementation Notes

1. The check should be case-insensitive when comparing item type names (MSBuild convention)
2. When checking for `Remove` operations, the item type must match exactly
3. The `Outputs` and `Returns` attributes may contain property expressions that reference items - these should be pattern-matched for `@(ItemType)` syntax
4. Multiple `Include` operations for the same item type in the same target only need ONE matching `Remove`
5. The check should handle the case where `Include` and `Remove` are on the SAME element (which is a no-op but valid)

## Related Checks

- BC0201 - Usage of undefined property (similar pattern of detecting unbalanced operations)
- BC0203 - Property declared but never used (similar concept of detecting unused declarations)

## References

- [MSBuild Item Element Documentation](https://docs.microsoft.com/visualstudio/msbuild/item-element-msbuild)
- [MSBuild Target Element Documentation](https://docs.microsoft.com/visualstudio/msbuild/target-element-msbuild)
- [MSBuild Best Practices](https://docs.microsoft.com/visualstudio/msbuild/msbuild-best-practices)
Loading