-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Two bugfixes for logging generator #51963
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 s_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<LoggerClass> logClasses, CancellationToken cancellationToken) | ||
{ | ||
|
@@ -34,6 +35,7 @@ public string Emit(IReadOnlyList<LoggerClass> logClasses, CancellationToken canc | |
GenType(lc); | ||
} | ||
|
||
GenEnumerationHelper(); | ||
return _builder.ToString(); | ||
} | ||
|
||
|
@@ -47,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; | ||
|
@@ -84,8 +86,6 @@ partial class {lc.Name} {lc.Constraints} | |
GenLogMethod(lm); | ||
} | ||
|
||
GenEnumerationHelper(lc); | ||
|
||
_builder.Append($@" | ||
}}"); | ||
|
||
|
@@ -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<global::System.Collections.Generic.KeyValuePair<string, object?>> | ||
{{"); | ||
GenFields(lm); | ||
|
@@ -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,15 +324,15 @@ 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); | ||
|
||
if (UseLoggerMessageDefine(lm)) | ||
{ | ||
_builder.Append($@" | ||
[{_generatedCodeAttribute}] | ||
[{s_generatedCodeAttribute}] | ||
private static readonly global::System.Action<global::Microsoft.Extensions.Logging.ILogger, "); | ||
|
||
GenDefineTypes(lm, brackets: false); | ||
|
@@ -345,7 +347,7 @@ private void GenLogMethod(LoggerMethod lm) | |
} | ||
|
||
_builder.Append($@" | ||
[{_generatedCodeAttribute}] | ||
[{s_generatedCodeAttribute}] | ||
{lm.Modifiers} void {lm.Name}({extension}"); | ||
|
||
GenParameters(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) | ||
[{s_generatedCodeAttribute}] | ||
internal static class __LoggerMessageGenerator | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just thinking out loud here, but I almost wonder if this deserves to be a public API in our library instead of generating this code into the user's project. I started by thinking "is putting this in the global namespace the right thing to do?" And then that quickly led to "why are we generating this code when it never changes based on the user's types?" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the very first API review proposal (Refer to comment #36022 (comment)), we proposed this static The consensus back then was for such cases to just generate them if/when needed. |
||
{{ | ||
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(); | ||
}} | ||
}}"); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just so the generated file name would become |
||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enumeration helper is either generated (only once), or not at all into
__LoggerMessageGenerator