diagnostic to detect malformed format strings in lsg template message (…

…#81503) fix #52226
dotnet · Feb 5, 2023 · 8521541 · 8521541
1 parent b708292
commit 8521541
Show file tree

Hide file tree

Showing 18 changed files with 337 additions and 209 deletions.
diff --git a/docs/project/list-of-diagnostics.md b/docs/project/list-of-diagnostics.md
@@ -133,7 +133,7 @@ The diagnostic id values reserved for .NET Libraries analyzer warnings are `SYSL
 |  __`SYSLIB1019`__ | Couldn't find a field of type Microsoft.Extensions.Logging.ILogger |
 |  __`SYSLIB1020`__ | Found multiple fields of type Microsoft.Extensions.Logging.ILogger |
 |  __`SYSLIB1021`__ | Can't have the same template with different casing |
-|  __`SYSLIB1022`__ | Can't have malformed format strings (like dangling {, etc)  |
+|  __`SYSLIB1022`__ | Logging method contains malformed format strings |
 |  __`SYSLIB1023`__ | Generating more than 6 arguments is not supported |
 |  __`SYSLIB1024`__ | Argument is using the unsupported out parameter modifier |
 |  __`SYSLIB1025`__ | Multiple logging methods cannot use the same event name within a class |

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/DiagnosticDescriptors.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/DiagnosticDescriptors.cs
@@ -170,7 +170,7 @@ public static class DiagnosticDescriptors
 
         public static DiagnosticDescriptor MalformedFormatStrings { get; } = new DiagnosticDescriptor(
             id: "SYSLIB1022",
-            title: new LocalizableResourceString(nameof(SR.MalformedFormatStringsMessage), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
+            title: new LocalizableResourceString(nameof(SR.MalformedFormatStringsTitle), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
             messageFormat: new LocalizableResourceString(nameof(SR.MalformedFormatStringsMessage), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
             category: "LoggingGenerator",
             DiagnosticSeverity.Error,

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
@@ -204,9 +204,15 @@ public IReadOnlyList<LoggerClass> GetLogClasses(IEnumerable<ClassDeclarationSynt
                                         SkipEnabledCheck = skipEnabledCheck
                                     };
 
-                                    ExtractTemplates(message, lm.TemplateMap, lm.TemplateList);
-
                                     bool keepMethod = true;   // whether or not we want to keep the method definition or if it's got errors making it so we should discard it instead
+
+                                    bool success = ExtractTemplates(message, lm.TemplateMap, lm.TemplateList);
+                                    if (!success)
+                                    {
+                                        Diag(DiagnosticDescriptors.MalformedFormatStrings, method.Identifier.GetLocation(), method.Identifier.ToString());
+                                        keepMethod = false;
+                                    }
+
                                     if (lm.Name[0] == '_')
                                     {
                                         // can't have logging method names that start with _ since that can lead to conflicting symbol names
@@ -595,81 +601,112 @@ private bool IsBaseOrIdentity(ITypeSymbol source, ITypeSymbol dest)
             /// <summary>
             /// Finds the template arguments contained in the message string.
             /// </summary>
-            private static void ExtractTemplates(string? message, IDictionary<string, string> templateMap, List<string> templateList)
+            /// <returns>A value indicating whether the extraction was successful.</returns>
+            private static bool ExtractTemplates(string? message, IDictionary<string, string> templateMap, List<string> templateList)
             {
                 if (string.IsNullOrEmpty(message))
                 {
-                    return;
+                    return true;
                 }
 
                 int scanIndex = 0;
-                int endIndex = message!.Length;
+                int endIndex = message.Length;
 
+                bool success = true;
                 while (scanIndex < endIndex)
                 {
                     int openBraceIndex = FindBraceIndex(message, '{', scanIndex, endIndex);
-                    int closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex, endIndex);
 
-                    if (closeBraceIndex == endIndex)
+                    if (openBraceIndex == -2) // found '}' instead of '{'
                     {
-                        scanIndex = endIndex;
+                        success = false;
+                        break;
                     }
-                    else
+                    else if (openBraceIndex == -1) // scanned the string and didn't find any remaining '{' or '}'
+                    {
+                        break;
+                    }
+
+                    int closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex + 1, endIndex);
+
+                    if (closeBraceIndex <= -1) // unclosed '{'
                     {
-                        // Format item syntax : { index[,alignment][ :formatString] }.
-                        int formatDelimiterIndex = FindIndexOfAny(message, _formatDelimiters, openBraceIndex, closeBraceIndex);
+                        success = false;
+                        break;
+                    }
+
+                    // Format item syntax : { index[,alignment][ :formatString] }.
+                    int formatDelimiterIndex = FindIndexOfAny(message, _formatDelimiters, openBraceIndex, closeBraceIndex);
+                    string templateName = message.Substring(openBraceIndex + 1, formatDelimiterIndex - openBraceIndex - 1);
 
-                        string templateName = message.Substring(openBraceIndex + 1, formatDelimiterIndex - openBraceIndex - 1);
-                        templateMap[templateName] = templateName;
-                        templateList.Add(templateName);
-                        scanIndex = closeBraceIndex + 1;
+                    if (string.IsNullOrWhiteSpace(templateName)) // braces with no named argument, such as {} and { }
+                    {
+                        success = false;
+                        break;
                     }
+
+                    templateMap[templateName] = templateName;
+                    templateList.Add(templateName);
+
+                    scanIndex = closeBraceIndex + 1;
                 }
+
+                return success;
             }
 
-            private static int FindBraceIndex(string message, char brace, int startIndex, int endIndex)
+            /// <summary>
+            /// Searches for the next brace index in the message.
+            /// </summary>
+            /// <remarks> The search skips any sequences of {{ or }}.</remarks>
+            /// <example>{{prefix{{{Argument}}}suffix}}</example>
+            /// <returns>The zero-based index position of the first occurrence of the searched brace; -1 if the searched brace was not found; -2 if the wrong brace was found.</returns>
+            private static int FindBraceIndex(string message, char searchedBrace, int startIndex, int endIndex)
             {
-                // Example: {{prefix{{{Argument}}}suffix}}.
-                int braceIndex = endIndex;
+                Debug.Assert(searchedBrace is '{' or '}');
+
+                int braceIndex = -1;
                 int scanIndex = startIndex;
-                int braceOccurrenceCount = 0;
 
                 while (scanIndex < endIndex)
                 {
-                    if (braceOccurrenceCount > 0 && message[scanIndex] != brace)
+                    char current = message[scanIndex];
+
+                    if (current is '{' or '}')
                     {
-                        if (braceOccurrenceCount % 2 == 0)
+                        char currentBrace = current;
+
+                        int scanIndexBeforeSkip = scanIndex;
+                        while (current == currentBrace && ++scanIndex < endIndex)
                         {
-                            // Even number of '{' or '}' found. Proceed search with next occurrence of '{' or '}'.
-                            braceOccurrenceCount = 0;
-                            braceIndex = endIndex;
+                            current = message[scanIndex];
                         }
-                        else
+
+                        int bracesCount = scanIndex - scanIndexBeforeSkip;
+                        if (bracesCount % 2 != 0) // if it is an even number of braces, just skip them, otherwise, we found an unescaped brace
                         {
-                            // An unescaped '{' or '}' found.
+                            if (currentBrace == searchedBrace)
+                            {
+                                if (currentBrace == '{')
+                                {
+                                    braceIndex = scanIndex - 1; // For '{' pick the last occurrence.
+                                }
+                                else
+                                {
+                                    braceIndex = scanIndexBeforeSkip; // For '}' pick the first occurrence.
+                                }
+                            }
+                            else
+                            {
+                                braceIndex = -2; // wrong brace found
+                            }
+
                             break;
                         }
                     }
-                    else if (message[scanIndex] == brace)
+                    else
                     {
-                        if (brace == '}')
-                        {
-                            if (braceOccurrenceCount == 0)
-                            {
-                                // For '}' pick the first occurrence.
-                                braceIndex = scanIndex;
-                            }
-                        }
-                        else
-                        {
-                            // For '{' pick the last occurrence.
-                            braceIndex = scanIndex;
-                        }
-
-                        braceOccurrenceCount++;
+                        scanIndex++;
                     }
-
-                    scanIndex++;
                 }
 
                 return braceIndex;

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/Strings.resx b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/Strings.resx
@@ -211,7 +211,7 @@
     <value>Can't have the same template with different casing</value>
   </data>
   <data name="MalformedFormatStringsMessage" xml:space="preserve">
-    <value>Can't have malformed format strings (like dangling {, etc)</value>
+    <value>Logging method '{0}' contains malformed format strings</value>
   </data>
   <data name="GeneratingForMax6ArgumentsMessage" xml:space="preserve">
     <value>Generating more than 6 arguments is not supported</value>
@@ -228,4 +228,7 @@
   <data name="ShouldntReuseEventNamesTitle" xml:space="preserve">
     <value>Multiple logging methods should not use the same event name within a class</value>
   </data>
-</root>
+  <data name="MalformedFormatStringsTitle" xml:space="preserve">
+    <value>Logging method contains malformed format strings</value>
+  </data>
+</root>
diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.cs.xlf b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.cs.xlf
@@ -68,8 +68,13 @@
         <note />
       </trans-unit>
       <trans-unit id="MalformedFormatStringsMessage">
-        <source>Can't have malformed format strings (like dangling {, etc)</source>
-        <target state="translated">Nemůže mít chybně formátované řetězce (třeba neuzavřené závorky { atd.).</target>
+        <source>Logging method '{0}' contains malformed format strings</source>
+        <target state="new">Logging method '{0}' contains malformed format strings</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="MalformedFormatStringsTitle">
+        <source>Logging method contains malformed format strings</source>
+        <target state="new">Logging method contains malformed format strings</target>
         <note />
       </trans-unit>
       <trans-unit id="MissingLogLevelMessage">

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.de.xlf b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.de.xlf
@@ -68,8 +68,13 @@
         <note />
       </trans-unit>
       <trans-unit id="MalformedFormatStringsMessage">
-        <source>Can't have malformed format strings (like dangling {, etc)</source>
-        <target state="translated">Nicht wohlgeformte Formatzeichenfolgen (beispielsweise mit überzähligen geschweiften Klammern) sind unzulässig.</target>
+        <source>Logging method '{0}' contains malformed format strings</source>
+        <target state="new">Logging method '{0}' contains malformed format strings</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="MalformedFormatStringsTitle">
+        <source>Logging method contains malformed format strings</source>
+        <target state="new">Logging method contains malformed format strings</target>
         <note />
       </trans-unit>
       <trans-unit id="MissingLogLevelMessage">

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.es.xlf b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.es.xlf
@@ -68,8 +68,13 @@
         <note />
       </trans-unit>
       <trans-unit id="MalformedFormatStringsMessage">
-        <source>Can't have malformed format strings (like dangling {, etc)</source>
-        <target state="translated">No puede tener cadenas con formato incorrecto (como { final, etc.)</target>
+        <source>Logging method '{0}' contains malformed format strings</source>
+        <target state="new">Logging method '{0}' contains malformed format strings</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="MalformedFormatStringsTitle">
+        <source>Logging method contains malformed format strings</source>
+        <target state="new">Logging method contains malformed format strings</target>
         <note />
       </trans-unit>
       <trans-unit id="MissingLogLevelMessage">

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.fr.xlf b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.fr.xlf
@@ -68,8 +68,13 @@
         <note />
       </trans-unit>
       <trans-unit id="MalformedFormatStringsMessage">
-        <source>Can't have malformed format strings (like dangling {, etc)</source>
-        <target state="translated">Chaînes de format incorrect (par exemple { non fermée, etc.)</target>
+        <source>Logging method '{0}' contains malformed format strings</source>
+        <target state="new">Logging method '{0}' contains malformed format strings</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="MalformedFormatStringsTitle">
+        <source>Logging method contains malformed format strings</source>
+        <target state="new">Logging method contains malformed format strings</target>
         <note />
       </trans-unit>
       <trans-unit id="MissingLogLevelMessage">

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.it.xlf b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.it.xlf
@@ -68,8 +68,13 @@
         <note />
       </trans-unit>
       <trans-unit id="MalformedFormatStringsMessage">
-        <source>Can't have malformed format strings (like dangling {, etc)</source>
-        <target state="translated">Impossibile avere stringhe di formato non valido (ad esempio, { tralasciato e così via)</target>
+        <source>Logging method '{0}' contains malformed format strings</source>
+        <target state="new">Logging method '{0}' contains malformed format strings</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="MalformedFormatStringsTitle">
+        <source>Logging method contains malformed format strings</source>
+        <target state="new">Logging method contains malformed format strings</target>
         <note />
       </trans-unit>
       <trans-unit id="MissingLogLevelMessage">

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.ja.xlf b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.ja.xlf
@@ -68,8 +68,13 @@
         <note />
       </trans-unit>
       <trans-unit id="MalformedFormatStringsMessage">
-        <source>Can't have malformed format strings (like dangling {, etc)</source>
-        <target state="translated">(dangling {など) 誤った形式の文字列を持つことはできません</target>
+        <source>Logging method '{0}' contains malformed format strings</source>
+        <target state="new">Logging method '{0}' contains malformed format strings</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="MalformedFormatStringsTitle">
+        <source>Logging method contains malformed format strings</source>
+        <target state="new">Logging method contains malformed format strings</target>
         <note />
       </trans-unit>
       <trans-unit id="MissingLogLevelMessage">

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.ko.xlf b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.ko.xlf
@@ -68,8 +68,13 @@
         <note />
       </trans-unit>
       <trans-unit id="MalformedFormatStringsMessage">
-        <source>Can't have malformed format strings (like dangling {, etc)</source>
-        <target state="translated">잘못된 형식의 문자열(예: 짝이 맞지 않는 중괄호({))은 사용할 수 없음</target>
+        <source>Logging method '{0}' contains malformed format strings</source>
+        <target state="new">Logging method '{0}' contains malformed format strings</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="MalformedFormatStringsTitle">
+        <source>Logging method contains malformed format strings</source>
+        <target state="new">Logging method contains malformed format strings</target>
         <note />
       </trans-unit>
       <trans-unit id="MissingLogLevelMessage">

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.pl.xlf b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Resources/xlf/Strings.pl.xlf
@@ -68,8 +68,13 @@
         <note />
       </trans-unit>
       <trans-unit id="MalformedFormatStringsMessage">
-        <source>Can't have malformed format strings (like dangling {, etc)</source>
-        <target state="translated">Nie może zawierać źle sformułowanego formatu ciągów (takich jak zawieszonego znaku „{”, itp.)</target>
+        <source>Logging method '{0}' contains malformed format strings</source>
+        <target state="new">Logging method '{0}' contains malformed format strings</target>
+        <note />
+      </trans-unit>
+      <trans-unit id="MalformedFormatStringsTitle">
+        <source>Logging method contains malformed format strings</source>
+        <target state="new">Logging method contains malformed format strings</target>
         <note />
       </trans-unit>
       <trans-unit id="MissingLogLevelMessage">