Skip to content

Commit

Permalink
Improve support for [TagName] (#4851)
Browse files Browse the repository at this point in the history 
- When using [TagName] to control the name of a logging tag, the
expectation is that the logging message (if present) should be using
the tag name instead of the parameter name. SO:

```
[LoggerMessage(1, LogLevel.Information, "My message {foo.bar}")]
public static partial void Log(this ILogger logger, [TagName("foo.bar") string msg);
```

Fixes #4848

Co-authored-by: Martin Taillefer <[email protected]>
  • Loading branch information
@geeknoid
geeknoid and Martin Taillefer authored Jan 3, 2024
1 parent bd8f28a commit bff3814
Show file tree
Hide file tree
Showing 11 changed files with 124 additions and 79 deletions.
75 changes: 27 additions & 48 deletions src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,19 @@ private void GenLogMethod(LoggingMethod lm)

if (GenVariableAssignments(lm, lambdaStateName, numReservedUnclassifiedTags, numReservedClassifiedTags))
{
var template = EscapeMessageString(lm.Message);
template = AddAtSymbolsToTemplates(template, lm.Parameters);
OutLn($@"return global::System.FormattableString.Invariant(${template});");
var mapped = Parsing.TemplateProcessor.MapTemplates(lm.Message, t =>
{
var p = lm.GetParameterForTemplate(t);
if (p != null)
{
return p.ParameterNameWithAtIfNeeded;
}
return t;
});

var s = EscapeMessageString(mapped!);
OutLn($@"return global::System.FormattableString.Invariant(${s});");
}
else if (string.IsNullOrEmpty(lm.Message))
{
Expand Down Expand Up @@ -294,14 +304,14 @@ void GenTagWrites(LoggingMethod lm, string stateName, out int numReservedUnclass
if (p.IsEnumerable)
{
value = p.PotentiallyNull
? $"{p.ParameterNameWithAt} != null ? {LoggerMessageHelperType}.Stringify({p.ParameterNameWithAt}) : null"
: $"{LoggerMessageHelperType}.Stringify({p.ParameterNameWithAt})";
? $"{p.ParameterNameWithAtIfNeeded} != null ? {LoggerMessageHelperType}.Stringify({p.ParameterNameWithAtIfNeeded}) : null"
: $"{LoggerMessageHelperType}.Stringify({p.ParameterNameWithAtIfNeeded})";
}
else
{
value = ShouldStringifyParameter(p)
? ConvertParameterToString(p, p.ParameterNameWithAt)
: p.ParameterNameWithAt;
? ConvertParameterToString(p, p.ParameterNameWithAtIfNeeded)
: p.ParameterNameWithAtIfNeeded;
}

OutLn($"{stateName}.TagArray[{--count}] = new({key}, {value});");
Expand Down Expand Up @@ -352,8 +362,8 @@ void GenTagWrites(LoggingMethod lm, string stateName, out int numReservedUnclass
var classification = MakeClassificationValue(p.ClassificationAttributeTypes);

var value = ShouldStringifyParameter(p)
? ConvertParameterToString(p, p.ParameterNameWithAt)
: p.ParameterNameWithAt;
? ConvertParameterToString(p, p.ParameterNameWithAtIfNeeded)
: p.ParameterNameWithAtIfNeeded;

OutLn($"{stateName}.ClassifiedTagArray[{--count}] = new({key}, {value}, {classification});");
}
Expand Down Expand Up @@ -469,10 +479,10 @@ void GenTagWrites(LoggingMethod lm, string stateName, out int numReservedUnclass
}
else
{
OutLn($"{stateName}.TagNamePrefix = nameof({p.ParameterNameWithAt});");
OutLn($"{stateName}.TagNamePrefix = nameof({p.ParameterNameWithAtIfNeeded});");
}

OutLn($"{p.TagProvider!.ContainingType}.{p.TagProvider.MethodName}({stateName}, {p.ParameterNameWithAt});");
OutLn($"{p.TagProvider!.ContainingType}.{p.TagProvider.MethodName}({stateName}, {p.ParameterNameWithAtIfNeeded});");
}
}
}
Expand All @@ -495,11 +505,11 @@ bool GenVariableAssignments(LoggingMethod lm, string lambdaStateName, int numRes
if (p.PotentiallyNull)
{
const string Null = "\"(null)\"";
OutLn($"var {atSign}{t} = {lambdaStateName}.TagArray[{index}].Value ?? {Null};");
OutLn($"var {atSign}{p.ParameterName} = {lambdaStateName}.TagArray[{index}].Value ?? {Null};");
}
else
{
OutLn($"var {atSign}{t} = {lambdaStateName}.TagArray[{index}].Value;");
OutLn($"var {atSign}{p.ParameterName} = {lambdaStateName}.TagArray[{index}].Value;");
}

generatedAssignments = true;
Expand All @@ -524,11 +534,11 @@ bool GenVariableAssignments(LoggingMethod lm, string lambdaStateName, int numRes
if (p.PotentiallyNull)
{
const string Null = "\"(null)\"";
OutLn($"var {atSign}{t} = {lambdaStateName}.RedactedTagArray[{index}].Value ?? {Null};");
OutLn($"var {atSign}{p.ParameterName} = {lambdaStateName}.RedactedTagArray[{index}].Value ?? {Null};");
}
else
{
OutLn($"var {atSign}{t} = {lambdaStateName}.RedactedTagArray[{index}].Value;");
OutLn($"var {atSign}{p.ParameterName} = {lambdaStateName}.RedactedTagArray[{index}].Value;");
}

generatedAssignments = true;
Expand Down Expand Up @@ -585,47 +595,16 @@ private string MakeClassificationValue(HashSet<string> classificationTypes)
return sb.ToString();
}

private string AddAtSymbolsToTemplates(string template, IEnumerable<LoggingMethodParameter> parameters)
{
StringBuilder? stringBuilder = null;
foreach (var item in parameters.Where(p => p.UsedAsTemplate))
{
if (!item.NeedsAtSign)
{
continue;
}

if (stringBuilder is null)
{
stringBuilder = _sbPool.GetStringBuilder();
_ = stringBuilder.Append(template);
}

_ = stringBuilder.Replace(item.ParameterName, item.ParameterNameWithAt);
}

var result = stringBuilder is null
? template
: stringBuilder.ToString();

if (stringBuilder != null)
{
_sbPool.ReturnStringBuilder(stringBuilder);
}

return result;
}

private void GenParameters(LoggingMethod lm)
{
OutEnumeration(lm.Parameters.Select(static p =>
{
if (p.Qualifier != null)
{
return $"{p.Qualifier} {p.Type} {p.ParameterNameWithAt}";
return $"{p.Qualifier} {p.Type} {p.ParameterNameWithAtIfNeeded}";
}
return $"{p.Type} {p.ParameterNameWithAt}";
return $"{p.Type} {p.ParameterNameWithAtIfNeeded}";
}));
}

Expand Down
20 changes: 15 additions & 5 deletions src/Generators/Microsoft.Gen.Logging/Model/LoggingMethod.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ internal sealed class LoggingMethod
{
foreach (var p in Parameters)
{
if (templateName.Equals(p.ParameterName, StringComparison.OrdinalIgnoreCase))
if (templateName.Equals(p.TagName, StringComparison.OrdinalIgnoreCase))
{
return p;
}
Expand All @@ -43,19 +43,29 @@ internal sealed class LoggingMethod
}

public List<string> GetTemplatesForParameter(LoggingMethodParameter lp)
=> GetTemplatesForParameter(lp.ParameterName);

public List<string> GetTemplatesForParameter(string parameterName)
{
HashSet<string> templates = [];
foreach (var t in Templates)
{
if (parameterName.Equals(t, StringComparison.OrdinalIgnoreCase))
if (lp.TagName.Equals(t, StringComparison.OrdinalIgnoreCase))
{
_ = templates.Add(t);
}
}

return templates.ToList();
}

public List<string> GetTemplatesForParameter(string parameterName)
{
foreach (var p in Parameters)
{
if (parameterName == p.ParameterName)
{
return GetTemplatesForParameter(p);
}
}

return [];
}
}
4 changes: 2 additions & 2 deletions src/Generators/Microsoft.Gen.Logging/Model/LoggingMethodParameter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.Gen.Logging.Model;
/// <summary>
/// A single parameter to a logger method.
/// </summary>
[DebuggerDisplay("{Name}")]
[DebuggerDisplay("{ParameterName}")]
internal sealed class LoggingMethodParameter
{
public string ParameterName = string.Empty;
Expand All @@ -35,7 +35,7 @@ internal sealed class LoggingMethodParameter
public List<LoggingProperty> Properties = [];
public TagProvider? TagProvider;

public string ParameterNameWithAt => NeedsAtSign ? "@" + ParameterName : ParameterName;
public string ParameterNameWithAtIfNeeded => NeedsAtSign ? "@" + ParameterName : ParameterName;

public string PotentiallyNullableType
=> (IsReference && !IsNullable)
Expand Down
14 changes: 6 additions & 8 deletions src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public IReadOnlyList<LoggingType> GetLogTypes(IEnumerable<TypeDeclarationSyntax>
bool parameterInTemplate = false;
foreach (var t in lm.Templates)
{
if (lp.ParameterName.Equals(t, StringComparison.OrdinalIgnoreCase))
if (lp.TagName.Equals(t, StringComparison.OrdinalIgnoreCase))
{
parameterInTemplate = true;
break;
Expand Down Expand Up @@ -273,9 +273,10 @@ public IReadOnlyList<LoggingType> GetLogTypes(IEnumerable<TypeDeclarationSyntax>
bool found = false;
foreach (var p in lm.Parameters)
{
if (t.Equals(p.ParameterName, StringComparison.OrdinalIgnoreCase))
if (t.Equals(p.TagName, StringComparison.OrdinalIgnoreCase))
{
found = true;
p.TagName = t;
break;
}
}
Expand Down Expand Up @@ -398,7 +399,7 @@ static bool IsAllowedKind(SyntaxKind kind) =>
keepMethod = false;
}

TemplateExtractor.ExtractTemplates(message, lm.Templates);
TemplateProcessor.ExtractTemplates(message, lm.Templates);

#pragma warning disable EA0003 // Use the character-based overloads of 'String.StartsWith' or 'String.EndsWith'
var templatesWithAtSymbol = lm.Templates.Where(x => x.StartsWith("@", StringComparison.Ordinal)).ToArray();
Expand Down Expand Up @@ -522,12 +523,9 @@ private void CheckTagNamesAreUnique(LoggingMethod lm, Dictionary<LoggingMethodPa
}
});
}
else
else if (!names.Add(parameter.TagName))
{
if (!names.Add(parameter.TagName))
{
Diag(DiagDescriptors.TagNameCollision, parameterSymbols[parameter].GetLocation(), parameter.ParameterName, parameter.TagName, lm.Name);
}
Diag(DiagDescriptors.TagNameCollision, parameterSymbols[parameter].GetLocation(), parameter.ParameterName, parameter.TagName, lm.Name);
}
}
}
Expand Down
58 changes: 49 additions & 9 deletions ....Gen.Logging/Parsing/TemplateExtractor.cs → ....Gen.Logging/Parsing/TemplateProcessor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@

