diff --git a/.claude/agents/msbuild-buildcheck-creator.md b/.claude/agents/msbuild-buildcheck-creator.md new file mode 100644 index 00000000000..cf434c1a21b --- /dev/null +++ b/.claude/agents/msbuild-buildcheck-creator.md @@ -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\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\n\n\n\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\n\n\n\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\n +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. diff --git a/documentation/specs/BuildCheck/Codes.md b/documentation/specs/BuildCheck/Codes.md index 1d2faddf3cd..55f31e6b709 100644 --- a/documentation/specs/BuildCheck/Codes.md +++ b/documentation/specs/BuildCheck/Codes.md @@ -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. @@ -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. -
-
-
+ +## 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 + + + <_TempArgs Include="arg1;arg2;arg3" /> + + + + +``` + +**Corrected example:** + +```xml + + + <_TempArgs Include="arg1;arg2;arg3" /> + + + + <_TempArgs Remove="@(_TempArgs)" /> + + +``` + +**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) diff --git a/documentation/specs/BuildCheck/ItemDisposalCheck.md b/documentation/specs/BuildCheck/ItemDisposalCheck.md new file mode 100644 index 00000000000..7c7d19dfd54 --- /dev/null +++ b/documentation/specs/BuildCheck/ItemDisposalCheck.md @@ -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 + + + + <_TempFiles Include="$(TempDir)/*.tmp" /> + + + + + + + + <_DotnetExecArgs Include="dotnet;tool;exec;$(CoolToolName)" /> + + + +``` + +### Negative Examples (should NOT fire) + +```xml + + + + <_TempFiles Include="$(TempDir)/*.tmp" /> + + + + <_TempFiles Remove="@(_TempFiles)" /> + + + + + + + + + + + + + + <_ComputedItems Include="@(SourceFiles->'%(Filename).obj')" /> + + + + + + + <_GeneratedFiles Include="$(OutputDir)/*.generated.cs" /> + + + + + + + <_ExistingItems Update="@(_ExistingItems)" SomeMetadata="value" /> + + +``` + +## 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) diff --git a/src/Build/BuildCheck/Checks/ItemDisposalCheck.cs b/src/Build/BuildCheck/Checks/ItemDisposalCheck.cs new file mode 100644 index 00000000000..649a129bfa7 --- /dev/null +++ b/src/Build/BuildCheck/Checks/ItemDisposalCheck.cs @@ -0,0 +1,182 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using Microsoft.Build.Collections; +using Microsoft.Build.Construction; +using Microsoft.Build.Evaluation; +using Microsoft.Build.Shared; + +namespace Microsoft.Build.Experimental.BuildCheck.Checks; + +/// +/// Check that detects when MSBuild Targets create "private" item lists (names starting with underscore) +/// that are not cleaned up at the end of the target. +/// +internal sealed class ItemDisposalCheck : Check +{ + private const string RuleId = "BC0303"; + + private readonly SimpleProjectRootElementCache _cache = new(); + private readonly HashSet _projectsSeen = new(MSBuildNameIgnoreCaseComparer.Default); + + public static readonly CheckRule SupportedRule = new( + RuleId, + "PrivateItemsNotDisposed", + ResourceUtilities.GetResourceString("BuildCheck_BC0303_Title"), + ResourceUtilities.GetResourceString("BuildCheck_BC0303_MessageFmt"), + new CheckConfiguration() { RuleId = RuleId, Severity = CheckResultSeverity.Warning, EvaluationCheckScope = EvaluationCheckScope.ProjectFileOnly }); + + public override string FriendlyName => "MSBuild.ItemDisposalCheck"; + + public override IReadOnlyList SupportedRules { get; } = [SupportedRule]; + + internal override bool IsBuiltIn => true; + + public override void Initialize(ConfigurationContext configurationContext) + { } + + public override void RegisterActions(IBuildCheckRegistrationContext registrationContext) => registrationContext.RegisterEvaluatedItemsAction(EvaluatedItemsAction); + + private void EvaluatedItemsAction(BuildCheckDataContext context) + { + if (!_projectsSeen.Add(context.Data.ProjectFilePath)) + { + return; + } + + ProjectRootElement? projectRoot; + try + { + projectRoot = ProjectRootElement.OpenProjectOrSolution( + context.Data.ProjectFilePath, + globalProperties: null, + toolsVersion: null, + _cache, + isExplicitlyLoaded: false); + } + catch + { + return; + } + + foreach (ProjectTargetElement target in projectRoot.Targets) + { + AnalyzeTarget(target, context.ReportResult); + } + } + + /// + /// Analyzes a single target for private items that are not properly disposed. + /// Single pass through item groups: Include adds to pending, Remove clears from pending. + /// Items still pending at the end (and not exposed via Outputs/Returns) are violations. + /// + private static void AnalyzeTarget(ProjectTargetElement target, Action reportResult) + { + // Track private items with Include that need cleanup (case-insensitive) + Dictionary? pendingPrivateItems = null; + + foreach (ProjectItemGroupElement itemGroup in target.ItemGroups) + { + foreach (ProjectItemElement item in itemGroup.Items) + { + string itemType = item.ItemType; + + // Only process private items (starting with underscore) + if (itemType.Length == 0 || itemType[0] != '_') + { + continue; + } + + bool hasInclude = item.Include.Length > 0; + bool hasRemove = item.Remove.Length > 0; + + if (hasInclude) + { + pendingPrivateItems ??= new(MSBuildNameIgnoreCaseComparer.Default); + pendingPrivateItems[itemType] = item; + } + + if (hasRemove) + { + pendingPrivateItems?.Remove(itemType); + } + } + } + + if (pendingPrivateItems is null || pendingPrivateItems.Count == 0) + { + return; + } + + // Get exposed items only if we have pending items to check + HashSet? exposedItemTypes = GetExposedItemTypes(target); + + foreach (KeyValuePair kvp in pendingPrivateItems) + { + if (exposedItemTypes?.Contains(kvp.Key) == true) + { + continue; + } + + reportResult(BuildCheckResult.CreateBuiltIn( + SupportedRule, + kvp.Value.IncludeLocation ?? kvp.Value.Location, + kvp.Key, + target.Name)); + } + } + + /// + /// Extracts all item types referenced in the target's Outputs and Returns attributes. + /// Returns null if neither attribute is set. + /// + private static HashSet? GetExposedItemTypes(ProjectTargetElement target) + { + string? outputs = target.Outputs; + string? returns = target.Returns; + + bool hasOutputs = !string.IsNullOrEmpty(outputs); + bool hasReturns = !string.IsNullOrEmpty(returns); + + if (!hasOutputs && !hasReturns) + { + return null; + } + + string[] expressions = (hasOutputs, hasReturns) switch + { + (true, true) => [outputs, returns], + (true, false) => [outputs], + (false, true) => [returns], + _ => [] + }; + + ItemsAndMetadataPair pair = ExpressionShredder.GetReferencedItemNamesAndMetadata(expressions); + + if (pair.Items is null) + { + return null; + } + + HashSet exposedItems = new(MSBuildNameIgnoreCaseComparer.Default); + foreach (string itemType in pair.Items) + { + exposedItems.Add(itemType); + } + + return exposedItems; + } + + /// + /// Internal method for testing - analyzes targets and collects results. + /// + internal void AnalyzeTargets(ProjectRootElement projectRoot, List results) + { + foreach (ProjectTargetElement target in projectRoot.Targets) + { + AnalyzeTarget(target, results.Add); + } + } +} diff --git a/src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs b/src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs index 89998bad255..5048206458b 100644 --- a/src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs +++ b/src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs @@ -155,6 +155,7 @@ internal readonly record struct BuiltInCheckFactory( new BuiltInCheckFactory([TargetFrameworkConfusionCheck.SupportedRule.Id], TargetFrameworkConfusionCheck.SupportedRule.DefaultConfiguration.IsEnabled ?? false, Construct), new BuiltInCheckFactory([TargetFrameworkUnexpectedCheck.SupportedRule.Id], TargetFrameworkUnexpectedCheck.SupportedRule.DefaultConfiguration.IsEnabled ?? false, Construct), new BuiltInCheckFactory([UntrustedLocationCheck.SupportedRule.Id], UntrustedLocationCheck.SupportedRule.DefaultConfiguration.IsEnabled ?? false, Construct), + new BuiltInCheckFactory([ItemDisposalCheck.SupportedRule.Id], ItemDisposalCheck.SupportedRule.DefaultConfiguration.IsEnabled ?? false, Construct), ], // BuildCheckDataSource.Execution diff --git a/src/Build/Resources/Strings.resx b/src/Build/Resources/Strings.resx index 68b59de6ee8..471141541b5 100644 --- a/src/Build/Resources/Strings.resx +++ b/src/Build/Resources/Strings.resx @@ -2251,6 +2251,14 @@ Utilization: {0} Average Utilization: {1:###.0} Location: '{0}' cannot be fully trusted, place your projects outside of that folder (Project: {1}). + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Logging type {0} is not understood by {1}. diff --git a/src/Build/Resources/xlf/Strings.cs.xlf b/src/Build/Resources/xlf/Strings.cs.xlf index 86d6fc9c60c..2a02a45fe9a 100644 --- a/src/Build/Resources/xlf/Strings.cs.xlf +++ b/src/Build/Resources/xlf/Strings.cs.xlf @@ -276,6 +276,16 @@ Složka Stažené soubory není důvěryhodná pro sestavování projektů. + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s Sestavení {0} za {1}s diff --git a/src/Build/Resources/xlf/Strings.de.xlf b/src/Build/Resources/xlf/Strings.de.xlf index 4adaf0b3973..139f05d99f3 100644 --- a/src/Build/Resources/xlf/Strings.de.xlf +++ b/src/Build/Resources/xlf/Strings.de.xlf @@ -276,6 +276,16 @@ Der Ordner "Downloads" ist für die Projekterstellung nicht vertrauenswürdig. + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s Erstellen von {0} in {1}s diff --git a/src/Build/Resources/xlf/Strings.es.xlf b/src/Build/Resources/xlf/Strings.es.xlf index ed841430d86..35d860bfb16 100644 --- a/src/Build/Resources/xlf/Strings.es.xlf +++ b/src/Build/Resources/xlf/Strings.es.xlf @@ -276,6 +276,16 @@ La carpeta descargas no es de confianza para la compilación de proyectos. + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s Compilación {0} en {1}s diff --git a/src/Build/Resources/xlf/Strings.fr.xlf b/src/Build/Resources/xlf/Strings.fr.xlf index 2a866b00d4e..c1003c0d658 100644 --- a/src/Build/Resources/xlf/Strings.fr.xlf +++ b/src/Build/Resources/xlf/Strings.fr.xlf @@ -276,6 +276,16 @@ Le dossier des téléchargements n’est pas approuvé pour la génération de projets. + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s Générer {0} dans {1}s diff --git a/src/Build/Resources/xlf/Strings.it.xlf b/src/Build/Resources/xlf/Strings.it.xlf index 99d4affd579..688c91e7cd0 100644 --- a/src/Build/Resources/xlf/Strings.it.xlf +++ b/src/Build/Resources/xlf/Strings.it.xlf @@ -276,6 +276,16 @@ La cartella Dei download non è attendibile per la compilazione di progetti. + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s Compilazione {0} in {1}s diff --git a/src/Build/Resources/xlf/Strings.ja.xlf b/src/Build/Resources/xlf/Strings.ja.xlf index 38b60d181d3..4d7c1ad29d8 100644 --- a/src/Build/Resources/xlf/Strings.ja.xlf +++ b/src/Build/Resources/xlf/Strings.ja.xlf @@ -276,6 +276,16 @@ ダウンロード フォルダーは、プロジェクトのビルドに対して信頼されていません。 + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s {1} 秒後に {0} をビルド diff --git a/src/Build/Resources/xlf/Strings.ko.xlf b/src/Build/Resources/xlf/Strings.ko.xlf index e5aa1a02533..54fdcd1d7fe 100644 --- a/src/Build/Resources/xlf/Strings.ko.xlf +++ b/src/Build/Resources/xlf/Strings.ko.xlf @@ -276,6 +276,16 @@ 프로젝트 빌드에 대해 다운로드 폴더를 신뢰할 수 없습니다. + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s {0} 빌드({1}초) diff --git a/src/Build/Resources/xlf/Strings.pl.xlf b/src/Build/Resources/xlf/Strings.pl.xlf index 866d90bcd9d..6097a91f8bf 100644 --- a/src/Build/Resources/xlf/Strings.pl.xlf +++ b/src/Build/Resources/xlf/Strings.pl.xlf @@ -276,6 +276,16 @@ Folder Pobrane nie jest zaufany do kompilowania projektów. + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s Kompiluj {0} w {1}s diff --git a/src/Build/Resources/xlf/Strings.pt-BR.xlf b/src/Build/Resources/xlf/Strings.pt-BR.xlf index d27c50e5464..0e584d4b82e 100644 --- a/src/Build/Resources/xlf/Strings.pt-BR.xlf +++ b/src/Build/Resources/xlf/Strings.pt-BR.xlf @@ -276,6 +276,16 @@ A pasta Downloads não é confiável para a compilação de projetos. + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s Construir {0} em {1}s diff --git a/src/Build/Resources/xlf/Strings.ru.xlf b/src/Build/Resources/xlf/Strings.ru.xlf index 55d613e49cd..4f86d02827e 100644 --- a/src/Build/Resources/xlf/Strings.ru.xlf +++ b/src/Build/Resources/xlf/Strings.ru.xlf @@ -276,6 +276,16 @@ Папка "Загрузки" недоверена для сборки проектов. + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s Сборка {0} через {1} с diff --git a/src/Build/Resources/xlf/Strings.tr.xlf b/src/Build/Resources/xlf/Strings.tr.xlf index 7a5e481602b..4477f749dce 100644 --- a/src/Build/Resources/xlf/Strings.tr.xlf +++ b/src/Build/Resources/xlf/Strings.tr.xlf @@ -276,6 +276,16 @@ İndirmeler klasörü, proje oluşturma için güvenilir değil. + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s "{1}" sn'de {0} oluşturun diff --git a/src/Build/Resources/xlf/Strings.zh-Hans.xlf b/src/Build/Resources/xlf/Strings.zh-Hans.xlf index d8da78803bc..135404f4538 100644 --- a/src/Build/Resources/xlf/Strings.zh-Hans.xlf +++ b/src/Build/Resources/xlf/Strings.zh-Hans.xlf @@ -276,6 +276,16 @@ 生成项目不信任下载文件夹。 + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s 在 {1} 秒内生成 {0} diff --git a/src/Build/Resources/xlf/Strings.zh-Hant.xlf b/src/Build/Resources/xlf/Strings.zh-Hant.xlf index d851b21ba23..aef563b8920 100644 --- a/src/Build/Resources/xlf/Strings.zh-Hant.xlf +++ b/src/Build/Resources/xlf/Strings.zh-Hant.xlf @@ -276,6 +276,16 @@ 專案建置不受信任的下載資料夾。 + + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Private item list '{0}' created in target '{1}' should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + + + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Private item lists created inside a target should be removed at the end of the target to avoid wasting memory. + Terms in quotes are not to be translated. + Build {0} in {1}s 在 {1} 秒內建置 {0} diff --git a/src/BuildCheck.UnitTests/ItemDisposalCheck_Tests.cs b/src/BuildCheck.UnitTests/ItemDisposalCheck_Tests.cs new file mode 100644 index 00000000000..71a1ddb495b --- /dev/null +++ b/src/BuildCheck.UnitTests/ItemDisposalCheck_Tests.cs @@ -0,0 +1,755 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using Microsoft.Build.Construction; +using Microsoft.Build.Experimental.BuildCheck; +using Microsoft.Build.Experimental.BuildCheck.Checks; +using Shouldly; +using Xunit; + +namespace Microsoft.Build.BuildCheck.UnitTests +{ + public sealed class ItemDisposalCheck_Tests + { + private readonly ItemDisposalCheck _check; + + public ItemDisposalCheck_Tests() + { + _check = new ItemDisposalCheck(); + } + + #region Positive Tests - Should Fire + + [Fact] + public void PrivateItem_NotRemoved_ShouldFire() + { + // Arrange + string projectContent = """ + + + + <_TempFiles Include="*.tmp" /> + + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(1); + results[0].CheckRule.Id.ShouldBe("BC0303"); + results[0].MessageArgs[0].ShouldBe("_TempFiles"); + results[0].MessageArgs[1].ShouldBe("TestTarget"); + } + + [Fact] + public void PrivateItem_UsedInExec_NotRemoved_ShouldFire() + { + // Arrange + string projectContent = """ + + + + <_DotnetExecArgs Include="dotnet;tool;exec;$(CoolToolName)" /> + + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(1); + results[0].CheckRule.Id.ShouldBe("BC0303"); + results[0].MessageArgs[0].ShouldBe("_DotnetExecArgs"); + results[0].MessageArgs[1].ShouldBe("RunTool"); + } + + [Fact] + public void MultiplePrivateItems_NoneRemoved_ShouldFireForEach() + { + // Arrange + string projectContent = """ + + + + <_Item1 Include="a.txt" /> + <_Item2 Include="b.txt" /> + <_Item3 Include="c.txt" /> + + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(3); + results.ShouldAllBe(r => r.CheckRule.Id == "BC0303"); + } + + [Fact] + public void PrivateItem_InMultipleTargets_EachNotRemoved_ShouldFireForEach() + { + // Arrange + string projectContent = """ + + + + <_Temp Include="a.txt" /> + + + + + <_Other Include="b.txt" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(2); + results.ShouldAllBe(r => r.CheckRule.Id == "BC0303"); + } + + [Fact] + public void PrivateItem_PartialRemove_ShouldFire() + { + // The Remove uses a different wildcard, so not all items are cleaned up + string projectContent = """ + + + + <_Files Include="**/*.cs" /> + + + <_Files Remove="*.cs" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // This is a bit nuanced - in static analysis we can't know if Remove covers all items + // So we check if there's ANY Remove for the same item type + // With a proper Remove present, this should NOT fire + results.Count.ShouldBe(0); + } + + #endregion + + #region Negative Tests - Should NOT Fire + + [Fact] + public void PrivateItem_ProperlyRemoved_ShouldNotFire() + { + // Arrange + string projectContent = """ + + + + <_TempFiles Include="*.tmp" /> + + + + <_TempFiles Remove="@(_TempFiles)" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_RemovedWithWildcard_ShouldNotFire() + { + // Arrange + string projectContent = """ + + + + <_TempFiles Include="*.tmp" /> + + + <_TempFiles Remove="**/*" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PublicItem_NotRemoved_ShouldNotFire() + { + // Items not starting with underscore are considered public + string projectContent = """ + + + + + + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_InOutputs_ShouldNotFire() + { + // Item is exposed via Outputs attribute - considered part of target's public contract + string projectContent = """ + + + + <_ComputedFiles Include="$(OutputDir)/*.dll" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_InReturns_ShouldNotFire() + { + // Item is exposed via Returns attribute - considered part of target's public contract + string projectContent = """ + + + + <_GeneratedFiles Include="@(SourceFiles->'%(Filename).obj')" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_InOutputsWithTransform_ShouldNotFire() + { + // Item is referenced in Outputs with a transform + string projectContent = """ + + + + <_Sources Include="**/*.cs" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_OnlyUpdate_ShouldNotFire() + { + // Update doesn't create new items, only modifies existing ones + string projectContent = """ + + + + <_ExistingItems Update="@(_ExistingItems)" SomeMetadata="value" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_OnlyRemove_ShouldNotFire() + { + // Remove only - no Include, nothing was created in this target + string projectContent = """ + + + + <_OldItems Remove="@(_OldItems)" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void ItemOutsideTarget_ShouldNotFire() + { + // Items outside targets are in the global scope - not target-private + string projectContent = """ + + + <_GlobalItem Include="*.cs" /> + + + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void EmptyTarget_ShouldNotFire() + { + // Arrange + string projectContent = """ + + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void TargetWithOnlyTasks_ShouldNotFire() + { + // Arrange + string projectContent = """ + + + + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + #endregion + + #region Edge Cases + + [Fact] + public void PrivateItem_CaseInsensitiveRemove_ShouldNotFire() + { + // MSBuild item types are case-insensitive + string projectContent = """ + + + + <_TempFiles Include="*.tmp" /> + + + <_tempfiles Remove="@(_TEMPFILES)" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_MultipleIncludes_SingleRemove_ShouldNotFire() + { + // Multiple includes for same type, one remove should cover all + string projectContent = """ + + + + <_Files Include="a.txt" /> + <_Files Include="b.txt" /> + <_Files Include="c.txt" /> + + + <_Files Remove="@(_Files)" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_ConditionalInclude_NoRemove_ShouldFire() + { + // Conditional includes still need cleanup + string projectContent = """ + + + + <_DebugFiles Include="*.pdb" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(1); + results[0].MessageArgs[0].ShouldBe("_DebugFiles"); + } + + [Fact] + public void PrivateItem_RemoveBeforeInclude_ShouldFire() + { + // Remove before Include doesn't clean up what Include adds + string projectContent = """ + + + + <_Files Remove="@(_Files)" /> + + + <_Files Include="*.txt" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(1); + } + + [Fact] + public void PrivateItem_InReturnsWithMultipleItems_ShouldNotFireForReferencedOnes() + { + // Only _Other should fire, _Result is in Returns + string projectContent = """ + + + + <_Other Include="temp.txt" /> + <_Result Include="output.dll" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(1); + results[0].MessageArgs[0].ShouldBe("_Other"); + } + + [Fact] + public void PrivateItem_ComplexOutputsExpression_ShouldNotFire() + { + // Complex Outputs expression referencing the item + string projectContent = """ + + + + <_InputFiles Include="*.input" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_InOutputsWithSemicolonInTransform_ShouldNotFire() + { + // Transform expression containing semicolons should be properly parsed + string projectContent = """ + + + + <_Files Include="*.cs" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_InReturnsWithComplexTransform_ShouldNotFire() + { + // Complex transform with nested properties and metadata + string projectContent = """ + + + + <_Items Include="src\**\*.cs" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void MultiplePrivateItems_InSameOutputsExpression_ShouldNotFireForAny() + { + // Multiple private items referenced in same Outputs expression + string projectContent = """ + + + + <_Sources Include="*.cs" /> + <_Resources Include="*.resx" /> + <_Content Include="*.txt" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_InOutputsWithCustomSeparator_ShouldNotFire() + { + // Item reference with custom separator containing semicolons + string projectContent = """ + + + + <_Files Include="a.txt;b.txt;c.txt" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void PrivateItem_InReturnsWithItemFunction_ShouldNotFire() + { + // Item function call in Returns expression + string projectContent = """ + + + + <_Duplicates Include="a;b;c;a;b" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void MixedPrivateItems_SomeInOutputsSomeNot_ShouldFireOnlyForNonExposed() + { + // Some private items in Outputs, others not + string projectContent = """ + + + + <_Exposed Include="out.dll" /> + <_NotExposed Include="temp.tmp" /> + <_AlsoNotExposed Include="scratch.dat" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(2); + results.ShouldContain(r => r.MessageArgs[0].ToString() == "_NotExposed"); + results.ShouldContain(r => r.MessageArgs[0].ToString() == "_AlsoNotExposed"); + } + + [Fact] + public void PrivateItem_InOutputsAndReturns_ShouldNotFire() + { + // Item referenced in both Outputs and Returns + string projectContent = """ + + + + <_Built Include="output.dll" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(0); + } + + [Fact] + public void SingleUnderscoreItem_ShouldFire() + { + // Edge case: single underscore item name + string projectContent = """ + + + + <_ Include="weird.txt" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(1); + results[0].MessageArgs[0].ShouldBe("_"); + } + + [Fact] + public void DoubleUnderscoreItem_ShouldFire() + { + // Double underscore prefix + string projectContent = """ + + + + <__VeryPrivate Include="secret.txt" /> + + + + """; + + // Act + var results = AnalyzeProject(projectContent); + + // Assert + results.Count.ShouldBe(1); + results[0].MessageArgs[0].ShouldBe("__VeryPrivate"); + } + + #endregion + + #region Helper Methods + + private List AnalyzeProject(string projectContent) + { + // Create a project from the content string + using var stringReader = new System.IO.StringReader(projectContent); + using var xmlReader = System.Xml.XmlReader.Create(stringReader); + var root = ProjectRootElement.Create(xmlReader); + + // Create a fresh results list for each test + var results = new List(); + + // Analyze all targets + _check.AnalyzeTargets(root, results); + + return results; + } + + #endregion + } +}