-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix CA1873 false positives for simple params logging args #53027
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
base: main
Are you sure you want to change the base?
Conversation
|
But it is expensive, isn't it? It's allocating an array even when logging is disabled. There's no overload that just takes a string or object or T, only a params array. |
|
Interesting point, then it's really not obvious from the warning itself and the usage. Looking at the code you don't expect it to allocate an array, I know that's what Would it be possible then to have an overload that take a single value for common cases without the array allocation? Otherwise, I will definitely update the code where it's used with guards, knowing that new information. I will interview some colleagues to see if I was alone with that missing piece of knowledge. |
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 fixes a CA1873 analyzer regression introduced in #51839 where simple string reference arguments to logging methods (e.g., logger.LogInformation("Value: {Value}", value)) incorrectly triggered expensive operation warnings.
Changes:
- Modified
IsPotentiallyExpensiveto recursively inspect implicit params array/collection elements instead of treating all params wrappers as expensive - Preserved existing behavior: explicit array/collection creation is still flagged as expensive
- Added regression tests for C# and Visual Basic
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Microsoft.CodeAnalysis.NetAnalyzers/src/Microsoft.CodeAnalysis.NetAnalyzers/Microsoft.NetCore.Analyzers/Performance/AvoidPotentiallyExpensiveCallWhenLogging.cs | Updated IsPotentiallyExpensive to check elements of implicit params arrays/collections; renamed helper from IsEmptyImplicitParamsArrayCreation to IsImplicitParamsArrayCreation |
| src/Microsoft.CodeAnalysis.NetAnalyzers/tests/Microsoft.CodeAnalysis.NetAnalyzers.UnitTests/Microsoft.NetCore.Analyzers/Performance/AvoidPotentiallyExpensiveCallWhenLoggingTests.cs | Added regression tests for C# and VB verifying no diagnostic for simple string reference params |
That's what analyzers are best at, highlighting things you can't see in the code.
Possibly, or a |
I'd be happy to see the analyzer updated with more details about what the cost is, e.g. array allocation, unknown method call, boxing of value types, string allocation (concatenation, interpolation, ...), etc. |
Summary
Fixes a CA1873 regression where simple string-reference logging arguments started producing warnings in 10.0.103.
Fixes #53006.
Problem
After #51839,
IsPotentiallyExpensivetreated compiler-generated implicitparamswrappers (array/collection) as expensive based on wrapper shape alone. This caused false positives for calls like:when
valueis a simple string reference.Fix
AvoidPotentiallyExpensiveCallWhenLoggingAnalyzer.IsPotentiallyExpensive:IArrayCreationOperation(params wrapper) by inspecting elements and only flagging when any contained element is actually expensive.ImplicitReferenceTypeParamsArrayCreation_NoDiagnostic_CSImplicitReferenceTypeParamsArrayCreation_NoDiagnostic_VBTesting
10.0.103: CA1873 appears forlogger.LogInformation("...", value)with stringvalue.10.0.101: same code does not produce CA1873.