using System;
using System.Collections.Generic;
using System.Text;

namespace Microsoft.Gen.Logging.Parsing;

internal static class TemplateExtractor
internal static class TemplateProcessor
{
private static readonly char[] _formatDelimiters = { ',', ':' };

Expand All @@ -29,18 +30,57 @@ internal static void ExtractTemplates(string? message, List<string> templates)

if (closeBraceIndex == endIndex)
{
scanIndex = endIndex;
return;
}
else
{
// Format item syntax : { index[,alignment][ :formatString] }.
var formatDelimiterIndex = FindIndexOfAny(message, _formatDelimiters, openBraceIndex, closeBraceIndex);

var templateName = message.Substring(openBraceIndex + 1, formatDelimiterIndex - openBraceIndex - 1).Trim();
templates.Add(templateName);
scanIndex = closeBraceIndex + 1;
// Format item syntax : { index[,alignment][ :formatString] }.
var formatDelimiterIndex = FindIndexOfAny(message, _formatDelimiters, openBraceIndex, closeBraceIndex);

var templateName = message.Substring(openBraceIndex + 1, formatDelimiterIndex - openBraceIndex - 1).Trim();
templates.Add(templateName);
scanIndex = closeBraceIndex + 1;
}
}

/// <summary>
/// Allows replacing individual template arguments with different strings.
/// </summary>
internal static string? MapTemplates(string? message, Func<string, string> mapTemplate)
{
if (string.IsNullOrEmpty(message))
{
return message;
}

var sb = new StringBuilder();

var scanIndex = 0;
var endIndex = message!.Length;
while (scanIndex < endIndex)
{
var openBraceIndex = FindBraceIndex(message, '{', scanIndex, endIndex);
var closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex, endIndex);

if (closeBraceIndex == endIndex)
{
break;
}

// Format item syntax : { index[,alignment][ :formatString] }.
var formatDelimiterIndex = FindIndexOfAny(message, _formatDelimiters, openBraceIndex, closeBraceIndex);

var templateName = message.Substring(openBraceIndex + 1, formatDelimiterIndex - openBraceIndex - 1).Trim();
var mapped = mapTemplate(templateName);

_ = sb.Append(message, scanIndex, openBraceIndex - scanIndex + 1);
_ = sb.Append(mapped);
_ = sb.Append(message, formatDelimiterIndex, closeBraceIndex - formatDelimiterIndex + 1);

scanIndex = closeBraceIndex + 1;
}

