Improve support for [TagName] (#4851)

- 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 <mataille@microsoft.com>

src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs

@@ -104,9 +104,19 @@ internal sealed partial class Emitter : EmitterBase
 
         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))
         {
@@ -294,14 +304,14 @@ internal sealed partial class Emitter : EmitterBase
                         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});");
@@ -352,8 +362,8 @@ internal sealed partial class Emitter : EmitterBase
                         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});");
                     }
@@ -469,10 +479,10 @@ internal sealed partial class Emitter : EmitterBase
                     }
                     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});");
                 }
             }
         }
@@ -495,11 +505,11 @@ internal sealed partial class Emitter : EmitterBase
                             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;
@@ -524,11 +534,11 @@ internal sealed partial class Emitter : EmitterBase
                             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;
@@ -585,47 +595,16 @@ internal sealed partial class Emitter : EmitterBase
         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}";
         }));
     }
 

src/Generators/Microsoft.Gen.Logging/Model/LoggingMethod.cs

@@ -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;
             }
@@ -43,14 +43,11 @@ 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);
             }
@@ -58,4 +55,17 @@ internal sealed class LoggingMethod
 
         return templates.ToList();
     }
+
+    public List<string> GetTemplatesForParameter(string parameterName)
+    {
+        foreach (var p in Parameters)
+        {
+            if (parameterName == p.ParameterName)
+            {
+                return GetTemplatesForParameter(p);
+            }
+        }
+
+        return [];
+    }
 }

src/Generators/Microsoft.Gen.Logging/Model/LoggingMethodParameter.cs

@@ -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;
@@ -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)

src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs

@@ -152,7 +152,7 @@ internal sealed partial class Parser
                         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;
@@ -273,9 +273,10 @@ internal sealed partial class Parser
                             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;
                                 }
                             }
@@ -398,7 +399,7 @@ internal sealed partial class Parser
                 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();
@@ -522,12 +523,9 @@ internal sealed partial class Parser
                     }
                 });
             }
-            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);
             }
         }
     }

src/Generators/Microsoft.Gen.Logging/Parsing/TemplateProcessor.cs

@@ -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 = { ',', ':' };
 
@@ -29,20 +30,59 @@ internal static class TemplateExtractor
 
             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)
     {
         var findIndex = message.IndexOfAny(chars, startIndex, endIndex - startIndex);

test/Generators/Microsoft.Gen.Logging/Generated/LogMethodTests.cs

@@ -756,6 +756,11 @@ public class LogMethodTests
         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]

test/Generators/Microsoft.Gen.Logging/Generated/TagNameTests.cs

@@ -17,12 +17,19 @@ public class TagNameTests
         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.LatestRecord.StructuredState.Should().NotBeNull().And.Equal(expectedState);
+        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}",
+        });
+
+        Assert.Equal("0", logger.Collector.LatestRecord.Message);
     }
 }

test/Generators/Microsoft.Gen.Logging/TestClasses/AtSymbolsTestExtensions.cs

@@ -32,5 +32,8 @@ namespace TestClasses
 
         [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);
     }
 }

test/Generators/Microsoft.Gen.Logging/TestClasses/TagNameExtensions.cs

@@ -9,5 +9,8 @@ namespace TestClasses
     {
         [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);
     }
 }

test/Generators/Microsoft.Gen.Logging/Unit/LoggingMethodParameterTests.cs

@@ -61,9 +61,9 @@ public class LoggingMethodParameterTests
             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;

test/Generators/Microsoft.Gen.Logging/Unit/TemplatesExtractorTests.cs

@@ -17,7 +17,7 @@ public class TemplatesExtractorTests
     [InlineData("NewLine \n Test", 8)]
     public void Should_FindIndexOfAny_Correctly(string message, int expectedResult)
     {
-        var result = TemplateExtractor.FindIndexOfAny(message, new[] { '\n', 'a', 'b', 'z' }, 0, message.Length);
+        var result = TemplateProcessor.FindIndexOfAny(message, new[] { '\n', 'a', 'b', 'z' }, 0, message.Length);
         Assert.Equal(expectedResult, result);
     }
 }