From 8a27fb1c1234c34772b4537191ff989d2cd124ee Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Tue, 1 Feb 2022 16:58:32 -0800 Subject: [PATCH 1/3] Fixes two logging source gen bugs - when using "in" or "ref" modifier / when dealing with constraints (#64593) * Fixes some logging source gen bugs: - Supports usage of "in" modifier - Improves support for generic constraints Fixes #58550, #62644 * Apply PR feedback * Add another test --- .../gen/LoggerMessageGenerator.Emitter.cs | 8 +- .../gen/LoggerMessageGenerator.Parser.cs | 14 ++- .../TestWithNestedClass.generated.txt | 4 +- .../LoggerMessageGeneratedCodeTests.cs | 69 ++++++++++ .../LoggerMessageGeneratorParserTests.cs | 15 +++ .../TestClasses/ConstaintsTestExtensions.cs | 118 ++++++++++++++++++ .../TestClasses/NestedClassTestsExtensions.cs | 8 +- .../TestClasses/ParameterTestExtensions.cs | 19 +++ 8 files changed, 247 insertions(+), 8 deletions(-) create mode 100644 src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ConstaintsTestExtensions.cs create mode 100644 src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ParameterTestExtensions.cs diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs index 53dd062b5d4b4c..06f0ef986523eb 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs @@ -78,7 +78,7 @@ namespace {lc.Namespace} // loop until you find top level nested class while (parent != null) { - parentClasses.Add($"partial {parent.Keyword} {parent.Name} {parent.Constraints}"); + parentClasses.Add($"partial {parent.Keyword} {parent.Name} "); parent = parent.ParentClass; } @@ -92,7 +92,7 @@ namespace {lc.Namespace} } _builder.Append($@" - {nestedIndentation}partial {lc.Keyword} {lc.Name} {lc.Constraints} + {nestedIndentation}partial {lc.Keyword} {lc.Name} {nestedIndentation}{{"); foreach (LoggerMethod lm in lc.Methods) @@ -309,6 +309,10 @@ private void GenParameters(LoggerMethod lm) _builder.Append(", "); } + if (p.Qualifier != null) + { + _builder.Append($"{p.Qualifier} "); + } _builder.Append($"{p.Type} {p.Name}"); } } 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..d2206a84b40599 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs @@ -323,6 +323,15 @@ public IReadOnlyList GetLogClasses(IEnumerable GetLogClasses(IEnumerable Keyword = parentLoggerClass.Keyword.ValueText, Namespace = nspace, Name = parentLoggerClass.Identifier.ToString() + parentLoggerClass.TypeParameterList, - Constraints = parentLoggerClass.ConstraintClauses.ToString(), ParentClass = null, }; @@ -681,7 +689,6 @@ internal class LoggerClass public string Keyword = string.Empty; public string Namespace = string.Empty; public string Name = string.Empty; - public string Constraints = string.Empty; public LoggerClass? ParentClass; } @@ -712,6 +719,7 @@ internal class LoggerParameter { public string Name = string.Empty; public string Type = string.Empty; + public string? Qualifier; public bool IsLogger; public bool IsException; public bool IsLogLevel; diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithNestedClass.generated.txt b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithNestedClass.generated.txt index b7b93bf27bd02d..07cd2a3aff4211 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithNestedClass.generated.txt +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithNestedClass.generated.txt @@ -9,11 +9,11 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses.NestedNamesp { partial record NestedRecord { - partial class NestedClassTestsExtensions where T1 : Class1 + partial class NestedClassTestsExtensions { partial class NestedMiddleParentClass { - partial class Nested where T2 : Class2 + partial class Nested { [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "%VERSION%")] private static readonly global::System.Action __M9Callback = 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..3b0650309a0179 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 @@ -3,8 +3,12 @@ using System; using System.Collections.Generic; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Logging.Generators.Tests.TestClasses; +using Microsoft.Extensions.Logging.Generators.Tests.TestClasses.UsesConstraintInAnotherNamespace; using Xunit; +using NamespaceForABC; +using ConstraintInAnotherNamespace; namespace Microsoft.Extensions.Logging.Generators.Tests { @@ -410,6 +414,71 @@ public void SkipEnabledCheckTests() Assert.Equal(1, logger.CallCount); Assert.Equal("LoggerMethodWithTrueSkipEnabledCheck", logger.LastEventId.Name); } + private struct MyStruct { } + + [Fact] + public void ConstraintsTests() + { + var logger = new MockLogger(); + + var printer = new MessagePrinter(); + logger.Reset(); + printer.Print(logger, new Message() { Text = "Hello" }); + Assert.Equal(LogLevel.Information, logger.LastLogLevel); + Assert.Null(logger.LastException); + Assert.Equal("The message is Hello.", logger.LastFormattedString); + Assert.Equal(1, logger.CallCount); + + var printer2 = new MessagePrinterHasConstraintOnLogClassAndLogMethod(); + logger.Reset(); + printer2.Print(logger, new Message() { Text = "Hello" }); + Assert.Equal(LogLevel.Information, logger.LastLogLevel); + Assert.Null(logger.LastException); + Assert.Equal("The message is `Hello`.", logger.LastFormattedString); + Assert.Equal(1, logger.CallCount); + + logger.Reset(); + ConstraintsTestExtensions.M0(logger, 12); + Assert.Equal(LogLevel.Debug, logger.LastLogLevel); + Assert.Null(logger.LastException); + Assert.Equal("M012", logger.LastFormattedString); + Assert.Equal(1, logger.CallCount); + + logger.Reset(); + ConstraintsTestExtensions1.M0(logger, 12); + Assert.Equal(LogLevel.Debug, logger.LastLogLevel); + Assert.Null(logger.LastException); + Assert.Equal("M012", logger.LastFormattedString); + Assert.Equal(1, logger.CallCount); + + logger.Reset(); + ConstraintsTestExtensions2.M0(logger, 12); + Assert.Equal(LogLevel.Debug, logger.LastLogLevel); + Assert.Null(logger.LastException); + Assert.Equal("M012", logger.LastFormattedString); + Assert.Equal(1, logger.CallCount); + + logger.Reset(); + ConstraintsTestExtensions3.M0(logger, 12); + Assert.Equal(LogLevel.Debug, logger.LastLogLevel); + Assert.Null(logger.LastException); + Assert.Equal("M012", logger.LastFormattedString); + Assert.Equal(1, logger.CallCount); + + logger.Reset(); + ConstraintsTestExtensions4.M0(logger, 12); + Assert.Equal(LogLevel.Debug, logger.LastLogLevel); + Assert.Null(logger.LastException); + Assert.Equal("M012", logger.LastFormattedString); + Assert.Equal(1, logger.CallCount); + + logger.Reset(); + ConstraintsTestExtensions5.M0(logger, 12); + Assert.Equal(LogLevel.Debug, logger.LastLogLevel); + Assert.Null(logger.LastException); + Assert.Equal("M012", logger.LastFormattedString); + Assert.Equal(1, logger.CallCount); + } [Fact] public void NestedClassTests() 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..3cbcc2e539e02d 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 @@ -606,6 +606,21 @@ partial class C Assert.Equal(DiagnosticDescriptors.LoggingMethodIsGeneric.Id, diagnostics[0].Id); } + [Theory] + [InlineData("ref")] + [InlineData("in")] + public async Task SupportsRefKindsInAndRef(string modifier) + { + IReadOnlyList diagnostics = await RunGenerator(@$" + partial class C + {{ + [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""Parameter {{P1}}"")] + static partial void M(ILogger logger, {modifier} int p1); + }}"); + + Assert.Empty(diagnostics); + } + [Fact] public async Task Templates() { diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ConstaintsTestExtensions.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ConstaintsTestExtensions.cs new file mode 100644 index 00000000000000..7e6f87271b535f --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ConstaintsTestExtensions.cs @@ -0,0 +1,118 @@ +// 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 +{ + using ConstraintInAnotherNamespace; + namespace UsesConstraintInAnotherNamespace + { + public partial class MessagePrinter + where T : Message + { + public void Print(ILogger logger, T message) + { + Log.Message(logger, message.Text); + } + + internal static partial class Log + { + [LoggerMessage(EventId = 1, Level = LogLevel.Information, Message = "The message is {Text}.")] + internal static partial void Message(ILogger logger, string? text); + } + } + + public partial class MessagePrinterHasConstraintOnLogClassAndLogMethod + where T : Message + { + public void Print(ILogger logger, T message) + { + Log.Message(logger, message); + } + + internal static partial class Log where U : Message + { + [LoggerMessage(EventId = 1, Level = LogLevel.Information, Message = "The message is {Text}.")] + internal static partial void Message(ILogger logger, U text); + } + } + } + + internal static partial class ConstraintsTestExtensions + where T : class + { + [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = "M0{p0}")] + public static partial void M0(ILogger logger, int p0); + + public static void Foo(T dummy) + { + } + } + + internal static partial class ConstraintsTestExtensions1 + where T : struct + { + [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = "M0{p0}")] + public static partial void M0(ILogger logger, int p0); + + public static void Foo(T dummy) + { + } + } + + internal static partial class ConstraintsTestExtensions2 + where T : unmanaged + { + [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = "M0{p0}")] + public static partial void M0(ILogger logger, int p0); + + public static void Foo(T dummy) + { + } + } + + internal static partial class ConstraintsTestExtensions3 + where T : new() + { + [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = "M0{p0}")] + public static partial void M0(ILogger logger, int p0); + + public static void Foo(T dummy) + { + } + } + + internal static partial class ConstraintsTestExtensions4 + where T : System.Attribute + { + [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = "M0{p0}")] + public static partial void M0(ILogger logger, int p0); + + public static void Foo(T dummy) + { + } + } + + internal static partial class ConstraintsTestExtensions5 + where T : notnull + { + [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = "M0{p0}")] + public static partial void M0(ILogger logger, int p0); + + public static void Foo(T dummy) + { + } + } +} + +namespace ConstraintInAnotherNamespace +{ + public class Message + { + public string? Text { get; set; } + + public override string ToString() + { + return $"`{Text}`"; + } + } +} diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/NestedClassTestsExtensions.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/NestedClassTestsExtensions.cs index 4b5b6cdbf90a26..ba6b87f6d5308a 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/NestedClassTestsExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/NestedClassTestsExtensions.cs @@ -3,6 +3,8 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses { + using NamespaceForABC; + internal static partial class NestedClassTestsExtensions where T : ABC { internal static partial class NestedMiddleParentClass @@ -26,7 +28,6 @@ internal static partial class NestedClass } } } - public class ABC {} public partial struct NestedStruct { @@ -61,3 +62,8 @@ internal static partial class Logger } } } + +namespace NamespaceForABC +{ + public class ABC {} +} \ No newline at end of file diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ParameterTestExtensions.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ParameterTestExtensions.cs new file mode 100644 index 00000000000000..f51fc2e62f6a96 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/ParameterTestExtensions.cs @@ -0,0 +1,19 @@ +// 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 ParameterTestExtensions + { + internal struct S + { + public override string ToString() => "Hello from S"; + } + + [LoggerMessage(EventId = 0, Level = LogLevel.Information, Message = "UseInParameter {s}")] + internal static partial void UseInParameter(ILogger logger, in S s); + + [LoggerMessage(EventId = 1, Level = LogLevel.Information, Message = "UseRefParameter {s}")] + internal static partial void UseRefParameter(ILogger logger, ref S s); + } +} From cb38fc7df45476f8f36ff8714e7b7cab3c066616 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Thu, 3 Feb 2022 09:58:47 -0800 Subject: [PATCH 2/3] Logging Source Generator - Adds support to `@` signed prefixed parameters (#64663) * Adds support to `@` signed prefixed parameters Fixes #60968 * Move repetitive logic to a new property * Remove NeedsAtSign --- .../gen/LoggerMessageGenerator.Emitter.cs | 10 +++++----- .../gen/LoggerMessageGenerator.Parser.cs | 11 +++++++++++ .../TestClasses/AtSymbolTestExtensions.cs | 11 +++++++++++ 3 files changed, 27 insertions(+), 5 deletions(-) create mode 100644 src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/AtSymbolTestExtensions.cs diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs index 06f0ef986523eb..249672b37058b1 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs @@ -196,7 +196,7 @@ private void GenFieldAssignments(LoggerMethod lm, string nestedIndentation) { foreach (LoggerParameter p in lm.TemplateParameters) { - _builder.AppendLine($" {nestedIndentation}this._{p.Name} = {p.Name};"); + _builder.AppendLine($" {nestedIndentation}this._{p.Name} = {p.CodeName};"); } } @@ -255,7 +255,7 @@ private void GenCallbackArguments(LoggerMethod lm) { foreach (LoggerParameter p in lm.TemplateParameters) { - _builder.Append($"{p.Name}, "); + _builder.Append($"{p.CodeName}, "); } } @@ -313,7 +313,7 @@ private void GenParameters(LoggerMethod lm) { _builder.Append($"{p.Qualifier} "); } - _builder.Append($"{p.Type} {p.Name}"); + _builder.Append($"{p.Type} {p.CodeName}"); } } @@ -331,7 +331,7 @@ private void GenArguments(LoggerMethod lm) _builder.Append(", "); } - _builder.Append($"{p.Type} {p.Name}"); + _builder.Append($"{p.Type} {p.CodeName}"); } } @@ -347,7 +347,7 @@ private void GenHolder(LoggerMethod lm) _builder.Append(", "); } - _builder.Append(p.Name); + _builder.Append(p.CodeName); } _builder.Append(')'); 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 d2206a84b40599..592ee84655139e 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs @@ -308,6 +308,15 @@ public IReadOnlyList GetLogClasses(IEnumerable 0) + { + ParameterSyntax paramSyntax = paramSymbol.DeclaringSyntaxReferences[0].GetSyntax(_cancellationToken) as ParameterSyntax; + if (paramSyntax != null && !string.IsNullOrEmpty(paramSyntax.Identifier.Text)) + { + needsAtSign = paramSyntax.Identifier.Text[0] == '@'; + } + } if (string.IsNullOrWhiteSpace(paramName)) { // semantic problem, just bail quietly @@ -341,6 +350,7 @@ public IReadOnlyList GetLogClasses(IEnumerable Date: Fri, 4 Feb 2022 12:08:36 -0800 Subject: [PATCH 3/3] Packaging Microsoft.Extensions.Logging.Abstractions in servicing branch --- .../src/Microsoft.Extensions.Logging.Abstractions.csproj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj b/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj index c4c29046201f0a..77092726df74cb 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj @@ -17,6 +17,8 @@ Microsoft.Extensions.Logging.LogLevel Microsoft.Extensions.Logging.Logger<T> Microsoft.Extensions.Logging.LoggerMessage Microsoft.Extensions.Logging.Abstractions.NullLogger + true + 1