_ = sb.Append(message, scanIndex, message.Length - scanIndex);
return sb.ToString();
}

internal static int FindIndexOfAny(string message, char[] chars, int startIndex, int endIndex)
Expand Down
5 changes: 5 additions & 0 deletions test/Generators/Microsoft.Gen.Logging/Generated/LogMethodTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,11 @@ public void AtSymbolsTest()
collector.Clear();
AtSymbolsTestExtensions.M5(logger, LogLevel.Debug, o);
Assert.Equal("42", collector.LatestRecord.StructuredState!.GetValue("class"));

collector.Clear();
AtSymbolsTestExtensions.M6(logger, "42");
Assert.Equal("42", collector.LatestRecord.StructuredState!.GetValue("class"));
Assert.Equal("M6 class 42", collector.LatestRecord.Message);
}

[Fact]
Expand Down
15 changes: 11 additions & 4 deletions test/Generators/Microsoft.Gen.Logging/Generated/TagNameTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,19 @@ public void Basic()
var logger = new FakeLogger();

TagNameExtensions.M0(logger, 0);

var expectedState = new Dictionary<string, string?>
logger.Collector.LatestRecord.StructuredState.Should().NotBeNull().And.Equal(new Dictionary<string, string?>
{
["TN1"] = "0",
};
});

