From 35d0022c00f3a086b26039d916c6ab8c1192fcea Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Tue, 27 Apr 2021 16:21:59 -0700 Subject: [PATCH 1/2] - replace nameof(Generator) with "LoggerMessage" - bugfix: up to 6 params, use LoggerMessage.Define - bugfix: "__Enumerate" method was missing for IEnumerable arguments - change "__Enumerate" method: made part of a utility method, generated once in __LoggerMessageGenerator --- .../gen/LoggerMessageGenerator.Emitter.cs | 93 +++++++++---------- .../gen/LoggerMessageGenerator.cs | 2 +- .../TestWithMoreThan6Params.generated.txt | 53 ++++++++++- ...ed.txt => TestWithTwoParams.generated.txt} | 10 +- .../LoggerMessageGeneratorEmitterTests.cs | 12 +-- 5 files changed, 105 insertions(+), 65 deletions(-) rename src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/{TestWithOneParam.generated.txt => TestWithTwoParams.generated.txt} (57%) diff --git a/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs b/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs index 91065fa530fc09..a57ac243c4daa7 100644 --- a/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs +++ b/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs @@ -16,11 +16,12 @@ internal class Emitter private const int MaxLoggerMessageDefineArguments = 6; private const int DefaultStringBuilderCapacity = 1024; - private readonly string _generatedCodeAttribute = + private static readonly string _generatedCodeAttribute = $"global::System.CodeDom.Compiler.GeneratedCodeAttribute(" + $"\"{typeof(Emitter).Assembly.GetName().Name}\", " + $"\"{typeof(Emitter).Assembly.GetName().Version}\")"; private readonly StringBuilder _builder = new StringBuilder(DefaultStringBuilderCapacity); + private bool _needEnumerationHelper; public string Emit(IReadOnlyList logClasses, CancellationToken cancellationToken) { @@ -34,6 +35,7 @@ public string Emit(IReadOnlyList logClasses, CancellationToken canc GenType(lc); } + GenEnumerationHelper(); return _builder.ToString(); } @@ -50,7 +52,7 @@ private static bool UseLoggerMessageDefine(LoggerMethod lm) int count = 0; foreach (string t in lm.TemplateList) { - if (!t.Equals(lm.TemplateParameters[count].Name, StringComparison.OrdinalIgnoreCase)) + if (!t.Equals(lm.TemplateParameters[count++].Name, StringComparison.OrdinalIgnoreCase)) { // order doesn't match, can't use LoggerMessage.Define return false; @@ -84,8 +86,6 @@ partial class {lc.Name} {lc.Constraints} GenLogMethod(lm); } - GenEnumerationHelper(lc); - _builder.Append($@" }}"); @@ -193,7 +193,9 @@ private void GenVariableAssignments(LoggerMethod lm) if (lm.TemplateParameters[index].IsEnumerable) { _builder.AppendLine($" var {t.Key} = " - + $"__Enumerate((global::System.Collections.IEnumerable ?)this._{lm.TemplateParameters[index].Name});"); + + $"global::__LoggerMessageGenerator.Enumerate((global::System.Collections.IEnumerable ?)this._{lm.TemplateParameters[index].Name});"); + + _needEnumerationHelper = true; } else { @@ -237,7 +239,7 @@ private void GenDefineTypes(LoggerMethod lm, bool brackets) } if (brackets) { - _builder.Append("<"); + _builder.Append('<'); } bool firstItem = true; @@ -257,7 +259,7 @@ private void GenDefineTypes(LoggerMethod lm, bool brackets) if (brackets) { - _builder.Append(">"); + _builder.Append('>'); } else { @@ -322,7 +324,7 @@ private void GenHolder(LoggerMethod lm) private void GenLogMethod(LoggerMethod lm) { string level = GetLogLevel(lm); - string extension = (lm.IsExtensionMethod ? "this " : string.Empty); + string extension = lm.IsExtensionMethod ? "this " : string.Empty; string eventName = string.IsNullOrWhiteSpace(lm.EventName) ? $"nameof({lm.Name})" : $"\"{lm.EventName}\""; string exceptionArg = GetException(lm); string logger = GetLogger(lm); @@ -443,63 +445,56 @@ static string GetLogLevel(LoggerMethod lm) } } - private void GenEnumerationHelper(LoggerClass lc) + private void GenEnumerationHelper() { - foreach (LoggerMethod lm in lc.Methods) + if (_needEnumerationHelper) { - if (UseLoggerMessageDefine(lm)) - { - foreach (LoggerParameter p in lm.TemplateParameters) - { - if (p.IsEnumerable) - { _builder.Append($@" - [{_generatedCodeAttribute}] - private static string __Enumerate(global::System.Collections.IEnumerable? enumerable) +[{_generatedCodeAttribute}] +internal static class __LoggerMessageGenerator +{{ + public static string Enumerate(global::System.Collections.IEnumerable? enumerable) + {{ + if (enumerable == null) {{ - if (enumerable == null) - {{ - return ""(null)""; - }} + return ""(null)""; + }} - var sb = new global::System.Text.StringBuilder(); - _ = sb.Append('['); + var sb = new global::System.Text.StringBuilder(); + _ = sb.Append('['); - bool first = true; - foreach (object e in enumerable) + bool first = true; + foreach (object e in enumerable) + {{ + if (!first) {{ - if (!first) - {{ - _ = sb.Append("", ""); - }} + _ = sb.Append("", ""); + }} - if (e == null) + if (e == null) + {{ + _ = sb.Append(""(null)""); + }} + else + {{ + if (e is global::System.IFormattable fmt) {{ - _ = sb.Append(""(null)""); + _ = sb.Append(fmt.ToString(null, global::System.Globalization.CultureInfo.InvariantCulture)); }} else {{ - if (e is global::System.IFormattable fmt) - {{ - _ = sb.Append(fmt.ToString(null, global::System.Globalization.CultureInfo.InvariantCulture)); - }} - else - {{ - _ = sb.Append(e); - }} + _ = sb.Append(e); }} - - first = false; }} - _ = sb.Append(']'); - - return sb.ToString(); + first = false; }} -"); - } - } - } + + _ = sb.Append(']'); + + return sb.ToString(); + }} +}}"); } } } diff --git a/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.cs b/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.cs index 9c1d2cbcfeb91b..73ca6a738aa860 100644 --- a/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.cs +++ b/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.cs @@ -39,7 +39,7 @@ public void Execute(GeneratorExecutionContext context) var e = new Emitter(); string result = e.Emit(logClasses, context.CancellationToken); - context.AddSource(nameof(LoggerMessageGenerator), SourceText.From(result, Encoding.UTF8)); + context.AddSource("LoggerMessage", SourceText.From(result, Encoding.UTF8)); } } diff --git a/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithMoreThan6Params.generated.txt b/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithMoreThan6Params.generated.txt index 32b7629942140a..90455e122c1d42 100644 --- a/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithMoreThan6Params.generated.txt +++ b/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithMoreThan6Params.generated.txt @@ -14,9 +14,9 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses private readonly global::System.Int32 _p4; private readonly global::System.Int32 _p5; private readonly global::System.Int32 _p6; - private readonly global::System.Int32 _p7; + private readonly global::System.Collections.Generic.IEnumerable _p7; - public __Method9Struct(global::System.Int32 p1, global::System.Int32 p2, global::System.Int32 p3, global::System.Int32 p4, global::System.Int32 p5, global::System.Int32 p6, global::System.Int32 p7) + public __Method9Struct(global::System.Int32 p1, global::System.Int32 p2, global::System.Int32 p3, global::System.Int32 p4, global::System.Int32 p5, global::System.Int32 p6, global::System.Collections.Generic.IEnumerable p7) { this._p1 = p1; this._p2 = p2; @@ -36,7 +36,7 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses var p4 = this._p4; var p5 = this._p5; var p6 = this._p6; - var p7 = this._p7; + var p7 = global::__LoggerMessageGenerator.Enumerate((global::System.Collections.IEnumerable ?)this._p7); return $"M9 {p1} {p2} {p3} {p4} {p5} {p6} {p7}"; } @@ -74,7 +74,7 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses } [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "6.0.0.0")] - public static partial void Method9(global::Microsoft.Extensions.Logging.ILogger logger, global::System.Int32 p1, global::System.Int32 p2, global::System.Int32 p3, global::System.Int32 p4, global::System.Int32 p5, global::System.Int32 p6, global::System.Int32 p7) + public static partial void Method9(global::Microsoft.Extensions.Logging.ILogger logger, global::System.Int32 p1, global::System.Int32 p2, global::System.Int32 p3, global::System.Int32 p4, global::System.Int32 p5, global::System.Int32 p6, global::System.Collections.Generic.IEnumerable p7) { if (logger.IsEnabled(global::Microsoft.Extensions.Logging.LogLevel.Error)) { @@ -87,4 +87,49 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses } } } +} +[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "6.0.0.0")] +internal static class __LoggerMessageGenerator +{ + public static string Enumerate(global::System.Collections.IEnumerable? enumerable) + { + if (enumerable == null) + { + return "(null)"; + } + + var sb = new global::System.Text.StringBuilder(); + _ = sb.Append('['); + + bool first = true; + foreach (object e in enumerable) + { + if (!first) + { + _ = sb.Append(", "); + } + + if (e == null) + { + _ = sb.Append("(null)"); + } + else + { + if (e is global::System.IFormattable fmt) + { + _ = sb.Append(fmt.ToString(null, global::System.Globalization.CultureInfo.InvariantCulture)); + } + else + { + _ = sb.Append(e); + } + } + + first = false; + } + + _ = sb.Append(']'); + + return sb.ToString(); + } } \ No newline at end of file diff --git a/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithOneParam.generated.txt b/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithTwoParams.generated.txt similarity index 57% rename from src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithOneParam.generated.txt rename to src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithTwoParams.generated.txt index 866b0469124d8c..ae5b89b74a4d47 100644 --- a/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithOneParam.generated.txt +++ b/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithTwoParams.generated.txt @@ -3,18 +3,18 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses { - partial class TestWithOneParam + partial class TestWithTwoParams { [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "6.0.0.0")] - private static readonly global::System.Action __M0Callback = - global::Microsoft.Extensions.Logging.LoggerMessage.Define(global::Microsoft.Extensions.Logging.LogLevel.Error, new global::Microsoft.Extensions.Logging.EventId(0, nameof(M0)), "M0 {A1}", true); + private static readonly global::System.Action, global::System.Exception?> __M0Callback = + global::Microsoft.Extensions.Logging.LoggerMessage.Define>(global::Microsoft.Extensions.Logging.LogLevel.Error, new global::Microsoft.Extensions.Logging.EventId(0, nameof(M0)), "M0 {a1} {a2}", true); [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "6.0.0.0")] - public static partial void M0(global::Microsoft.Extensions.Logging.ILogger logger, global::System.Int32 a1) + public static partial void M0(global::Microsoft.Extensions.Logging.ILogger logger, global::System.Int32 a1, global::System.Collections.Generic.IEnumerable a2) { if (logger.IsEnabled(global::Microsoft.Extensions.Logging.LogLevel.Error)) { - __M0Callback(logger, a1, null); + __M0Callback(logger, a1, a2, null); } } } diff --git a/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs b/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs index c66a267f7c32aa..d727420f52a291 100644 --- a/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs +++ b/src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs @@ -35,18 +35,18 @@ public async Task TestEmitter() } [Fact] - public async Task TestBaseline_TestWithOneParam_Success() + public async Task TestBaseline_TestWithTwoParams_Success() { string testSourceCode = @" namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses { - internal static partial class TestWithOneParam + internal static partial class TestWithTwoParams { - [LoggerMessage(EventId = 0, Level = LogLevel.Error, Message = ""M0 {A1}"")] - public static partial void M0(ILogger logger, int a1); + [LoggerMessage(EventId = 0, Level = LogLevel.Error, Message = ""M0 {a1} {a2}"")] + public static partial void M0(ILogger logger, int a1, System.Collections.Generic.IEnumerable a2); } }"; - await VerifyAgainstBaselineUsingFile("TestWithOneParam.generated.txt", testSourceCode); + await VerifyAgainstBaselineUsingFile("TestWithTwoParams.generated.txt", testSourceCode); } [Fact] @@ -59,7 +59,7 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses internal static partial class TestWithMoreThan6Params { [LoggerMessage(EventId = 8, Level = LogLevel.Error, Message = ""M9 {p1} {p2} {p3} {p4} {p5} {p6} {p7}"")] - public static partial void Method9(ILogger logger, int p1, int p2, int p3, int p4, int p5, int p6, int p7); + public static partial void Method9(ILogger logger, int p1, int p2, int p3, int p4, int p5, int p6, System.Collections.Generic.IEnumerable p7); } }"; await VerifyAgainstBaselineUsingFile("TestWithMoreThan6Params.generated.txt", testSourceCode); From 25f2eef20c6388a8183e9e5dd106f722cfdfbb42 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Wed, 28 Apr 2021 13:27:16 -0700 Subject: [PATCH 2/2] PR feedback --- .../gen/LoggerMessageGenerator.Emitter.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs b/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs index a57ac243c4daa7..748bfd375bbe34 100644 --- a/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs +++ b/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs @@ -16,7 +16,7 @@ internal class Emitter private const int MaxLoggerMessageDefineArguments = 6; private const int DefaultStringBuilderCapacity = 1024; - private static readonly string _generatedCodeAttribute = + private static readonly string s_generatedCodeAttribute = $"global::System.CodeDom.Compiler.GeneratedCodeAttribute(" + $"\"{typeof(Emitter).Assembly.GetName().Name}\", " + $"\"{typeof(Emitter).Assembly.GetName().Version}\")"; @@ -49,10 +49,10 @@ private static bool UseLoggerMessageDefine(LoggerMethod lm) if (result) { // make sure the order of the templates matches the order of the logging method parameter - int count = 0; - foreach (string t in lm.TemplateList) + for (int i = 0; i < lm.TemplateList.Count; i++) { - if (!t.Equals(lm.TemplateParameters[count++].Name, StringComparison.OrdinalIgnoreCase)) + string t = lm.TemplateList[i]; + if (!t.Equals(lm.TemplateParameters[i].Name, StringComparison.OrdinalIgnoreCase)) { // order doesn't match, can't use LoggerMessage.Define return false; @@ -99,7 +99,7 @@ partial class {lc.Name} {lc.Constraints} private void GenStruct(LoggerMethod lm) { _builder.AppendLine($@" - [{_generatedCodeAttribute}] + [{s_generatedCodeAttribute}] private readonly struct __{lm.Name}Struct : global::System.Collections.Generic.IReadOnlyList> {{"); GenFields(lm); @@ -332,7 +332,7 @@ private void GenLogMethod(LoggerMethod lm) if (UseLoggerMessageDefine(lm)) { _builder.Append($@" - [{_generatedCodeAttribute}] + [{s_generatedCodeAttribute}] private static readonly global::System.Action