From e41f91243ff97d8e72ed4814ebc34f71fa1ccdc7 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Fri, 26 Nov 2021 10:54:56 +0100 Subject: [PATCH] Suppress warnings on uninitialized DbSet properties (#26795) Closes #21608 --- src/EFCore.Analyzers/EFCore.Analyzers.csproj | 13 ++ .../InternalUsageDiagnosticAnalyzer.cs | 17 +- .../Properties/AnalyzerStrings.Designer.cs | 66 ++++++++ .../Properties/AnalyzerStrings.resx | 30 ++++ .../UninitializedDbSetDiagnosticSuppressor.cs | 84 ++++++++++ .../InternalUsageDiagnosticAnalyzerTest.cs | 18 +- .../DiagnosticAnalyzerTestBase.cs | 19 ++- ...nitializedDbSetDiagnosticSuppressorTest.cs | 154 ++++++++++++++++++ 8 files changed, 372 insertions(+), 29 deletions(-) create mode 100644 src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs create mode 100644 src/EFCore.Analyzers/Properties/AnalyzerStrings.resx create mode 100644 src/EFCore.Analyzers/UninitializedDbSetDiagnosticSuppressor.cs create mode 100644 test/EFCore.Analyzers.Tests/UninitializedDbSetDiagnosticSuppressorTest.cs diff --git a/src/EFCore.Analyzers/EFCore.Analyzers.csproj b/src/EFCore.Analyzers/EFCore.Analyzers.csproj index 22d17e2d7bd..88999dabc34 100644 --- a/src/EFCore.Analyzers/EFCore.Analyzers.csproj +++ b/src/EFCore.Analyzers/EFCore.Analyzers.csproj @@ -19,6 +19,11 @@ + + True + True + AnalyzerStrings.resx + @@ -45,4 +50,12 @@ + + + PublicResXFileCodeGenerator + AnalyzerStrings.Designer.cs + Microsoft.EntityFrameworkCore + + + diff --git a/src/EFCore.Analyzers/InternalUsageDiagnosticAnalyzer.cs b/src/EFCore.Analyzers/InternalUsageDiagnosticAnalyzer.cs index cc09a2bb8d2..2e5054b0faa 100644 --- a/src/EFCore.Analyzers/InternalUsageDiagnosticAnalyzer.cs +++ b/src/EFCore.Analyzers/InternalUsageDiagnosticAnalyzer.cs @@ -10,26 +10,17 @@ namespace Microsoft.EntityFrameworkCore; [DiagnosticAnalyzer(LanguageNames.CSharp)] -public class InternalUsageDiagnosticAnalyzer : DiagnosticAnalyzer +public sealed class InternalUsageDiagnosticAnalyzer : DiagnosticAnalyzer { public const string Id = "EF1001"; - - public const string MessageFormat - = "{0} is an internal API that supports the Entity Framework Core infrastructure and " - + "not subject to the same compatibility standards as public APIs. " - + "It may be changed or removed without notice in any release."; - - protected const string DefaultTitle = "Internal EF Core API usage."; - protected const string Category = "Usage"; - private static readonly int EFLen = "EntityFrameworkCore".Length; private static readonly DiagnosticDescriptor _descriptor = new( Id, - title: DefaultTitle, - messageFormat: MessageFormat, - category: Category, + title: AnalyzerStrings.InternalUsageTitle, + messageFormat: AnalyzerStrings.InternalUsageMessageFormat, + category: "Usage", defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true); diff --git a/src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs b/src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs new file mode 100644 index 00000000000..74922826a46 --- /dev/null +++ b/src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs @@ -0,0 +1,66 @@ +//------------------------------------------------------------------------------ +// +// This code was generated by a tool. +// +// Changes to this file may cause incorrect behavior and will be lost if +// the code is regenerated. +// +//------------------------------------------------------------------------------ + +namespace Microsoft.EntityFrameworkCore { + using System; + + + [System.CodeDom.Compiler.GeneratedCodeAttribute("System.Resources.Tools.StronglyTypedResourceBuilder", "4.0.0.0")] + [System.Diagnostics.DebuggerNonUserCodeAttribute()] + [System.Runtime.CompilerServices.CompilerGeneratedAttribute()] + public class AnalyzerStrings { + + private static System.Resources.ResourceManager resourceMan; + + private static System.Globalization.CultureInfo resourceCulture; + + [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode")] + internal AnalyzerStrings() { + } + + [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Advanced)] + public static System.Resources.ResourceManager ResourceManager { + get { + if (object.Equals(null, resourceMan)) { + System.Resources.ResourceManager temp = new System.Resources.ResourceManager("Microsoft.EntityFrameworkCore.Properties.AnalyzerStrings", typeof(AnalyzerStrings).Assembly); + resourceMan = temp; + } + return resourceMan; + } + } + + [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Advanced)] + public static System.Globalization.CultureInfo Culture { + get { + return resourceCulture; + } + set { + resourceCulture = value; + } + } + + public static string UninitializedDbSetWarningSuppressionJustification { + get { + return ResourceManager.GetString("UninitializedDbSetWarningSuppressionJustification", resourceCulture); + } + } + + public static string InternalUsageMessageFormat { + get { + return ResourceManager.GetString("InternalUsageMessageFormat", resourceCulture); + } + } + + public static string InternalUsageTitle { + get { + return ResourceManager.GetString("InternalUsageTitle", resourceCulture); + } + } + } +} diff --git a/src/EFCore.Analyzers/Properties/AnalyzerStrings.resx b/src/EFCore.Analyzers/Properties/AnalyzerStrings.resx new file mode 100644 index 00000000000..b94f9ac8eb7 --- /dev/null +++ b/src/EFCore.Analyzers/Properties/AnalyzerStrings.resx @@ -0,0 +1,30 @@ + + + + + + + + + + text/microsoft-resx + + + 1.3 + + + System.Resources.ResXResourceReader, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + System.Resources.ResXResourceWriter, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + + DbSet properties on DbContext subclasses are automatically populated by the DbContext constructor. + + + {0} is an internal API that supports the Entity Framework Core infrastructure and not subject to the same compatibility standards as public APIs. It may be changed or removed without notice in any release. + + + Internal EF Core API usage. + + \ No newline at end of file diff --git a/src/EFCore.Analyzers/UninitializedDbSetDiagnosticSuppressor.cs b/src/EFCore.Analyzers/UninitializedDbSetDiagnosticSuppressor.cs new file mode 100644 index 00000000000..50bac217aee --- /dev/null +++ b/src/EFCore.Analyzers/UninitializedDbSetDiagnosticSuppressor.cs @@ -0,0 +1,84 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Microsoft.EntityFrameworkCore; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class UninitializedDbSetDiagnosticSuppressor : DiagnosticSuppressor +{ + private static readonly SuppressionDescriptor _suppressUninitializedDbSetRule = new( + id: "SPR1001", + suppressedDiagnosticId: "CS8618", + justification: AnalyzerStrings.UninitializedDbSetWarningSuppressionJustification); + + /// + public override ImmutableArray SupportedSuppressions { get; } + = ImmutableArray.Create(_suppressUninitializedDbSetRule); + + /// + public override void ReportSuppressions(SuppressionAnalysisContext context) + { + INamedTypeSymbol? dbSetTypeSymbol = null; + INamedTypeSymbol? dbContextTypeSymbol = null; + + foreach (var diagnostic in context.ReportedDiagnostics) + { + // We have an warning about an uninitialized non-nullable property. + // Get the node, and make sure it's a property who's type syntactically contains DbSet (fast check before getting the + // semantic model, which is heavier). + if (diagnostic.Location.SourceTree is not { } sourceTree + || sourceTree.GetRoot().FindNode(diagnostic.Location.SourceSpan) is not PropertyDeclarationSyntax propertyDeclarationSyntax + || !propertyDeclarationSyntax.Type.ToString().Contains("DbSet")) + { + continue; + } + + // Get the semantic symbol and do some basic checks + if (context.GetSemanticModel(sourceTree).GetDeclaredSymbol(propertyDeclarationSyntax) is not IPropertySymbol propertySymbol + || propertySymbol.IsStatic + || propertySymbol.IsReadOnly) + { + continue; + } + + if (dbSetTypeSymbol is null || dbContextTypeSymbol is null) + { + dbSetTypeSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.EntityFrameworkCore.DbSet`1"); + dbContextTypeSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.EntityFrameworkCore.DbContext"); + + if (dbSetTypeSymbol is null || dbContextTypeSymbol is null) + { + return; + } + } + + // Check that the property is actually a DbSet, and that its containing type inherits from DbContext + if (propertySymbol.Type.OriginalDefinition.Equals(dbSetTypeSymbol, SymbolEqualityComparer.Default) + && InheritsFrom(propertySymbol.ContainingType, dbContextTypeSymbol)) + { + context.ReportSuppression(Suppression.Create(SupportedSuppressions[0], diagnostic)); + } + + static bool InheritsFrom(ITypeSymbol typeSymbol, ITypeSymbol baseTypeSymbol) + { + var baseType = typeSymbol.BaseType; + while (baseType is not null) + { + if (baseType.Equals(baseTypeSymbol, SymbolEqualityComparer.Default)) + { + return true; + } + + baseType = baseType.BaseType; + } + + return false; + } + } + } +} diff --git a/test/EFCore.Analyzers.Tests/InternalUsageDiagnosticAnalyzerTest.cs b/test/EFCore.Analyzers.Tests/InternalUsageDiagnosticAnalyzerTest.cs index 415fb20917e..b406149d66d 100644 --- a/test/EFCore.Analyzers.Tests/InternalUsageDiagnosticAnalyzerTest.cs +++ b/test/EFCore.Analyzers.Tests/InternalUsageDiagnosticAnalyzerTest.cs @@ -8,9 +8,6 @@ namespace Microsoft.EntityFrameworkCore; public class InternalUsageDiagnosticAnalyzerTest : DiagnosticAnalyzerTestBase { - protected override DiagnosticAnalyzer CreateDiagnosticAnalyzer() - => new InternalUsageDiagnosticAnalyzer(); - [ConditionalFact] public Task Invocation_on_type_in_internal_namespace() => Test( @@ -43,7 +40,7 @@ class MyClass : Microsoft.EntityFrameworkCore.Storage.Internal.RawRelationalPara Assert.Equal(DiagnosticSeverity.Warning, diagnostic.Severity); Assert.Equal( string.Format( - InternalUsageDiagnosticAnalyzer.MessageFormat, + AnalyzerStrings.InternalUsageMessageFormat, "Microsoft.EntityFrameworkCore.Storage.Internal.RawRelationalParameter"), diagnostic.GetMessage()); @@ -58,7 +55,7 @@ class MyClass : Microsoft.EntityFrameworkCore.Storage.Internal.RawRelationalPara Assert.Equal(DiagnosticSeverity.Warning, diagnostic.Severity); Assert.Equal( string.Format( - InternalUsageDiagnosticAnalyzer.MessageFormat, + AnalyzerStrings.InternalUsageMessageFormat, "Microsoft.EntityFrameworkCore.Storage.Internal.RawRelationalParameter"), diagnostic.GetMessage()); @@ -204,9 +201,7 @@ private async Task Test( Assert.Equal(InternalUsageDiagnosticAnalyzer.Id, diagnostic.Id); Assert.Equal(DiagnosticSeverity.Warning, diagnostic.Severity); - Assert.Equal( - string.Format(InternalUsageDiagnosticAnalyzer.MessageFormat, expectedInternalApi), - diagnostic.GetMessage()); + Assert.Equal(string.Format(AnalyzerStrings.InternalUsageMessageFormat, expectedInternalApi), diagnostic.GetMessage()); var span = diagnostic.Location.SourceSpan; Assert.Equal(expectedDiagnosticSpan, fullSource[span.Start..span.End]); @@ -222,9 +217,7 @@ private async Task TestFullSource( Assert.Equal(InternalUsageDiagnosticAnalyzer.Id, diagnostic.Id); Assert.Equal(DiagnosticSeverity.Warning, diagnostic.Severity); - Assert.Equal( - string.Format(InternalUsageDiagnosticAnalyzer.MessageFormat, expectedInternalApi), - diagnostic.GetMessage()); + Assert.Equal(string.Format(AnalyzerStrings.InternalUsageMessageFormat, expectedInternalApi), diagnostic.GetMessage()); var span = diagnostic.Location.SourceSpan; Assert.Equal(expectedDiagnosticSpan, fullSource[span.Start..span.End]); @@ -232,4 +225,7 @@ private async Task TestFullSource( protected override Task<(Diagnostic[], string)> GetDiagnosticsAsync(string source, params string[] extraUsings) => base.GetDiagnosticsAsync(source, extraUsings.Concat(new[] { "Microsoft.EntityFrameworkCore.Internal" }).ToArray()); + + protected override DiagnosticAnalyzer CreateDiagnosticAnalyzer() + => new InternalUsageDiagnosticAnalyzer(); } diff --git a/test/EFCore.Analyzers.Tests/TestUtilities/DiagnosticAnalyzerTestBase.cs b/test/EFCore.Analyzers.Tests/TestUtilities/DiagnosticAnalyzerTestBase.cs index a89e0ca950a..9c8dc1d92e5 100644 --- a/test/EFCore.Analyzers.Tests/TestUtilities/DiagnosticAnalyzerTestBase.cs +++ b/test/EFCore.Analyzers.Tests/TestUtilities/DiagnosticAnalyzerTestBase.cs @@ -22,7 +22,11 @@ protected async Task AssertNoDiagnostics(string source, params string[] extraUsi Assert.Empty(diagnostics); } - protected virtual async Task<(Diagnostic[], string)> GetDiagnosticsAsync(string source, params string[] extraUsings) + protected virtual Task<(Diagnostic[], string)> GetDiagnosticsAsync(string source, params string[] extraUsings) + => GetDiagnosticsAsync(source, analyzerDiagnosticsOnly: true, extraUsings); + + protected virtual async Task<(Diagnostic[], string)> GetDiagnosticsAsync( + string source, bool analyzerDiagnosticsOnly, params string[] extraUsings) { var sb = new StringBuilder(); foreach (var @using in _usings.Concat(extraUsings)) @@ -42,10 +46,10 @@ protected async Task AssertNoDiagnostics(string source, params string[] extraUsi .AppendLine("}"); var fullSource = sb.ToString(); - return (await GetDiagnosticsFullSourceAsync(fullSource), fullSource); + return (await GetDiagnosticsFullSourceAsync(fullSource, analyzerDiagnosticsOnly), fullSource); } - protected async Task GetDiagnosticsFullSourceAsync(string source) + protected async Task GetDiagnosticsFullSourceAsync(string source, bool analyzerDiagnosticsOnly = true) { var compilation = await CreateProject(source).GetCompilationAsync(); var errors = compilation.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error); @@ -60,7 +64,9 @@ var compilationWithAnalyzers analyzer.SupportedDiagnostics.ToDictionary(d => d.Id, d => ReportDiagnostic.Default))) .WithAnalyzers(ImmutableArray.Create(analyzer)); - var diagnostics = await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync(); + var diagnostics = analyzerDiagnosticsOnly + ? await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync() + : await compilationWithAnalyzers.GetAllDiagnosticsAsync(); return diagnostics.OrderBy(d => d.Location.SourceSpan.Start).ToArray(); } @@ -89,6 +95,9 @@ var metadataReferences .AddDocument(documentId, fileName, SourceText.From(source)); return solution.GetProject(projectId) - .WithCompilationOptions(new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)); + .WithCompilationOptions( + new CSharpCompilationOptions( + OutputKind.DynamicallyLinkedLibrary, + nullableContextOptions: NullableContextOptions.Enable)); } } diff --git a/test/EFCore.Analyzers.Tests/UninitializedDbSetDiagnosticSuppressorTest.cs b/test/EFCore.Analyzers.Tests/UninitializedDbSetDiagnosticSuppressorTest.cs new file mode 100644 index 00000000000..0b24a946d56 --- /dev/null +++ b/test/EFCore.Analyzers.Tests/UninitializedDbSetDiagnosticSuppressorTest.cs @@ -0,0 +1,154 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.EntityFrameworkCore.TestUtilities; +using Xunit; + +namespace Microsoft.EntityFrameworkCore; + +public class UninitializedDbSetDiagnosticSuppressorTest : DiagnosticAnalyzerTestBase +{ + [ConditionalFact] + public async Task DbSet_property_on_DbContext_is_suppressed() + { + var source = @" +public class MyDbContext : Microsoft.EntityFrameworkCore.DbContext +{ + public Microsoft.EntityFrameworkCore.DbSet Blogs { get; set; } +} + +public class Blog +{ + public int Id { get; set; } +}"; + + var diagnostic = Assert.Single(await GetDiagnosticsFullSourceAsync(source)); + + Assert.Equal("CS8618", diagnostic.Id); + Assert.True(diagnostic.IsSuppressed); + } + + [ConditionalFact] + public async Task Non_public_DbSet_property_on_DbContext_is_suppressed() + { + var source = @" +public class MyDbContext : Microsoft.EntityFrameworkCore.DbContext +{ + private Microsoft.EntityFrameworkCore.DbSet Blogs { get; set; } +} + +public class Blog +{ + public int Id { get; set; } +}"; + + var diagnostic = Assert.Single(await GetDiagnosticsFullSourceAsync(source)); + + Assert.Equal("CS8618", diagnostic.Id); + Assert.True(diagnostic.IsSuppressed); + } + + [ConditionalFact] + public async Task DbSet_property_with_non_public_setter_on_DbContext_is_suppressed() + { + var source = @" +public class MyDbContext : Microsoft.EntityFrameworkCore.DbContext +{ + public Microsoft.EntityFrameworkCore.DbSet Blogs { get; private set; } +} + +public class Blog +{ + public int Id { get; set; } +}"; + + var diagnostic = Assert.Single(await GetDiagnosticsFullSourceAsync(source)); + + Assert.Equal("CS8618", diagnostic.Id); + Assert.True(diagnostic.IsSuppressed); + } + + [ConditionalFact] + public async Task DbSet_property_without_setter_on_DbContext_is_not_suppressed() + { + var source = @" +public class MyDbContext : Microsoft.EntityFrameworkCore.DbContext +{ + public Microsoft.EntityFrameworkCore.DbSet Blogs { get; } +} + +public class Blog +{ + public int Id { get; set; } +}"; + + var diagnostic = Assert.Single(await GetDiagnosticsFullSourceAsync(source)); + + Assert.Equal("CS8618", diagnostic.Id); + Assert.False(diagnostic.IsSuppressed); + } + + [ConditionalFact] + public async Task Static_DbSet_property_on_DbContext_is_not_suppressed() + { + var source = @" +public class MyDbContext : Microsoft.EntityFrameworkCore.DbContext +{ + public static Microsoft.EntityFrameworkCore.DbSet Blogs { get; set; } +} + +public class Blog +{ + public int Id { get; set; } +}"; + + var diagnostic = Assert.Single(await GetDiagnosticsFullSourceAsync(source)); + + Assert.Equal("CS8618", diagnostic.Id); + Assert.False(diagnostic.IsSuppressed); + } + + [ConditionalFact] + public async Task Non_DbSet_property_on_DbContext_is_not_suppressed() + { + var source = @" +public class MyDbContext : Microsoft.EntityFrameworkCore.DbContext +{ + public string Name { get; set; } +}"; + + var diagnostic = Assert.Single(await GetDiagnosticsFullSourceAsync(source)); + + Assert.Equal("CS8618", diagnostic.Id); + Assert.False(diagnostic.IsSuppressed); + } + + [ConditionalFact] + public async Task DbSet_property_on_non_DbContext_is_not_suppressed() + { + var source = @" +public class Foo +{ + public Microsoft.EntityFrameworkCore.DbSet Blogs { get; set; } +} + +public class Blog +{ + public int Id { get; set; } +}"; + + var diagnostic = Assert.Single(await GetDiagnosticsFullSourceAsync(source)); + + Assert.Equal("CS8618", diagnostic.Id); + Assert.False(diagnostic.IsSuppressed); + } + + protected Task GetDiagnosticsFullSourceAsync(string source) + => base.GetDiagnosticsFullSourceAsync(source, analyzerDiagnosticsOnly: false); + + protected override DiagnosticAnalyzer CreateDiagnosticAnalyzer() + => new UninitializedDbSetDiagnosticSuppressor(); +}