Add letter casing mismatch validation and start triggering SYSLIB1021#124521
Add letter casing mismatch validation and start triggering SYSLIB1021#124521mrek-msft wants to merge 1 commit intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements validation for letter casing mismatches in LoggerMessage template parameters, addressing issue #52228. The change adds detection for inconsistent casing of the same template parameter (e.g., {par1} and {PAr1}) and triggers the previously defined but unused SYSLIB1021 diagnostic error.
Changes:
- Added
ValidateTemplatesCasingConsistencymethod to detect case-insensitive template duplicates with different casing - Added diagnostic emission when mixed casing is detected in template parameters
- Added test case
MixedCasingthat verifies the diagnostic is triggered for the scenario described in the issue
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| LoggerMessageGenerator.Parser.cs | Implements ValidateTemplatesCasingConsistency validation method and integrates it into the parsing flow to trigger SYSLIB1021 diagnostic |
| LoggerMessageGeneratorParserTests.cs | Adds test case verifying that mixed-casing template parameters (e.g., {par1} and {PAr1}) correctly trigger the SYSLIB1021 diagnostic |
| Assert.Equal(DiagnosticDescriptors.InconsistentTemplateCasing.Id, diagnostics[0].Id); | ||
| } | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
Consider adding a test case for templates with the exact same casing appearing multiple times (e.g., Message = "{par1} {par1}"). This would help document whether such usage is allowed or should be flagged as an error. The current validation only checks for case mismatches, but doesn't validate whether having the same parameter multiple times is intentional.
| [Fact] | |
| [Fact] | |
| public async Task DuplicateTemplateNameSameCasing_IsAllowed() | |
| { | |
| IReadOnlyList<Diagnostic> diagnostics = await RunGenerator(@" | |
| partial class C | |
| { | |
| [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M1 {par1} {par1} {a}"")] | |
| static partial void M1(ILogger logger, int par1, int a); | |
| } | |
| "); | |
| Assert.Empty(diagnostics); | |
| } | |
| [Fact] |
| } | ||
|
|
||
| /// <summary> | ||
| /// Validates that templates list does not contains templates with mixed casing like {hello} and {Hello} |
There was a problem hiding this comment.
Grammar error in the comment: "does not contains" should be "does not contain".
| /// Validates that templates list does not contains templates with mixed casing like {hello} and {Hello} | |
| /// Validates that templates list does not contain templates with mixed casing like {hello} and {Hello} |
| { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
There is an unnecessary blank line before the closing brace. This violates the consistent style seen elsewhere in the codebase where closing braces immediately follow the last statement.
|
I thought about case which I think we broke as part of #124257, I think SYSLIB1021 would handle it, but if we are OK with behavior described in my last comment and mark this regression as part of breaking change or if we mitigate it differently, I agree it is not very useful. |
|
We allow case mismatch between the message parameters and the logging method parameters. I would say, let's keep the current behavior allowing different casing for the message parameters and document that in the breaking change. Usually if this is unintentional, users will get another diagnostic for parameter's count mismatch. |
Fix #52228
Adds test case listed in issue and add code which validates template arguments and possibly triggers SYSLIB1021 diagnostics which is already defined, and documented, but as far as I know there were no place triggering it before which was briefly mentioned in issue: