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