Skip to content

Commit 3245528

Browse files
maryamariyanmaryamariyan
authored
Two bugfixes for logging generator (#51963)
* - 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 * PR feedback
1 parent 25c2bb3 commit 3245528

File tree

5 files changed

+110
-70
lines changed

5 files changed

+110
-70
lines changed

‎src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs

Lines changed: 49 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,12 @@ internal class Emitter
1616
private const int MaxLoggerMessageDefineArguments = 6;
1717
private const int DefaultStringBuilderCapacity = 1024;
1818

19-
private readonly string _generatedCodeAttribute =
19+
private static readonly string s_generatedCodeAttribute =
2020
$"global::System.CodeDom.Compiler.GeneratedCodeAttribute(" +
2121
$"\"{typeof(Emitter).Assembly.GetName().Name}\", " +
2222
$"\"{typeof(Emitter).Assembly.GetName().Version}\")";
2323
private readonly StringBuilder _builder = new StringBuilder(DefaultStringBuilderCapacity);
24+
private bool _needEnumerationHelper;
2425

2526
public string Emit(IReadOnlyList<LoggerClass> logClasses, CancellationToken cancellationToken)
2627
{
@@ -34,6 +35,7 @@ public string Emit(IReadOnlyList<LoggerClass> logClasses, CancellationToken canc
3435
GenType(lc);
3536
}
3637

38+
GenEnumerationHelper();
3739
return _builder.ToString();
3840
}
3941

@@ -47,10 +49,10 @@ private static bool UseLoggerMessageDefine(LoggerMethod lm)
4749
if (result)
4850
{
4951
// make sure the order of the templates matches the order of the logging method parameter
50-
int count = 0;
51-
foreach (string t in lm.TemplateList)
52+
for (int i = 0; i < lm.TemplateList.Count; i++)
5253
{
53-
if (!t.Equals(lm.TemplateParameters[count].Name, StringComparison.OrdinalIgnoreCase))
54+
string t = lm.TemplateList[i];
55+
if (!t.Equals(lm.TemplateParameters[i].Name, StringComparison.OrdinalIgnoreCase))
5456
{
5557
// order doesn't match, can't use LoggerMessage.Define
5658
return false;
@@ -84,8 +86,6 @@ partial class {lc.Name} {lc.Constraints}
8486
GenLogMethod(lm);
8587
}
8688

87-
GenEnumerationHelper(lc);
88-
8989
_builder.Append($@"
9090
}}");
9191

@@ -99,7 +99,7 @@ partial class {lc.Name} {lc.Constraints}
9999
private void GenStruct(LoggerMethod lm)
100100
{
101101
_builder.AppendLine($@"
102-
[{_generatedCodeAttribute}]
102+
[{s_generatedCodeAttribute}]
103103
private readonly struct __{lm.Name}Struct : global::System.Collections.Generic.IReadOnlyList<global::System.Collections.Generic.KeyValuePair<string, object?>>
104104
{{");
105105
GenFields(lm);
@@ -193,7 +193,9 @@ private void GenVariableAssignments(LoggerMethod lm)
193193
if (lm.TemplateParameters[index].IsEnumerable)
194194
{
195195
_builder.AppendLine($" var {t.Key} = "
196-
+ $"__Enumerate((global::System.Collections.IEnumerable ?)this._{lm.TemplateParameters[index].Name});");
196+
+ $"global::__LoggerMessageGenerator.Enumerate((global::System.Collections.IEnumerable ?)this._{lm.TemplateParameters[index].Name});");
197+
198+
_needEnumerationHelper = true;
197199
}
198200
else
199201
{
@@ -237,7 +239,7 @@ private void GenDefineTypes(LoggerMethod lm, bool brackets)
237239
}
238240
if (brackets)
239241
{
240-
_builder.Append("<");
242+
_builder.Append('<');
241243
}
242244

243245
bool firstItem = true;
@@ -257,7 +259,7 @@ private void GenDefineTypes(LoggerMethod lm, bool brackets)
257259

258260
if (brackets)
259261
{
260-
_builder.Append(">");
262+
_builder.Append('>');
261263
}
262264
else
263265
{
@@ -322,15 +324,15 @@ private void GenHolder(LoggerMethod lm)
322324
private void GenLogMethod(LoggerMethod lm)
323325
{
324326
string level = GetLogLevel(lm);
325-
string extension = (lm.IsExtensionMethod ? "this " : string.Empty);
327+
string extension = lm.IsExtensionMethod ? "this " : string.Empty;
326328
string eventName = string.IsNullOrWhiteSpace(lm.EventName) ? $"nameof({lm.Name})" : $"\"{lm.EventName}\"";
327329
string exceptionArg = GetException(lm);
328330
string logger = GetLogger(lm);
329331

330332
if (UseLoggerMessageDefine(lm))
331333
{
332334
_builder.Append($@"
333-
[{_generatedCodeAttribute}]
335+
[{s_generatedCodeAttribute}]
334336
private static readonly global::System.Action<global::Microsoft.Extensions.Logging.ILogger, ");
335337

336338
GenDefineTypes(lm, brackets: false);
@@ -345,7 +347,7 @@ private void GenLogMethod(LoggerMethod lm)
345347
}
346348

347349
_builder.Append($@"
348-
[{_generatedCodeAttribute}]
350+
[{s_generatedCodeAttribute}]
349351
{lm.Modifiers} void {lm.Name}({extension}");
350352

351353
GenParameters(lm);
@@ -443,63 +445,56 @@ static string GetLogLevel(LoggerMethod lm)
443445
}
444446
}
445447

446-
private void GenEnumerationHelper(LoggerClass lc)
448+
private void GenEnumerationHelper()
447449
{
448-
foreach (LoggerMethod lm in lc.Methods)
450+
if (_needEnumerationHelper)
449451
{
450-
if (UseLoggerMessageDefine(lm))
451-
{
452-
foreach (LoggerParameter p in lm.TemplateParameters)
453-
{
454-
if (p.IsEnumerable)
455-
{
456452
_builder.Append($@"
457-
[{_generatedCodeAttribute}]
458-
private static string __Enumerate(global::System.Collections.IEnumerable? enumerable)
453+
[{s_generatedCodeAttribute}]
454+
internal static class __LoggerMessageGenerator
455+
{{
456+
public static string Enumerate(global::System.Collections.IEnumerable? enumerable)
457+
{{
458+
if (enumerable == null)
459459
{{
460-
if (enumerable == null)
461-
{{
462-
return ""(null)"";
463-
}}
460+
return ""(null)"";
461+
}}
464462
465-
var sb = new global::System.Text.StringBuilder();
466-
_ = sb.Append('[');
463+
var sb = new global::System.Text.StringBuilder();
464+
_ = sb.Append('[');
467465
468-
bool first = true;
469-
foreach (object e in enumerable)
466+
bool first = true;
467+
foreach (object e in enumerable)
468+
{{
469+
if (!first)
470470
{{
471-
if (!first)
472-
{{
473-
_ = sb.Append("", "");
474-
}}
471+
_ = sb.Append("", "");
472+
}}
475473
476-
if (e == null)
474+
if (e == null)
475+
{{
476+
_ = sb.Append(""(null)"");
477+
}}
478+
else
479+
{{
480+
if (e is global::System.IFormattable fmt)
477481
{{
478-
_ = sb.Append(""(null)"");
482+
_ = sb.Append(fmt.ToString(null, global::System.Globalization.CultureInfo.InvariantCulture));
479483
}}
480484
else
481485
{{
482-
if (e is global::System.IFormattable fmt)
483-
{{
484-
_ = sb.Append(fmt.ToString(null, global::System.Globalization.CultureInfo.InvariantCulture));
485-
}}
486-
else
487-
{{
488-
_ = sb.Append(e);
489-
}}
486+
_ = sb.Append(e);
490487
}}
491-
492-
first = false;
493488
}}
494489
495-
_ = sb.Append(']');
496-
497-
return sb.ToString();
490+
first = false;
498491
}}
499-
");
500-
}
501-
}
502-
}
492+
493+
_ = sb.Append(']');
494+
495+
return sb.ToString();
496+
}}
497+
}}");
503498
}
504499
}
505500
}

‎src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public void Execute(GeneratorExecutionContext context)
3939
var e = new Emitter();
4040
string result = e.Emit(logClasses, context.CancellationToken);
4141

42-
context.AddSource(nameof(LoggerMessageGenerator), SourceText.From(result, Encoding.UTF8));
42+
context.AddSource("LoggerMessage", SourceText.From(result, Encoding.UTF8));
4343
}
4444
}
4545

‎src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithMoreThan6Params.generated.txt

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,9 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
1414
private readonly global::System.Int32 _p4;
1515
private readonly global::System.Int32 _p5;
1616
private readonly global::System.Int32 _p6;
17-
private readonly global::System.Int32 _p7;
17+
private readonly global::System.Collections.Generic.IEnumerable<global::System.Int32> _p7;
1818

19-
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)
19+
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<global::System.Int32> p7)
2020
{
2121
this._p1 = p1;
2222
this._p2 = p2;
@@ -36,7 +36,7 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
3636
var p4 = this._p4;
3737
var p5 = this._p5;
3838
var p6 = this._p6;
39-
var p7 = this._p7;
39+
var p7 = global::__LoggerMessageGenerator.Enumerate((global::System.Collections.IEnumerable ?)this._p7);
4040

4141
return $"M9 {p1} {p2} {p3} {p4} {p5} {p6} {p7}";
4242
}
@@ -74,7 +74,7 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
7474
}
7575

7676
[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "6.0.0.0")]
77-
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)
77+
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<global::System.Int32> p7)
7878
{
7979
if (logger.IsEnabled(global::Microsoft.Extensions.Logging.LogLevel.Error))
8080
{
@@ -87,4 +87,49 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
8787
}
8888
}
8989
}
90+
}
91+
[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "6.0.0.0")]
92+
internal static class __LoggerMessageGenerator
93+
{
94+
public static string Enumerate(global::System.Collections.IEnumerable? enumerable)
95+
{
96+
if (enumerable == null)
97+
{
98+
return "(null)";
99+
}
100+
101+
var sb = new global::System.Text.StringBuilder();
102+
_ = sb.Append('[');
103+
104+
bool first = true;
105+
foreach (object e in enumerable)
106+
{
107+
if (!first)
108+
{
109+
_ = sb.Append(", ");
110+
}
111+
112+
if (e == null)
113+
{
114+
_ = sb.Append("(null)");
115+
}
116+
else
117+
{
118+
if (e is global::System.IFormattable fmt)
119+
{
120+
_ = sb.Append(fmt.ToString(null, global::System.Globalization.CultureInfo.InvariantCulture));
121+
}
122+
else
123+
{
124+
_ = sb.Append(e);
125+
}
126+
}
127+
128+
first = false;
129+
}
130+
131+
_ = sb.Append(']');
132+
133+
return sb.ToString();
134+
}
90135
}

‎src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithOneParam.generated.txt renamed to ‎src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/Baselines/TestWithTwoParams.generated.txt

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,18 @@
33

44
namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
55
{
6-
partial class TestWithOneParam
6+
partial class TestWithTwoParams
77
{
88
[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "6.0.0.0")]
9-
private static readonly global::System.Action<global::Microsoft.Extensions.Logging.ILogger, global::System.Int32, global::System.Exception?> __M0Callback =
10-
global::Microsoft.Extensions.Logging.LoggerMessage.Define<global::System.Int32>(global::Microsoft.Extensions.Logging.LogLevel.Error, new global::Microsoft.Extensions.Logging.EventId(0, nameof(M0)), "M0 {A1}", true);
9+
private static readonly global::System.Action<global::Microsoft.Extensions.Logging.ILogger, global::System.Int32, global::System.Collections.Generic.IEnumerable<global::System.Int32>, global::System.Exception?> __M0Callback =
10+
global::Microsoft.Extensions.Logging.LoggerMessage.Define<global::System.Int32, global::System.Collections.Generic.IEnumerable<global::System.Int32>>(global::Microsoft.Extensions.Logging.LogLevel.Error, new global::Microsoft.Extensions.Logging.EventId(0, nameof(M0)), "M0 {a1} {a2}", true);
1111

1212
[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "6.0.0.0")]
13-
public static partial void M0(global::Microsoft.Extensions.Logging.ILogger logger, global::System.Int32 a1)
13+
public static partial void M0(global::Microsoft.Extensions.Logging.ILogger logger, global::System.Int32 a1, global::System.Collections.Generic.IEnumerable<global::System.Int32> a2)
1414
{
1515
if (logger.IsEnabled(global::Microsoft.Extensions.Logging.LogLevel.Error))
1616
{
17-
__M0Callback(logger, a1, null);
17+
__M0Callback(logger, a1, a2, null);
1818
}
1919
}
2020
}

‎src/libraries/Microsoft.Extensions.Logging/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,18 +35,18 @@ public async Task TestEmitter()
3535
}
3636

3737
[Fact]
38-
public async Task TestBaseline_TestWithOneParam_Success()
38+
public async Task TestBaseline_TestWithTwoParams_Success()
3939
{
4040
string testSourceCode = @"
4141
namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
4242
{
43-
internal static partial class TestWithOneParam
43+
internal static partial class TestWithTwoParams
4444
{
45-
[LoggerMessage(EventId = 0, Level = LogLevel.Error, Message = ""M0 {A1}"")]
46-
public static partial void M0(ILogger logger, int a1);
45+
[LoggerMessage(EventId = 0, Level = LogLevel.Error, Message = ""M0 {a1} {a2}"")]
46+
public static partial void M0(ILogger logger, int a1, System.Collections.Generic.IEnumerable<int> a2);
4747
}
4848
}";
49-
await VerifyAgainstBaselineUsingFile("TestWithOneParam.generated.txt", testSourceCode);
49+
await VerifyAgainstBaselineUsingFile("TestWithTwoParams.generated.txt", testSourceCode);
5050
}
5151

5252
[Fact]
@@ -59,7 +59,7 @@ namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
5959
internal static partial class TestWithMoreThan6Params
6060
{
6161
[LoggerMessage(EventId = 8, Level = LogLevel.Error, Message = ""M9 {p1} {p2} {p3} {p4} {p5} {p6} {p7}"")]
62-
public static partial void Method9(ILogger logger, int p1, int p2, int p3, int p4, int p5, int p6, int p7);
62+
public static partial void Method9(ILogger logger, int p1, int p2, int p3, int p4, int p5, int p6, System.Collections.Generic.IEnumerable<int> p7);
6363
}
6464
}";
6565
await VerifyAgainstBaselineUsingFile("TestWithMoreThan6Params.generated.txt", testSourceCode);

0 commit comments

Comments
 (0)