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

Suppress warnings on uninitialized DbSet properties #26795

Merged
merged 4 commits into from
Nov 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/EFCore.Analyzers/EFCore.Analyzers.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@

<ItemGroup>
<Compile Include="..\Shared\CodeAnnotations.cs" />
<Compile Update="Properties\AnalyzerStrings.Designer.cs">
<DesignTime>True</DesignTime>
<AutoGen>True</AutoGen>
<DependentUpon>AnalyzerStrings.resx</DependentUpon>
</Compile>
</ItemGroup>

<ItemGroup>
Expand All @@ -45,4 +50,12 @@
<AdditionalFiles Include="AnalyzerReleases.Unshipped.md" />
</ItemGroup>

<ItemGroup>
<EmbeddedResource Update="Properties\AnalyzerStrings.resx">
<Generator>PublicResXFileCodeGenerator</Generator>
<LastGenOutput>AnalyzerStrings.Designer.cs</LastGenOutput>
<CustomToolNamespace>Microsoft.EntityFrameworkCore</CustomToolNamespace>
</EmbeddedResource>
</ItemGroup>

</Project>
17 changes: 4 additions & 13 deletions src/EFCore.Analyzers/InternalUsageDiagnosticAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
66 changes: 66 additions & 0 deletions src/EFCore.Analyzers/Properties/AnalyzerStrings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 30 additions & 0 deletions src/EFCore.Analyzers/Properties/AnalyzerStrings.resx
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?xml version="1.0" encoding="utf-8"?>

<root>
<xsd:schema id="root" xmlns="" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:msdata="urn:schemas-microsoft-com:xml-msdata">
<xsd:element name="root" msdata:IsDataSet="true">

</xsd:element>
</xsd:schema>
<resheader name="resmimetype">
<value>text/microsoft-resx</value>
</resheader>
<resheader name="version">
<value>1.3</value>
</resheader>
<resheader name="reader">
<value>System.Resources.ResXResourceReader, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="UninitializedDbSetWarningSuppressionJustification" xml:space="preserve">
<value>DbSet properties on DbContext subclasses are automatically populated by the DbContext constructor.</value>
</data>
<data name="InternalUsageMessageFormat" xml:space="preserve">
<value>{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.</value>
</data>
<data name="InternalUsageTitle" xml:space="preserve">
<value>Internal EF Core API usage.</value>
</data>
</root>
84 changes: 84 additions & 0 deletions src/EFCore.Analyzers/UninitializedDbSetDiagnosticSuppressor.cs
Original file line number Diff line number Diff line change
@@ -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(
roji marked this conversation as resolved.
Show resolved Hide resolved
id: "SPR1001",
suppressedDiagnosticId: "CS8618",
justification: AnalyzerStrings.UninitializedDbSetWarningSuppressionJustification);

/// <inheritdoc />
public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions { get; }
= ImmutableArray.Create(_suppressUninitializedDbSetRule);

/// <inheritdoc />
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"))
roji marked this conversation as resolved.
Show resolved Hide resolved
{
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<T>, 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;
}
Comment on lines +67 to +81
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional to look at the full hierarchy, not only the parent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - the user's DbContext type can be lower down the hierarchy, rather than directly extend DbContext. A good example is ASP.NET Identity, where user context types can extend IdentityDbContext, which itself extends DbContext. The moment DbContext is somewhere in the base type list, we know its constructor will get invoked, and therefore the DbSets will get populated.

}
}
}
18 changes: 7 additions & 11 deletions test/EFCore.Analyzers.Tests/InternalUsageDiagnosticAnalyzerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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());

Expand All @@ -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());

Expand Down Expand Up @@ -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]);
Expand All @@ -222,14 +217,15 @@ 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]);
}

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();
}
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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<Diagnostic[]> GetDiagnosticsFullSourceAsync(string source)
protected async Task<Diagnostic[]> GetDiagnosticsFullSourceAsync(string source, bool analyzerDiagnosticsOnly = true)
{
var compilation = await CreateProject(source).GetCompilationAsync();
var errors = compilation.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error);
Expand All @@ -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();
}
Expand Down Expand Up @@ -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));
}
}
Loading