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

Add CS8602 suppressor #24151

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,6 @@
</PropertyGroup>
<PropertyGroup Label="Other dependencies">
<MicrosoftCodeAnalysisVersion>3.7.0</MicrosoftCodeAnalysisVersion>
<MicrosoftCodeAnalysisTestingVersion>1.0.1-beta1.20623.3</MicrosoftCodeAnalysisTestingVersion>
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
</PropertyGroup>
</Project>
64 changes: 64 additions & 0 deletions src/EFCore.Analyzers/NullableDiagnosticSuppressor.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Immutable;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Microsoft.EntityFrameworkCore.Utilities;

namespace Microsoft.EntityFrameworkCore
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class NullableDiagnosticSuppressor : DiagnosticSuppressor
{
private static readonly ImmutableArray<OperationKind> _invocationKind = ImmutableArray.Create(OperationKind.Invocation);

private static readonly SuppressionDescriptor _descriptor = new(
id: "EFS0001",
suppressedDiagnosticId: "CS8602",
justification: "The dereference is safe inside this expression tree.");
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved

public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions => ImmutableArray.Create(_descriptor);

public override void ReportSuppressions(SuppressionAnalysisContext context)
{
var linqExpressionType = context.Compilation.GetTypeByMetadataName("System.Linq.Expressions.Expression`1");
if (linqExpressionType is null)
{
return;
}

var efQueryableExtensionsType = context.Compilation.GetTypeByMetadataName("Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions");
Copy link
Member

Choose a reason for hiding this comment

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

As I wrote in #21663 (comment), this won't take into account the common case of queryable operators, which aren't on EntityFrameworkQueryableExtensions (e.g. Where). To handle those reliably, rather than checking the queryable operators themselves, you'd need to go up the tree to make sure the tree is rooted on an EF Core DbSet.

if (efQueryableExtensionsType is null)
{
return;
}

foreach (var diagnostic in context.ReportedDiagnostics)
{
if (diagnostic.Location.SourceTree is not SyntaxTree tree)
{
continue;
}

var root = tree.GetRoot(context.CancellationToken);
var node = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true);
var model = context.GetSemanticModel(tree);
var operation = model.GetOperation(node, context.CancellationToken);
if (operation is null)
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
{
continue;
}

if (operation.IsWithinExpressionTree(linqExpressionType, out var anonymousOrLocalFunctionOperation) &&
anonymousOrLocalFunctionOperation.GetAncestor(_invocationKind) is IInvocationOperation invocation &&
SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.ContainingType, efQueryableExtensionsType))
{
context.ReportSuppression(Suppression.Create(_descriptor, diagnostic));
}

Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
65 changes: 65 additions & 0 deletions src/EFCore.Analyzers/Utilities/IOperationExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

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

namespace Microsoft.EntityFrameworkCore.Utilities
{
internal static class IOperationExtensions
{
private static readonly ImmutableArray<OperationKind> lambdaAndLocalFunctionKinds =
ImmutableArray.Create(OperationKind.AnonymousFunction, OperationKind.LocalFunction);

public static bool IsWithinExpressionTree(this IOperation operation, INamedTypeSymbol linqExpressionTreeType, [NotNullWhen(true)] out IOperation? anonymousOrLocalFunctionOperation)
{
if (operation.GetAncestor(lambdaAndLocalFunctionKinds) is IOperation op &&
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it checks only for operations directly within an expression lambda. Consider the following:

_ = await ctx.Blogs
    .Where(b => b.Posts.Any(p => p.Title.StartsWith("A")))
    .ToListAsync();

p.Title.StartsWith may generate a nullability warning, but the Any it's in is on Posts, which is an enumerable, not a queryable. To catch this you need to go further up the tree; this may not be trivial to do correctly, while keeping analyzer perf in mind.

Copy link
Member Author

Choose a reason for hiding this comment

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

@roji I haven't used ASP.NET and EF for long, so I might be wrong, but I think Where in the above snippet is from System.Linq and there is no expression tree involved, is that correct?
If that's correct, how it's guaranteed there will be no exception thrown if expression trees aren't involved?

Copy link
Member

Choose a reason for hiding this comment

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

The Where above is indeed Queryable.Where, which means it accepts an expression tree as a parameter. However, note that b.Posts (the navigation property) is not a DbSet/IQueryable, but rather an ordinary List (or similar); this means that Any is Enumerable.Any, not Queryable.Any.

The main point here is that EF Core ensures that anything within the Expression tree would be null-safe; for example, if there's any Post with a null title, the above wouldn't throw an NRE when executed with EF Core (although it would definitely throw if executed in Linq to Objects). This suppressor would ideally identify this by walking all the way up from the warning; if it finds an EF Core DbSet, it knows that the warning can be suppressed.

op.Parent?.Type?.OriginalDefinition is { } lambdaType &&
linqExpressionTreeType.Equals(lambdaType, SymbolEqualityComparer.Default))
{
anonymousOrLocalFunctionOperation = op;
return true;
}

anonymousOrLocalFunctionOperation = default;
return false;
}

/// <summary>
/// Gets the first ancestor of this operation with:
/// 1. Any OperationKind from the specified <paramref name="ancestorKinds"/>.
/// 2. If <paramref name="predicate"/> is non-null, it succeeds for the ancestor.
/// Returns null if there is no such ancestor.
/// </summary>
public static IOperation? GetAncestor(this IOperation root, ImmutableArray<OperationKind> ancestorKinds, Func<IOperation, bool>? predicate = null)
{
if (root == null)
{
throw new ArgumentNullException(nameof(root));
}

var ancestor = root;
do
{
ancestor = ancestor.Parent;
} while (ancestor != null && !ancestorKinds.Contains(ancestor.Kind));

if (ancestor != null)
{
if (predicate != null && !predicate(ancestor))
{
return GetAncestor(ancestor, ancestorKinds, predicate);
}
return ancestor;
}
else
{
return default;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

nit: empty lines

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change


}
}
20 changes: 20 additions & 0 deletions src/EFCore.Analyzers/Utilities/NotNullWhenAttribute.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace System.Diagnostics.CodeAnalysis
{
/// <summary>Specifies that when a method returns <see cref="ReturnValue"/>, the parameter will not be null even if the corresponding type allows it.</summary>
[AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
internal sealed class NotNullWhenAttribute : Attribute
{
/// <summary>Initializes the attribute with the specified return value condition.</summary>
/// <param name="returnValue">
/// The return value condition. If the method returns this value, the associated parameter will not be null.
/// </param>
public NotNullWhenAttribute(bool returnValue) => ReturnValue = returnValue;

/// <summary>Gets the return value condition.</summary>
public bool ReturnValue { get; }
}

}
4 changes: 3 additions & 1 deletion test/EFCore.Analyzers.Tests/EFCore.Analyzers.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<AssemblyName>Microsoft.EntityFrameworkCore.Analyzers.Tests</AssemblyName>
<RootNamespace>Microsoft.EntityFrameworkCore</RootNamespace>
<PreserveCompilationContext>true</PreserveCompilationContext>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
Expand All @@ -13,7 +14,8 @@
<ProjectReference Include="..\EFCore.Tests\EFCore.Tests.csproj" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="$(MicrosoftCodeAnalysisVersion)" />
<PackageReference Include="Microsoft.CodeAnalysis.VisualBasic" Version="$(MicrosoftCodeAnalysisVersion)" />
<PackageReference Include="Microsoft.Extensions.DependencyModel" Version="$(MicrosoftExtensionsDependencyModelVersion)" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.XUnit" Version="$(MicrosoftCodeAnalysisTestingVersion)" />
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
<PackageReference Include="Microsoft.Extensions.DependencyModel" Version="$(MicrosoftExtensionsDependencyModelVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<PackageReference Include="Microsoft.Extensions.DependencyModel" Version="$(MicrosoftExtensionsDependencyModelVersion)" />
<PackageReference Include="Microsoft.Extensions.DependencyModel" Version="$(MicrosoftExtensionsDependencyModelVersion)" />

</ItemGroup>

<ItemGroup>
Expand Down
85 changes: 85 additions & 0 deletions test/EFCore.Analyzers.Tests/NullableDiagnosticSuppressorTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Testing;
using Xunit;

using VerifyWithSuppressor = Microsoft.EntityFrameworkCore.TestUtilities.Verifiers.CSharpCodeFixVerifier<
Microsoft.EntityFrameworkCore.NullableDiagnosticSuppressor,
Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>;

using VerifyWithoutSuppressor = Microsoft.EntityFrameworkCore.TestUtilities.Verifiers.CSharpCodeFixVerifier<
Microsoft.CodeAnalysis.Testing.EmptyDiagnosticAnalyzer,
Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>;

namespace Microsoft.EntityFrameworkCore
{
public class NullableDiagnosticSuppressorTest
{
/// <summary>
/// Assert that given code have warnings specified by markup that are suppressed by <see cref="NullableDiagnosticSuppressor"/>
/// </summary>
private static async Task AssertWarningIsSuppressedAsync(string code)
{
await VerifyWithoutSuppressor.VerifyAnalyzerAsync(code);

await new VerifyWithSuppressor.Test()
{
TestState = { Sources = { code }, MarkupHandling = MarkupMode.Ignore }
}.RunAsync();
}

/// <summary>
/// Assert that given code have warnings specified by markup that are not suppressed by <see cref="NullableDiagnosticSuppressor"/>
/// </summary>
private static async Task AssertWarningIsNotSuppressedAsync(string code)
{
await VerifyWithSuppressor.VerifyAnalyzerAsync(code);
await VerifyWithoutSuppressor.VerifyAnalyzerAsync(code);
}

[Fact]
public async Task TestCompilerWarningAsync()
{
var code = @"
#nullable enable

class C
{
public void M(C? c)
{
_ = {|CS8602:c|}.ToString();
}
}";
await AssertWarningIsNotSuppressedAsync(code);
}

[Fact]
public async Task TestEFCoreIncludeIsSuppressed()
{
var code = @"
#nullable enable

using System.Linq;
using Microsoft.EntityFrameworkCore;

class SomeModel
{
public NavigationModel? Navigation { get; set; }
}

class NavigationModel
{
public string DeeperNavigation { get; set; } = null!;
}

static class C
{
public static void M(IQueryable<SomeModel> q)
{
_ = q.Include(m => m.Navigation).ThenInclude(n => {|CS8602:n|}.DeeperNavigation);
}
}
";
await AssertWarningIsSuppressedAsync(code);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ protected async Task AssertNoDiagnostics(string source, params string[] extraUsi
protected async Task<Diagnostic[]> GetDiagnosticsFullSourceAsync(string source)
{
var compilation = await CreateProject(source).GetCompilationAsync();
var errors = compilation.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error);
var errors = compilation!.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error);

Assert.Empty(errors);

Expand Down Expand Up @@ -92,7 +92,7 @@ var metadataReferences
.AddMetadataReferences(projectId, metadataReferences)
.AddDocument(documentId, fileName, SourceText.From(source));

return solution.GetProject(projectId)
return solution.GetProject(projectId)!
.WithCompilationOptions(new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary));
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
using System.Collections.Immutable;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Testing;

namespace Microsoft.EntityFrameworkCore.TestUtilities.Verifiers
{
public static class AdditionalMetadataReferences
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
{
public static ReferenceAssemblies Default { get; } = CreateDefaultReferenceAssemblies();

private static ReferenceAssemblies CreateDefaultReferenceAssemblies()
{
var referenceAssemblies = ReferenceAssemblies.Default;

referenceAssemblies = referenceAssemblies.AddPackages(ImmutableArray.Create(
new PackageIdentity("Microsoft.EntityFrameworkCore", "5.0.3")
));

return referenceAssemblies;
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
using System;
using System.Collections.Immutable;
using System.Net;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Testing;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Testing;
using Microsoft.CodeAnalysis.Testing.Verifiers;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.EntityFrameworkCore.TestUtilities.Verifiers
{
public static partial class CSharpCodeFixVerifier<TAnalyzer, TCodeFix>
where TAnalyzer : DiagnosticAnalyzer, new()
where TCodeFix : CodeFixProvider, new()
{
public class Test : CSharpCodeFixTest<TAnalyzer, TCodeFix, XUnitVerifier>
{
static Test()
{
// If we have outdated defaults from the host unit test application targeting an older .NET Framework, use more
// reasonable TLS protocol version for outgoing connections.
#pragma warning disable CA5364 // Do Not Use Deprecated Security Protocols
#pragma warning disable CS0618 // Type or member is obsolete
if (ServicePointManager.SecurityProtocol == (SecurityProtocolType.Ssl3 | SecurityProtocolType.Tls))
#pragma warning restore CS0618 // Type or member is obsolete
#pragma warning restore CA5364 // Do Not Use Deprecated Security Protocols
{
ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls12;
}
}

public Test()
{
ReferenceAssemblies = AdditionalMetadataReferences.Default;

SolutionTransforms.Add((solution, projectId) =>
{
var project = solution.GetProject(projectId)!;
var parseOptions = (CSharpParseOptions)project.ParseOptions!;
solution = solution.WithProjectParseOptions(projectId, parseOptions.WithLanguageVersion(LanguageVersion));

var compilationOptions = project.CompilationOptions!;

solution = solution.WithProjectCompilationOptions(projectId, compilationOptions);

if (AnalyzerConfigDocument is not null)
{
solution = solution.AddAnalyzerConfigDocument(
DocumentId.CreateNewId(projectId, debugName: ".editorconfig"),
".editorconfig",
SourceText.From($"is_global = true" + Environment.NewLine + AnalyzerConfigDocument),
filePath: @"z:\.editorconfig");
}

return solution;
});
}

protected override bool IsCompilerDiagnosticIncluded(Diagnostic diagnostic, CompilerDiagnostics compilerDiagnostics)
{
return !diagnostic.IsSuppressed;
}

public LanguageVersion LanguageVersion { get; set; } = LanguageVersion.CSharp9;

public string? AnalyzerConfigDocument { get; set; }
}
}

}
Loading