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

Add support for primary constructors in LoggerMessageGenerator #101660

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -15,6 +15,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Loggin
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Generators.Roslyn4.4", "gen\Microsoft.Extensions.Logging.Generators.Roslyn4.4.csproj", "{BF948816-45E1-4F0F-985A-0B4DB4D3BF40}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Generators.Roslyn4.8", "gen\Microsoft.Extensions.Logging.Generators.Roslyn4.8.csproj", "{40FA38ED-D2C9-4608-B85E-F731655C1864}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Abstractions", "ref\Microsoft.Extensions.Logging.Abstractions.csproj", "{7F536552-0E2A-4642-B7CF-863727C2F9CD}"
kimsey0 marked this conversation as resolved.
Show resolved Hide resolved
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Abstractions", "src\Microsoft.Extensions.Logging.Abstractions.csproj", "{75C579F7-F20B-41F1-8CAF-641DE7ADA4EE}"
Expand All @@ -23,6 +25,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Loggin
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests", "tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj", "{1CB869A7-2EEC-4A53-9C33-DF9E0C75825B}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Generators.Roslyn4.8.Tests", "tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn4.8.Tests.csproj", "{D6167506-0671-46A3-94E5-7A98032DCEC6}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "LibraryImportGenerator", "..\System.Runtime.InteropServices\gen\LibraryImportGenerator\LibraryImportGenerator.csproj", "{852D4E16-58C3-47C2-A6BC-A5B12B37209F}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Interop.SourceGeneration", "..\System.Runtime.InteropServices\gen\Microsoft.Interop.SourceGeneration\Microsoft.Interop.SourceGeneration.csproj", "{6645D0C4-83D1-4426-B9CD-67096CB7A60F}"
Expand Down Expand Up @@ -135,6 +139,14 @@ Global
{F3186815-B9A5-455F-B0DF-E39D4235C24F}.Debug|Any CPU.Build.0 = Debug|Any CPU
{F3186815-B9A5-455F-B0DF-E39D4235C24F}.Release|Any CPU.ActiveCfg = Release|Any CPU
{F3186815-B9A5-455F-B0DF-E39D4235C24F}.Release|Any CPU.Build.0 = Release|Any CPU
{40FA38ED-D2C9-4608-B85E-F731655C1864}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{40FA38ED-D2C9-4608-B85E-F731655C1864}.Debug|Any CPU.Build.0 = Debug|Any CPU
{40FA38ED-D2C9-4608-B85E-F731655C1864}.Release|Any CPU.ActiveCfg = Release|Any CPU
{40FA38ED-D2C9-4608-B85E-F731655C1864}.Release|Any CPU.Build.0 = Release|Any CPU
{D6167506-0671-46A3-94E5-7A98032DCEC6}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{D6167506-0671-46A3-94E5-7A98032DCEC6}.Debug|Any CPU.Build.0 = Debug|Any CPU
{D6167506-0671-46A3-94E5-7A98032DCEC6}.Release|Any CPU.ActiveCfg = Release|Any CPU
{D6167506-0671-46A3-94E5-7A98032DCEC6}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand Down Expand Up @@ -162,6 +174,8 @@ Global
{8215F79E-510B-4CA1-B775-50C47BB58360} = {58760833-B4F5-429D-9ABD-15FDF83E25CD}
{F3186815-B9A5-455F-B0DF-E39D4235C24F} = {14DFA192-3C7E-4F10-B5FD-3953BC82A6B1}
{14DFA192-3C7E-4F10-B5FD-3953BC82A6B1} = {58760833-B4F5-429D-9ABD-15FDF83E25CD}
{40FA38ED-D2C9-4608-B85E-F731655C1864} = {03F31CEE-D63E-4E7F-949F-139B33DC3385}
{D6167506-0671-46A3-94E5-7A98032DCEC6} = {4DE63935-DCA9-4D63-9C1F-AAE79C89CA8B}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {450DA749-CBDC-4BDC-950F-8A491CF59D49}
Expand Down
Expand Up @@ -632,6 +632,30 @@ private static string GenerateClassName(TypeDeclarationSyntax typeDeclaration)

bool onMostDerivedType = true;

#if ROSLYN4_8_OR_GREATER
kimsey0 marked this conversation as resolved.
Show resolved Hide resolved
IEnumerable<IMethodSymbol> primaryConstructors = classType.InstanceConstructors
.Where(ic => ic.DeclaringSyntaxReferences
.Any(ds => ds.GetSyntax() is ClassDeclarationSyntax));

foreach (IMethodSymbol primaryConstructor in primaryConstructors)
Copy link
Member

Choose a reason for hiding this comment

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

So, this in itself is a great example of why I said you're not going to get lookup right if you do it by yourself. Consider this example; I've created a protected field in the base type with a wider type than the logger parameter in the derived type, and that protected member shadows the primary constructor parameter. Another similar example is this, where I've shadowed the primary constructor parameter with a field directly in the type itself. I'm not opposed to the idea of adding a new LookupSymbols API here to allow SG authors to say "I need to know what names are available when I implement the body of this type", but it's also important to note that available names are by nature impacted by the usings in the file. Let me talk with @AlekseyTs and see if we can come up with any ideas that avoid you having to try and get this correct.

Copy link
Member

Choose a reason for hiding this comment

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

You should also test scenarios like this:

class Test(ILogger logger)
{
    private readonly ILogger _logger = logger;
}

and ensure that you're using the right logger for that scenario.

Copy link
Member

Choose a reason for hiding this comment

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

@333fred is it possible to lookup the fields first and then if didn't find any try to look up the primary constructors?

Also, for detecting the primary constructor, should check constructor.Body == null && constructor.ExpressionBody == null?

Copy link
Member

Choose a reason for hiding this comment

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

private readonly ILogger _logger = logger;

good test as we don't support private fields.

Copy link
Member

Choose a reason for hiding this comment

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

@333fred is it possible to lookup the fields first and then if didn't find any try to look up the primary constructors?

Sure, that would be possible. Talking with Aleksey, that's likely the approach you want to take in general. However, I'm going to strongly recommend that you explicitly consider these cases and lay out, in comments and documentation, the exact rules you follow. IE, what happens when a primary constructor parameter is shadowed? What happens when a base type has a protected member that is named differently than the parameter (ie, has an _)? This will make it easier to verify the behavior is what you're expecting, and to handle bugs as by-design or not.

Also, for detecting the primary constructor, should check constructor.Body == null && constructor.ExpressionBody == null?

You only need the check that kimsey0 put in already. It's important to note that you can only detect primary constructors from source, you cannot detect them from metadata.

good test as we don't support private fields.

Wait, what? I'll reiterate that I think you need spell out exactly what the rules are, in comments above the code, so that you can verify the code follows them. I thought I might understand the rules, but clearly I do not.

Copy link
Author

Choose a reason for hiding this comment

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

(I posted the comment above before seeing 333fred's new comment at the same time.)

I'm happy to discuss, document, and test the exact rules this source generator should follow. Should that discussion happen in this pull request or perhaps in #91121? Your comments above expose some edge cases, but there are probably more that I may not think of.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, what? I'll reiterate that I think you need spell out exactly what the rules are, in comments above the code, so that you can verify the code follows them. I thought I might understand the rules, but clearly I do not.

I am wrong here, looking more seeing we can support private fields https://learn.microsoft.com/en-us/dotnet/core/extensions/logger-message-generator#basic-usage this is because we generate the code inside the same class and LoggerMessage is not static. Sorry for confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Should that discussion happen in this pull request or perhaps in #91121?

Either way is fine. we can continue discussing in the PR and then put the conclusion in the issue. I am fine to move the discussion to the issue too.

Your comments above expose some edge cases, but there are probably more that I may not think of.

I think when applying the suggestion which looking up the fields first will address most these corner cases. Of course we'll need to document the behavior.

Copy link
Author

@kimsey0 kimsey0 May 23, 2024

Choose a reason for hiding this comment

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

I have added the three test cases suggested by @333fred, as well as one that shadows the primary constructor parameter with an ILogger field:

class Test(ILogger logger)
{
    private readonly ILogger logger = logger;
}

I've changed the code to support these cases by:

  1. First looking for an ILogger field.
  2. If none is found, look for an ILogger primary constructor parameter.
  3. Ignore any primary constructor parameters which are shadowed by a field.

In practice, this means that:

  1. Fields are prioritized over primary constructor parameters.
  2. Shadowed primary constructor parameters are treated as if they don't exist.

I think this is a reasonable behavior, though one could argue that it would make sense to have a specific warning when a ILogger primary constructor parameter is found that is shadowed by a non-ILogger field.

How should this behavior be documented? Just on the learn page?

Copy link
Member

Choose a reason for hiding this comment

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

though one could argue that it would make sense to have a specific warning when a ILogger primary constructor parameter is found that is shadowed by a non-ILogger field.

Yes, we can have a diagnostic for that. Would it be better to have it as Info instead of warning?

How should this behavior be documented? Just on the learn page?

Right, this is the right place to document the behavior. We'll need to update this doc anyway for primary constructor usage with a sample.

{
foreach (IParameterSymbol parameter in primaryConstructor.Parameters)
{
if (IsBaseOrIdentity(parameter.Type, loggerSymbol))
{
if (loggerField == null)
{
loggerField = parameter.Name;
}
else
{
return (null, true);
}
}
}
}
#endif

while (classType is { SpecialType: not SpecialType.System_Object })
{
foreach (IFieldSymbol fs in classType.GetMembers().OfType<IFieldSymbol>())
Expand Down
Expand Up @@ -45,7 +45,7 @@ private static void Execute(Compilation compilation, ImmutableArray<ClassDeclara
return;
}

IEnumerable<ClassDeclarationSyntax> distinctClasses = classes.Distinct();
ImmutableHashSet<ClassDeclarationSyntax> distinctClasses = classes.ToImmutableHashSet();

var p = new Parser(compilation, context.ReportDiagnostic, context.CancellationToken);
IReadOnlyList<LoggerClass> logClasses = p.GetLogClasses(distinctClasses);
Expand Down
@@ -0,0 +1,20 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Member

@tarekgh tarekgh Apr 30, 2024

Choose a reason for hiding this comment

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

I don't think we need to create a new library for that version.

Copy link
Member

@tarekgh tarekgh Apr 30, 2024

Choose a reason for hiding this comment

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

@ericstj do you see a problem if we upgrade

to:

<RoslynApiVersion>$(MicrosoftCodeAnalysisVersion_4_8)</RoslynApiVersion>

Copy link
Member

Choose a reason for hiding this comment

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

If we update to that Roslyn version it means we remove the one that targets 4.4.

Updating for the inbox generators in 9.0 could be OK (if we can guarantee that 4.8 is present in all SDK/VS versions where 9.0 is supported).

Updating for the nuget package generators is ok so long as we don't take away any features / perf from folks using roslyn 4.4. Of course that's moot if noone is using 4.4-4.7.

Somewhere there is a matrix of compiler versions that are supported that could help answer this question.

Copy link
Author

Choose a reason for hiding this comment

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

Looks like it doesn't matter in the end. I don't actually need to have code in the generator that's conditionally compiled for Roslyn 4.8. It's only in the test project I need to be sure primary constructors are supported. @tarekgh, is this approach of just upgrading the RoslynApiVersion in Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj OK?

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a new Microsoft.Extensions.Logging.Generators.Roslyn4.8.Tests.csproj that target Roslyn 4.8?


<PropertyGroup>
<AnalyzerRoslynVersion>4.8</AnalyzerRoslynVersion>
<RoslynApiVersion>$(MicrosoftCodeAnalysisVersion_4_8)</RoslynApiVersion>
<DefineConstants>$(DefineConstants);ROSLYN4_0_OR_GREATER;ROSLYN4_4_OR_GREATER;ROSLYN4_8_OR_GREATER</DefineConstants>
</PropertyGroup>

<Import Project="Microsoft.Extensions.Logging.Generators.targets" />

<ItemGroup>
<Compile Include="$(CoreLibSharedDir)System\Collections\Generic\ValueListBuilder.cs" Link="Production\ValueListBuilder.cs" />
<Compile Include="$(CoreLibSharedDir)System\Collections\Generic\ValueListBuilder.Pop.cs" Link="Production\ValueListBuilder.Pop.cs" />
</ItemGroup>

<ItemGroup>
<Compile Remove="LoggerMessageGenerator.Roslyn3.11.cs" />
</ItemGroup>

</Project>
@@ -0,0 +1,21 @@
// <auto-generated/>
#nullable enable

namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
{
partial class TestWithLoggerFromPrimaryConstructor
{
[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "%VERSION%")]
private static readonly global::System.Action<global::Microsoft.Extensions.Logging.ILogger, global::System.Exception?> __M0Callback =
global::Microsoft.Extensions.Logging.LoggerMessage.Define(global::Microsoft.Extensions.Logging.LogLevel.Debug, new global::Microsoft.Extensions.Logging.EventId(0, nameof(M0)), "M0", new global::Microsoft.Extensions.Logging.LogDefineOptions() { SkipEnabledCheck = true });

[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "%VERSION%")]
public partial void M0()
{
if (logger.IsEnabled(global::Microsoft.Extensions.Logging.LogLevel.Debug))
{
__M0Callback(logger, null);
}
}
}
}
Expand Up @@ -26,7 +26,7 @@ public void FindsLoggerFieldInBaseClass()
}

[Fact]
public void FindsLoggerFieldInAnotherParialClass()
public void FindsLoggerFieldInAnotherPartialClass()
{
var logger = new MockLogger();

Expand All @@ -36,6 +36,30 @@ public void FindsLoggerFieldInAnotherParialClass()
Assert.Equal("Test.", logger.LastFormattedString);
}

#if ROSLYN4_8_OR_GREATER
[Fact]
public void FindsLoggerInPrimaryConstructorParameter()
{
var logger = new MockLogger();

logger.Reset();

new ClassWithPrimaryConstructor(logger).Test();
Assert.Equal("Test.", logger.LastFormattedString);
}

[Fact]
public void FindsLoggerInPrimaryConstructorParameterInDifferentPartialDeclaration()
{
var logger = new MockLogger();

logger.Reset();

new ClassWithPrimaryConstructorInDifferentPartialDeclaration(logger).Test();
Assert.Equal("Test.", logger.LastFormattedString);
}
#endif

[Fact]
public void BasicTests()
{
Expand Down
Expand Up @@ -187,6 +187,23 @@ public sealed class BarAttribute : Attribute { }
await VerifyAgainstBaselineUsingFile("TestWithNestedClassWithGenericTypesWithAttributes.generated.txt", testSourceCode);
}

#if ROSLYN4_8_OR_GREATER
[Fact]
public async Task TestBaseline_TestWithLoggerFromPrimaryConstructor_Success()
{
string testSourceCode = @"
namespace Microsoft.Extensions.Logging.Generators.Tests.TestClasses
{
internal partial class TestWithLoggerFromPrimaryConstructor(ILogger logger)
{
[LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M0"")]
public partial void M0();
}
}";
await VerifyAgainstBaselineUsingFile("TestWithLoggerFromPrimaryConstructor.generated.txt", testSourceCode);
}
#endif

[Fact]
public void GenericTypeParameterAttributesAreRetained()
{
Expand Down
Expand Up @@ -417,6 +417,62 @@ internal partial class Logger
}
#endif

[Fact]
public async Task FieldOnOtherPartialDeclarationOK()
{
IReadOnlyList<Diagnostic> diagnostics = await RunGenerator(@"
partial class C
{
private ILogger _logger;

public C(ILogger logger)
{
_logger = logger;
}
}

partial class C
{
[LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M1"")]
public partial void M1();
}
");

Assert.Empty(diagnostics);
}

#if ROSLYN4_8_OR_GREATER
[Fact]
public async Task PrimaryConstructorOK()
{
IReadOnlyList<Diagnostic> diagnostics = await RunGenerator(@"
partial class C(ILogger logger)
{
[LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M1"")]
public partial void M1();
}
");

Assert.Empty(diagnostics);
}

[Fact]
public async Task PrimaryConstructorOnOtherPartialDeclarationOK()
{
IReadOnlyList<Diagnostic> diagnostics = await RunGenerator(@"
partial class C(ILogger logger);

partial class C
{
[LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M1"")]
public partial void M1();
}
");

Assert.Empty(diagnostics);
}
#endif

[Theory]
[InlineData("false")]
[InlineData("true")]
Expand Down
@@ -0,0 +1,22 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<RoslynApiVersion>$(MicrosoftCodeAnalysisVersion_4_8)</RoslynApiVersion>
<DefineConstants>$(DefineConstants);ROSLYN4_0_OR_GREATER;ROSLYN4_8_OR_GREATER</DefineConstants>
<IsHighAotMemoryUsageTest>true</IsHighAotMemoryUsageTest>
<EmccLinkOptimizationFlag Condition="'$(ContinuousIntegrationBuild)' == 'true'">-O1</EmccLinkOptimizationFlag>
<!-- this Roslyn version brings in NS1.x dependencies -->
<FlagNetStandard1XDependencies>false</FlagNetStandard1XDependencies>
</PropertyGroup>

<ItemGroup>
<HighAotMemoryUsageAssembly Include="Microsoft.CodeAnalysis.CSharp.dll" />
</ItemGroup>

<Import Project="Microsoft.Extensions.Logging.Generators.targets" />

<ItemGroup>
<ProjectReference Include="..\..\gen\Microsoft.Extensions.Logging.Generators.Roslyn4.8.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="true" />
</ItemGroup>

</Project>
Expand Up @@ -57,6 +57,22 @@ public partial class PartialClassWithLoggerField
public partial void Test();
}

#if ROSLYN4_8_OR_GREATER
public partial class ClassWithPrimaryConstructor(ILogger logger)
{
[LoggerMessage(0, LogLevel.Debug, "Test.")]
public partial void Test();
}

public partial class ClassWithPrimaryConstructorInDifferentPartialDeclaration(ILogger logger);

public partial class ClassWithPrimaryConstructorInDifferentPartialDeclaration
{
[LoggerMessage(0, LogLevel.Debug, "Test.")]
public partial void Test();
}
#endif

// Used to test use outside of a namespace
internal static partial class NoNamespace
{
Expand Down