Add an analyzer for a match where an if would do

analyzers/AGENTS.md

@@ -16,6 +16,7 @@ feedback arrives while you work instead of in review. They are ordinary F# analy
 | [`FANTOMAS-XMLDOC-001`](#fantomas-xmldoc-001) | No doc comment the signature file already carries |
 | [`FANTOMAS-OPENS-001`](#fantomas-opens-001) | No `open` nothing in the file uses |
 | [`FANTOMAS-PARENS-001`](#fantomas-parens-001) | No parentheses the code parses the same without |
+| [`FANTOMAS-SNOBMATCH-001`](#fantomas-snobmatch-001) | No `match` where an `if` would do |
 
 ## FANTOMAS-PIPEBACK-001
 
@@ -321,6 +322,63 @@ The rule reports debt that predates it, so it is guidance for code you are writi
 rather than a reason to sweep the codebase. Remove the pairs in the code you touch. Leave the ones
 you had no reason to open alone.
 
+## FANTOMAS-SNOBMATCH-001
+
+Do not write a `match` where an `if` would do:
+
+```fsharp
+if f i head then
+    go (i + 1) (head :: before) tail
+else
+    List.rev before, after
+```
+
+rather than
+
+```fsharp
+match f i head with
+| true -> go (i + 1) (head :: before) tail
+| false -> List.rev before, after
+```
+
+A `match` is for taking a value apart. Where it is only asking whether something is true, it tells
+the reader to expect a destructuring and then does not deliver one, and it spends two `|` and two
+patterns on a question that has a keyword of its own. That is what the name is about: the match is
+dressed up for an occasion the code is not having.
+
+It speaks only for the boolean case, which is the one where the rewrite is mechanical. Both arms
+are `true` and `false` in either order, or one of them against a wildcard, and either way the
+scrutinee becomes the condition and is written once, exactly where it already was. Nothing is
+duplicated, nothing new is bound, and there is no judgement to make about whether the scrutinee is
+cheap enough to evaluate twice.
+
+The shape next door is the one this deliberately leaves alone, a constant arm and a binder holding
+the scrutinee under a second name:
+
+```fsharp
+match text.IndexOf('=') with
+| -1 -> text
+| at -> text.Substring(0, at)
+```
+
+Every instance of that in this repository is this `IndexOf` idiom, where an `if` would have to call
+`IndexOf` twice or grow a `let` above it, so reporting it would be asking for a change worth less
+than the match it replaced. Should a case turn up where the binder is dead weight, the rule can grow
+to reach it.
+
+Three things it says nothing about, each because the rewrite is more than a rewrite:
+
+- **`match!`**, which would need a `let!` above the `if` to have a value to test.
+- **`function`**, which would need a parameter invented to have something to test.
+- **A `when` guard** on either arm, which means the two arms no longer cover the scrutinee between
+  them and the match is asking something its patterns do not say.
+
+It also stays quiet on a conditional directive inside the match, like the other rules that read
+arms, because the two arms it sees are then not the arms every build sees.
+
+**The reported range is the whole match expression**, since the whole of it is what an `if`
+replaces. There is no fix attached, for the reason every other rule here has none.
+
 ## Suppressing a finding
 
 Use the SDK's comment syntax rather than reshaping the code around it. The SDK filters the messages

analyzers/Fantomas.Analyzers.Tests/Fantomas.Analyzers.Tests.fsproj

@@ -23,6 +23,7 @@
     <Compile Include="XmlDocAnalyzerTests.fs" />
     <Compile Include="UnusedOpensAnalyzerTests.fs" />
     <Compile Include="UnnecessaryParensAnalyzerTests.fs" />
+    <Compile Include="SnobMatchAnalyzerTests.fs" />
   </ItemGroup>
   <ItemGroup>
     <ProjectReference Include="..\Fantomas.Analyzers\Fantomas.Analyzers.fsproj" />

analyzers/Fantomas.Analyzers.Tests/SnobMatchAnalyzerTests.fs

@@ -0,0 +1,136 @@
+module Fantomas.Analyzers.Tests.SnobMatchAnalyzerTests
+
+open NUnit.Framework
+open Fantomas.Analyzers.Tests.TestHelpers
+open Fantomas.Analyzers.SnobMatchAnalyzer
+
+[<Test>]
+let ``a match on true and false is reported`` () =
+    let source: string =
+        """module M
+
+let f (x: bool) : int =
+    match x with
+    | true -> 1
+    | false -> 0"""
+
+    analyzeSource cliAnalyzer source |> assertLines [ 4 ]
+
+[<Test>]
+let ``a match on false and true is reported`` () =
+    let source: string =
+        """module M
+
+let f (x: bool) : int =
+    match x with
+    | false -> 0
+    | true -> 1"""
+
+    analyzeSource cliAnalyzer source |> assertLines [ 4 ]
+
+[<Test>]
+let ``a match on true and a wildcard is reported`` () =
+    let source: string =
+        """module M
+
+let f (x: bool) : int =
+    match x with
+    | true -> 1
+    | _ -> 0"""
+
+    analyzeSource cliAnalyzer source |> assertLines [ 4 ]
+
+// The scrutinee is an expression rather than a name, and stays one: the rewrite moves it into the
+// condition and writes it once, which is what holds this rule to the boolean case.
+[<Test>]
+let ``a match on a call returning a boolean is reported`` () =
+    let source: string =
+        """module M
+
+let f (xs: int list) : int =
+    match List.isEmpty xs with
+    | true -> 0
+    | false -> List.head xs"""
+
+    analyzeSource cliAnalyzer source |> assertLines [ 4 ]
+
+[<Test>]
+let ``a match on a union is not reported`` () =
+    let source: string =
+        """module M
+
+let f (x: int option) : int =
+    match x with
+    | Some value -> value
+    | None -> 0"""
+
+    analyzeSource cliAnalyzer source |> assertLines []
+
+// The wider shape the rule could grow into, and deliberately not part of it yet: the binder holds
+// the scrutinee, so an `if` has to either evaluate it twice or bind it above.
+[<Test>]
+let ``a match on an integer constant and a binder is not reported`` () =
+    let source: string =
+        """module M
+
+let f (text: string) : string =
+    match text.IndexOf('=') with
+    | -1 -> text
+    | at -> text.Substring(0, at)"""
+
+    analyzeSource cliAnalyzer source |> assertLines []
+
+[<Test>]
+let ``a guarded arm is not reported`` () =
+    let source: string =
+        """module M
+
+let f (x: bool) (y: bool) : int =
+    match x with
+    | true when y -> 1
+    | _ -> 0"""
+
+    analyzeSource cliAnalyzer source |> assertLines []
+
+[<Test>]
+let ``a match bang is not reported`` () =
+    let source: string =
+        """module M
+
+let f (x: Async<bool>) : Async<int> =
+    async {
+        match! x with
+        | true -> return 1
+        | false -> return 0
+    }"""
+
+    analyzeSource cliAnalyzer source |> assertLines []
+
+[<Test>]
+let ``a function is not reported`` () =
+    let source: string =
+        """module M
+
+let f: bool -> int =
+    function
+    | true -> 1
+    | false -> 0"""
+
+    analyzeSource cliAnalyzer source |> assertLines []
+
+[<Test>]
+let ``a conditional directive inside the match is not reported`` () =
+    let source: string =
+        """module M
+
+let f (x: bool) : int =
+    match x with
+    | true ->
+#if DEBUG
+        1
+#else
+        2
+#endif
+    | false -> 0"""
+
+    analyzeSource cliAnalyzer source |> assertLines []

analyzers/Fantomas.Analyzers/Fantomas.Analyzers.fsproj

@@ -45,6 +45,8 @@
     <Compile Include="UnusedOpensAnalyzer.fs" />
     <Compile Include="UnnecessaryParensAnalyzer.fsi" />
     <Compile Include="UnnecessaryParensAnalyzer.fs" />
+    <Compile Include="SnobMatchAnalyzer.fsi" />
+    <Compile Include="SnobMatchAnalyzer.fs" />
   </ItemGroup>
   <ItemGroup>
     <!-- Must track the fsharp-analyzers version pinned in .config/dotnet-tools.json. -->

analyzers/Fantomas.Analyzers/SnobMatchAnalyzer.fs

@@ -0,0 +1,92 @@
+module Fantomas.Analyzers.SnobMatchAnalyzer
+
+open FSharp.Analyzers.SDK
+open FSharp.Analyzers.SDK.ASTCollecting
+open FSharp.Compiler.Syntax
+open FSharp.Compiler.Text
+open Fantomas.Analyzers.Common
+
+[<Literal>]
+let Code: string = "FANTOMAS-SNOBMATCH-001"
+
+[<Literal>]
+let Name: string = "SnobMatchAnalyzer"
+
+[<Literal>]
+let ShortDescription: string =
+    "Detects a two armed match on a boolean, which is an if expression dressed up as pattern matching."
+
+[<Literal>]
+let HelpUri: string =
+    "https://github.com/fsprojects/fantomas/blob/main/analyzers/AGENTS.md#fantomas-snobmatch-001"
+
+// Whether a pair of arm patterns is a boolean test written out as two arms.
+//
+// `true` against `false` is the whole of it, in either order, and `true` or `false` against a
+// wildcard says the same thing with the other value left unnamed. A boolean has two values, so
+// these cover the scrutinee between them and the second arm is reached exactly when the first is
+// not, which is what an `if` means.
+let isBooleanTest (first: SynPat) (second: SynPat) : bool =
+    match first, second with
+    | SynPat.Const(SynConst.Bool _, _), SynPat.Wild _ -> true
+    | SynPat.Const(SynConst.Bool firstValue, _), SynPat.Const(SynConst.Bool secondValue, _) -> firstValue <> secondValue
+    | _ -> false
+
+// Whether this expression is a match an `if` would say better.
+//
+// `SynExpr.Match` alone, where the two rules about arm layout reach through `matchClausesOf` for
+// `match!` and `function` as well. This rule is about a rewrite rather than about layout, and the
+// rewrite differs per form: a `match!` on a boolean needs a `let!` before it can be an `if`, and a
+// `function` needs a parameter invented to have something to test. Both are more than the rule is
+// worth, so both are left alone.
+//
+// A `when` guard on either arm means the arms no longer cover the scrutinee between them, so the
+// match is asking something the patterns do not say. A conditional directive inside means the two
+// arms this reads are not the arms every build sees.
+let shouldBeAnIf (directives: range list) (expr: SynExpr) : range option =
+    match expr with
+    | SynExpr.Match(
+        clauses = [ SynMatchClause(pat = first; whenExpr = None); SynMatchClause(pat = second; whenExpr = None) ]
+        range = matchRange) when isBooleanTest first second ->
+
+        let holdsADirective: bool =
+            directives
+            |> List.exists (fun (directive: range) -> Range.rangeContainsRange matchRange directive)
+
+        if holdsADirective then None else Some matchRange
+    | _ -> None
+
+// Reported on the whole match expression, because the whole of it is what goes.
+let analyze (parsedInput: ParsedInput) : Message list =
+    let _, directives = triviaOf parsedInput
+    let findings: ResizeArray<range> = ResizeArray<range>()
+
+    let walker: SyntaxCollectorBase =
+        { new SyntaxCollectorBase() with
+            override _.WalkExpr(_path: SyntaxVisitorPath, expr: SynExpr) : unit =
+                match shouldBeAnIf directives expr with
+                | None -> ()
+                | Some matchRange -> findings.Add matchRange
+        }
+
+    walkAst walker parsedInput
+
+    findings
+    |> Seq.map (fun (matchRange: range) ->
+        {
+            Type = Name
+            Message =
+                "Write this as an `if`. A `match` is for taking a value apart, and this one only asks whether a boolean is true, which `if ... then ... else ...` says with no pattern in sight and the scrutinee still written once."
+            Code = Code
+            Severity = Severity.Warning
+            Range = matchRange
+            Fixes = []
+        }
+    )
+    |> Seq.toList
+
+let cliAnalyzer (ctx: CliContext) : Async<Message list> =
+    async { return analyze ctx.ParseFileResults.ParseTree }
+
+let editorAnalyzer (ctx: EditorContext) : Async<Message list> =
+    async { return analyze ctx.ParseFileResults.ParseTree }

analyzers/Fantomas.Analyzers/SnobMatchAnalyzer.fsi

@@ -0,0 +1,30 @@
+module Fantomas.Analyzers.SnobMatchAnalyzer
+
+open FSharp.Analyzers.SDK
+
+[<Literal>]
+val Code: string = "FANTOMAS-SNOBMATCH-001"
+
+[<Literal>]
+val Name: string = "SnobMatchAnalyzer"
+
+[<Literal>]
+val ShortDescription: string =
+    "Detects a two armed match on a boolean, which is an if expression dressed up as pattern matching."
+
+[<Literal>]
+val HelpUri: string = "https://github.com/fsprojects/fantomas/blob/main/analyzers/AGENTS.md#fantomas-snobmatch-001"
+
+/// Reports a `match` on a boolean, on the whole expression, because the whole of it is what an
+/// `if` replaces.
+///
+/// This is the narrowest shape of the rule and the only one where the rewrite is mechanical: the
+/// scrutinee becomes the condition and is written once, exactly where it was. It stays quiet on a
+/// guard, on anything other than two arms, on `match!` and `function`, which cannot be rewritten
+/// without inventing a `let!` or a parameter, and on a conditional directive inside the match. No
+/// fix is offered, as with every rule here.
+[<CliAnalyzer(Name, ShortDescription, HelpUri)>]
+val cliAnalyzer: ctx: CliContext -> Async<Message list>
+
+[<EditorAnalyzer(Name, ShortDescription, HelpUri)>]
+val editorAnalyzer: ctx: EditorContext -> Async<Message list>

src/Fantomas.Core/Utils.fs

@@ -71,9 +71,10 @@ module List =
             | [] -> List.rev before, after
             | head :: tail ->
 
-            match f i head with
-            | true -> go (i + 1) (head :: before) tail
-            | false -> List.rev before, after
+            if f i head then
+                go (i + 1) (head :: before) tail
+            else
+                List.rev before, after
 
         go 0 [] xs