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.

Sign up for GitHub

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

Jump to bottom

Add support for primary constructors in LoggerMessageGenerator #101660

Merged
merged 9 commits into from
May 28, 2024
Merged

Add support for primary constructors in LoggerMessageGenerator #101660

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d7497fc
Add support for primary constructors in LoggerMessageGenerator
kimsey0 Apr 28, 2024
15643ec
Get the primary constructor parameters types from the constructor sym…
kimsey0 Apr 30, 2024
9fc6ce4
Prioritize fields over primary constructor parameters and ignore shad…
kimsey0 May 23, 2024
2d6748c
Make checking for primary constructors non-conditional on Roslyn vers…
kimsey0 May 23, 2024
6a8b473
Reintroduce Roslyn 4.8 test project
kimsey0 May 24, 2024
6cf2b2f
Add info-level diagnostic for logger primary constructor parameters t…
kimsey0 May 26, 2024
69a5c00
Update list of diagnostics with new logging message generator diagnostic
kimsey0 May 27, 2024
e029d54
Only add non-logger field names to set of shadowed names
kimsey0 May 27, 2024
fd480f2
Add comment explaining the use of the set of shadowed names with an e…
kimsey0 May 27, 2024
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
14 changes: 14 additions & 0 deletions ...s/Microsoft.Extensions.Logging.Abstractions/Microsoft.Extensions.Logging.Abstractions.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
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
24 changes: 24 additions & 0 deletions src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. If both the behavior described above as well as the code implementing it is acceptable to all parties, I think this is ready for review again.

Copy link
Member

@tarekgh tarekgh May 24, 2024 •
edited

Choose a reason for hiding this comment

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

I'll try to review it when I get a chance. PR is not a draft anymore. Thanks for all the help you have provided here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Thanks a lot!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to commit and push the info diagnostic, but just did that. I don't know if there are any guidelines for how exactly these diagnostic messages should be written or how the translation flow goes? (We can also just revert this. I think it's useful, but not necessary.)

Copy link
Member

Choose a reason for hiding this comment

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

You don't have to worry about the translation of the added messages. I am seeing the added diagnostic is useful to catch the concerned case.

{
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
2 changes: 1 addition & 1 deletion ...braries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Roslyn4.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
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
20 changes: 20 additions & 0 deletions ...ensions.Logging.Abstractions/gen/Microsoft.Extensions.Logging.Generators.Roslyn4.8.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<Project Sdk="Microsoft.NET.Sdk">
kimsey0 marked this conversation as resolved.
Show resolved Hide resolved

<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>
21 changes: 21 additions & 0 deletions ...ons.Logging.Generators.Tests/Baselines/TestWithLoggerFromPrimaryConstructor.generated.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
}
26 changes: 25 additions & 1 deletion ...ns/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratedCodeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
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
17 changes: 17 additions & 0 deletions ...tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
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
56 changes: 56 additions & 0 deletions .../tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
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
22 changes: 22 additions & 0 deletions ...s.Logging.Generators.Tests/Microsoft.Extensions.Logging.Generators.Roslyn4.8.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>
16 changes: 16 additions & 0 deletions ...ons/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
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