Bring parity to dotnet extensions logging generator behavior

eng/Versions.props

@@ -82,5 +82,19 @@
     Versions below this comment are not managed by automation and can be changed as needed.
   -->
   <PropertyGroup Label="Manual">
+    <!-- Compatibility with VS 17.4/.NET SDK 7.0.1xx -->
+    <MicrosoftCodeAnalysisVersion_4_4>4.4.0</MicrosoftCodeAnalysisVersion_4_4>
+    <!-- Compatibility with VS 17.8/.NET SDK 8.0.1xx -->
+    <MicrosoftCodeAnalysisVersion_4_8>4.8.0</MicrosoftCodeAnalysisVersion_4_8>
+    <!-- Compatibility with the latest Visual Studio Preview release -->
+    <!--
+      The exact version is always a moving target. This version should never go ahead of the version of Roslyn that is included in the most recent
+      public Visual Studio preview version. If it were to go ahead, then any components depending on this version would not work in Visual Studio
+      and would cause a major regression for any local development that depends on those components contributing to the build.
+      This version must also not go ahead of the most recently release .NET SDK version, as that would break the source-build build.
+      Source-build builds the product with the most recent previously source-built release. Thankfully, these two requirements line up nicely
+      such that any version that satisfies the VS version requirement will also satisfy the .NET SDK version requirement because of how we ship.
+    -->
+    <MicrosoftCodeAnalysisVersion_LatestVS>4.8.0</MicrosoftCodeAnalysisVersion_LatestVS>
   </PropertyGroup>
 </Project>

eng/packages/General.props

@@ -5,10 +5,10 @@
     <PackageVersion Include="Microsoft.Bcl.HashCode" Version="1.1.1" />
     <PackageVersion Include="Microsoft.Bcl.TimeProvider" Version="$(MicrosoftBclTimeProviderVersion)" />
     <PackageVersion Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.4" />
-    <PackageVersion Include="Microsoft.CodeAnalysis.Common" Version="4.4.0" />
-    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.4.0" />
-    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.4.0" />
-    <PackageVersion Include="Microsoft.CodeAnalysis" Version="4.4.0" />
+    <PackageVersion Include="Microsoft.CodeAnalysis.Common" Version="$(MicrosoftCodeAnalysisVersion_4_4)" />
+    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="$(MicrosoftCodeAnalysisVersion_4_4)" />
+    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="$(MicrosoftCodeAnalysisVersion_4_4)" />
+    <PackageVersion Include="Microsoft.CodeAnalysis" Version="$(MicrosoftCodeAnalysisVersion_4_4)" />
     <PackageVersion Include="Microsoft.Extensions.Caching.Abstractions" Version="$(MicrosoftExtensionsCachingAbstractionsVersion)" />
     <PackageVersion Include="Microsoft.Extensions.Configuration.Abstractions" Version="$(MicrosoftExtensionsConfigurationAbstractionsVersion)" />
     <PackageVersion Include="Microsoft.Extensions.Configuration.Binder" Version="$(MicrosoftExtensionsConfigurationBinderVersion)" />

src/Generators/Microsoft.Gen.Logging/Microsoft.Gen.Logging.csproj

@@ -7,6 +7,7 @@
 
   <PropertyGroup>
     <AnalyzerLanguage>cs</AnalyzerLanguage>
+    <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
     <InjectIsExternalInitOnLegacy>true</InjectIsExternalInitOnLegacy>
   </PropertyGroup>
 

src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs

@@ -245,4 +245,17 @@ internal sealed class DiagDescriptors : DiagDescriptorsBase
         messageFormat: Resources.DefaultToStringMessage,
         category: Category,
         DiagnosticSeverity.Warning);
+
+    public static DiagnosticDescriptor MalformedFormatStrings { get; } = Make(
+        id: DiagnosticIds.LoggerMessage.LOGGEN037,
+        title: Resources.MalformedFormatStringsTitle,
+        messageFormat: Resources.MalformedFormatStringsMessage,
+        category: Category);
+
+    public static DiagnosticDescriptor PrimaryConstructorParameterLoggerHidden { get; } = Make(
+        id: DiagnosticIds.LoggerMessage.LOGGEN038,
+        title: Resources.PrimaryConstructorParameterLoggerHiddenTitle,
+        messageFormat: Resources.PrimaryConstructorParameterLoggerHiddenMessage,
+        category: Category,
+        DiagnosticSeverity.Info);
 }

