Add analyzer λ1012: prefer the …OrNone method over …OrDefault with a null check

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)

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 |

Documentation/src/analyzer-rules/λ1012.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<T>` 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<string?>`) 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<T>`, 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<string>.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<string> names)
+    => names.FirstOrDefault() ?? "nobody";
+
+static int Example(IEnumerable<string> names)
+    => names.SingleOrDefault()?.Length ?? 0;
+
+static void Example(IEnumerable<string> names)
+{
+    if (names.LastOrDefault() is { } last)
+    {
+        Console.WriteLine(last);
+    }
+}
+```
+
+### Allowed
+```cs
+static string Example(IEnumerable<string> names)
+    => names.FirstOrNone().GetOrElse("nobody");
+
+static int Example(IEnumerable<string> names)
+    => names.SingleOrNone().Select(name => name.Length).GetOrElse(0);
+
+static void Example(IEnumerable<string> names)
+    => names.LastOrNone().Switch(none: NoOperation, some: Console.WriteLine);
+
+// A null element is a value here, not a missing element.
+static string Example(IEnumerable<string?> names)
+    => names.FirstOrDefault() ?? "nobody";
+```

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
+{
+    /// <summary>Adds <c>using &lt;namespace&gt;;</c> next to the existing usings: into the namespace declaration enclosing
+    /// <paramref name="anchor"/> when the usings are there, otherwise at the top of the file. The <paramref name="anchor"/>
+    /// must be a node of <paramref name="root"/>.</summary>
+    public static SyntaxNode AddUsing(SyntaxNode root, SyntaxNode anchor, string namespaceName)
+        => anchor.FirstAncestorOrSelf<BaseNamespaceDeclarationSyntax>(namespaceDeclaration => namespaceDeclaration.Usings.Count > 0) is { } namespaceDeclaration
+            ? root.ReplaceNode(namespaceDeclaration, Insert(namespaceDeclaration, ParseUsingDirective($"using {namespaceName};")))
+            : Insert((CompilationUnitSyntax)root, ParseUsingDirective($"using {namespaceName};"));
+
+    /// <summary>Inserts the directive after the last using that is not inside a <c>#if</c> 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 <c>#if</c> would open a second region without an <c>#endif</c>.</summary>
+    public static TContainer Insert<TContainer>(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,
+            },
+        };
+    }
+
+    /// <summary>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.</summary>
+    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<DirectiveTriviaSyntax>()
+            .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)));
+
+    /// <summary>The line ending the file uses, so that the added directive does not introduce a mixed one.</summary>
+    private static SyntaxTrivia EndOfLine(SyntaxNode node)
+        => (node.Ancestors().LastOrDefault() ?? node).DescendantTrivia().FirstOrDefault(trivia => trivia.IsKind(SyntaxKind.EndOfLineTrivia)) is { RawKind: not 0 } endOfLine
+            ? endOfLine
+            : CarriageReturnLineFeed;
+}

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),
         };
 
-    /// <summary>Inserts the directive after the last using that is not inside a <c>#if</c> 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.</summary>
+    /// <summary>The <c>using static</c> goes after the last using outside a <c>#if</c> region, see <see cref="UsingDirectives.Insert{TContainer}"/>.</summary>
     private static TContainer WithUsingStatic<TContainer>(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<DirectiveTriviaSyntax>()
-            .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)));
 }