diff --git a/src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs b/src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs index 86ad40de801..2269012bec3 100644 --- a/src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs +++ b/src/Generators/Microsoft.Gen.Logging/Emission/Emitter.Method.cs @@ -540,8 +540,8 @@ bool GenVariableAssignments(LoggingMethod lm, string lambdaStateName, int numRes static (string name, bool isNullable) GetLogger(LoggingMethod lm) { - string logger = lm.LoggerField; - bool isNullable = lm.LoggerFieldNullable; + string logger = lm.LoggerMember; + bool isNullable = lm.LoggerMemberNullable; foreach (var p in lm.Parameters) { diff --git a/src/Generators/Microsoft.Gen.Logging/Model/LoggingMethod.cs b/src/Generators/Microsoft.Gen.Logging/Model/LoggingMethod.cs index 321aa84180c..72cf4b616a0 100644 --- a/src/Generators/Microsoft.Gen.Logging/Model/LoggingMethod.cs +++ b/src/Generators/Microsoft.Gen.Logging/Model/LoggingMethod.cs @@ -24,8 +24,8 @@ internal sealed class LoggingMethod public bool IsExtensionMethod; public bool IsStatic; public string Modifiers = string.Empty; - public string LoggerField = "_logger"; - public bool LoggerFieldNullable; + public string LoggerMember = "_logger"; + public bool LoggerMemberNullable; public bool HasXmlDocumentation; public string GetParameterNameInTemplate(LoggingMethodParameter parameter) diff --git a/src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs b/src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs index 275fc2dc82c..bac2e7a81e8 100644 --- a/src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs +++ b/src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs @@ -108,16 +108,16 @@ internal sealed class DiagDescriptors : DiagDescriptorsBase category: Category, DiagnosticSeverity.Warning); - public static DiagnosticDescriptor MissingLoggerField { get; } = Make( + public static DiagnosticDescriptor MissingLoggerMember { get; } = Make( id: DiagnosticIds.LoggerMessage.LOGGEN015, - title: Resources.MissingLoggerFieldTitle, - messageFormat: Resources.MissingLoggerFieldMessage, + title: Resources.MissingLoggerMemberTitle, + messageFormat: Resources.MissingLoggerMemberMessage, category: Category); - public static DiagnosticDescriptor MultipleLoggerFields { get; } = Make( + public static DiagnosticDescriptor MultipleLoggerMembers { get; } = Make( id: DiagnosticIds.LoggerMessage.LOGGEN016, - title: Resources.MultipleLoggerFieldsTitle, - messageFormat: Resources.MultipleLoggerFieldsMessage, + title: Resources.MultipleLoggerMembersTitle, + messageFormat: Resources.MultipleLoggerMembersMessage, category: Category); public static DiagnosticDescriptor CantUseDataClassificationWithLogPropertiesOrTagProvider { get; } = Make( @@ -176,7 +176,7 @@ internal sealed class DiagDescriptors : DiagDescriptorsBase messageFormat: Resources.LoggingMethodParameterRefKindMessage, category: Category); - public static DiagnosticDescriptor LogPropertiesProviderWithRedaction { get; } = Make( + public static DiagnosticDescriptor TagProviderWithRedaction { get; } = Make( id: DiagnosticIds.LoggerMessage.LOGGEN026, title: Resources.TagProviderWithRedactionTitle, messageFormat: Resources.TagProviderWithRedactionMessage, diff --git a/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs b/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs index bc7b101fdcc..4a8793c49b5 100644 --- a/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs +++ b/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs @@ -58,9 +58,9 @@ public IReadOnlyList GetLogTypes(IEnumerable LoggingType? lt = null; string nspace = string.Empty; - string? loggerField = null; - bool loggerFieldNullable = false; - IFieldSymbol? secondLoggerField = null; + string? loggerMember = null; + bool loggerMemberNullable = false; + ISymbol? secondLoggerMember = null; ids.Clear(); foreach (var method in typeDec.Members.Where(m => m.IsKind(SyntaxKind.MethodDeclaration)).Cast()) @@ -199,25 +199,25 @@ public IReadOnlyList GetLogTypes(IEnumerable { if (!parsingState.FoundLogger) { - if (loggerField == null) + if (loggerMember == null) { - (loggerField, secondLoggerField, loggerFieldNullable) = FindField(sm, typeDec, symbols.ILoggerSymbol); + (loggerMember, secondLoggerMember, loggerMemberNullable) = FindMember(sm, typeDec, symbols.ILoggerSymbol); } - if (secondLoggerField != null) + if (secondLoggerMember != null) { - Diag(DiagDescriptors.MultipleLoggerFields, secondLoggerField.GetLocation(), typeDec.Identifier.Text); + Diag(DiagDescriptors.MultipleLoggerMembers, secondLoggerMember.GetLocation(), typeDec.Identifier.Text); keepMethod = false; } - else if (loggerField == null) + else if (loggerMember == null) { - Diag(DiagDescriptors.MissingLoggerField, method.Identifier.GetLocation(), typeDec.Identifier.Text); + Diag(DiagDescriptors.MissingLoggerMember, method.Identifier.GetLocation(), typeDec.Identifier.Text); keepMethod = false; } else { - lm.LoggerField = loggerField; - lm.LoggerFieldNullable = loggerFieldNullable; + lm.LoggerMember = loggerMember; + lm.LoggerMemberNullable = loggerMemberNullable; } } @@ -592,35 +592,67 @@ private void CheckMethodParametersAreUnique(LoggingMethod lm, Dictionary - /// Looks up a localized string similar to Couldn't find a field of type "Microsoft.Extensions.Logging.ILogger" in type "{0}". + /// Looks up a localized string similar to Couldn't find a field or property of type "Microsoft.Extensions.Logging.ILogger" in type "{0}". /// - internal static string MissingLoggerFieldMessage { + internal static string MissingLoggerMemberMessage { get { - return ResourceManager.GetString("MissingLoggerFieldMessage", resourceCulture); + return ResourceManager.GetString("MissingLoggerMemberMessage", resourceCulture); } } /// - /// Looks up a localized string similar to Couldn't find a field of type "Microsoft.Extensions.Logging.ILogger". + /// Looks up a localized string similar to Couldn't find a field or property of type "Microsoft.Extensions.Logging.ILogger". /// - internal static string MissingLoggerFieldTitle { + internal static string MissingLoggerMemberTitle { get { - return ResourceManager.GetString("MissingLoggerFieldTitle", resourceCulture); + return ResourceManager.GetString("MissingLoggerMemberTitle", resourceCulture); } } @@ -421,20 +421,20 @@ internal static string MissingRequiredTypeTitle { } /// - /// Looks up a localized string similar to Found multiple fields of type "Microsoft.Extensions.Logging.ILogger" in type "{0}". + /// Looks up a localized string similar to Found multiple fields or properties of type "Microsoft.Extensions.Logging.ILogger" in type "{0}". /// - internal static string MultipleLoggerFieldsMessage { + internal static string MultipleLoggerMembersMessage { get { - return ResourceManager.GetString("MultipleLoggerFieldsMessage", resourceCulture); + return ResourceManager.GetString("MultipleLoggerMembersMessage", resourceCulture); } } /// - /// Looks up a localized string similar to Multiple fields of type "Microsoft.Extensions.Logging.ILogger" were found. + /// Looks up a localized string similar to Multiple fields or properties of type "Microsoft.Extensions.Logging.ILogger" were found. /// - internal static string MultipleLoggerFieldsTitle { + internal static string MultipleLoggerMembersTitle { get { - return ResourceManager.GetString("MultipleLoggerFieldsTitle", resourceCulture); + return ResourceManager.GetString("MultipleLoggerMembersTitle", resourceCulture); } } @@ -457,7 +457,7 @@ internal static string ParameterHasNoCorrespondingTemplateTitle { } /// - /// Looks up a localized string similar to Parameter "{0}" of logging method "{1}" has sensitive field/property in its type. + /// Looks up a localized string similar to Parameter "{0}" of logging method "{1}" has a sensitive field/property in its type. /// internal static string RecordTypeSensitiveArgumentIsInTemplateMessage { get { diff --git a/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx b/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx index 344aeb3d034..1a56b134b3f 100644 --- a/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx +++ b/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx @@ -207,17 +207,17 @@ Don't include logger parameters as templates - - Couldn't find a field of type "Microsoft.Extensions.Logging.ILogger" in type "{0}" + + Couldn't find a field or property of type "Microsoft.Extensions.Logging.ILogger" in type "{0}" - - Couldn't find a field of type "Microsoft.Extensions.Logging.ILogger" + + Couldn't find a field or property of type "Microsoft.Extensions.Logging.ILogger" - - Found multiple fields of type "Microsoft.Extensions.Logging.ILogger" in type "{0}" + + Found multiple fields or properties of type "Microsoft.Extensions.Logging.ILogger" in type "{0}" - - Multiple fields of type "Microsoft.Extensions.Logging.ILogger" were found + + Multiple fields or properties of type "Microsoft.Extensions.Logging.ILogger" were found Can't log properties of items of type "{0}" diff --git a/src/Generators/Shared/SymbolHelpers.cs b/src/Generators/Shared/SymbolHelpers.cs index d32bbd7c1f2..2f973bd139b 100644 --- a/src/Generators/Shared/SymbolHelpers.cs +++ b/src/Generators/Shared/SymbolHelpers.cs @@ -16,4 +16,40 @@ public static string GetFullNamespace(ISymbol symbol) { return symbol.ContainingNamespace.IsGlobalNamespace ? string.Empty : symbol.ContainingNamespace.ToString(); } + + /// + /// Can code in a given type access a given member? + /// + /// + /// Note that this implementation assumes that the target member is within the origin type + /// or a base class of the origin type. + /// + public static bool CanAccess(this INamedTypeSymbol originType, ISymbol targetMember) + { + if (SymbolEqualityComparer.Default.Equals(originType, targetMember.ContainingType)) + { + // target member is from the origin type, we're good + return true; + } + + if (targetMember.DeclaredAccessibility == Accessibility.Private) + { + // can't access a private member from a different type + return false; + } + + if (SymbolEqualityComparer.Default.Equals(originType.ContainingAssembly, targetMember.ContainingAssembly)) + { + // target member is in the same assembly as the origin type, so we're good + return true; + } + + if (targetMember.DeclaredAccessibility is Accessibility.Internal or Accessibility.ProtectedAndInternal) + { + // can't access internal members of other assemblies (sorry, we don't support IVT right now) + return false; + } + + return true; + } } diff --git a/test/Generators/Microsoft.Gen.Logging/Generated/LoggerFromMemberTests.cs b/test/Generators/Microsoft.Gen.Logging/Generated/LoggerFromMemberTests.cs new file mode 100644 index 00000000000..ee612123f82 --- /dev/null +++ b/test/Generators/Microsoft.Gen.Logging/Generated/LoggerFromMemberTests.cs @@ -0,0 +1,104 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.Extensions.Logging.Testing; +using TestClasses; +using Xunit; + +namespace Microsoft.Gen.Logging.Test; + +public class LoggerFromMemberTests +{ + [Fact] + public void LoggerInPropertyTestClass() + { + using var logger = Utils.GetLogger(); + var collector = logger.FakeLogCollector; + + var o = new LoggerInPropertyTestClass + { + Logger = logger + }; + + o.M0("arg0"); + Assert.Null(collector.LatestRecord.Exception); + Assert.Equal("M0 arg0", collector.LatestRecord.Message); + Assert.Equal(1, collector.Count); + } + + [Fact] + public void LoggerInNullablePropertyTestClass() + { + using var logger = Utils.GetLogger(); + var collector = logger.FakeLogCollector; + + var o = new LoggerInNullablePropertyTestClass + { + Logger = logger + }; + + o.M0("arg0"); + Assert.Null(collector.LatestRecord.Exception); + Assert.Equal("M0 arg0", collector.LatestRecord.Message); + Assert.Equal(1, collector.Count); + + o.Logger = null; + o.M0("arg0"); + Assert.Equal(1, collector.Count); + } + + [Fact] + public void LoggerInPropertyDerivedTestClass() + { + using var logger = Utils.GetLogger(); + var collector = logger.FakeLogCollector; + + var o = new LoggerInPropertyDerivedTestClass + { + Logger = logger + }; + + o.M0("arg0"); + Assert.Null(collector.LatestRecord.Exception); + Assert.Equal("M0 arg0", collector.LatestRecord.Message); + Assert.Equal(1, collector.Count); + } + + [Fact] + public void LoggerInNullablePropertyDerivedTestClass() + { + using var logger = Utils.GetLogger(); + var collector = logger.FakeLogCollector; + + var o = new LoggerInNullablePropertyDerivedTestClass + { + Logger = logger + }; + + o.M0("arg0"); + Assert.Null(collector.LatestRecord.Exception); + Assert.Equal("M0 arg0", collector.LatestRecord.Message); + Assert.Equal(1, collector.Count); + + o.Logger = null; + o.M0("arg0"); + Assert.Equal(1, collector.Count); + } + + [Fact] + public void GenericLoggerInPropertyTestClass() + { + var logger = new FakeLogger(); + var collector = logger.Collector; + + var o = new GenericLoggerInPropertyTestClass + { + Logger = logger + }; + + o.M0("arg0"); + Assert.Null(collector.LatestRecord.Exception); + Assert.Equal("M0 arg0", collector.LatestRecord.Message); + Assert.Equal(1, collector.Count); + } +} diff --git a/test/Generators/Microsoft.Gen.Logging/TestClasses/NonStaticTestClasses.cs b/test/Generators/Microsoft.Gen.Logging/TestClasses/NonStaticTestClasses.cs new file mode 100644 index 00000000000..6c6e7cfcd36 --- /dev/null +++ b/test/Generators/Microsoft.Gen.Logging/TestClasses/NonStaticTestClasses.cs @@ -0,0 +1,51 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.Extensions.Logging; + +#pragma warning disable SA1402 + +namespace TestClasses +{ + public partial class LoggerInPropertyTestClass + { + public ILogger Logger { get; set; } = null!; + + [LoggerMessage(0, LogLevel.Debug, "M0 {p0}")] + public partial void M0(string p0); + } + + public partial class LoggerInNullablePropertyTestClass + { + public ILogger? Logger { get; set; } + + [LoggerMessage(0, LogLevel.Debug, "M0 {p0}")] + public partial void M0(string p0); + } + + public partial class GenericLoggerInPropertyTestClass + { + public ILogger Logger { get; set; } = null!; + + [LoggerMessage(0, LogLevel.Debug, "M0 {p0}")] + public partial void M0(string p0); + } + + public partial class LoggerInPropertyDerivedTestClass : LoggerInPropertyTestClass + { + [LoggerMessage(1, LogLevel.Debug, "M1 {p0}")] + public partial void M1(string p0); + } + + public partial class LoggerInNullablePropertyDerivedTestClass : LoggerInNullablePropertyTestClass + { + [LoggerMessage(1, LogLevel.Debug, "M1 {p0}")] + public partial void M1(string p0); + } + + public partial class GenericLoggerInPropertyDerivedTestClass : LoggerInNullablePropertyTestClass + { + [LoggerMessage(1, LogLevel.Debug, "M1 {p0}")] + public partial void M1(string p0); + } +} diff --git a/test/Generators/Microsoft.Gen.Logging/Unit/LoggingMethodTests.cs b/test/Generators/Microsoft.Gen.Logging/Unit/LoggingMethodTests.cs index 668bf68be74..5c7ff5533a4 100644 --- a/test/Generators/Microsoft.Gen.Logging/Unit/LoggingMethodTests.cs +++ b/test/Generators/Microsoft.Gen.Logging/Unit/LoggingMethodTests.cs @@ -15,7 +15,7 @@ public void Fields_Should_BeInitialized() Assert.Empty(instance.Name); Assert.Empty(instance.Message); Assert.Empty(instance.Modifiers); - Assert.Equal("_logger", instance.LoggerField); + Assert.Equal("_logger", instance.LoggerMember); } [Fact] diff --git a/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.cs b/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.cs index bfd7dc44c6c..d2824eb28cc 100644 --- a/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.cs +++ b/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.cs @@ -24,37 +24,51 @@ public partial class ParserTests public async Task IncompatibleAttributes() { await RunGenerator(@$" - using Microsoft.Extensions.Compliance.Testing; - - class MyClass2 + namespace TestClasses {{ - public int A {{ get; set; }} - }} + public partial class LoggerInPropertyTestClass + {{ + public ILogger Logger {{ get; set; }} = null!; - class MyClass - {{ - public string P0 {{ get; set; }} = ""Hello""; + [LoggerMessage(0, LogLevel.Debug, ""M0 {{p0}}"")] + public partial void M0(string p0); + }} - [PrivateData, LogProperties] - public MyClass2 /*0+*/P1/*-0*/ {{ get; set; }} = ""Hello""; + public partial class LoggerInNullablePropertyTestClass + {{ + public ILogger? Logger {{ get; set; }} - [PrivateData, TagProvider(typeof(Provider), nameof(Provider.Provide)] - public string /*1+*/P2/*-1*/ {{ get; set; }} = ""Hello""; - }} + [LoggerMessage(0, LogLevel.Debug, ""M0 {{p0}}"")] + public partial void M0(string p0); + }} - static class Provider - {{ - public static void Provide(ITagCollector collector, string value) {{ }} - }} + public partial class GenericLoggerInPropertyTestClass + {{ + public ILogger Logger {{ get; set; }} = null!; - internal partial class C - {{ - [LoggerMessage(0, LogLevel.Debug, ""M0 {{p0}}"")] - partial static void M0(ILogger logger, [PrivateData, LogProperties] MyClass /*2+*/p0/*-2*/); + [LoggerMessage(0, LogLevel.Debug, ""M0 {{p0}}"")] + public partial void M0(string p0); + }} + + public partial class LoggerInPropertyDerivedTestClass : LoggerInPropertyTestClass + {{ + [LoggerMessage(1, LogLevel.Debug, ""M1 {{p0}}"")] + public partial void M1(string p0); + }} - [LoggerMessage(1, LogLevel.Debug, ""M1 {{p0}}"")] - partial static void M1(ILogger logger, [PrivateData, TagProvider(typeof(Provider), nameof(Provider.Provide))] string /*3+*/p0/*-3*/); - }}", DiagDescriptors.CantUseDataClassificationWithLogPropertiesOrTagProvider); + public partial class LoggerInNullablePropertyDerivedTestClass : LoggerInNullablePropertyTestClass + {{ + [LoggerMessage(1, LogLevel.Debug, ""M1 {{p0}}"")] + public partial void M1(string p0); + }} + + public partial class GenericLoggerInPropertyDerivedTestClass : LoggerInNullablePropertyTestClass + {{ + [LoggerMessage(1, LogLevel.Debug, ""M1 {{p0}}"")] + public partial void M1(string p0); + }} + }} + "); } [Fact] @@ -559,11 +573,11 @@ partial class C } "; - await RunGenerator(Source, DiagDescriptors.MissingLoggerField); + await RunGenerator(Source, DiagDescriptors.MissingLoggerMember); } [Fact] - public async Task MultipleILoggerFields() + public async Task MultipleILoggerMembers() { const string Source = @" partial class C @@ -574,9 +588,43 @@ partial class C [LoggerMessage(0, LogLevel.Debug, ""M1"")] public partial void M1(); } + + partial class D + { + public ILogger? Logger { get; set; } = null!; + public ILogger /*1+*/_logger2/*-1*/; + + [LoggerMessage(0, LogLevel.Debug, ""M1"")] + public partial void M1(); + } + + partial class E + { + public ILogger Logger { get; set; } = null!; + public ILogger /*2+*/_logger2/*-2*/; + + [LoggerMessage(0, LogLevel.Debug, ""M1"")] + public partial void M1(); + } + + partial class F + { + public ILogger? /*3+*/Logger/*-3*/ { get; set; } + + [LoggerMessage(0, LogLevel.Debug, ""M1"")] + public partial void M1(); + } + + partial class G : F + { + private readonly ILogger? _logger; + + [LoggerMessage(1, LogLevel.Debug, ""M2"")] + public partial void M2(); + } "; - await RunGenerator(Source, DiagDescriptors.MultipleLoggerFields); + await RunGenerator(Source, DiagDescriptors.MultipleLoggerMembers); } [Fact]