diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs index 6af2a7856ed41e..6bf627eea91c34 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs @@ -779,8 +779,12 @@ private static bool ExtractTemplates(string? message, Dictionary break; } - templateMap[templateName] = templateName; - templateList.Add(templateName); + // Only add the template name to the list if it hasn't been seen before + if (!templateMap.ContainsKey(templateName)) + { + templateMap[templateName] = templateName; + templateList.Add(templateName); + } scanIndex = closeBraceIndex + 1; } diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogValuesFormatter.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogValuesFormatter.cs index 2fdba83aa3206c..6dc6bb8e971238 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogValuesFormatter.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogValuesFormatter.cs @@ -17,6 +17,7 @@ internal sealed class LogValuesFormatter { private const string NullValue = "(null)"; private readonly List _valueNames = new List(); + #if NET private readonly CompositeFormat _format; #else @@ -33,6 +34,7 @@ public LogValuesFormatter(string format) OriginalFormat = format; + Dictionary? valueNameIndices = null; var vsb = new ValueStringBuilder(stackalloc char[256]); int scanIndex = 0; int endIndex = format.Length; @@ -66,8 +68,40 @@ public LogValuesFormatter(string format) formatDelimiterIndex = formatDelimiterIndex < 0 ? closeBraceIndex : formatDelimiterIndex + openBraceIndex; vsb.Append(format.AsSpan(scanIndex, openBraceIndex - scanIndex + 1)); - vsb.Append(_valueNames.Count.ToString(CultureInfo.InvariantCulture)); - _valueNames.Add(format.Substring(openBraceIndex + 1, formatDelimiterIndex - openBraceIndex - 1)); + string valueName = format.Substring(openBraceIndex + 1, formatDelimiterIndex - openBraceIndex - 1); + + int valueIndex; + if (valueNameIndices != null) + { + // Use dictionary for lookup when we have many placeholders + if (!valueNameIndices.TryGetValue(valueName, out valueIndex)) + { + valueIndex = _valueNames.Count; + _valueNames.Add(valueName); + valueNameIndices[valueName] = valueIndex; + } + } + else + { + // For small number of placeholders, use linear search to avoid dictionary allocation and hashing overhead + valueIndex = FindValueName(valueName); + if (valueIndex < 0) + { + valueIndex = _valueNames.Count; + _valueNames.Add(valueName); + + // Switch to dictionary when we have many unique placeholders + if (_valueNames.Count > 4) + { + valueNameIndices = new Dictionary(_valueNames.Count, StringComparer.OrdinalIgnoreCase); + for (int i = 0; i < _valueNames.Count; i++) + { + valueNameIndices[_valueNames[i]] = i; + } + } + } + } + vsb.Append(valueIndex.ToString(CultureInfo.InvariantCulture)); vsb.Append(format.AsSpan(formatDelimiterIndex, closeBraceIndex - formatDelimiterIndex + 1)); scanIndex = closeBraceIndex + 1; @@ -85,6 +119,19 @@ public LogValuesFormatter(string format) public string OriginalFormat { get; } public List ValueNames => _valueNames; + private int FindValueName(string valueName) + { + for (int i = 0; i < _valueNames.Count; i++) + { + if (string.Equals(_valueNames[i], valueName, StringComparison.OrdinalIgnoreCase)) + { + return i; + } + } + + return -1; + } + private static int FindBraceIndex(string format, char brace, int startIndex, int endIndex) { // Example: {{prefix{{{Argument}}}suffix}}. diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs index 75befc10e7b681..768475fad1939d 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs @@ -948,6 +948,41 @@ partial class C Assert.Empty(diagnostics); } + [Fact] + public async Task ValidTemplates_WithDuplicatePlaceholders() + { + IReadOnlyList diagnostics = await RunGenerator(@" + partial class C + { + [LoggerMessage(EventId = 1, Level = LogLevel.Debug, Message = ""Hello {Name}. How are you {Name}"")] + static partial void M1(ILogger logger, string name); + + [LoggerMessage(EventId = 2, Level = LogLevel.Debug, Message = ""Hello {Name}. You are {Age} years old. How are you {Name}"")] + static partial void M2(ILogger logger, string name, int age); + + [LoggerMessage(EventId = 3, Level = LogLevel.Debug, Message = ""{Age} {Name} {Age}"")] + static partial void M3(ILogger logger, int age, string name); + + [LoggerMessage(EventId = 4, Level = LogLevel.Debug, Message = ""{Name} {Name} {Name}"")] + static partial void M4(ILogger logger, string name); + + [LoggerMessage(EventId = 5, Level = LogLevel.Debug, Message = ""Age: {Age}, Name: {Name}, Age: {Age}, Name: {Name}"")] + static partial void M5(ILogger logger, int age, string name); + + [LoggerMessage(EventId = 6, Level = LogLevel.Debug, Message = ""Hello {Name}. How are you {name}"")] + static partial void M6(ILogger logger, string name); + + [LoggerMessage(EventId = 7, Level = LogLevel.Debug, Message = ""{Name} {NAME} {name}"")] + static partial void M7(ILogger logger, string name); + + [LoggerMessage(EventId = 8, Level = LogLevel.Debug, Message = ""Hello {Name}. You are {age} years old. How are you {NAME}"")] + static partial void M8(ILogger logger, string name, int age); + } + "); + + Assert.Empty(diagnostics); + } + [Fact] public async Task Cancellation() { diff --git a/src/libraries/Microsoft.Extensions.Logging/tests/Common/FormattedLogValuesTest.cs b/src/libraries/Microsoft.Extensions.Logging/tests/Common/FormattedLogValuesTest.cs index 4f5b3a8af4746b..a237afac9bcddd 100644 --- a/src/libraries/Microsoft.Extensions.Logging/tests/Common/FormattedLogValuesTest.cs +++ b/src/libraries/Microsoft.Extensions.Logging/tests/Common/FormattedLogValuesTest.cs @@ -236,6 +236,56 @@ public void FormatsEnumerableValues(string messageFormat, object[] arguments, st Assert.Equal(expected, logValues.ToString()); } + [Theory] + [InlineData("Hello David. How are you David", "Hello {Name}. How are you {Name}", new object[] { "David" })] + [InlineData("Hello David. You are 100 years old. How are you David", "Hello {Name}. You are {Age} years old. How are you {Name}", new object[] { "David", 100 })] + [InlineData("100 David 100", "{Age} {Name} {Age}", new object[] { 100, "David" })] + [InlineData("David David David", "{Name} {Name} {Name}", new object[] { "David" })] + [InlineData("Age: 100, Name: David, Age: 100, Name: David", "Age: {Age}, Name: {Name}, Age: {Age}, Name: {Name}", new object[] { 100, "David" })] + [InlineData("Hello David. How are you David", "Hello {Name}. How are you {name}", new object[] { "David" })] + [InlineData("David David David", "{Name} {NAME} {name}", new object[] { "David" })] + [InlineData("Hello David. You are 100 years old. How are you David", "Hello {Name}. You are {age} years old. How are you {NAME}", new object[] { "David", 100 })] + public void LogValues_WithDuplicatePlaceholders(string expected, string format, object[] args) + { + var logValues = new FormattedLogValues(format, args); + Assert.Equal(expected, logValues.ToString()); + + // Original format is expected to be returned from GetValues. + Assert.Equal(format, logValues.First(v => v.Key == "{OriginalFormat}").Value); + } + + [Fact] + public void LogValues_WithDuplicatePlaceholders_CorrectKeyValuePairs() + { + var format = "Hello {Name}. How are you {Name}"; + var name = "David"; + var logValues = new FormattedLogValues(format, name); + + var state = logValues.ToArray(); + Assert.Equal(new[] + { + new KeyValuePair("Name", name), + new KeyValuePair("{OriginalFormat}", format), + }, state); + } + + [Fact] + public void LogValues_WithMultipleDuplicatePlaceholders_CorrectKeyValuePairs() + { + var format = "Hello {Name}. You are {Age} years old. How are you {Name}"; + var name = "David"; + var age = 100; + var logValues = new FormattedLogValues(format, name, age); + + var state = logValues.ToArray(); + Assert.Equal(new[] + { + new KeyValuePair("Name", name), + new KeyValuePair("Age", age), + new KeyValuePair("{OriginalFormat}", format), + }, state); + } + private class MediaType { public MediaType(string type, string subType) diff --git a/src/libraries/Microsoft.Extensions.Logging/tests/Common/LoggerMessageTest.cs b/src/libraries/Microsoft.Extensions.Logging/tests/Common/LoggerMessageTest.cs index a177db3ad35d7e..b5b6435b10c433 100644 --- a/src/libraries/Microsoft.Extensions.Logging/tests/Common/LoggerMessageTest.cs +++ b/src/libraries/Microsoft.Extensions.Logging/tests/Common/LoggerMessageTest.cs @@ -514,5 +514,111 @@ public void LogValues_OutOfRangeAccess_ThrowsIndexOutOfRangeExceptionWithDefault Assert.NotEqual("index", exception.Message); Assert.False(string.IsNullOrEmpty(exception.Message)); } + + [Fact] + public void LoggerMessage_Define_WithDuplicatePlaceholder_SingleParameter() + { + var testSink = new TestSink(); + var testLogger = new TestLogger("testlogger", testSink, enabled: true); + var action = LoggerMessage.Define(LogLevel.Information, new EventId(0, "LogSomething"), "Hello {Name}. How are you {Name}"); + + action(testLogger, "David", null); + + Assert.Single(testSink.Writes); + var writeContext = testSink.Writes.First(); + var actualLogValues = Assert.IsAssignableFrom>>(writeContext.State); + + Assert.Equal("Hello David. How are you David", actualLogValues.ToString()); + Assert.Equal("Hello {Name}. How are you {Name}", actualLogValues.First(v => v.Key == "{OriginalFormat}").Value); + } + + [Fact] + public void LoggerMessage_Define_WithDuplicatePlaceholder_MultipleParameters() + { + var testSink = new TestSink(); + var testLogger = new TestLogger("testlogger", testSink, enabled: true); + var action = LoggerMessage.Define(LogLevel.Information, new EventId(0, "LogSomething"), "Hello {Name}. You are {Age} years old. How are you {Name}"); + + action(testLogger, "David", 100, null); + + Assert.Single(testSink.Writes); + var writeContext = testSink.Writes.First(); + var actualLogValues = Assert.IsAssignableFrom>>(writeContext.State); + + Assert.Equal("Hello David. You are 100 years old. How are you David", actualLogValues.ToString()); + Assert.Equal("Hello {Name}. You are {Age} years old. How are you {Name}", actualLogValues.First(v => v.Key == "{OriginalFormat}").Value); + } + + [Fact] + public void LoggerMessage_Define_WithAllDuplicatePlaceholders() + { + var testSink = new TestSink(); + var testLogger = new TestLogger("testlogger", testSink, enabled: true); + var action = LoggerMessage.Define(LogLevel.Information, new EventId(0, "LogSomething"), "{Name} {Name} {Name}"); + + action(testLogger, "David", null); + + Assert.Single(testSink.Writes); + var writeContext = testSink.Writes.First(); + var actualLogValues = Assert.IsAssignableFrom>>(writeContext.State); + + Assert.Equal("David David David", actualLogValues.ToString()); + Assert.Equal("{Name} {Name} {Name}", actualLogValues.First(v => v.Key == "{OriginalFormat}").Value); + } + + [Fact] + public void LoggerMessage_DefineScope_WithDuplicatePlaceholder() + { + var testSink = new TestSink(); + var testLogger = new TestLogger("testlogger", testSink, enabled: true); + var scopeFunc = LoggerMessage.DefineScope("Hello {Name}. How are you {Name}"); + + using (scopeFunc(testLogger, "David")) + { + Assert.Single(testSink.Scopes); + var scopeContext = testSink.Scopes.First(); + var actualLogValues = Assert.IsAssignableFrom>>(scopeContext.Scope); + + Assert.Equal("Hello David. How are you David", actualLogValues.ToString()); + Assert.Equal("Hello {Name}. How are you {Name}", actualLogValues.First(v => v.Key == "{OriginalFormat}").Value); + } + } + + [Fact] + public void LoggerMessage_Define_WithDuplicatePlaceholder_MatchingArgCount_Throws() + { + Assert.Throws(() => + LoggerMessage.Define(LogLevel.Information, new EventId(0, "LogSomething"), "Hello {Name}. How are you {Name}")); + } + + [Fact] + public void LoggerMessage_Define_WithDuplicatePlaceholder_DifferentCasing() + { + var testSink = new TestSink(); + var testLogger = new TestLogger("testlogger", testSink, enabled: true); + var action = LoggerMessage.Define(LogLevel.Information, new EventId(0, "LogSomething"), "Hello {Name}. How are you {name}"); + + action(testLogger, "David", null); + + Assert.Single(testSink.Writes); + var writeContext = testSink.Writes.First(); + var actualLogValues = Assert.IsAssignableFrom>>(writeContext.State); + Assert.Equal("Hello David. How are you David", actualLogValues.ToString()); + } + + [Fact] + public void LoggerMessage_Define_WithAllDuplicatePlaceholders_DifferentCasing() + { + var testSink = new TestSink(); + var testLogger = new TestLogger("testlogger", testSink, enabled: true); + var action = LoggerMessage.Define(LogLevel.Information, new EventId(0, "LogSomething"), "{Name} {NAME} {name}"); + + action(testLogger, "David", null); + + Assert.Single(testSink.Writes); + var writeContext = testSink.Writes.First(); + var actualLogValues = Assert.IsAssignableFrom>>(writeContext.State); + Assert.Equal("David David David", actualLogValues.ToString()); + } } }