dotnet / roslyn-analyzers

MIT License
1.56k stars 462 forks source link

Add CA1873: Avoid potentially expensive logging #7290

Open mpidash opened 3 months ago

mpidash commented 3 months ago

Fixes https://github.com/dotnet/runtime/issues/78402.

Rule ID CA1873
Title Avoid potentially expensive logging
Message Evaluation of this argument may be expensive and unnecessary if logging is disabled
Description In many situations, logging is disabled or set to a log level that results in an unnecessary evaluation for this argument.
Category Performance
Severity Suggestion

This analyzer detects

and flags them if they evaluate expensive arguments without checking if logging is enabled with ILogger.IsEnabled.


There are 12 findings in dotnet/runtime (8 of them in Microsoft.Extensions.Logging.Abstractions which will probably disable this warning), 157 findings in dotnet/roslyn and 497 in dotnet/aspnetcore (including testing code). I skimmed through them and could not find any false positives.

mpidash commented 3 months ago

cc @stephentoub @Youssef1313 (thanks for providing the prototype 👍)

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 84.32432% with 29 lines in your changes missing coverage. Please review.

Project coverage is 96.51%. Comparing base (43709af) to head (fbf9763).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #7290 +/- ## ========================================== + Coverage 96.49% 96.51% +0.02% ========================================== Files 1443 1445 +2 Lines 345799 348926 +3127 Branches 11374 11414 +40 ========================================== + Hits 333665 336779 +3114 + Misses 9251 9250 -1 - Partials 2883 2897 +14 ```
stephentoub commented 2 months ago

I skimmed through them and could not find any false positives.

ca1873-runtime.txt

Thanks. The LoggerExtensions.cs ones are all false positives in that we'll want to suppress them, but that's also effectively the implementation of logging rather than consumption of logging, and we frequently have to suppress rules in such implementations. The others for runtime look like valid diagnostics.