-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Analyzer] Flagging issues in logger message templates w/ incomplete braces pairs #101698
Comments
Tagging subscribers to this area: @dotnet/area-extensions-logging |
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 |
I changed it to warning and we can discuss that in the review. |
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? |
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. |
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. |
@Kritner this is approved now. We can proceed with the implementation. Thanks for willing to help here. |
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 |
dotnet/roslyn-analyzers#7286 has been updated with a merge from main, marked the PR ready for review |
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" ofCA2017
. 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 templateparams
lead to the same thing or notSuggested 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
Yeah so it's weird... we're not
NoWarn
ing 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...to
More relevant comments:
dotnet/roslyn-analyzers#7285 (comment)
dotnet/roslyn-analyzers#7285 (comment)
dotnet/roslyn-analyzers#7286 (comment)
The text was updated successfully, but these errors were encountered: