-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add CA1877: Collapse nested Path.Combine/Path.Join calls #51456
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.
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
Conversation
|
@copilot, it looks like you ran dotnet format and committed the results of doing so, changing hundreds of files. Please revert all of those unnecessary changes. |
...lyzers/Microsoft.NetCore.Analyzers/Performance/CSharpCollapseMultiplePathOperations.Fixer.cs
Outdated
Show resolved
Hide resolved
...lysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Performance/CollapseMultiplePathOperations.cs
Outdated
Show resolved
Hide resolved
...lysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Performance/CollapseMultiplePathOperations.cs
Show resolved
Hide resolved
...lysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Performance/CollapseMultiplePathOperations.cs
Show resolved
Hide resolved
...alyzers/Microsoft.NetCore.Analyzers/Performance/BasicCollapseMultiplePathOperations.Fixer.vb
Outdated
Show resolved
Hide resolved
...ers.UnitTests/Microsoft.NetCore.Analyzers/Performance/CollapseMultiplePathOperationsTests.cs
Show resolved
Hide resolved
...lyzers/Microsoft.NetCore.Analyzers/Performance/CSharpCollapseMultiplePathOperations.Fixer.cs
Outdated
Show resolved
Hide resolved
...odeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx
Show resolved
Hide resolved
...lysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Performance/CollapseMultiplePathOperations.cs
Outdated
Show resolved
Hide resolved
...lyzers/Microsoft.NetCore.Analyzers/Performance/CSharpCollapseMultiplePathOperations.Fixer.cs
Outdated
Show resolved
Hide resolved
...lysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Performance/CollapseMultiplePathOperations.cs
Outdated
Show resolved
Hide resolved
...lysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Performance/CollapseMultiplePathOperations.cs
Outdated
Show resolved
Hide resolved
...lysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Performance/CollapseMultiplePathOperations.cs
Outdated
Show resolved
Hide resolved
...ers.UnitTests/Microsoft.NetCore.Analyzers/Performance/CollapseMultiplePathOperationsTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements analyzer CA1876 to detect and collapse nested Path.Combine and Path.Join method calls into single operations, improving both performance and code readability. The analyzer identifies cases where nested calls of the same method type can be flattened, and provides an automated code fix to perform the transformation.
Key Changes:
- Added CA1876 analyzer that detects nested
Path.Combine/Path.Joincalls of the same method type - Implemented C# code fixer to automatically collapse nested calls into a single operation
- Added comprehensive unit tests covering various nesting scenarios
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| CollapseMultiplePathOperations.cs | Core analyzer implementation with span-aware argument counting and symbol-based method detection |
| CSharpCollapseMultiplePathOperations.Fixer.cs | C# code fixer that recursively collects arguments from nested calls |
| CollapseMultiplePathOperationsTests.cs | Comprehensive test suite covering nested calls, different positions, and mixed method scenarios |
| MicrosoftNetCoreAnalyzersResources.resx | Resource strings for diagnostic messages and code fix titles |
| DiagnosticCategoryAndIdRanges.txt | Updated Performance category range to include CA1876 |
| AnalyzerReleases.Unshipped.md | Added CA1876 to unshipped analyzer releases |
| Microsoft.CodeAnalysis.NetAnalyzers.sarif | SARIF metadata for CA1876 rule |
| Microsoft.CodeAnalysis.NetAnalyzers.md | Documentation entry for CA1876 |
| RulesMissingDocumentation.md | Removed entries for recently documented rules |
| *.xlf files | Localization files with new strings marked for translation |
...lyzers/Microsoft.NetCore.Analyzers/Performance/CSharpCollapseMultiplePathOperations.Fixer.cs
Outdated
Show resolved
Hide resolved
...lyzers/Microsoft.NetCore.Analyzers/Performance/CSharpCollapseMultiplePathOperations.Fixer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.CodeAnalysis.NetAnalyzers/src/RulesMissingDocumentation.md
Show resolved
Hide resolved
...lyzers/Microsoft.NetCore.Analyzers/Performance/CSharpCollapseMultiplePathOperations.Fixer.cs
Show resolved
Hide resolved
|
@copilot add test for case like the following too: string path = Path.Combine(Path.Join(Path.Combine(Path.Combine("a", "b"), "c"), "d"), "e"); |
... Added in commit f4413c6. The test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modulo restoring the missing doc links, LGTM!
f4413c6 to
a76c271
Compare
|
/ba-g dead letter |
Implementation for Path.Combine/Join collapsing analyzer (CA1876)
Changes from Review
Analyzer Improvements:
Fixer Improvements:
.ResultcallTest Improvements:
Path.Combine(Path.Join(Path.Combine(Path.Combine("a", "b"), "c"), "d"), "e")- verifies no diagnosticSummary
This PR adds analyzer CA1876 that detects nested Path.Combine/Path.Join calls and suggests collapsing them into a single operation for better performance.
Example
Before:
After:
Test Results
Security & Quality
Note: Per agent instructions, this implements only the nested call pattern (e.g.,
Path.Combine(Path.Combine(a, b), c)), not the intermediate variable pattern described in the issue.Original prompt
This section details on the original issue you should resolve
<issue_title>Analyzer/fixer proposal: Collapse multiple
Path.CombineorPath.Joinin a row</issue_title><issue_description>Suggested severity: Info
Suggested category: Reliability
When generating multiple combined/joined path segments in a row, where each depends on the previous one (excluding the first one of course), and only the last resulting segment is the one being properly consumed somewhere else, then they can be collapsed into a single invocation of
CombineorJoin.The maximum number of segments in a row that can be fixed are 3, since the largestCombine/Joinoverloads take 4 parameters.These are the APIs that can benefit from the analyzer:
An additional improvement: whenever possible, switch to using the
ReadOnlySpan<char>overload instead of thestringoverload, if the number of arguments allows it.Flag
paramsoverloadJoinandTryJoinoverloads that takeReadOnlySpan<char>parameters,Joincan only collapse 4,TryJoinonly 3Do not flag
Combine, the analyzer should not flag cases where there are potentialnullarguments.cc @buyaa-n </issue_description>
<agent_instructions>Please add an analyzer/fixer to the netanalyzers project to flag and fix cases where Path.Combine or Path.Join are called multiple times but could be combined into a single call. Do not worry about cases where the intermediate results are stored into locals or fields; we're only focused in the most common cases where the ar...
Path.CombineorPath.Joinin a row runtime#62057💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.