From 1d77d983b193d24509fa018aefea20eb05105bbc Mon Sep 17 00:00:00 2001
From: Maryam Ariyan <maryam.ariyan@microsoft.com>
Date: Tue, 25 Jan 2022 19:26:13 -0800
Subject: [PATCH] Fixes bad log method generation in certain cases.

In certain cases when developer by mistake places ILogger, Exception, or LogLevel in the message template,  the code generator will produce the expected warning and makes sure the code will indeed compile and run correctly.

Prior to this fix, the code generator would fail to compile with when either of ILogger, Exception or LogLevel were placed in message template incorrectly.

Fixes #64310
---
 .../gen/LoggerMessageGenerator.Parser.cs      |  6 +++++-
 .../LoggerMessageGeneratedCodeTests.cs        | 21 +++++++++++++++++++
 .../LoggerMessageGeneratorEmitterTests.cs     |  2 +-
 .../LoggerMessageGeneratorParserTests.cs      | 12 +++++++++--
 ...ExceptionTestExtensions.WithDiagnostics.cs | 15 +++++++++++++
 .../LevelTestExtensions.WithDiagnostics.cs    | 18 ++++++++++++++++
 6 files changed, 70 insertions(+), 4 deletions(-)
 create mode 100644 src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ExceptionTestExtensions.WithDiagnostics.cs
 create mode 100644 src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/LevelTestExtensions.WithDiagnostics.cs

diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
index f33f506a213787..a645479b20b14a 100644
--- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
+++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
@@ -341,17 +341,21 @@ public IReadOnlyList<LoggerClass> GetLogClasses(IEnumerable<ClassDeclarationSynt
                                             foundException |= lp.IsException;
                                             foundLogLevel |= lp.IsLogLevel;
 
+                                            bool forceAsTemplateParams = false;
                                             if (lp.IsLogger && lm.TemplateMap.ContainsKey(paramName))
                                             {
                                                 Diag(DiagnosticDescriptors.ShouldntMentionLoggerInMessage, paramSymbol.Locations[0], paramName);
+                                                forceAsTemplateParams = true;
                                             }
                                             else if (lp.IsException && lm.TemplateMap.ContainsKey(paramName))
                                             {
                                                 Diag(DiagnosticDescriptors.ShouldntMentionExceptionInMessage, paramSymbol.Locations[0], paramName);
+                                                forceAsTemplateParams = true;
                                             }
                                             else if (lp.IsLogLevel && lm.TemplateMap.ContainsKey(paramName))
                                             {
                                                 Diag(DiagnosticDescriptors.ShouldntMentionLogLevelInMessage, paramSymbol.Locations[0], paramName);
+                                                forceAsTemplateParams = true;
                                             }
                                             else if (lp.IsLogLevel && level != null && !lm.TemplateMap.ContainsKey(paramName))
                                             {
@@ -370,7 +374,7 @@ public IReadOnlyList<LoggerClass> GetLogClasses(IEnumerable<ClassDeclarationSynt
                                             }
 
                                             lm.AllParameters.Add(lp);
-                                            if (lp.IsTemplateParameter)
+                                            if (lp.IsTemplateParameter || forceAsTemplateParams)
                                             {
                                                 lm.TemplateParameters.Add(lp);
                                             }
diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratedCodeTests.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratedCodeTests.cs
index d55bf4a8028d52..eced8b527b1200 100644
--- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratedCodeTests.cs
+++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratedCodeTests.cs
@@ -341,6 +341,20 @@ public void LevelTests()
             Assert.Equal(LogLevel.Warning, logger.LastLogLevel);
             Assert.Equal(1, logger.CallCount);
             Assert.Equal(11, logger.LastEventId.Id);
+
+            logger.Reset();
+            LevelTestExtensions.M12(logger, LogLevel.Trace);
+            Assert.Null(logger.LastException);
+            Assert.Equal("M12 Trace", logger.LastFormattedString);
+            Assert.Equal(LogLevel.Trace, logger.LastLogLevel);
+            Assert.Equal(1, logger.CallCount);
+
+            logger.Reset();
+            LevelTestExtensions.M13(logger, LogLevel.Trace);
+            Assert.Null(logger.LastException);
+            Assert.Equal("M13 Microsoft.Extensions.Logging.Generators.Tests.MockLogger", logger.LastFormattedString);
+            Assert.Equal(LogLevel.Trace, logger.LastLogLevel);
+            Assert.Equal(1, logger.CallCount);
         }
 
         [Fact]
@@ -361,6 +375,13 @@ public void ExceptionTests()
             Assert.Equal("M1 System.ArgumentException: Bar", logger.LastFormattedString);
             Assert.Equal(LogLevel.Debug, logger.LastLogLevel);
             Assert.Equal(1, logger.CallCount);
+
+            logger.Reset();
+            ExceptionTestExtensions.M2(logger, "One", new ArgumentException("Foo"));
+            Assert.Equal("Foo", logger.LastException!.Message);
+            Assert.Equal("M2 One: System.ArgumentException: Foo", logger.LastFormattedString);
+            Assert.Equal(LogLevel.Debug, logger.LastLogLevel);
+            Assert.Equal(1, logger.CallCount);
         }
 
         [Fact]
diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs
index 097dcc722d4007..335ca33c18cd3b 100644
--- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs
+++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs
@@ -27,7 +27,7 @@ public async Task TestEmitter()
                     new[] { typeof(ILogger).Assembly, typeof(LoggerMessageAttribute).Assembly },
                     new[] { testSourceCode }).ConfigureAwait(false);
 
-                Assert.Empty(d);
+                Assert.True(src.Contains("WithDiagnostics") ? !d.IsEmpty : d.IsEmpty);
                 Assert.Single(r);
             }
         }
diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs
index 09b93e633335c7..350561d56d3608 100644
--- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs
+++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs
@@ -204,11 +204,15 @@ partial class C
                 {
                     [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M1 {ex} {ex2}"")]
                     static partial void M1(ILogger logger, System.Exception ex, System.Exception ex2);
+
+                    [LoggerMessage(EventId = 2, Level = LogLevel.Debug, Message = ""M2 {arg1}: {ex}"")]
+                    static partial void M2(ILogger logger, string arg1, System.Exception ex);
                 }
             ");
 
-            Assert.Single(diagnostics);
+            Assert.Equal(2, diagnostics.Count);
             Assert.Equal(DiagnosticDescriptors.ShouldntMentionExceptionInMessage.Id, diagnostics[0].Id);
+            Assert.Equal(DiagnosticDescriptors.ShouldntMentionExceptionInMessage.Id, diagnostics[1].Id);
         }
 
         [Fact]
@@ -234,11 +238,15 @@ partial class C
                 {
                     [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M1 {logger}"")]
                     static partial void M1(ILogger logger);
+
+                    [LoggerMessage(EventId = 2, Message = ""M2 {logger}"")]
+                    static partial void M2(ILogger logger, LogLevel level);
                 }
             ");
 
-            Assert.Single(diagnostics);
+            Assert.Equal(2, diagnostics.Count);
             Assert.Equal(DiagnosticDescriptors.ShouldntMentionLoggerInMessage.Id, diagnostics[0].Id);
+            Assert.Equal(DiagnosticDescriptors.ShouldntMentionLoggerInMessage.Id, diagnostics[1].Id);
         }
 
         [Fact]
diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ExceptionTestExtensions.WithDiagnostics.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ExceptionTestExtensions.WithDiagnostics.cs
new file mode 100644
index 00000000000000..8a2299cf864cd4
--- /dev/null
+++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ExceptionTestExtensions.WithDiagnostics.cs
@@ -0,0 +1,15 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+
+namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
+{
+    internal static partial class ExceptionTestExtensions
+    {
+#pragma warning disable SYSLIB1013
+        [LoggerMessage(EventId = 2, Level = LogLevel.Debug, Message = "M2 {arg1}: {ex}")]
+        public static partial void M2(ILogger logger, string arg1, Exception ex);
+#pragma warning disable SYSLIB1013
+    }
+}
diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/LevelTestExtensions.WithDiagnostics.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/LevelTestExtensions.WithDiagnostics.cs
new file mode 100644
index 00000000000000..aabcb67ae4bd07
--- /dev/null
+++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/LevelTestExtensions.WithDiagnostics.cs
@@ -0,0 +1,18 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
+{
+    internal static partial class LevelTestExtensions
+    {
+#pragma warning disable SYSLIB1002
+        [LoggerMessage(EventId = 12, Message = "M12 {level}")]
+        public static partial void M12(ILogger logger, LogLevel level);
+#pragma warning restore SYSLIB1002
+
+#pragma warning disable SYSLIB1018
+        [LoggerMessage(EventId = 13, Message = "M13 {logger}")]
+        public static partial void M13(ILogger logger, LogLevel level);
+#pragma warning restore SYSLIB1018
+    }
+}