diff --git a/analyzers/AGENTS.md b/analyzers/AGENTS.md
index 7e3cacecb3..17be0ea68c 100644
--- a/analyzers/AGENTS.md
+++ b/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
diff --git a/analyzers/Fantomas.Analyzers.Tests/Fantomas.Analyzers.Tests.fsproj b/analyzers/Fantomas.Analyzers.Tests/Fantomas.Analyzers.Tests.fsproj
index 164874e836..ef23d8b8f2 100644
--- a/analyzers/Fantomas.Analyzers.Tests/Fantomas.Analyzers.Tests.fsproj
+++ b/analyzers/Fantomas.Analyzers.Tests/Fantomas.Analyzers.Tests.fsproj
@@ -23,6 +23,7 @@
+
diff --git a/analyzers/Fantomas.Analyzers.Tests/SnobMatchAnalyzerTests.fs b/analyzers/Fantomas.Analyzers.Tests/SnobMatchAnalyzerTests.fs
new file mode 100644
index 0000000000..3b8f84cedf
--- /dev/null
+++ b/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
+
+[]
+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 ]
+
+[]
+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 ]
+
+[]
+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.
+[]
+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 ]
+
+[]
+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.
+[]
+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 []
+
+[]
+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 []
+
+[]
+let ``a match bang is not reported`` () =
+ let source: string =
+ """module M
+
+let f (x: Async) : Async =
+ async {
+ match! x with
+ | true -> return 1
+ | false -> return 0
+ }"""
+
+ analyzeSource cliAnalyzer source |> assertLines []
+
+[]
+let ``a function is not reported`` () =
+ let source: string =
+ """module M
+
+let f: bool -> int =
+ function
+ | true -> 1
+ | false -> 0"""
+
+ analyzeSource cliAnalyzer source |> assertLines []
+
+[]
+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 []
diff --git a/analyzers/Fantomas.Analyzers/Fantomas.Analyzers.fsproj b/analyzers/Fantomas.Analyzers/Fantomas.Analyzers.fsproj
index df531a4003..2f22639a44 100644
--- a/analyzers/Fantomas.Analyzers/Fantomas.Analyzers.fsproj
+++ b/analyzers/Fantomas.Analyzers/Fantomas.Analyzers.fsproj
@@ -45,6 +45,8 @@
+
+
diff --git a/analyzers/Fantomas.Analyzers/SnobMatchAnalyzer.fs b/analyzers/Fantomas.Analyzers/SnobMatchAnalyzer.fs
new file mode 100644
index 0000000000..adc061f0a9
--- /dev/null
+++ b/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
+
+[]
+let Code: string = "FANTOMAS-SNOBMATCH-001"
+
+[]
+let Name: string = "SnobMatchAnalyzer"
+
+[]
+let ShortDescription: string =
+ "Detects a two armed match on a boolean, which is an if expression dressed up as pattern matching."
+
+[]
+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 = ResizeArray()
+
+ 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 =
+ async { return analyze ctx.ParseFileResults.ParseTree }
+
+let editorAnalyzer (ctx: EditorContext) : Async =
+ async { return analyze ctx.ParseFileResults.ParseTree }
diff --git a/analyzers/Fantomas.Analyzers/SnobMatchAnalyzer.fsi b/analyzers/Fantomas.Analyzers/SnobMatchAnalyzer.fsi
new file mode 100644
index 0000000000..4027cd6a29
--- /dev/null
+++ b/analyzers/Fantomas.Analyzers/SnobMatchAnalyzer.fsi
@@ -0,0 +1,30 @@
+module Fantomas.Analyzers.SnobMatchAnalyzer
+
+open FSharp.Analyzers.SDK
+
+[]
+val Code: string = "FANTOMAS-SNOBMATCH-001"
+
+[]
+val Name: string = "SnobMatchAnalyzer"
+
+[]
+val ShortDescription: string =
+ "Detects a two armed match on a boolean, which is an if expression dressed up as pattern matching."
+
+[]
+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.
+[]
+val cliAnalyzer: ctx: CliContext -> Async
+
+[]
+val editorAnalyzer: ctx: EditorContext -> Async
diff --git a/src/Fantomas.Core/Utils.fs b/src/Fantomas.Core/Utils.fs
index c7f0f68593..cb86fa12bc 100644
--- a/src/Fantomas.Core/Utils.fs
+++ b/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