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 1 commit
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
90 changes: 90 additions & 0 deletions src/EFCore.Analyzers/UninitializedDbSetDiagnosticSuppressor.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Immutable;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;

namespace Microsoft.EntityFrameworkCore;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public class UninitializedDbSetDiagnosticSuppressor : DiagnosticSuppressor
roji marked this conversation as resolved.
Show resolved Hide resolved
{
private static readonly SuppressionDescriptor _suppressUninitializedDbSetRule = new(
roji marked this conversation as resolved.
Show resolved Hide resolved
id: "SPR1001",
suppressedDiagnosticId: "CS8618",
justification: "DbSet properties on DbContext subclasses are automatically populated by the DbContext constructor");
Copy link
Member

Choose a reason for hiding this comment

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

Is this being non-localized intentionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that nothing else in EF Core is localized at this point.

It's true we manage exception strings via resx, but those sometimes get reused (or asserted upon in tests). I'd rather extract these strings out based on need rather than proactively, though can do that if the team prefers.

Copy link
Contributor

Choose a reason for hiding this comment

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

@roji We have so far taken the approach that everything (possible) is localization ready. I'd prefer we keep doing that, but we can discuss with the team.


/// <inheritdoc />
public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions => ImmutableArray.Create(_suppressUninitializedDbSetRule);
roji marked this conversation as resolved.
Show resolved Hide resolved

/// <inheritdoc />
public override void ReportSuppressions(SuppressionAnalysisContext context)
{
INamedTypeSymbol? dbSetTypeSymbol = null;
INamedTypeSymbol? dbContextTypeSymbol = null;

foreach (var diagnostic in context.ReportedDiagnostics)
{
if (diagnostic.Id != "CS8618" || !diagnostic.Location.IsInSource)
roji marked this conversation as resolved.
Show resolved Hide resolved
{
continue;
}

// 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).
var sourceTree = diagnostic.Location.SourceTree;
var node = sourceTree?.GetRoot().FindNode(diagnostic.Location.SourceSpan);
roji marked this conversation as resolved.
Show resolved Hide resolved
if (node 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(node) is not IPropertySymbol propertySymbol
roji marked this conversation as resolved.
Show resolved Hide resolved
|| 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.

}
}
}
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 @@ -232,4 +229,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();
}
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));
}
}
Original file line number Diff line number Diff line change
@@ -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<Blog> 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);
}
roji marked this conversation as resolved.
Show resolved Hide resolved

[ConditionalFact]
public async Task Non_public_DbSet_property_on_DbContext_is_suppressed()
{
var source = @"
public class MyDbContext : Microsoft.EntityFrameworkCore.DbContext
{
private Microsoft.EntityFrameworkCore.DbSet<Blog> 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<Blog> 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<Blog> 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<Blog> 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<Blog> 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<Diagnostic[]> GetDiagnosticsFullSourceAsync(string source)
=> base.GetDiagnosticsFullSourceAsync(source, analyzerDiagnosticsOnly: false);

protected override DiagnosticAnalyzer CreateDiagnosticAnalyzer()
=> new UninitializedDbSetDiagnosticSuppressor();
}