src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx

@@ -339,4 +339,16 @@
   <data name="DefaultToStringTitle" xml:space="preserve">
     <value>A value being logged doesn't have an effective way to be converted into a string</value>
   </data>
+  <data name="MalformedFormatStringsMessage" xml:space="preserve">
+    <value>Logging method '{0}' contains malformed format strings</value>
+  </data>
+  <data name="MalformedFormatStringsTitle" xml:space="preserve">
+    <value>Logging method contains malformed format strings</value>
+  </data>
+  <data name="PrimaryConstructorParameterLoggerHiddenTitle" xml:space="preserve">
+    <value>Primary constructor parameter of type Microsoft.Extensions.Logging.ILogger is hidden by a field</value>
+  </data>
+  <data name="PrimaryConstructorParameterLoggerHiddenMessage" xml:space="preserve">
+    <value>Class '{0}' has a primary constructor parameter of type Microsoft.Extensions.Logging.ILogger that is hidden by a field in the class or a base class, preventing its use</value>
+  </data>
 </root>

src/Generators/Microsoft.Gen.Logging/Parsing/TemplateProcessor.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Text;
 
 namespace Microsoft.Gen.Logging.Parsing;
@@ -14,32 +15,60 @@ internal static class TemplateProcessor
     /// <summary>
     /// Finds the template arguments contained in the message string.
     /// </summary>
-    internal static void ExtractTemplates(string? message, List<string> templates)
+    internal static bool ExtractTemplates(string? message, List<string> templates)
     {
         if (string.IsNullOrEmpty(message))
         {
-            return;
+            return true;
         }
 
         var scanIndex = 0;
         var endIndex = message!.Length;
+
+        bool success = true;
         while (scanIndex < endIndex)
         {
             var openBraceIndex = FindBraceIndex(message, '{', scanIndex, endIndex);
-            var closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex, endIndex);
 
-            if (closeBraceIndex == endIndex)
+#pragma warning disable S109 // Magic numbers should not be used
+            if (openBraceIndex == -2)
+            {
+                // found '}' instead of '{'
+                success = false;
+                break;
+            }
+            else if (openBraceIndex == -1)
+            {
+                // scanned the string and didn't find any remaining '{' or '}'
+                break;
+            }
+#pragma warning restore S109 // Magic numbers should not be used
+
+            int closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex + 1, endIndex);
+
+            if (closeBraceIndex <= -1)
             {
-                return;
+                success = false;
+                break;
             }
 
             // Format item syntax : { index[,alignment][ :formatString] }.
             var formatDelimiterIndex = FindIndexOfAny(message, _formatDelimiters, openBraceIndex, closeBraceIndex);
-
             var templateName = message.Substring(openBraceIndex + 1, formatDelimiterIndex - openBraceIndex - 1).Trim();
+
+            if (string.IsNullOrWhiteSpace(templateName))
+            {
+                // braces with no named argument, such as "{}" and "{ }"
+                success = false;
+                break;
+            }
+
             templates.Add(templateName);
+
             scanIndex = closeBraceIndex + 1;
         }
+
+        return success;
     }
 
     /// <summary>
@@ -59,9 +88,23 @@ internal static void ExtractTemplates(string? message, List<string> templates)
         while (scanIndex < endIndex)
         {
             var openBraceIndex = FindBraceIndex(message, '{', scanIndex, endIndex);
-            var closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex, endIndex);
 
-            if (closeBraceIndex == endIndex)
+#pragma warning disable S109 // Magic numbers should not be used
+            if (openBraceIndex == -2)
+            {
+                // found '}' instead of '{'
+                break;
+            }
+            else if (openBraceIndex == -1)
+            {
+                // scanned the string and didn't find any remaining '{' or '}'
+                break;
+            }
+#pragma warning restore S109 // Magic numbers should not be used
+
+            var closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex + 1, endIndex);
+
+            if (closeBraceIndex <= -1)
             {
                 break;
             }
