Skip to content
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

Bring parity to dotnet extensions logging generator behavior #5370

Merged
merged 24 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c31c017
support @ symbols in logging message templates, and align tempalte ex…
dariusclay Aug 17, 2024
0ea09fe
fix correctness issue
dariusclay Aug 17, 2024
5352563
Handle lookup of ILogger member respecting backwards compatibility wi…
dariusclay Aug 17, 2024
8f2ae4c
undo naming change
dariusclay Aug 17, 2024
c21dfd8
undo testing code
dariusclay Aug 17, 2024
fc6c9bd
remove unnecessary project property
dariusclay Aug 17, 2024
522cb95
clean up spans in tests
dariusclay Aug 17, 2024
4b20465
rename parameter back to original
dariusclay Aug 17, 2024
ed3758e
remove using
dariusclay Aug 17, 2024
4fa2ba2
update generated test classes
dariusclay Aug 17, 2024
ddd2993
add test classes for primary constructors
dariusclay Aug 17, 2024
3949074
add generated tests for primary constructors
dariusclay Aug 17, 2024
cfc9418
need roslyn 4.8 to support primary constructors properly
dariusclay Aug 27, 2024
8cca66a
Merge branch 'main' into 5332-logging-generator-behavior-parity
dariusclay Aug 27, 2024
765fadd
Bump everything to Roslyn 4.8
dariusclay Aug 27, 2024
d4d13a0
Suppress RS1035 until violations can be addressed
dariusclay Aug 27, 2024
b6568cc
Supressing RS1035 in the source instead of for all generator projects
dariusclay Aug 28, 2024
733d695
update list to include LOGGEN037 and LOGGEN038
dariusclay Aug 28, 2024
ab58b2c
Use constants in place of magic numbers
dariusclay Aug 28, 2024
7b8e4c6
addressing PR comments
dariusclay Aug 28, 2024
ad8cb22
remove extraneous using
dariusclay Aug 28, 2024
1d87e1e
increase code coverage
dariusclay Aug 28, 2024
2780154
adding more tests
dariusclay Aug 29, 2024
30b4787
Merge branch 'main' into 5332-logging-generator-behavior-parity
dariusclay Aug 30, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,19 @@
Versions below this comment are not managed by automation and can be changed as needed.
-->
<PropertyGroup Label="Manual">
<!-- Compatibility with VS 17.4/.NET SDK 7.0.1xx -->
RussKie marked this conversation as resolved.
Show resolved Hide resolved
<MicrosoftCodeAnalysisVersion_4_4>4.4.0</MicrosoftCodeAnalysisVersion_4_4>
<!-- Compatibility with VS 17.8/.NET SDK 8.0.1xx -->
<MicrosoftCodeAnalysisVersion_4_8>4.8.0</MicrosoftCodeAnalysisVersion_4_8>
dariusclay marked this conversation as resolved.
Show resolved Hide resolved
<!-- Compatibility with the latest Visual Studio Preview release -->
<!--
The exact version is always a moving target. This version should never go ahead of the version of Roslyn that is included in the most recent
public Visual Studio preview version. If it were to go ahead, then any components depending on this version would not work in Visual Studio
and would cause a major regression for any local development that depends on those components contributing to the build.
This version must also not go ahead of the most recently release .NET SDK version, as that would break the source-build build.
Source-build builds the product with the most recent previously source-built release. Thankfully, these two requirements line up nicely
such that any version that satisfies the VS version requirement will also satisfy the .NET SDK version requirement because of how we ship.
-->
<MicrosoftCodeAnalysisVersion_LatestVS>4.8.0</MicrosoftCodeAnalysisVersion_LatestVS>
dariusclay marked this conversation as resolved.
Show resolved Hide resolved
</PropertyGroup>
</Project>
8 changes: 4 additions & 4 deletions eng/packages/General.props
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
<PackageVersion Include="Microsoft.Bcl.HashCode" Version="1.1.1" />
<PackageVersion Include="Microsoft.Bcl.TimeProvider" Version="$(MicrosoftBclTimeProviderVersion)" />
<PackageVersion Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.4" />
<PackageVersion Include="Microsoft.CodeAnalysis.Common" Version="4.4.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.4.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.4.0" />
<PackageVersion Include="Microsoft.CodeAnalysis" Version="4.4.0" />
<PackageVersion Include="Microsoft.CodeAnalysis.Common" Version="$(MicrosoftCodeAnalysisVersion_LatestVS)" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="$(MicrosoftCodeAnalysisVersion_LatestVS)" />
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="$(MicrosoftCodeAnalysisVersion_LatestVS)" />
<PackageVersion Include="Microsoft.CodeAnalysis" Version="$(MicrosoftCodeAnalysisVersion_LatestVS)" />
<PackageVersion Include="Microsoft.Extensions.Caching.Abstractions" Version="$(MicrosoftExtensionsCachingAbstractionsVersion)" />
<PackageVersion Include="Microsoft.Extensions.Configuration.Abstractions" Version="$(MicrosoftExtensionsConfigurationAbstractionsVersion)" />
<PackageVersion Include="Microsoft.Extensions.Configuration.Binder" Version="$(MicrosoftExtensionsConfigurationBinderVersion)" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

<PropertyGroup>
<AnalyzerLanguage>cs</AnalyzerLanguage>
<EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
dariusclay marked this conversation as resolved.
Show resolved Hide resolved
<InjectIsExternalInitOnLegacy>true</InjectIsExternalInitOnLegacy>
</PropertyGroup>

Expand Down
13 changes: 13 additions & 0 deletions src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs
Original file line number Diff line number Diff line change
Expand Up @@ -245,4 +245,17 @@ internal sealed class DiagDescriptors : DiagDescriptorsBase
messageFormat: Resources.DefaultToStringMessage,
category: Category,
DiagnosticSeverity.Warning);

public static DiagnosticDescriptor MalformedFormatStrings { get; } = Make(
id: DiagnosticIds.LoggerMessage.LOGGEN037,
title: Resources.MalformedFormatStringsTitle,
messageFormat: Resources.MalformedFormatStringsMessage,
category: Category);

public static DiagnosticDescriptor PrimaryConstructorParameterLoggerHidden { get; } = Make(
id: DiagnosticIds.LoggerMessage.LOGGEN038,
title: Resources.PrimaryConstructorParameterLoggerHiddenTitle,
messageFormat: Resources.PrimaryConstructorParameterLoggerHiddenMessage,
category: Category,
DiagnosticSeverity.Info);
}
262 changes: 149 additions & 113 deletions src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs

Large diffs are not rendered by default.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -339,4 +339,16 @@
<data name="DefaultToStringTitle" xml:space="preserve">
<value>A value being logged doesn't have an effective way to be converted into a string</value>
</data>
<data name="MalformedFormatStringsMessage" xml:space="preserve">
<value>Logging method '{0}' contains malformed format strings</value>
</data>
<data name="MalformedFormatStringsTitle" xml:space="preserve">
<value>Logging method contains malformed format strings</value>
</data>
<data name="PrimaryConstructorParameterLoggerHiddenTitle" xml:space="preserve">
<value>Primary constructor parameter of type Microsoft.Extensions.Logging.ILogger is hidden by a field</value>
</data>
<data name="PrimaryConstructorParameterLoggerHiddenMessage" xml:space="preserve">
<value>Class '{0}' has a primary constructor parameter of type Microsoft.Extensions.Logging.ILogger that is hidden by a field in the class or a base class, preventing its use</value>
</data>
</root>
141 changes: 98 additions & 43 deletions src/Generators/Microsoft.Gen.Logging/Parsing/TemplateProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

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

namespace Microsoft.Gen.Logging.Parsing;
Expand All @@ -14,32 +15,60 @@ internal static class TemplateProcessor
/// <summary>
/// Finds the template arguments contained in the message string.
/// </summary>
internal static void ExtractTemplates(string? message, List<string> templates)
internal static bool ExtractTemplates(string? message, List<string> templates)
{
if (string.IsNullOrEmpty(message))
{
return;
return true;
}

var scanIndex = 0;
var endIndex = message!.Length;

bool success = true;
while (scanIndex < endIndex)
{
var openBraceIndex = FindBraceIndex(message, '{', scanIndex, endIndex);
var closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex, endIndex);

if (closeBraceIndex == endIndex)
#pragma warning disable S109 // Magic numbers should not be used
if (openBraceIndex == -2)
{
// found '}' instead of '{'
success = false;
break;
}
else if (openBraceIndex == -1)
{
// scanned the string and didn't find any remaining '{' or '}'
break;
}
#pragma warning restore S109 // Magic numbers should not be used

int closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex + 1, endIndex);

if (closeBraceIndex <= -1)
{
return;
success = false;
break;
}

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

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

if (string.IsNullOrWhiteSpace(templateName))
{
// braces with no named argument, such as "{}" and "{ }"
success = false;
break;
}

templates.Add(templateName);

scanIndex = closeBraceIndex + 1;
}

return success;
}

/// <summary>
Expand All @@ -59,9 +88,23 @@ internal static void ExtractTemplates(string? message, List<string> templates)
while (scanIndex < endIndex)
{
var openBraceIndex = FindBraceIndex(message, '{', scanIndex, endIndex);
var closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex, endIndex);

if (closeBraceIndex == endIndex)
#pragma warning disable S109 // Magic numbers should not be used
if (openBraceIndex == -2)
{
// found '}' instead of '{'
break;
}
else if (openBraceIndex == -1)
{
// scanned the string and didn't find any remaining '{' or '}'
break;
}
#pragma warning restore S109 // Magic numbers should not be used

var closeBraceIndex = FindBraceIndex(message, '}', openBraceIndex + 1, endIndex);

if (closeBraceIndex <= -1)
{
break;
}
Expand All @@ -86,56 +129,68 @@ internal static void ExtractTemplates(string? message, List<string> templates)
internal static int FindIndexOfAny(string message, char[] chars, int startIndex, int endIndex)
{
var findIndex = message.IndexOfAny(chars, startIndex, endIndex - startIndex);
return findIndex == -1
? endIndex
: findIndex;
return findIndex == -1 ? endIndex : findIndex;
}

private static int FindBraceIndex(string message, char brace, int startIndex, int endIndex)
/// <summary>
/// Searches for the next brace index in the message.
/// </summary>
/// <remarks> The search skips any sequences of {{ or }}.</remarks>
/// <example>{{prefix{{{Argument}}}suffix}}.</example>
/// <returns>The zero-based index position of the first occurrence of the searched brace; -1 if the searched brace was not found; -2 if the wrong brace was found.</returns>
dariusclay marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// <returns>The zero-based index position of the first occurrence of the searched brace; -1 if the searched brace was not found; -2 if the wrong brace was found.</returns>
/// <returns>The zero-based index position of the first occurrence of the searched brace; <see cref="NoBracesFound"/> if the searched brace was not found; <see cref="WrongBraceFound"/> if the wrong brace was found.</returns>

