From 624164de9361226c601c365e2721664e7c3846ed Mon Sep 17 00:00:00 2001 From: Florian Verdonck Date: Sat, 12 Sep 2026 21:35:18 +0200 Subject: [PATCH] Add an analyzer for a match where an if would do A match is for taking a value apart. One that only asks whether a boolean is true tells the reader to expect a destructuring and then does not deliver one, so FANTOMAS-SNOBMATCH-001 asks for an if instead. The rule is held to the boolean case, where the rewrite is mechanical: the scrutinee becomes the condition and is written once. The shape next door, a constant arm and a binder holding the scrutinee under a second name, is left alone, because every instance of it in this repository is the IndexOf idiom, where an if has to call IndexOf twice or bind it above. It also stays quiet on match!, on function, on a when guard and on a conditional directive inside the match, each because the rewrite would be more than a rewrite. List.partitionWhile held the only instance and is now an if. --- analyzers/AGENTS.md | 58 ++++++++ .../Fantomas.Analyzers.Tests.fsproj | 1 + .../SnobMatchAnalyzerTests.fs | 136 ++++++++++++++++++ .../Fantomas.Analyzers.fsproj | 2 + .../Fantomas.Analyzers/SnobMatchAnalyzer.fs | 92 ++++++++++++ .../Fantomas.Analyzers/SnobMatchAnalyzer.fsi | 30 ++++ src/Fantomas.Core/Utils.fs | 7 +- 7 files changed, 323 insertions(+), 3 deletions(-) create mode 100644 analyzers/Fantomas.Analyzers.Tests/SnobMatchAnalyzerTests.fs create mode 100644 analyzers/Fantomas.Analyzers/SnobMatchAnalyzer.fs create mode 100644 analyzers/Fantomas.Analyzers/SnobMatchAnalyzer.fsi 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