diff --git a/Documentation/src/SUMMARY.md b/Documentation/src/SUMMARY.md index 96c852c9a..3a07c8a02 100644 --- a/Documentation/src/SUMMARY.md +++ b/Documentation/src/SUMMARY.md @@ -52,6 +52,7 @@ * [λ1008](./analyzer-rules/λ1008.md) * [λ1009](./analyzer-rules/λ1009.md) * [λ1010](./analyzer-rules/λ1010.md) + * [λ1012](./analyzer-rules/λ1012.md) * [λ1014](./analyzer-rules/λ1014.md) * [λ1015](./analyzer-rules/λ1015.md) * [λ1016](./analyzer-rules/λ1016.md) diff --git a/Documentation/src/analyzer-rules/analyzer-rules.md b/Documentation/src/analyzer-rules/analyzer-rules.md index 09916a763..85426d419 100644 --- a/Documentation/src/analyzer-rules/analyzer-rules.md +++ b/Documentation/src/analyzer-rules/analyzer-rules.md @@ -48,6 +48,7 @@ contains a rule. | [λ1006](./λ1006.md) | Warning | Prefer `OrElse` over `Match` | | [λ1007](./λ1007.md) | Warning | Prefer `SelectMany` over `Match` | | [λ1008](./λ1008.md) | Warning | Prefer `ToNullable` over `Match` | +| [λ1012](./λ1012.md) | Warning | Prefer the `…OrNone` method over `…OrDefault` with a null check | | [λ1014](./λ1014.md) | Warning | Throw expression in an eagerly evaluated fallback | | [λ1015](./λ1015.md) | Info | Fallback is computed eagerly | | [λ1016](./λ1016.md) | Warning | Pass the fallback value directly | diff --git "a/Documentation/src/analyzer-rules/\316\2731012.md" "b/Documentation/src/analyzer-rules/\316\2731012.md" new file mode 100644 index 000000000..0c1fa4454 --- /dev/null +++ "b/Documentation/src/analyzer-rules/\316\2731012.md" @@ -0,0 +1,72 @@ +# λ1012: Prefer the `…OrNone` method over `…OrDefault` with a null check +`FirstOrDefault`, `LastOrDefault`, `SingleOrDefault` and `ElementAtOrDefault` signal a missing element with `null`. +Funcky's `FirstOrNone`, `LastOrNone`, `SingleOrNone` and `ElementAtOrNone` return an `Option` instead. + +## Cause +The result of one of the LINQ `…OrDefault` methods is immediately tested for `null`: + +* `xs.FirstOrDefault() ?? fallback` +* `xs.FirstOrDefault()?.Member` +* `xs.FirstOrDefault() is null`, `is not null`, `== null`, `!= null` +* `xs.FirstOrDefault() is { } x`, `is not { } x`, `is string s` + +The rule applies to sequences of a reference type. With nullable reference types enabled, sequences of a nullable +element type (`IEnumerable`) are left alone: there a `null` result is ambiguous between "no element" and +"the element is `null`". The overloads with an explicit default value (`FirstOrDefault(fallback)`) and the `IQueryable` +variants are not reported either. + +## Reason for rule +A `null` has to be checked by every caller, and nothing forces the check. `FirstOrNone` returns an `Option`, so the +missing element is a `None` and the handling is a `GetOrElse`, `Select`, `Where` or `Match` on the option. The code +says what happens when there is no element, and the compiler says when that case is forgotten. + +## How to fix violations +Replace the `…OrDefault` call with its `…OrNone` counterpart and handle the option. For `xs.FirstOrDefault() ?? fallback` +the code fix produces `xs.FirstOrNone().GetOrElse(fallback)`. A `??` evaluates its fallback only when the left side is +`null`, so a fallback that does work (a call, an object creation, an interpolated string, a `throw`) is passed as a +function, `GetOrElse(() => CreateFallback())`, and keeps that behaviour. There is no fix when the `??` widens the type +(`values.FirstOrDefault() ?? new object()` is an `object`, which `Option.GetOrElse` cannot return) or when the +fallback cannot move into a lambda (an `await`, a `ref` parameter). The other shapes have no single idiomatic +replacement, so they are reported without a fix: + +| Before | After | +|--------|-------| +| `xs.FirstOrDefault() ?? fallback` | `xs.FirstOrNone().GetOrElse(fallback)` | +| `xs.FirstOrDefault()?.Name` | `xs.FirstOrNone().Select(x => x.Name)` | +| `xs.FirstOrDefault() is null` | `!xs.Any()` or `xs.FirstOrNone().Match(none: true, some: False)` | +| `if (xs.FirstOrDefault() is { } x) { A(x); } else { B(); }` | `xs.FirstOrNone().Switch(none: B, some: A)` | + +## Examples + +### Disallowed +```cs +static string Example(IEnumerable names) + => names.FirstOrDefault() ?? "nobody"; + +static int Example(IEnumerable names) + => names.SingleOrDefault()?.Length ?? 0; + +static void Example(IEnumerable names) +{ + if (names.LastOrDefault() is { } last) + { + Console.WriteLine(last); + } +} +``` + +### Allowed +```cs +static string Example(IEnumerable names) + => names.FirstOrNone().GetOrElse("nobody"); + +static int Example(IEnumerable names) + => names.SingleOrNone().Select(name => name.Length).GetOrElse(0); + +static void Example(IEnumerable names) + => names.LastOrNone().Switch(none: NoOperation, some: Console.WriteLine); + +// A null element is a value here, not a missing element. +static string Example(IEnumerable names) + => names.FirstOrDefault() ?? "nobody"; +``` diff --git a/Funcky.Analyzers/Funcky.Analyzers.CodeFixes/CodeAnalysisExtensions/UsingDirectives.cs b/Funcky.Analyzers/Funcky.Analyzers.CodeFixes/CodeAnalysisExtensions/UsingDirectives.cs new file mode 100644 index 000000000..1fcba4f56 --- /dev/null +++ b/Funcky.Analyzers/Funcky.Analyzers.CodeFixes/CodeAnalysisExtensions/UsingDirectives.cs @@ -0,0 +1,86 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory; + +namespace Funcky.Analyzers.CodeAnalysisExtensions; + +internal static class UsingDirectives +{ + /// Adds using <namespace>; next to the existing usings: into the namespace declaration enclosing + /// when the usings are there, otherwise at the top of the file. The + /// must be a node of . + public static SyntaxNode AddUsing(SyntaxNode root, SyntaxNode anchor, string namespaceName) + => anchor.FirstAncestorOrSelf(namespaceDeclaration => namespaceDeclaration.Usings.Count > 0) is { } namespaceDeclaration + ? root.ReplaceNode(namespaceDeclaration, Insert(namespaceDeclaration, ParseUsingDirective($"using {namespaceName};"))) + : Insert((CompilationUnitSyntax)root, ParseUsingDirective($"using {namespaceName};")); + + /// Inserts the directive after the last using that is not inside a #if region, so that it is not swallowed by the + /// condition; when every using is conditional it goes in front of them all. It takes the indentation and line ending of its + /// neighbour, but none of its comments or directives: copying a leading #if would open a second region without an #endif. + public static TContainer Insert(TContainer container, UsingDirectiveSyntax usingDirective) + where TContainer : SyntaxNode + { + var usings = container switch + { + CompilationUnitSyntax compilationUnit => compilationUnit.Usings, + BaseNamespaceDeclarationSyntax namespaceDeclaration => namespaceDeclaration.Usings, + _ => default, + }; + var endOfLine = EndOfLine(container); + + return (usings.LastOrDefault(existing => !IsInsideConditionalRegion(existing)), usings.FirstOrDefault()) switch + { + ({ } anchor, _) => container.InsertNodesAfter(anchor, [usingDirective.WithLeadingTrivia(FormattingOnly(anchor.GetLeadingTrivia())).WithTrailingTrivia(FormattingOnly(anchor.GetTrailingTrivia()))]), + (null, { } first) => container.InsertNodesBefore(first, [usingDirective.WithLeadingTrivia(FormattingOnly(first.GetLeadingTrivia())).WithTrailingTrivia(endOfLine)]), + (null, null) => container switch + { + CompilationUnitSyntax compilationUnit => (TContainer)(SyntaxNode)WithFirstUsing(compilationUnit, usingDirective.WithTrailingTrivia(endOfLine, endOfLine)), + BaseNamespaceDeclarationSyntax namespaceDeclaration => (TContainer)(SyntaxNode)namespaceDeclaration.AddUsings(usingDirective.WithTrailingTrivia(endOfLine, endOfLine)), + _ => container, + }, + }; + } + + /// The directive is parsed rather than built from tokens so that it carries no elastic trivia, which the formatter would otherwise + /// normalize together with the surrounding line endings. + public static UsingDirectiveSyntax ParseUsingDirective(string directive) + => ParseCompilationUnit(directive).Usings.Single(); + + // `#define` and `#undef` must precede every token, the using included. In a file without usings they lead the first + // member, so they (and whatever stands before them) move in front of the new using; an `#if` or a comment stays where it is. + private static CompilationUnitSyntax WithFirstUsing(CompilationUnitSyntax compilationUnit, UsingDirectiveSyntax usingDirective) + { + var firstToken = compilationUnit.GetFirstToken(includeZeroWidth: true); + var leadingTrivia = firstToken.LeadingTrivia; + var headerLength = leadingTrivia.Select((trivia, index) => (Trivia: trivia, Index: index)).Where(pair => pair.Trivia.IsKind(SyntaxKind.DefineDirectiveTrivia) || pair.Trivia.IsKind(SyntaxKind.UndefDirectiveTrivia)).Select(pair => pair.Index + 1).LastOrDefault(); + + return headerLength == 0 + ? compilationUnit.AddUsings(usingDirective) + : compilationUnit + .ReplaceToken(firstToken, firstToken.WithLeadingTrivia(leadingTrivia.Skip(headerLength))) + .AddUsings(usingDirective.WithLeadingTrivia(leadingTrivia.Take(headerLength))); + } + + // Counting the directives before the using: more `#if` than `#endif` means it sits inside a region. + private static bool IsInsideConditionalRegion(UsingDirectiveSyntax usingDirective) + => (usingDirective.Ancestors().LastOrDefault() ?? usingDirective) + .DescendantNodes(descendIntoTrivia: true) + .OfType() + .Where(directive => directive.SpanStart < usingDirective.SpanStart) + .Sum(directive => directive switch + { + IfDirectiveTriviaSyntax => 1, + EndIfDirectiveTriviaSyntax => -1, + _ => 0, + }) > 0; + + private static SyntaxTriviaList FormattingOnly(SyntaxTriviaList trivia) + => TriviaList(trivia.Where(t => t.IsKind(SyntaxKind.WhitespaceTrivia) || t.IsKind(SyntaxKind.EndOfLineTrivia))); + + /// The line ending the file uses, so that the added directive does not introduce a mixed one. + private static SyntaxTrivia EndOfLine(SyntaxNode node) + => (node.Ancestors().LastOrDefault() ?? node).DescendantTrivia().FirstOrDefault(trivia => trivia.IsKind(SyntaxKind.EndOfLineTrivia)) is { RawKind: not 0 } endOfLine + ? endOfLine + : CarriageReturnLineFeed; +} diff --git a/Funcky.Analyzers/Funcky.Analyzers.CodeFixes/Functions/PreferNamedFunctionCodeFix.cs b/Funcky.Analyzers/Funcky.Analyzers.CodeFixes/Functions/PreferNamedFunctionCodeFix.cs index 9a4bb7378..d9b9f3e28 100644 --- a/Funcky.Analyzers/Funcky.Analyzers.CodeFixes/Functions/PreferNamedFunctionCodeFix.cs +++ b/Funcky.Analyzers/Funcky.Analyzers.CodeFixes/Functions/PreferNamedFunctionCodeFix.cs @@ -1,5 +1,6 @@ using System.Collections.Immutable; using System.Composition; +using Funcky.Analyzers.CodeAnalysisExtensions; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CodeActions; using Microsoft.CodeAnalysis.CodeFixes; @@ -109,53 +110,14 @@ private static SyntaxNode Replace(SemanticModel semanticModel, SyntaxNode root, (true, null) => WithUsingStatic((CompilationUnitSyntax)root.ReplaceNode(anonymousFunction, replacement), semanticModel, position: 0), }; - /// Inserts the directive after the last using that is not inside a #if region, so that it is not swallowed by the - /// condition; when every using is conditional it goes in front of them all. It takes the indentation and line ending of its neighbour. - /// The directive is parsed rather than built from tokens so that it carries no elastic trivia, which the formatter would otherwise - /// normalize together with the surrounding line endings. + /// The using static goes after the last using outside a #if region, see . private static TContainer WithUsingStatic(TContainer container, SemanticModel semanticModel, int position) where TContainer : SyntaxNode - { - var usings = container switch - { - CompilationUnitSyntax compilationUnit => compilationUnit.Usings, - BaseNamespaceDeclarationSyntax namespaceDeclaration => namespaceDeclaration.Usings, - _ => default, - }; - var usingDirective = ParseCompilationUnit($"using static {QualifiedFunctionalName(semanticModel, position)};").Usings.Single(); - - return (usings.LastOrDefault(usingDirective => !IsInsideConditionalRegion(usingDirective)), usings.FirstOrDefault()) switch - { - ({ } anchor, _) => container.InsertNodesAfter(anchor, [usingDirective.WithLeadingTrivia(FormattingOnly(anchor.GetLeadingTrivia())).WithTrailingTrivia(FormattingOnly(anchor.GetTrailingTrivia()))]), - (null, { } first) => container.InsertNodesBefore(first, [usingDirective.WithTrailingTrivia(CarriageReturnLineFeed)]), - (null, null) => container switch - { - CompilationUnitSyntax compilationUnit => (TContainer)(SyntaxNode)compilationUnit.AddUsings(usingDirective.WithTrailingTrivia(CarriageReturnLineFeed, CarriageReturnLineFeed)), - BaseNamespaceDeclarationSyntax namespaceDeclaration => (TContainer)(SyntaxNode)namespaceDeclaration.AddUsings(usingDirective.WithTrailingTrivia(CarriageReturnLineFeed, CarriageReturnLineFeed)), - _ => container, - }, - }; - } + => UsingDirectives.Insert(container, UsingDirectives.ParseUsingDirective($"using static {QualifiedFunctionalName(semanticModel, position)};")); // Inside a namespace that contains its own `Funcky` namespace a relative `Funcky.Functional` would bind to that one. private static string QualifiedFunctionalName(SemanticModel semanticModel, int position) => semanticModel.LookupNamespacesAndTypes(position, name: FunctionalNamespace).Any(symbol => symbol is not INamespaceSymbol { ContainingNamespace.IsGlobalNamespace: true }) ? $"global::{FunctionalNamespace}.{FunctionalTypeName}" : $"{FunctionalNamespace}.{FunctionalTypeName}"; - - // Counting the directives before the using: more `#if` than `#endif` means it sits inside a region. - private static bool IsInsideConditionalRegion(UsingDirectiveSyntax usingDirective) - => (usingDirective.Ancestors().LastOrDefault() ?? usingDirective) - .DescendantNodes(descendIntoTrivia: true) - .OfType() - .Where(directive => directive.SpanStart < usingDirective.SpanStart) - .Sum(directive => directive switch - { - IfDirectiveTriviaSyntax => 1, - EndIfDirectiveTriviaSyntax => -1, - _ => 0, - }) > 0; - - private static SyntaxTriviaList FormattingOnly(SyntaxTriviaList trivia) - => TriviaList(trivia.Where(t => t.IsKind(SyntaxKind.WhitespaceTrivia) || t.IsKind(SyntaxKind.EndOfLineTrivia))); } diff --git a/Funcky.Analyzers/Funcky.Analyzers.CodeFixes/OrNone/PreferOrNoneCodeFix.cs b/Funcky.Analyzers/Funcky.Analyzers.CodeFixes/OrNone/PreferOrNoneCodeFix.cs new file mode 100644 index 000000000..cc02d2093 --- /dev/null +++ b/Funcky.Analyzers/Funcky.Analyzers.CodeFixes/OrNone/PreferOrNoneCodeFix.cs @@ -0,0 +1,136 @@ +using System.Collections.Immutable; +using System.Composition; +using Funcky.Analyzers.CodeAnalysisExtensions; +using Funcky.Analyzers.Fallback; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Operations; +using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory; + +namespace Funcky.Analyzers.OrNone; + +/// Rewrites xs.FirstOrDefault() ?? fallback to xs.FirstOrNone().GetOrElse(fallback). +/// The other null checks (?., is null, …) have no single idiomatic shape, so they are reported without a fix. +[Shared] +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(PreferOrNoneCodeFix))] +public sealed class PreferOrNoneCodeFix : CodeFixProvider +{ + private const string ExtensionsNamespace = "Funcky.Extensions"; + + private const string GetOrElseMethodName = "GetOrElse"; + + private enum FallbackKind + { + Eager, + Lazy, + } + + public override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create(PreferOrNoneAnalyzer.DiagnosticId); + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + if (await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false) is not { } root + || await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false) is not { } semanticModel) + { + return; + } + + foreach (var diagnostic in context.Diagnostics) + { + if (root.FindInvocationExpression(diagnostic.Location.SourceSpan) is { Expression: MemberAccessExpressionSyntax memberAccess } invocation + && diagnostic.Properties.TryGetValue(PreferOrNoneAnalyzer.OrNoneMethodNameProperty, out var orNoneMethodName) + && orNoneMethodName is not null + && FindCoalesceExpression(invocation) is { } coalesce + && KeepsTheResultType(semanticModel, invocation, coalesce, context.CancellationToken) + && ClassifyFallback(semanticModel, Unparenthesized(coalesce.Right), context.CancellationToken) is { } fallbackKind) + { + context.RegisterCodeFix( + CodeAction.Create( + $"Replace with '{orNoneMethodName}().{GetOrElseMethodName}(…)'", + cancellationToken => ReplaceWithGetOrElseAsync(context.Document, new Rewrite(coalesce, invocation, memberAccess, orNoneMethodName, fallbackKind), cancellationToken), + nameof(PreferOrNoneCodeFix)), + diagnostic); + } + } + } + + private static BinaryExpressionSyntax? FindCoalesceExpression(InvocationExpressionSyntax invocation) + => SkipParentheses(invocation) is { Parent: BinaryExpressionSyntax { RawKind: (int)SyntaxKind.CoalesceExpression } coalesce } left && coalesce.Left == left + ? coalesce + : null; + + private static SyntaxNode SkipParentheses(SyntaxNode node) + => node.Parent is ParenthesizedExpressionSyntax parenthesized ? SkipParentheses(parenthesized) : node; + + // The operation tree has no node for parentheses in C#, and as an argument the fallback needs none. + private static ExpressionSyntax Unparenthesized(ExpressionSyntax expression) + => expression is ParenthesizedExpressionSyntax parenthesized ? Unparenthesized(parenthesized.Expression) : expression; + + /// A ?? may widen the type: values.FirstOrDefault() ?? new object() is an object, but + /// Option<string>.GetOrElse only takes a string. The fix is offered when the coalesce has the element type. + private static bool KeepsTheResultType(SemanticModel semanticModel, InvocationExpressionSyntax invocation, BinaryExpressionSyntax coalesce, CancellationToken cancellationToken) + => SymbolEquals(semanticModel.GetTypeInfo(coalesce, cancellationToken).Type, semanticModel.GetTypeInfo(invocation, cancellationToken).Type); + + /// A ?? evaluates its fallback only when the left side is null. A fallback that does work (a call, an object + /// creation, an interpolated string, a throw) therefore goes to the lazy overload GetOrElse(() => fallback), a plain + /// value to the eager one, so that the rewritten code is what λ1014, λ1015 and λ1016 ask for. There is no fix when the + /// work cannot move into a lambda (an await, a ref parameter, a variable declared in the expression). + private static FallbackKind? ClassifyFallback(SemanticModel semanticModel, ExpressionSyntax fallback, CancellationToken cancellationToken) + => (DoesWork(semanticModel, fallback, cancellationToken), LambdaWrapping.CanWrapInLambda(semanticModel, fallback, cancellationToken)) switch + { + (false, _) => FallbackKind.Eager, + (true, true) => FallbackKind.Lazy, + (true, false) => null, + }; + + private static bool DoesWork(SemanticModel semanticModel, ExpressionSyntax fallback, CancellationToken cancellationToken) + => semanticModel.GetOperation(fallback, cancellationToken) is { } operation + && DescendantsAndSelfOutsideFunctions(operation).Any(descendant => descendant + is IInvocationOperation + or IObjectCreationOperation or IAnonymousObjectCreationOperation or IArrayCreationOperation + or IDynamicInvocationOperation or IDynamicObjectCreationOperation + or IAwaitOperation or IInterpolatedStringOperation or IThrowOperation); + + // What happens inside a nested function is deferred already. + private static IEnumerable DescendantsAndSelfOutsideFunctions(IOperation operation) + => operation is IAnonymousFunctionOperation or ILocalFunctionOperation + ? [] + : [operation, .. operation.ChildOperations.SelectMany(DescendantsAndSelfOutsideFunctions)]; + + private static async Task ReplaceWithGetOrElseAsync(Document document, Rewrite rewrite, CancellationToken cancellationToken) + { + if (await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false) is not { } root + || await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false) is not { } semanticModel) + { + return document; + } + + var orNoneInvocation = rewrite.Invocation.WithExpression(rewrite.MemberAccess.WithName(RenamedTo(rewrite.MemberAccess.Name, rewrite.OrNoneMethodName))); + var getOrElse = InvocationExpression( + MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, orNoneInvocation.WithoutTrailingTrivia(), IdentifierName(GetOrElseMethodName)), + ArgumentList(SingletonSeparatedList(Argument(Fallback(Unparenthesized(rewrite.Coalesce.Right), rewrite.FallbackKind))))) + .WithTriviaFrom(rewrite.Coalesce); + + var newRoot = root.ReplaceNode(rewrite.Coalesce, getOrElse); + return document.WithSyntaxRoot(IsExtensionsNamespaceImported(semanticModel, rewrite.Invocation, orNoneInvocation) ? newRoot : UsingDirectives.AddUsing(newRoot, newRoot.FindNode(rewrite.Coalesce.Span), ExtensionsNamespace)); + } + + /// Only the identifier changes: explicit type arguments (FirstOrDefault<object>()) select the element type and stay. + private static SimpleNameSyntax RenamedTo(SimpleNameSyntax name, string newName) + => name.WithIdentifier(Identifier(newName).WithTriviaFrom(name.Identifier)); + + private static ExpressionSyntax Fallback(ExpressionSyntax fallback, FallbackKind fallbackKind) + => fallbackKind == FallbackKind.Lazy + ? ParenthesizedLambdaExpression(fallback.WithoutTrivia()) + : fallback.WithoutTrivia(); + + private static bool IsExtensionsNamespaceImported(SemanticModel semanticModel, InvocationExpressionSyntax original, InvocationExpressionSyntax orNoneInvocation) + => semanticModel.GetSpeculativeSymbolInfo(original.SpanStart, orNoneInvocation, SpeculativeBindingOption.BindAsExpression) is { Symbol: IMethodSymbol } or { CandidateSymbols.IsEmpty: false }; + + private sealed record Rewrite(BinaryExpressionSyntax Coalesce, InvocationExpressionSyntax Invocation, MemberAccessExpressionSyntax MemberAccess, string OrNoneMethodName, FallbackKind FallbackKind); +} diff --git a/Funcky.Analyzers/Funcky.Analyzers.Test/OrNone/PreferOrNoneTest.cs b/Funcky.Analyzers/Funcky.Analyzers.Test/OrNone/PreferOrNoneTest.cs new file mode 100644 index 000000000..c653caa4f --- /dev/null +++ b/Funcky.Analyzers/Funcky.Analyzers.Test/OrNone/PreferOrNoneTest.cs @@ -0,0 +1,408 @@ +using Microsoft.CodeAnalysis.Testing; +using Xunit; +using VerifyCS = Funcky.Analyzers.Test.CSharpCodeFixVerifier; + +namespace Funcky.Analyzers.Test.OrNone; + +public sealed class PreferOrNoneTest +{ + private const string FunckyStubs = """ + + namespace Funcky.Monads + { + public readonly struct Option + { + public TItem GetOrElse(TItem fallback) => fallback; + public TItem GetOrElse(System.Func fallback) => fallback(); + } + } + + namespace Funcky.Extensions + { + using System; + using System.Collections.Generic; + using Funcky.Monads; + + public static class EnumerableExtensions + { + public static Option FirstOrNone(this IEnumerable source) => default; + public static Option FirstOrNone(this IEnumerable source, Func predicate) => default; + public static Option LastOrNone(this IEnumerable source) => default; + public static Option LastOrNone(this IEnumerable source, Func predicate) => default; + public static Option SingleOrNone(this IEnumerable source) => default; + public static Option SingleOrNone(this IEnumerable source, Func predicate) => default; + public static Option ElementAtOrNone(this IEnumerable source, int index) => default; + } + } + """; + + [Fact] + public async Task CoalesceAfterFirstOrDefaultIsReportedAndRewrittenToGetOrElseWithUsing() + { + const string inputCode = """ + using System.Collections.Generic; + using System.Linq; + + public static class C + { + public static string M(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? "fallback"; + } + """; + const string fixedCode = """ + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static string M(IEnumerable values) => values.FirstOrNone().GetOrElse("fallback"); + } + """; + await VerifyCS.VerifyCodeFixAsync(inputCode + FunckyStubs, fixedCode + FunckyStubs); + } + + [Fact] + public async Task EveryOrDefaultMethodAndOverloadIsRewritten() + { + const string inputCode = """ + using System; + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static string First(IEnumerable values) => {|λ1012:values.FirstOrDefault(value => value.Length > 2)|} ?? "fallback"; + + public static string Last(IEnumerable values) => {|λ1012:values.LastOrDefault()|} ?? "fallback"; + + public static string Single(IEnumerable values) => {|λ1012:values.SingleOrDefault(value => value.Length > 2)|} ?? "fallback"; + + public static string ElementAt(IEnumerable values) => {|λ1012:values.ElementAtOrDefault(2)|} ?? "fallback"; + + public static string Parenthesized(IEnumerable values) => ({|λ1012:values.FirstOrDefault()|}) ?? "fallback"; + + public static string Throwing(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? throw new InvalidOperationException("empty"); + } + """; + const string fixedCode = """ + using System; + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static string First(IEnumerable values) => values.FirstOrNone(value => value.Length > 2).GetOrElse("fallback"); + + public static string Last(IEnumerable values) => values.LastOrNone().GetOrElse("fallback"); + + public static string Single(IEnumerable values) => values.SingleOrNone(value => value.Length > 2).GetOrElse("fallback"); + + public static string ElementAt(IEnumerable values) => values.ElementAtOrNone(2).GetOrElse("fallback"); + + public static string Parenthesized(IEnumerable values) => values.FirstOrNone().GetOrElse("fallback"); + + public static string Throwing(IEnumerable values) => values.FirstOrNone().GetOrElse(() => throw new InvalidOperationException("empty")); + } + """; + await VerifyCS.VerifyCodeFixAsync(inputCode + FunckyStubs, fixedCode + FunckyStubs); + } + + [Fact] + public async Task UsingIsAddedInsideTheNamespaceWhenTheUsingsAreThere() + { + const string inputCode = """ + namespace Example + { + using System.Collections.Generic; + using System.Linq; + + public static class C + { + public static string M(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? "fallback"; + } + } + """; + const string fixedCode = """ + namespace Example + { + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static string M(IEnumerable values) => values.FirstOrNone().GetOrElse("fallback"); + } + } + """; + await VerifyCS.VerifyCodeFixAsync(inputCode + FunckyStubs, fixedCode + FunckyStubs); + } + + [Fact] + public async Task OtherNullChecksAreReportedWithoutFix() + { + const string inputCode = """ + using System.Collections.Generic; + using System.Linq; + + public static class C + { + public static bool IsNull(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} is null; + + public static bool IsNotNull(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} is not null; + + public static bool EqualsNull(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} == null; + + public static bool NullNotEquals(IEnumerable values) => null != {|λ1012:values.LastOrDefault()|}; + + public static int Length(IEnumerable values) => {|λ1012:values.SingleOrDefault()|}?.Length ?? 0; + + public static int Pattern(IEnumerable values) + => {|λ1012:values.FirstOrDefault()|} is { } first ? first.Length : -1; + + public static int NegatedPattern(IEnumerable values) + => {|λ1012:values.FirstOrDefault()|} is not { } first ? -1 : first.Length; + + public static int TypePattern(IEnumerable values) + => {|λ1012:values.FirstOrDefault()|} is string text ? text.Length : -1; + } + """; + await VerifyCS.VerifyAnalyzerAsync(inputCode + FunckyStubs); + } + + [Fact] + public async Task OrDefaultWithoutNullCheckIsNotReported() + { + const string inputCode = """ + using System.Collections.Generic; + using System.Linq; + + public static class C + { + public static string M(IEnumerable values) => values.FirstOrDefault(); + + public static int N(IEnumerable values) + { + var first = values.FirstOrDefault(); + return first is null ? -1 : first.Length; + } + + public static bool O(IEnumerable values) => values.FirstOrDefault() == "hello"; + + public static bool P(IEnumerable values) => values.FirstOrDefault() is { Length: > 2 }; + } + """; + await VerifyCS.VerifyAnalyzerAsync(inputCode + FunckyStubs); + } + + [Fact] + public async Task ValueTypesNullableElementsAndQueryablesAreNotReported() + { + const string inputCode = """ + #nullable enable + using System.Collections.Generic; + using System.Linq; + + public static class C + { + public static int Int(IEnumerable values) => values.FirstOrDefault() == 0 ? -1 : 1; + + public static int NullableInt(IEnumerable values) => values.FirstOrDefault() ?? -1; + + public static string NullableString(IEnumerable values) => values.FirstOrDefault() ?? "fallback"; + + public static string Queryable(IQueryable values) => values.FirstOrDefault() ?? "fallback"; + + public static string Static(IEnumerable values) => Enumerable.FirstOrDefault(values) ?? "fallback"; + } + """; + await VerifyCS.VerifyAnalyzerAsync(inputCode + FunckyStubs); + } + + [Fact] + public async Task ExplicitTypeArgumentsStayWhenRenaming() + { + const string inputCode = """ + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static object M(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? new object(); + } + """; + const string fixedCode = """ + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static object M(IEnumerable values) => values.FirstOrNone().GetOrElse(() => new object()); + } + """; + await VerifyCS.VerifyCodeFixAsync(inputCode + FunckyStubs, fixedCode + FunckyStubs); + } + + [Fact] + public async Task CoalesceWithAWiderResultTypeIsReportedWithoutFix() + { + const string inputCode = """ + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static object M(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? new object(); + + public static object N(IEnumerable values, object fallback) => {|λ1012:values.FirstOrDefault()|} ?? fallback; + } + """; + await VerifyCS.VerifyCodeFixAsync(inputCode + FunckyStubs, inputCode + FunckyStubs); + } + + [Fact] + public async Task FallbackThatDoesWorkIsPassedLazily() + { + const string inputCode = """ + using System; + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static string Call(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? CreateFallback(); + + public static string Creation(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? new string('x', 3); + + public static string Interpolated(IEnumerable values, int count) => {|λ1012:values.FirstOrDefault()|} ?? $"fallback {count}"; + + public static string NestedThrow(IEnumerable values, string other) => {|λ1012:values.FirstOrDefault()|} ?? (other ?? throw new InvalidOperationException()); + + public static string Variable(IEnumerable values, string other) => {|λ1012:values.FirstOrDefault()|} ?? other; + + public static string Field(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? string.Empty; + + private static string CreateFallback() => "fallback"; + } + """; + const string fixedCode = """ + using System; + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static string Call(IEnumerable values) => values.FirstOrNone().GetOrElse(() => CreateFallback()); + + public static string Creation(IEnumerable values) => values.FirstOrNone().GetOrElse(() => new string('x', 3)); + + public static string Interpolated(IEnumerable values, int count) => values.FirstOrNone().GetOrElse(() => $"fallback {count}"); + + public static string NestedThrow(IEnumerable values, string other) => values.FirstOrNone().GetOrElse(() => other ?? throw new InvalidOperationException()); + + public static string Variable(IEnumerable values, string other) => values.FirstOrNone().GetOrElse(other); + + public static string Field(IEnumerable values) => values.FirstOrNone().GetOrElse(string.Empty); + + private static string CreateFallback() => "fallback"; + } + """; + await VerifyCS.VerifyCodeFixAsync(inputCode + FunckyStubs, fixedCode + FunckyStubs); + } + + [Fact] + public async Task FallbackThatCannotMoveIntoALambdaIsReportedWithoutFix() + { + const string inputCode = """ + using System.Collections.Generic; + using System.Linq; + using System.Threading.Tasks; + using Funcky.Extensions; + + public static class C + { + public static async Task Awaited(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? await Task.FromResult("fallback"); + + public static string RefParameter(IEnumerable values, ref string seed) => {|λ1012:values.FirstOrDefault()|} ?? Create(seed); + + private static string Create(string seed) => seed; + } + """; + await VerifyCS.VerifyCodeFixAsync(inputCode + FunckyStubs, inputCode + FunckyStubs); + } + + [Fact] + public async Task UsingIsAddedOutsideAConditionalRegion() + { + const string inputCode = """ + using System.Collections.Generic; + using System.Linq; + #if DEBUG + using System.Diagnostics; + #endif + + public static class C + { + public static string M(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? "fallback"; + } + """; + const string fixedCode = """ + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + #if DEBUG + using System.Diagnostics; + #endif + + public static class C + { + public static string M(IEnumerable values) => values.FirstOrNone().GetOrElse("fallback"); + } + """; + await VerifyCS.VerifyCodeFixAsync(inputCode + FunckyStubs, fixedCode + FunckyStubs); + } + + [Fact] + public async Task NonNullableElementsAreReportedWithNullableAnnotatedLinq() + { + const string inputCode = """ + #nullable enable + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static string M(IEnumerable values) => {|λ1012:values.FirstOrDefault()|} ?? "fallback"; + + public static string N(IEnumerable values) => values.FirstOrDefault() ?? "fallback"; + } + """; + const string fixedCode = """ + #nullable enable + using System.Collections.Generic; + using System.Linq; + using Funcky.Extensions; + + public static class C + { + public static string M(IEnumerable values) => values.FirstOrNone().GetOrElse("fallback"); + + public static string N(IEnumerable values) => values.FirstOrDefault() ?? "fallback"; + } + """; + await new VerifyCS.Test + { + ReferenceAssemblies = ReferenceAssemblies.Net.Net80, + TestCode = inputCode + FunckyStubs, + FixedCode = fixedCode + FunckyStubs, + }.RunAsync(); + } +} diff --git a/Funcky.Analyzers/Funcky.Analyzers/AnalyzerReleases.Unshipped.md b/Funcky.Analyzers/Funcky.Analyzers/AnalyzerReleases.Unshipped.md index d106f0489..732aee942 100644 --- a/Funcky.Analyzers/Funcky.Analyzers/AnalyzerReleases.Unshipped.md +++ b/Funcky.Analyzers/Funcky.Analyzers/AnalyzerReleases.Unshipped.md @@ -5,6 +5,7 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- λ1011 | Funcky.Style | Warning | AlternativeMonadAnalyzer +λ1012 | Funcky.Style | Warning | PreferOrNoneAnalyzer λ1013 | Funcky.Style | Info | SequenceReturnCollectionExpressionAnalyzer λ1014 | Funcky.Style | Warning | ThrowInEagerFallbackAnalyzer λ1015 | Funcky.Style | Info | EagerlyComputedFallbackAnalyzer diff --git a/Funcky.Analyzers/Funcky.Analyzers/OrNone/PreferOrNoneAnalyzer.cs b/Funcky.Analyzers/Funcky.Analyzers/OrNone/PreferOrNoneAnalyzer.cs new file mode 100644 index 000000000..f37f7bac1 --- /dev/null +++ b/Funcky.Analyzers/Funcky.Analyzers/OrNone/PreferOrNoneAnalyzer.cs @@ -0,0 +1,134 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace Funcky.Analyzers.OrNone; + +/// Reports FirstOrDefault, LastOrDefault, SingleOrDefault and ElementAtOrDefault +/// whose result is immediately tested for null. The …OrNone variants in Funcky.Extensions make the +/// missing element a None instead of a null that has to be checked by hand. +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class PreferOrNoneAnalyzer : DiagnosticAnalyzer +{ + public const string DiagnosticId = $"{DiagnosticName.Prefix}{DiagnosticName.Usage}12"; + + public const string OrNoneMethodNameProperty = nameof(OrNoneMethodNameProperty); + + private const string OrDefaultSuffix = "OrDefault"; + + private const string OrNoneSuffix = "OrNone"; + + private static readonly ImmutableArray OrDefaultMethodNames = ImmutableArray.Create("FirstOrDefault", "LastOrDefault", "SingleOrDefault", "ElementAtOrDefault"); + + private static readonly DiagnosticDescriptor Descriptor = new( + id: DiagnosticId, + title: "Prefer the …OrNone method over …OrDefault with a null check", + messageFormat: "Prefer '{0}' over '{1}' followed by a null check", + category: DiagnosticName.Category, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "FirstOrDefault, LastOrDefault, SingleOrDefault and ElementAtOrDefault signal a missing element with null, which the caller then has to test for. The …OrNone variants return an Option, so the missing element is a None and the handling is a GetOrElse, Select or Match instead of a null check.", + helpLinkUri: DiagnosticName.HelpLink(DiagnosticId)); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Descriptor); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterCompilationStartAction(OnCompilationStart); + } + + private static void OnCompilationStart(CompilationStartAnalysisContext context) + { + if (context.Compilation.GetEnumerableType() is { } enumerableType + && context.Compilation.GetEnumerableExtensionType() is { } enumerableExtensionsType) + { + context.RegisterOperationAction(AnalyzeInvocation(enumerableType, enumerableExtensionsType), OperationKind.Invocation); + } + } + + private static Action AnalyzeInvocation(INamedTypeSymbol enumerableType, INamedTypeSymbol enumerableExtensionsType) + => context => + { + var invocation = (IInvocationOperation)context.Operation; + + if (MatchOrDefaultMethod(invocation, enumerableType) is { } orDefaultMethodName + && OrNoneMethodName(orDefaultMethodName) is var orNoneMethodName + && HasOrNoneVariant(enumerableExtensionsType, orNoneMethodName) + && IsNullChecked(invocation)) + { + context.ReportDiagnostic(Diagnostic.Create( + Descriptor, + invocation.Syntax.GetLocation(), + ImmutableDictionary.Empty.Add(OrNoneMethodNameProperty, orNoneMethodName), + orNoneMethodName, + orDefaultMethodName)); + } + }; + + /// The LINQ methods called in extension form, without the overloads that take an explicit default value, over a + /// reference type that is not nullable-annotated: there a null result can only mean "no element". The element type + /// is the type argument, not the return type: the nullable-annotated LINQ returns TSource? for every sequence. + /// The operation tree presents an extension call unreduced, with the receiver as the first argument. + private static string? MatchOrDefaultMethod(IInvocationOperation invocation, INamedTypeSymbol enumerableType) + => invocation.TargetMethod is { IsExtensionMethod: true, TypeArguments: [{ IsReferenceType: true, NullableAnnotation: not NullableAnnotation.Annotated }] } method + && SymbolEquals(method.ContainingType, enumerableType) + && OrDefaultMethodNames.Contains(method.Name) + && method.Parameters.All(parameter => parameter.Name != "defaultValue") + && IsCalledInExtensionForm(invocation) + ? method.Name + : null; + + private static bool IsCalledInExtensionForm(IInvocationOperation invocation) + => invocation.Syntax is InvocationExpressionSyntax { Expression: MemberAccessExpressionSyntax, ArgumentList.Arguments.Count: var argumentCount } + && argumentCount == invocation.TargetMethod.Parameters.Length - 1; + + private static string OrNoneMethodName(string orDefaultMethodName) + => orDefaultMethodName.Substring(0, orDefaultMethodName.Length - OrDefaultSuffix.Length) + OrNoneSuffix; + + private static bool HasOrNoneVariant(INamedTypeSymbol enumerableExtensionsType, string orNoneMethodName) + => enumerableExtensionsType.GetMembers(orNoneMethodName).Any(member => member is IMethodSymbol { IsStatic: true, IsExtensionMethod: true }); + + /// Whether the result of the invocation is consumed by a null test: ??, ?., == null, != null + /// or an is pattern that distinguishes null from a value. + private static bool IsNullChecked(IInvocationOperation invocation) + { + var (child, parent) = SkipTransparentParents(invocation); + return parent switch + { + ICoalesceOperation coalesce => coalesce.Value == child, + IConditionalAccessOperation conditionalAccess => conditionalAccess.Operation == child, + IIsPatternOperation isPattern => isPattern.Value == child && IsNullTest(isPattern.Pattern), + IBinaryOperation { OperatorKind: BinaryOperatorKind.Equals or BinaryOperatorKind.NotEquals } binary + => (binary.LeftOperand == child && IsNullLiteral(binary.RightOperand)) || (binary.RightOperand == child && IsNullLiteral(binary.LeftOperand)), + _ => false, + }; + } + + private static (IOperation Child, IOperation? Parent) SkipTransparentParents(IOperation operation) + => operation.Parent is IParenthesizedOperation or IConversionOperation { IsImplicit: true } + ? SkipTransparentParents(operation.Parent) + : (operation, operation.Parent); + + private static bool IsNullTest(IPatternOperation pattern) + => pattern switch + { + IConstantPatternOperation constantPattern => IsNullLiteral(constantPattern.Value), + INegatedPatternOperation negatedPattern => IsNullTest(negatedPattern.Pattern), + IRecursivePatternOperation { DeconstructionSubpatterns.IsEmpty: true, PropertySubpatterns.IsEmpty: true } => true, + IDeclarationPatternOperation { MatchesNull: false } => true, + ITypePatternOperation => true, + _ => false, + }; + + private static bool IsNullLiteral(IOperation operation) + => operation switch + { + ILiteralOperation { ConstantValue: { HasValue: true, Value: null } } => true, + IConversionOperation { IsImplicit: true } conversion => IsNullLiteral(conversion.Operand), + _ => false, + }; +} diff --git a/changelog.md b/changelog.md index 7960cdff2..1b74b7075 100644 --- a/changelog.md +++ b/changelog.md @@ -60,6 +60,11 @@ warnings (an info for pure performance hints), come with a code fix and carry th `null` with an `ArgumentNullException`, so `Option option = null;` and even `option == null` throw at run time; with nullable reference types disabled the compiler is silent about it. The code fix replaces the literal with `Option.None`. +* New: rule `λ1012` reports `FirstOrDefault`, `LastOrDefault`, `SingleOrDefault` and `ElementAtOrDefault` whose result is + immediately tested for `null` (`?? fallback`, `?.`, `is null`, `is {} x`, `== null`, …) and suggests the `…OrNone` + variant, which makes the missing element a `None` instead of a `null`. The code fix rewrites `xs.FirstOrDefault() ?? fallback` + to `xs.FirstOrNone().GetOrElse(fallback)`, passes a fallback that does work as a function and adds the + `using Funcky.Extensions;` when needed. ## Unreleased: Funcky 3.7.0 | Funcky.Analyzers 1.5.0