private static int FindBraceIndex(string message, char searchedBrace, int startIndex, int endIndex)
{
// Example: {{prefix{{{Argument}}}suffix}}.
var braceIndex = endIndex;
var scanIndex = startIndex;
var braceOccurrenceCount = 0;
Debug.Assert(searchedBrace is '{' or '}', "Searched brace must be { or }");

int braceIndex = -1;
int scanIndex = startIndex;

while (scanIndex < endIndex)
{
if (braceOccurrenceCount > 0 && message[scanIndex] != brace)
char current = message[scanIndex];

if (current is '{' or '}')
{
#pragma warning disable S109 // Magic numbers should not be used
if (braceOccurrenceCount % 2 == 0)
#pragma warning restore S109 // Magic numbers should not be used
char currentBrace = current;

int scanIndexBeforeSkip = scanIndex;
while (current == currentBrace && ++scanIndex < endIndex)
{
// Even number of '{' or '}' found. Proceed search with next occurrence of '{' or '}'.
braceOccurrenceCount = 0;
braceIndex = endIndex;
current = message[scanIndex];
}
else

int bracesCount = scanIndex - scanIndexBeforeSkip;
#pragma warning disable S109 // Magic numbers should not be used
// if it is an even number of braces, just skip them, otherwise, we found an unescaped brace
if (bracesCount % 2 != 0)
{
// An unescaped '{' or '}' found.
if (currentBrace == searchedBrace)
{
if (currentBrace == '{')
{
// For '{' pick the last occurrence.
braceIndex = scanIndex - 1;
}
else
{
// For '}' pick the first occurrence.
braceIndex = scanIndexBeforeSkip;
}
}
else
{
// wrong brace found
braceIndex = -2;
}
#pragma warning restore S109 // Magic numbers should not be used

break;
}
}
else if (message[scanIndex] == brace)
else
{
if (brace == '}')
{
if (braceOccurrenceCount == 0)
{
// For '}' pick the first occurrence.
braceIndex = scanIndex;
}
}
else
{
// For '{' pick the last occurrence.
braceIndex = scanIndex;
}

braceOccurrenceCount++;
scanIndex++;
}

scanIndex++;
}

return braceIndex;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

<PropertyGroup>
<AnalyzerLanguage>cs</AnalyzerLanguage>
<EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
<InjectIsExternalInitOnLegacy>true</InjectIsExternalInitOnLegacy>
</PropertyGroup>

Expand Down
2 changes: 2 additions & 0 deletions src/Shared/DiagnosticIds/DiagnosticIds.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ internal static class LoggerMessage
internal const string LOGGEN034 = nameof(LOGGEN034);
internal const string LOGGEN035 = nameof(LOGGEN035);
internal const string LOGGEN036 = nameof(LOGGEN036);
internal const string LOGGEN037 = nameof(LOGGEN037);
internal const string LOGGEN038 = nameof(LOGGEN038);
dariusclay marked this conversation as resolved.
Show resolved Hide resolved
}

internal static class Metrics
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,19 @@ public void AtSymbolsTest()
@class = 42,
};

collector.Clear();
AtSymbolsTestExtensions.UseAtSymbol3(logger, LogLevel.Debug, "42", 42);
Assert.Equal("42", collector.LatestRecord.StructuredState!.GetValue("@myevent2"));
Assert.Equal("42", collector.LatestRecord.StructuredState!.GetValue("otherevent"));
Assert.Equal("UseAtSymbol3, 42 42", collector.LatestRecord.Message);

collector.Clear();
AtSymbolsTestExtensions.UseAtSymbol4(logger, LogLevel.Debug, "42", 42, new ArgumentException("Foo"));
Assert.Equal("42", collector.LatestRecord.StructuredState!.GetValue("@myevent3"));
Assert.Equal("42", collector.LatestRecord.StructuredState!.GetValue("otherevent"));
Assert.Equal("UseAtSymbol4 with error, 42 42", collector.LatestRecord.Message);
Assert.NotNull(collector.LatestRecord.Exception);

collector.Clear();
AtSymbolsTestExtensions.M3(logger, LogLevel.Debug, o);
Assert.Equal("42", collector.LatestRecord.StructuredState!.GetValue("event.class"));
Expand All @@ -761,6 +774,11 @@ public void AtSymbolsTest()
AtSymbolsTestExtensions.M6(logger, "42");
Assert.Equal("42", collector.LatestRecord.StructuredState!.GetValue("class"));
Assert.Equal("M6 class 42", collector.LatestRecord.Message);

collector.Clear();
AtSymbolsTestExtensions.M7(logger, "42");
Assert.Equal("42", collector.LatestRecord.StructuredState!.GetValue("@param"));
Assert.Equal("M7 param 42", collector.LatestRecord.Message);
}

[Fact]
Expand Down
Loading
Loading