[Analyzer] Flagging issues in logger message templates w/ incomplete braces pairs

[Kritner]

Posting this issue to this repo as per the suggestion of @tarekgh

dotnet/roslyn-analyzers#7285
dotnet/roslyn-analyzers#7286

tldr: Invalid braces in a message template aren't caught by CA2017, and when encountered lead to runtime exceptions.

The PR and issues (linked and relevant snippets below) go about introducing a new analyzer CA2023 because the changes introduced in some ways change the existing "meaning" of CA2017. Additionally this seems like it should probably be a compiler error rather than warning since otherwise a runtime exception occurs - though tbf i don't recall if having too few or too many message template params lead to the same thing or not

Suggested category: Reliability (and related to) https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2017
Suggested severity: warning (https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-options#severity-level)

---

Issue:

malformed message template strings for at a minimum logged messages should be throwing compiler errors IMO, rather than the current runtime errors seen with .net8.

Repro:
https://github.com/Kritner/MessageTemplateNet8

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;

HostApplicationBuilder builder = Host.CreateApplicationBuilder(args);
builder.Services.AddLogging(loggingBuilder => loggingBuilder.AddConsole());

var host = builder.Build();

var logger = host.Services.GetRequiredService<ILogger<Program>>();

logger.LogInformation("Hello world");

var i = 5;
logger.LogInformation("My value {i}}", i);

---

I feel like this needs to be a compiler error, lest you run into the same run time errors I've encountered.

It seems this scenario is already covered with https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2017 @Kritner could you enable that analyzer in your repo (it is enabled and warns by default, but the rule might disabled for your project) and check the diagnostics?

Yeah so it's weird... we're not NoWarning against this particular "CA2017", but we don't get the string template being flagged as a CA2017... I can easily make the CA2017 appear (and get a compiler error yay) if I change...

logger.LogInformation("My value {i}}", i);

to

logger.LogInformation("My value {i}", i, i+1);

---

More relevant comments:
dotnet/roslyn-analyzers#7285 (comment)
dotnet/roslyn-analyzers#7285 (comment)
dotnet/roslyn-analyzers#7286 (comment)

[dotnet-policy-service]

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

[tarekgh]

@Kritner

Suggested severity: error (https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-options#severity-level)

Thinking loud, would it be better to have this as a warning instead of error? I recall some external logger allow mismatches.

[Kritner]

Thinking loud, would it be better to have this as a warning instead of error? I recall some external logger allow mismatches.

I’m sure that’s fine - i don’t know offhand what constitutes one over the other, just seemed like an error since failure to correct leads to runtime exceptions

but i guess that can be up to the consuming library, the built in net logging however does experience the exception

[tarekgh]

I changed it to warning and we can discuss that in the review.

[Kritner]

Hey i just wanted to check in to make sure there wasn't anything needed from me - i'm not totally clear on what "the review" mentioned is. Is this something that's done on a scheduled basis and internal, or something i'm somehow involved with, something else?

[tarekgh]

No action is required from you at this time. The issue has been marked as ready for design review, but it is not a priority compared to other work currently under review. Therefore, it may take some time before it is scheduled for review. Thanks for checking.

[terrajobst]

Video

This analyzer makes sense. Should probably be a different diagnostic ID but can likely be handled inside the existing analyzer for the argument validation.

We should use same category and same severity as for the other one that validates number of arguments.

[tarekgh]

@Kritner this is approved now. We can proceed with the implementation. Thanks for willing to help here.

[Kritner]

Sounds good! I guess i just need to dust off the ol PR and get it updated with latest from main? ... and install VS and all my tools again cuz i got a new computer since this convo started :D

[Kritner]

dotnet/roslyn-analyzers#7286 has been updated with a merge from main, marked the PR ready for review