From 18863043451d369d97ef04f2a6d9967315583f69 Mon Sep 17 00:00:00 2001 From: Steve Dunn Date: Sun, 26 Jun 2022 19:49:05 +0100 Subject: [PATCH 1/5] First pass --- .../gen/LoggerMessageGenerator.Parser.cs | 35 +++++++++---------- .../LoggerMessageGeneratedCodeTests.cs | 22 ++++++++++++ .../TestClasses/MiscTestExtensions.cs | 29 +++++++++++++++ 3 files changed, 68 insertions(+), 18 deletions(-) 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 504008c4c367dd..944a2e4b0841ad 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs @@ -534,29 +534,28 @@ static bool IsAllowedKind(SyntaxKind kind) => private (string? loggerField, bool multipleLoggerFields) FindLoggerField(SemanticModel sm, TypeDeclarationSyntax classDec, ITypeSymbol loggerSymbol) { + List fields = new(); + + INamedTypeSymbol? classType = sm.GetDeclaredSymbol(classDec, _cancellationToken); + while (classType is { } ct && ct.SpecialType != SpecialType.System_Object) + { + fields.AddRange(classType.GetMembers().OfType()); + classType = classType.BaseType; + } + string? loggerField = null; - foreach (MemberDeclarationSyntax m in classDec.Members) + foreach (IFieldSymbol v in fields) { - if (m is FieldDeclarationSyntax fds) + if (IsBaseOrIdentity(v.Type, loggerSymbol)) { - foreach (VariableDeclaratorSyntax v in fds.Declaration.Variables) + if (loggerField == null) { - var fs = sm.GetDeclaredSymbol(v, _cancellationToken) as IFieldSymbol; - if (fs != null) - { - if (IsBaseOrIdentity(fs.Type, loggerSymbol)) - { - if (loggerField == null) - { - loggerField = v.Identifier.Text; - } - else - { - return (null, true); - } - } - } + loggerField = v.Name; + } + else + { + return (null, true); } } } diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratedCodeTests.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratedCodeTests.cs index 59c5965fefef36..f3419669a361f1 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratedCodeTests.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratedCodeTests.cs @@ -14,6 +14,28 @@ namespace Microsoft.Extensions.Logging.Generators.Tests { public class LoggerMessageGeneratedCodeTests { + [Fact] + public void FindsLoggerFieldInBaseClass() + { + var logger = new MockLogger(); + + logger.Reset(); + + new DerivedClass(logger).Test(); + Assert.Equal("Test.", logger.LastFormattedString); + } + + [Fact] + public void FindsLoggerFieldInAnotherParialClass() + { + var logger = new MockLogger(); + + logger.Reset(); + + new PartialClassWithLoggerField(logger).Test(); + Assert.Equal("Test.", logger.LastFormattedString); + } + [Fact] public void BasicTests() { diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs index 5abecef3899069..f2bd54b1ce2022 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs @@ -3,6 +3,35 @@ using Microsoft.Extensions.Logging; +public class BaseClass +{ + protected ILogger _logger; + + public BaseClass(ILogger logger) => _logger = logger; +} + +public partial class DerivedClass : BaseClass +{ + public DerivedClass(ILogger logger) : base(logger) { } + + [LoggerMessage(0, LogLevel.Debug, "Test.")] + public partial void Test(); +} + +public partial class PartialClassWithLoggerField +{ + private ILogger _logger; + + public PartialClassWithLoggerField(ILogger logger) => _logger = logger; +} + +public partial class PartialClassWithLoggerField +{ + [LoggerMessage(0, LogLevel.Debug, "Test.")] + public partial void Test(); +} + + // Used to test use outside of a namespace internal static partial class NoNamespace { From 4d54269e208efbfa5366d26c36b310e30cdfdf40 Mon Sep 17 00:00:00 2001 From: Steve Dunn Date: Mon, 27 Jun 2022 19:57:18 +0100 Subject: [PATCH 2/5] Update src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs Co-authored-by: Sam Harwell --- .../gen/LoggerMessageGenerator.Parser.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 944a2e4b0841ad..26e3ccb9a8347e 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs @@ -537,7 +537,7 @@ static bool IsAllowedKind(SyntaxKind kind) => List fields = new(); INamedTypeSymbol? classType = sm.GetDeclaredSymbol(classDec, _cancellationToken); - while (classType is { } ct && ct.SpecialType != SpecialType.System_Object) + while (classType is { SpecialType: not SpecialType.System_Object }) { fields.AddRange(classType.GetMembers().OfType()); classType = classType.BaseType; From b9c356f768e9bc6f816bd777a761a23e4d3ecc8c Mon Sep 17 00:00:00 2001 From: Steve Dunn Date: Wed, 29 Jun 2022 07:07:44 +0100 Subject: [PATCH 3/5] PR feedback --- .../gen/LoggerMessageGenerator.Parser.cs | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) 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 26e3ccb9a8347e..eb71e9121b4928 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs @@ -534,30 +534,28 @@ static bool IsAllowedKind(SyntaxKind kind) => private (string? loggerField, bool multipleLoggerFields) FindLoggerField(SemanticModel sm, TypeDeclarationSyntax classDec, ITypeSymbol loggerSymbol) { - List fields = new(); + string? loggerField = null; INamedTypeSymbol? classType = sm.GetDeclaredSymbol(classDec, _cancellationToken); - while (classType is { SpecialType: not SpecialType.System_Object }) - { - fields.AddRange(classType.GetMembers().OfType()); - classType = classType.BaseType; - } - string? loggerField = null; - - foreach (IFieldSymbol v in fields) + while (classType is { SpecialType: not SpecialType.System_Object }) { - if (IsBaseOrIdentity(v.Type, loggerSymbol)) + foreach (IFieldSymbol fs in classType.GetMembers().OfType()) { - if (loggerField == null) + if (IsBaseOrIdentity(fs.Type, loggerSymbol)) { - loggerField = v.Name; - } - else - { - return (null, true); + if (loggerField == null) + { + loggerField = fs.Name; + } + else + { + return (null, true); + } } } + + classType = classType.BaseType; } return (loggerField, false); From b772fafcc46e76752b0c8ba187422667e325be08 Mon Sep 17 00:00:00 2001 From: Steve Dunn Date: Tue, 12 Jul 2022 20:19:34 +0100 Subject: [PATCH 4/5] Exclude privates from being considered from derived types when looking for the logger field --- .../gen/LoggerMessageGenerator.Parser.cs | 7 +++++ .../TestClasses/MiscTestExtensions.cs | 26 +++++++++++++++++++ 2 files changed, 33 insertions(+) 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 eb71e9121b4928..cc8dc0b5aaef2a 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs @@ -538,10 +538,16 @@ static bool IsAllowedKind(SyntaxKind kind) => INamedTypeSymbol? classType = sm.GetDeclaredSymbol(classDec, _cancellationToken); + bool onMostDerivedType = true; + while (classType is { SpecialType: not SpecialType.System_Object }) { foreach (IFieldSymbol fs in classType.GetMembers().OfType()) { + if (!onMostDerivedType && fs.DeclaredAccessibility == Accessibility.Private) + { + continue; + } if (IsBaseOrIdentity(fs.Type, loggerSymbol)) { if (loggerField == null) @@ -555,6 +561,7 @@ static bool IsAllowedKind(SyntaxKind kind) => } } + onMostDerivedType = false; classType = classType.BaseType; } diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs index f2bd54b1ce2022..3ad27de317da66 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs @@ -3,6 +3,32 @@ using Microsoft.Extensions.Logging; +// No explicit tests use the following two types, but the fact +// that they are here means we exercise a constraint that we +// exclude private fields of base types. +// If that logic ever changes, then just by having these two classes +// will mean that compilation fails with: +// error SYSLIB1020: Found multiple fields of type Microsoft.Extensions.Logging.ILogger in class DerivedClass_with_private_logger +public class BaseClass_with_private_logger +{ + private ILogger _logger; + + public BaseClass_with_private_logger(ILogger logger) => _logger = logger; +} + +public partial class DerivedClass_with_private_logger : BaseClass_with_private_logger +{ + private ILogger _logger; + + public DerivedClass_with_private_logger(ILogger logger) : base(logger) + { + _logger = logger; + } + + [LoggerMessage(0, LogLevel.Debug, "Test.")] + public partial void Test(); +} + public class BaseClass { protected ILogger _logger; From a5ae268196645390a88b133f06b4b54507ab28f1 Mon Sep 17 00:00:00 2001 From: Steve Dunn Date: Sun, 24 Jul 2022 16:31:51 +0100 Subject: [PATCH 5/5] PR feedback --- .../TestClasses/MiscTestExtensions.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs index 3ad27de317da66..05984d67570bc2 100644 --- a/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MiscTestExtensions.cs @@ -9,18 +9,18 @@ // If that logic ever changes, then just by having these two classes // will mean that compilation fails with: // error SYSLIB1020: Found multiple fields of type Microsoft.Extensions.Logging.ILogger in class DerivedClass_with_private_logger -public class BaseClass_with_private_logger +public class BaseClassWithPrivateLogger { private ILogger _logger; - public BaseClass_with_private_logger(ILogger logger) => _logger = logger; + public BaseClassWithPrivateLogger(ILogger logger) => _logger = logger; } -public partial class DerivedClass_with_private_logger : BaseClass_with_private_logger +public partial class DerivedClassWithPrivateLogger : BaseClassWithPrivateLogger { private ILogger _logger; - public DerivedClass_with_private_logger(ILogger logger) : base(logger) + public DerivedClassWithPrivateLogger(ILogger logger) : base(logger) { _logger = logger; } @@ -57,7 +57,6 @@ public partial class PartialClassWithLoggerField public partial void Test(); } - // Used to test use outside of a namespace internal static partial class NoNamespace {