[Analyzer] Replacing direct LoggerExtensions.Log* usage with source-generated logging methods

[stephentoub]

Just as we have a regex source generator analyzer that flags use of Regex and creates a replacement that uses the source generator, we should consider an analyzer that will push code to use the logging source generator. The analyzer would flag direct use of LoggerExtensions.Log* methods and recommend replacing them with use of the logging generator, with the fixer generating a partial method stub for the generator and changing the call site to use it.

Performance rules Category
Severity = none (off by default)

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Just as we have a regex source generator analyzer that flags use of Regex and creates a replacement that uses the source generator, we should consider an analyzer that will push code to use the logging source generator. The analyzer would flag direct use of LoggerExtensions.Log* methods and recommend replacing them with use of the logging generator, with the fixer generating a partial method stub for the generator and changing the call site to use it.

Author:	stephentoub
Assignees:	-
Labels:	area-Extensions-Logging
Milestone:	-

[tarekgh]

I'll be careful doing that. using LoggerExtensions.Log still legitimate and many users already using it. Even producing analyzer info and not warning for this is not desirable for the users. I wouldn't mind having this analyzer, but it should be off by default. @stephentoub what you think about that?

CC @ericstj @Youssef1313

[pinkfloydx33]

Please off by default...especially for quick utility apps that don't really care about perf gains from logging. I'd hate to suddenly have a ton of diagnostics... at any level. Regex is one thing, but log messages are typically at a totally different scale.

And please not before fixing issues with object destructuring syntax in the logger messages, ie. "My {@Thing} did something" and the various other requests still open that prevent using the generator in some cases.

[stephentoub]

I'd hate to suddenly have a ton of analyzer warnings (or broken builds with TreatWarningsAsErrors) like I did with the Regex generator

The regex analyzer defaults to info level; that wouldn't show up in command-line builds at all, and in VS would show up only as suggestions. Unless you changed the default level, it should never break the build. If we added an analyzer for logging it similarly wouldn't be warning or error; none of our performance-focused analyzers are.

And please not before fixing issues with object destructuring syntax in the logger messages, ie. "My {@thing} did something" and the various other requests still open that prevent using the generator in some cases.

An analyzer shouldn't fire if it the thing it's targeting can't actually be replaced. For example, the regex analyzer won't raise a diagnostic if the pattern isn't a const, because that can't be rewritten to use the source generator. Similarly, if there are messages that can't statically be proven to work with the logging generator, the analyzer shouldn't raise a diagnostic.

[tarekgh]

or logging it similarly wouldn't be warning or error

Logging users didn't even like suggestions to be shown either and they were confused between suggestions and warning. For this rule, I believe it should be off by default and users can opt-in using it. The suggested methods we need to analyze are used a lot across the board and are still legitimate to use.

[pinkfloydx33]

The regex analyzer defaults to info level;

You're right... my mistake. But the info messages that didn't show up when building locally triggered during Sonar Cloud analysis and apparently they're picking up SYSLIB info messages as code smells, failing my build. Sorry, Misinterpreted what happened on my end....

I fixed the regex generator ones anyways, so all good. But I'd hate to have a ton of info/suggestion-level diags for all the logger calls showing up in VS (or apparently angering sonar cloud!)

[tarekgh]

I have edited the description to have this analyzer off by default and will be part of performance category. Let me know if you have any concerns about that.

[tarekgh]

CC @Youssef1313

[bartonjs]

Looks good as proposed.

Since the fixer will have trouble creating the partial method + attribute when the message template is not a string literal, a different diagnostic ID should be used for literal templates and non-literal templates.

Consider using a second pair of diagnostic IDs for LoggerMessage.Define-based logging.

Category:Performance
Severity:none (requires manual opt-in)

[mrahhal]

Have a clarifying question before deciding to work on this.

The fixer would have to create a partial method stub as explained, but this needs to be in a partial class (in the docs, this is shown as a static partial Log class). How do we know where to generate this method stub in?

Just tried SYSLIB1045 (regex) in code, and it seems to simply add partial to the current class. Guessing we can just do the same?

[tarekgh]

@mrahhal yes, please mimic regex behavior and include the method inside the current class with adding partial specifier.