@@ -86,56 +129,68 @@ internal static void ExtractTemplates(string? message, List<string> templates)
     internal static int FindIndexOfAny(string message, char[] chars, int startIndex, int endIndex)
     {
         var findIndex = message.IndexOfAny(chars, startIndex, endIndex - startIndex);
-        return findIndex == -1
-            ? endIndex
-            : findIndex;
+        return findIndex == -1 ? endIndex : findIndex;
     }
 
-    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}}.
-        var braceIndex = endIndex;
-        var scanIndex = startIndex;
-        var braceOccurrenceCount = 0;
+        Debug.Assert(searchedBrace is '{' or '}', "Searched brace must be { or }");
+
+        int braceIndex = -1;
+        int scanIndex = startIndex;
 
         while (scanIndex < endIndex)
         {
-            if (braceOccurrenceCount > 0 && message[scanIndex] != brace)
+            char current = message[scanIndex];
+
+            if (current is '{' or '}')
             {
-#pragma warning disable S109 // Magic numbers should not be used
-                if (braceOccurrenceCount % 2 == 0)
-#pragma warning restore S109 // Magic numbers should not be used
+                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;
+#pragma warning disable S109 // Magic numbers should not be used
+                // if it is an even number of braces, just skip them, otherwise, we found an unescaped brace
+                if (bracesCount % 2 != 0)
                 {
-                    // An unescaped '{' or '}' found.
+                    if (currentBrace == searchedBrace)
+                    {
+                        if (currentBrace == '{')
+                        {
+                            // For '{' pick the last occurrence.
+                            braceIndex = scanIndex - 1;
+                        }
+                        else
+                        {
+                            // For '}' pick the first occurrence.
+                            braceIndex = scanIndexBeforeSkip;
+                        }
+                    }
+                    else
+                    {
+                        // wrong brace found
+                        braceIndex = -2;
+                    }
+#pragma warning restore S109 // Magic numbers should not be used
+
                     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;

src/Generators/Microsoft.Gen.Metrics/Microsoft.Gen.Metrics.csproj

@@ -7,6 +7,7 @@
 
   <PropertyGroup>
     <AnalyzerLanguage>cs</AnalyzerLanguage>
+    <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
     <InjectIsExternalInitOnLegacy>true</InjectIsExternalInitOnLegacy>
   </PropertyGroup>
 

src/Shared/DiagnosticIds/DiagnosticIds.cs

@@ -86,6 +86,8 @@ internal static class LoggerMessage
         internal const string LOGGEN034 = nameof(LOGGEN034);
         internal const string LOGGEN035 = nameof(LOGGEN035);
         internal const string LOGGEN036 = nameof(LOGGEN036);
+        internal const string LOGGEN037 = nameof(LOGGEN037);
+        internal const string LOGGEN038 = nameof(LOGGEN038);
     }
 
     internal static class Metrics

test/Generators/Microsoft.Gen.Logging/Generated/LogMethodExtendedTests.cs

@@ -0,0 +1,46 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using Microsoft.Extensions.Logging.Testing;
+using TestClasses;
+using Xunit;
+
+using static TestClasses.LogPropertiesRedactionExtensions;
+
+namespace Microsoft.Gen.Logging.Test;
+
+public class LogMethodExtendedTests
+{
+    [Fact]
+    public void FindsLoggerInPrimaryConstructorParameter()
+    {
+        using var logger = Utils.GetLogger();
+
+        var collector = logger.FakeLogCollector;
+
+        new ClassWithPrimaryConstructor(logger).Test();
+        Assert.Equal("Test.", collector.LatestRecord.Message);
+    }
+
+    [Fact]
+    public void FindsLoggerInPrimaryConstructorParameterInDifferentPartialDeclaration()
+    {
+        using var logger = Utils.GetLogger();
+
+        var collector = logger.FakeLogCollector;
+
+        new ClassWithPrimaryConstructorInDifferentPartialDeclaration(logger).Test();
+        Assert.Equal("Test.", collector.LatestRecord.Message);
+    }
+
+    [Fact]
+    public void FindsLoggerInFieldInitializedFromPrimaryConstructorParameter()
+    {
+        using var logger = Utils.GetLogger();
+
+        var collector = logger.FakeLogCollector;
+
+        new ClassWithPrimaryConstructorAndField(logger).Test();
+        Assert.Equal("Test.", collector.LatestRecord.Message);
+    }
+}