logger.Collector.Clear();
TagNameExtensions.M1(logger, 0);
logger.Collector.LatestRecord.StructuredState.Should().NotBeNull().And.Equal(new Dictionary<string, string?>
{
["foo.bar"] = "0",
["{OriginalFormat}"] = "{foo.bar}",
});

logger.Collector.LatestRecord.StructuredState.Should().NotBeNull().And.Equal(expectedState);
Assert.Equal("0", logger.Collector.LatestRecord.Message);
}
}
3 changes: 3 additions & 0 deletions test/Generators/Microsoft.Gen.Logging/TestClasses/AtSymbolsTestExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,8 @@ public class SpecialNames

[LoggerMessage("M5")]
internal static partial void M5(ILogger logger, LogLevel level, [LogProperties(OmitReferenceName = true)] SpecialNames @event);

[LoggerMessage(LogLevel.Information, "M6 class {class}")]
internal static partial void M6(ILogger logger, string @class);
}
}
3 changes: 3 additions & 0 deletions test/Generators/Microsoft.Gen.Logging/TestClasses/TagNameExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,8 @@ internal static partial class TagNameExtensions
{
[LoggerMessage(LogLevel.Warning)]
internal static partial void M0(ILogger logger, [TagName("TN1")] int p0);

[LoggerMessage(LogLevel.Warning, Message = "{foo.bar}")]
internal static partial void M1(ILogger logger, [TagName("foo.bar")] int p0);
}
}
4 changes: 2 additions & 2 deletions test/Generators/Microsoft.Gen.Logging/Unit/LoggingMethodParameterTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ public void Misc()
NeedsAtSign = false,
};

Assert.Equal(lp.ParameterName, lp.ParameterNameWithAt);
Assert.Equal(lp.ParameterName, lp.ParameterNameWithAtIfNeeded);
lp.NeedsAtSign = true;
Assert.Equal("@" + lp.ParameterName, lp.ParameterNameWithAt);
Assert.Equal("@" + lp.ParameterName, lp.ParameterNameWithAtIfNeeded);

lp.Type = "Foo";
lp.IsReference = false;
Expand Down
Loading

0 comments on commit bff3814

Please sign in to comment.