From c05e7d95d1a679dc73ec72d2cd1af9f7f98f2a49 Mon Sep 17 00:00:00 2001 From: Jason Gardella Date: Fri, 12 Jun 2020 17:06:57 -0400 Subject: [PATCH 01/10] Start implementing rule for no partial functions --- .../Application/Configuration.fs | 5 +- src/FSharpLint.Core/DefaultConfiguration.json | 1 + src/FSharpLint.Core/FSharpLint.Core.fsproj | 1 + .../Framework/AbstractSyntaxArray.fs | 2 +- src/FSharpLint.Core/Framework/Ast.fs | 14 ++--- .../Rules/Conventions/NoPartialFunctions.fs | 54 +++++++++++++++++++ src/FSharpLint.Core/Rules/Identifiers.fs | 1 + .../FSharpLint.Core.Tests.fsproj | 1 + .../Rules/Conventions/NoPartialFunctions.fs | 22 ++++++++ .../Rules/Hints/HintMatcher.fs | 6 +-- .../Rules/TestRuleBase.fs | 5 +- 11 files changed, 99 insertions(+), 13 deletions(-) create mode 100644 src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs create mode 100644 tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs diff --git a/src/FSharpLint.Core/Application/Configuration.fs b/src/FSharpLint.Core/Application/Configuration.fs index 82808ddd8..bed4201aa 100644 --- a/src/FSharpLint.Core/Application/Configuration.fs +++ b/src/FSharpLint.Core/Application/Configuration.fs @@ -422,7 +422,8 @@ type Configuration = TrailingWhitespaceOnLine:RuleConfig option MaxLinesInFile:RuleConfig option TrailingNewLineInFile:EnabledConfig option - NoTabCharacters:EnabledConfig option } + NoTabCharacters:EnabledConfig option + NoPartialFunctions:EnabledConfig option } with static member Zero = { Global = None @@ -496,6 +497,7 @@ with MaxLinesInFile = None TrailingNewLineInFile = None NoTabCharacters = None + NoPartialFunctions = None } // fsharplint:enable RecordFieldNames @@ -632,6 +634,7 @@ let flattenConfig (config:Configuration) = config.MaxLinesInFile |> Option.bind (constructRuleWithConfig MaxLinesInFile.rule) config.TrailingNewLineInFile |> Option.bind (constructRuleIfEnabled TrailingNewLineInFile.rule) config.NoTabCharacters |> Option.bind (constructRuleIfEnabled NoTabCharacters.rule) + config.NoPartialFunctions |> Option.bind (constructRuleIfEnabled NoPartialFunctions.rule) |] |> Array.choose id let astNodeRules = ResizeArray() diff --git a/src/FSharpLint.Core/DefaultConfiguration.json b/src/FSharpLint.Core/DefaultConfiguration.json index 431ea7ae0..2c3f27d54 100644 --- a/src/FSharpLint.Core/DefaultConfiguration.json +++ b/src/FSharpLint.Core/DefaultConfiguration.json @@ -268,6 +268,7 @@ }, "trailingNewLineInFile": { "enabled": false }, "noTabCharacters": { "enabled": true }, + "noPartialFunctions": { "enabled": false }, "hints": { "add": [ "not (a = b) ===> a <> b", diff --git a/src/FSharpLint.Core/FSharpLint.Core.fsproj b/src/FSharpLint.Core/FSharpLint.Core.fsproj index 89a6eb3f9..fffc7b7fb 100644 --- a/src/FSharpLint.Core/FSharpLint.Core.fsproj +++ b/src/FSharpLint.Core/FSharpLint.Core.fsproj @@ -45,6 +45,7 @@ + diff --git a/src/FSharpLint.Core/Framework/AbstractSyntaxArray.fs b/src/FSharpLint.Core/Framework/AbstractSyntaxArray.fs index c31672565..8f8149f37 100644 --- a/src/FSharpLint.Core/Framework/AbstractSyntaxArray.fs +++ b/src/FSharpLint.Core/Framework/AbstractSyntaxArray.fs @@ -154,7 +154,7 @@ module AbstractSyntaxArray = /// Get hash code of an ast node to be used for the fuzzy match of hints against the ast. let private getHashCode node = match node with - | Identifier(idents) -> getIdentHash idents + | Identifier(idents, _) -> getIdentHash idents | Pattern(SynPat.Const(SynConst.Bool(x), _)) | Expression(SynExpr.Const(SynConst.Bool(x), _)) -> hash x | Pattern(SynPat.Const(SynConst.Byte(x), _)) diff --git a/src/FSharpLint.Core/Framework/Ast.fs b/src/FSharpLint.Core/Framework/Ast.fs index ca656e3fb..917a1cb21 100644 --- a/src/FSharpLint.Core/Framework/Ast.fs +++ b/src/FSharpLint.Core/Framework/Ast.fs @@ -1,5 +1,7 @@ namespace FSharpLint.Framework +open FSharp.Compiler.Range + /// Used to walk the FSharp Compiler's abstract syntax tree, /// so that each node can be visited by a list of visitors. module Ast = @@ -30,7 +32,7 @@ module Ast = | ConstructorArguments of SynArgPats | TypeParameter of SynTypar | InterfaceImplementation of SynInterfaceImpl - | Identifier of string list + | Identifier of string list * range : range | File of ParsedInput /// Concatenates the nested-list structure of `SynAttributes` into a `SynAttribute list` to keep other code @@ -142,7 +144,7 @@ module Ast = let inline private fieldNode x = Node(ExtraSyntaxInfo.None, Field x) let inline private matchNode x = Node(ExtraSyntaxInfo.None, Match x) let inline private constructorArgumentsNode x = Node(ExtraSyntaxInfo.None, ConstructorArguments x) - let inline private identifierNode x = Node(ExtraSyntaxInfo.None, Identifier x) + let inline private identifierNode (x, range) = Node(ExtraSyntaxInfo.None, Identifier (x, range)) /// Gets a string literal from the AST. let (|StringLiteral|_|) node = @@ -355,9 +357,9 @@ module Ast = | SynExpr.LetOrUse(_, _, bindings, expression, _) -> add <| expressionNode expression bindings |> List.revIter (bindingNode >> add) - | SynExpr.Ident(ident) -> add <| identifierNode([ident.idText]) - | SynExpr.LongIdent(_, LongIdentWithDots(ident, _), _, _) -> - add <| identifierNode(ident |> List.map (fun x -> x.idText)) + | SynExpr.Ident(ident) -> add <| identifierNode([ident.idText], ident.idRange) + | SynExpr.LongIdent(_, LongIdentWithDots(ident, _), _, range) -> + add <| identifierNode(ident |> List.map (fun x -> x.idText), range) | SynExpr.IfThenElse(cond, body, Some(elseExpr), _, _, _, _) -> add <| Node(ExtraSyntaxInfo.Else, AstNode.Expression elseExpr) add <| expressionNode body @@ -394,7 +396,7 @@ module Ast = add <| typeNode synType add <| simplePatternNode simplePattern | SynSimplePat.Attrib(simplePattern, _, _) -> add <| simplePatternNode simplePattern - | SynSimplePat.Id(identifier, _, _, _, _, _) -> add <| identifierNode([identifier.idText]) + | SynSimplePat.Id(identifier, _, _, _, _, _) -> add <| identifierNode([identifier.idText], identifier.idRange) let inline private matchChildren node add = match node with diff --git a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs new file mode 100644 index 000000000..d21d72edc --- /dev/null +++ b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs @@ -0,0 +1,54 @@ +module FSharpLint.Rules.NoPartialFunctions + +open FSharp.Compiler.Range +open FSharpLint.Framework +open FSharpLint.Framework.Suggestion +open FSharpLint.Framework.Ast +open FSharpLint.Framework.Rules + +type private Replacement = + | PatternMatch + | Function of functionName:string + +let private partialFunctions = + [ + // Option + ("Option.get", PatternMatch) + + // List + ("List.find", Function "List.tryFind") + ] |> Map.ofList + +let private checkIfPartial (identifier:string) (range:range) = + Map.tryFind identifier partialFunctions + |> Option.map (function + | PatternMatch -> + { + Range = range + Message = sprintf "Consider using pattern matching instead of partial function '%s'" identifier + SuggestedFix = None + TypeChecks = [] + } + | Function replacementFunction -> + { + Range = range + Message = sprintf "Consider using '%s' instead of partial function '%s'" replacementFunction identifier + SuggestedFix = None + TypeChecks = [] + }) + +let runner (args:AstNodeRuleParams) = + match args.AstNode with + | AstNode.Identifier (identifier, range) -> + checkIfPartial (String.concat "." identifier) range + |> Option.toArray + | _ -> + Array.empty + +let rule = + { Name = "NoPartialFunctions" + Identifier = Identifiers.NoPartialFunctions + RuleConfig = { AstNodeRuleConfig.Runner = runner + Cleanup = ignore } } + |> AstNodeRule + diff --git a/src/FSharpLint.Core/Rules/Identifiers.fs b/src/FSharpLint.Core/Rules/Identifiers.fs index ad1e82bc5..f6e9dec47 100644 --- a/src/FSharpLint.Core/Rules/Identifiers.fs +++ b/src/FSharpLint.Core/Rules/Identifiers.fs @@ -70,3 +70,4 @@ let MaxLinesInFile = identifier 62 let TrailingNewLineInFile = identifier 63 let NoTabCharacters = identifier 64 let Hints = identifier 65 +let NoPartialFunctions = identifier 66 diff --git a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj index b168cca36..37bb72b3b 100644 --- a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj +++ b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj @@ -32,6 +32,7 @@ + diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs new file mode 100644 index 000000000..fa67779f9 --- /dev/null +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs @@ -0,0 +1,22 @@ +module FSharpLint.Core.Tests.Rules.Conventions.NoPartialFunctions + +open NUnit.Framework +open FSharpLint.Rules + +[] +type TestConventionsNoPartialFunctions() = + inherit TestAstNodeRuleBase.TestAstNodeRuleBase(NoPartialFunctions.rule) + + [] + member this.``Error for partial function which should be replaced with pattern matching``() = + this.Parse(""" +let x = Option.get None""") + + this.AssertErrorWithMessageExists("Consider using pattern matching instead of partial function 'Option.get'") + + [] + member this.``Error for partial function which should be replaced with another function``() = + this.Parse(""" +let x = List.find 1 [2; 3; 4]""") + + this.AssertErrorWithMessageExists( "Consider using 'List.tryFind' instead of partial function 'List.find'") diff --git a/tests/FSharpLint.Core.Tests/Rules/Hints/HintMatcher.fs b/tests/FSharpLint.Core.Tests/Rules/Hints/HintMatcher.fs index 0e76f1323..c574f3b42 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Hints/HintMatcher.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Hints/HintMatcher.fs @@ -620,7 +620,7 @@ module Goat do ignore 0""") - this.ErrorWithMessageExists("`0` might be able to be refactored into `FSharpLint.( + )`.") |> Assert.IsTrue + this.AssertErrorWithMessageExists("`0` might be able to be refactored into `FSharpLint.( + )`.") [] member this.``Suggestion as a message presents correct error message.``() = @@ -632,7 +632,7 @@ module Goat do ()""") - this.ErrorWithMessageExists("`()`; suggestion: Message.") |> Assert.IsTrue + this.AssertErrorWithMessageExists("`()`; suggestion: Message.") [] member this.``Hints matches null in an expression correctly.``() = @@ -645,7 +645,7 @@ do let x = System.Collections.ArrayList() x = null |> ignore""") - this.ErrorWithMessageExists("`x = null`; suggestion: Use pattern matching to null check.") |> Assert.IsTrue + this.AssertErrorWithMessageExists("`x = null`; suggestion: Use pattern matching to null check.") /// Regression test for: http://codereview.stackexchange.com/questions/134296/f-function-to-concatenate-some-dsl-scripts-with-indentation#comment251110_134297 [] diff --git a/tests/FSharpLint.Core.Tests/Rules/TestRuleBase.fs b/tests/FSharpLint.Core.Tests/Rules/TestRuleBase.fs index 1b2e89755..fd5a41415 100644 --- a/tests/FSharpLint.Core.Tests/Rules/TestRuleBase.fs +++ b/tests/FSharpLint.Core.Tests/Rules/TestRuleBase.fs @@ -69,8 +69,9 @@ type TestRuleBase () = this.ErrorsAt(startLine, startColumn) |> Seq.exists (fun s -> s.Details.Message = message) - member __.ErrorWithMessageExists(message) = - suggestions |> Seq.exists (fun s -> s.Details.Message = message) + member __.AssertErrorWithMessageExists(message) = + let foundSuggestions = suggestions |> Seq.map (fun s -> s.Details.Message) + Assert.IsTrue(foundSuggestions |> Seq.contains message, sprintf "Couldn't find message '%s', found: [%s]" message (String.concat "," foundSuggestions)) member this.AssertNoWarnings() = Assert.IsFalse(this.ErrorsExist, "Expected no errors, but was: " + this.ErrorMsg) From 8603fe1f28baeeffcaca1d6c485b71f9bb01101e Mon Sep 17 00:00:00 2001 From: Jason Gardella Date: Fri, 12 Jun 2020 17:22:58 -0400 Subject: [PATCH 02/10] Add collection functions --- .../Rules/Conventions/NoPartialFunctions.fs | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs index d21d72edc..b7ee9dfe9 100644 --- a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs +++ b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs @@ -15,8 +15,45 @@ let private partialFunctions = // Option ("Option.get", PatternMatch) + // Array + ("Array.exactlyOne", Function "Array.tryExactlyOne") + ("Array.get", Function "Array.tryItem") + ("Array.item", Function "Array.tryItem") + ("Array.find", Function "Array.tryFind") + ("Array.findIndex", Function "Array.tryFindIndex") + ("Array.findBack", Function "Array.tryFindBack") + ("Array.head", Function "Array.tryHead") + ("Array.last", Function "Array.tryLast") + ("Array.tail", Function "Array.tryLast") + ("Array.reduce", Function "Array.fold") + ("Array.reduceBack", Function "Array.foldBack") + ("Array.pick", Function "Array.tryPick") + + // Seq + ("Seq.exactlyOne", Function "Seq.tryExactlyOne") + ("Seq.item", Function "Seq.tryItem") + ("Seq.find", Function "Seq.tryFind") + ("Seq.findIndex", Function "Seq.tryFindIndex") + ("Seq.findBack", Function "Seq.tryFindBack") + ("Seq.head", Function "Seq.tryHead") + ("Seq.last", Function "Seq.tryLast") + ("Seq.tail", Function "Seq.tryLast") + ("Seq.reduce", Function "Seq.fold") + ("Seq.reduceBack", Function "Seq.foldBack") + ("Seq.pick", Function "Seq.tryPick") + // List + ("List.exactlyOne", Function "List.tryExactlyOne") + ("List.item", Function "List.tryItem") ("List.find", Function "List.tryFind") + ("List.findIndex", Function "List.tryFindIndex") + ("List.findBack", Function "List.tryFindBack") + ("List.head", Function "List.tryHead") + ("List.last", Function "List.tryLast") + ("List.tail", Function "List.tryLast") + ("List.reduce", Function "List.fold") + ("List.reduceBack", Function "List.foldBack") + ("List.pick", Function "List.tryPick") ] |> Map.ofList let private checkIfPartial (identifier:string) (range:range) = From 75024118bfd1b39d6a0040e8209d186eb973d7d6 Mon Sep 17 00:00:00 2001 From: Jason Gardella Date: Fri, 12 Jun 2020 17:29:29 -0400 Subject: [PATCH 03/10] Add quickfix for partial function replacement --- .../Rules/Conventions/NoPartialFunctions.fs | 10 +++++----- .../Rules/Conventions/NoPartialFunctions.fs | 15 +++++++++++---- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs index b7ee9dfe9..0d772aa00 100644 --- a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs +++ b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs @@ -10,7 +10,7 @@ type private Replacement = | PatternMatch | Function of functionName:string -let private partialFunctions = +let private partialFunctionIdentifiers = [ // Option ("Option.get", PatternMatch) @@ -56,8 +56,8 @@ let private partialFunctions = ("List.pick", Function "List.tryPick") ] |> Map.ofList -let private checkIfPartial (identifier:string) (range:range) = - Map.tryFind identifier partialFunctions +let private checkIfPartialIdentifier (identifier:string) (range:range) = + Map.tryFind identifier partialFunctionIdentifiers |> Option.map (function | PatternMatch -> { @@ -70,14 +70,14 @@ let private checkIfPartial (identifier:string) (range:range) = { Range = range Message = sprintf "Consider using '%s' instead of partial function '%s'" replacementFunction identifier - SuggestedFix = None + SuggestedFix = Some (Lazy.CreateFromValue (Some { FromText = identifier; FromRange = range; ToText = replacementFunction })) TypeChecks = [] }) let runner (args:AstNodeRuleParams) = match args.AstNode with | AstNode.Identifier (identifier, range) -> - checkIfPartial (String.concat "." identifier) range + checkIfPartialIdentifier (String.concat "." identifier) range |> Option.toArray | _ -> Array.empty diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs index fa67779f9..cc303683e 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs @@ -9,14 +9,21 @@ type TestConventionsNoPartialFunctions() = [] member this.``Error for partial function which should be replaced with pattern matching``() = - this.Parse(""" -let x = Option.get None""") + this.Parse("let x = Option.get None") this.AssertErrorWithMessageExists("Consider using pattern matching instead of partial function 'Option.get'") [] member this.``Error for partial function which should be replaced with another function``() = - this.Parse(""" -let x = List.find 1 [2; 3; 4]""") + this.Parse("let x = List.find 1 [2; 3; 4]") this.AssertErrorWithMessageExists( "Consider using 'List.tryFind' instead of partial function 'List.find'") + + [] + member this.``Quickfix for partial function which should be replaced with another function``() = + let source = "let x = List.find 1 [2; 3; 4]" + this.Parse(source) + + let expected = "let x = List.tryFind 1 [2; 3; 4]" + Assert.AreEqual(expected, this.ApplyQuickFix source) + this.AssertErrorWithMessageExists( "Consider using 'List.tryFind' instead of partial function 'List.find'") From 04357894533c0bdbadc2198e852ca3dac58e2e3d Mon Sep 17 00:00:00 2001 From: Jason Gardella Date: Fri, 12 Jun 2020 17:34:01 -0400 Subject: [PATCH 04/10] Move error strings to resources file --- .../Rules/Conventions/NoPartialFunctions.fs | 7 ++++--- src/FSharpLint.Core/Text.resx | 6 ++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs index 0d772aa00..b32e4d038 100644 --- a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs +++ b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs @@ -1,5 +1,6 @@ module FSharpLint.Rules.NoPartialFunctions +open System open FSharp.Compiler.Range open FSharpLint.Framework open FSharpLint.Framework.Suggestion @@ -62,15 +63,15 @@ let private checkIfPartialIdentifier (identifier:string) (range:range) = | PatternMatch -> { Range = range - Message = sprintf "Consider using pattern matching instead of partial function '%s'" identifier + Message = String.Format(Resources.GetString ("RulesConventionsNoPartialFunctionsPatternMatchError"), identifier) SuggestedFix = None TypeChecks = [] } | Function replacementFunction -> { Range = range - Message = sprintf "Consider using '%s' instead of partial function '%s'" replacementFunction identifier - SuggestedFix = Some (Lazy.CreateFromValue (Some { FromText = identifier; FromRange = range; ToText = replacementFunction })) + Message = String.Format(Resources.GetString "RulesConventionsNoPartialFunctionsReplacementError", replacementFunction, identifier) + SuggestedFix = Some (lazy ( Some { FromText = identifier; FromRange = range; ToText = replacementFunction })) TypeChecks = [] }) diff --git a/src/FSharpLint.Core/Text.resx b/src/FSharpLint.Core/Text.resx index 922ccbc0e..d41b12ee5 100644 --- a/src/FSharpLint.Core/Text.resx +++ b/src/FSharpLint.Core/Text.resx @@ -309,4 +309,10 @@ Recursive async functions ending with a `do!` recursive call will leak memory; prefer `return!`. + + Consider using '{0}' instead of partial function '{1}'. + + + Consider using pattern matching instead of partial function '%s'. + From c2569121526ea0416ca4168226497c40376c3334 Mon Sep 17 00:00:00 2001 From: Jason Gardella Date: Fri, 12 Jun 2020 22:07:04 -0400 Subject: [PATCH 05/10] Add config for NoPartialFunctions rule --- .../Application/Configuration.fs | 4 +- src/FSharpLint.Core/DefaultConfiguration.json | 8 ++- .../Rules/Conventions/NoPartialFunctions.fs | 60 ++++++++++++------- src/FSharpLint.Core/Text.resx | 5 +- .../Rules/Conventions/NoPartialFunctions.fs | 20 +++++-- 5 files changed, 67 insertions(+), 30 deletions(-) diff --git a/src/FSharpLint.Core/Application/Configuration.fs b/src/FSharpLint.Core/Application/Configuration.fs index bed4201aa..38df71ca7 100644 --- a/src/FSharpLint.Core/Application/Configuration.fs +++ b/src/FSharpLint.Core/Application/Configuration.fs @@ -423,7 +423,7 @@ type Configuration = MaxLinesInFile:RuleConfig option TrailingNewLineInFile:EnabledConfig option NoTabCharacters:EnabledConfig option - NoPartialFunctions:EnabledConfig option } + NoPartialFunctions:RuleConfig option } with static member Zero = { Global = None @@ -634,7 +634,7 @@ let flattenConfig (config:Configuration) = config.MaxLinesInFile |> Option.bind (constructRuleWithConfig MaxLinesInFile.rule) config.TrailingNewLineInFile |> Option.bind (constructRuleIfEnabled TrailingNewLineInFile.rule) config.NoTabCharacters |> Option.bind (constructRuleIfEnabled NoTabCharacters.rule) - config.NoPartialFunctions |> Option.bind (constructRuleIfEnabled NoPartialFunctions.rule) + config.NoPartialFunctions |> Option.bind (constructRuleWithConfig NoPartialFunctions.rule) |] |> Array.choose id let astNodeRules = ResizeArray() diff --git a/src/FSharpLint.Core/DefaultConfiguration.json b/src/FSharpLint.Core/DefaultConfiguration.json index 2c3f27d54..660e91f51 100644 --- a/src/FSharpLint.Core/DefaultConfiguration.json +++ b/src/FSharpLint.Core/DefaultConfiguration.json @@ -268,7 +268,13 @@ }, "trailingNewLineInFile": { "enabled": false }, "noTabCharacters": { "enabled": true }, - "noPartialFunctions": { "enabled": false }, + "noPartialFunctions": { + "enabled": false, + "config": { + "allowedPartials": [], + "additionalPartials": [] + } + }, "hints": { "add": [ "not (a = b) ===> a <> b", diff --git a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs index b32e4d038..1fcc73983 100644 --- a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs +++ b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs @@ -7,6 +7,12 @@ open FSharpLint.Framework.Suggestion open FSharpLint.Framework.Ast open FSharpLint.Framework.Rules +[] +type Config = { + AllowedPartials:string list + AdditionalPartials:string list +} + type private Replacement = | PatternMatch | Function of functionName:string @@ -57,36 +63,46 @@ let private partialFunctionIdentifiers = ("List.pick", Function "List.tryPick") ] |> Map.ofList -let private checkIfPartialIdentifier (identifier:string) (range:range) = - Map.tryFind identifier partialFunctionIdentifiers - |> Option.map (function - | PatternMatch -> - { - Range = range - Message = String.Format(Resources.GetString ("RulesConventionsNoPartialFunctionsPatternMatchError"), identifier) - SuggestedFix = None - TypeChecks = [] - } - | Function replacementFunction -> - { - Range = range - Message = String.Format(Resources.GetString "RulesConventionsNoPartialFunctionsReplacementError", replacementFunction, identifier) - SuggestedFix = Some (lazy ( Some { FromText = identifier; FromRange = range; ToText = replacementFunction })) - TypeChecks = [] - }) +let private checkIfPartialIdentifier (config:Config) (identifier:string) (range:range) = + if List.contains identifier config.AllowedPartials then + None + elif List.contains identifier config.AdditionalPartials then + Some { + Range = range + Message = String.Format(Resources.GetString ("RulesConventionsNoPartialFunctionsAdditionalError"), identifier) + SuggestedFix = None + TypeChecks = [] + } + else + Map.tryFind identifier partialFunctionIdentifiers + |> Option.filter (fun _ -> not (List.contains identifier config.AllowedPartials)) + |> Option.map (function + | PatternMatch -> + { + Range = range + Message = String.Format(Resources.GetString ("RulesConventionsNoPartialFunctionsPatternMatchError"), identifier) + SuggestedFix = None + TypeChecks = [] + } + | Function replacementFunction -> + { + Range = range + Message = String.Format(Resources.GetString "RulesConventionsNoPartialFunctionsReplacementError", replacementFunction, identifier) + SuggestedFix = Some (lazy ( Some { FromText = identifier; FromRange = range; ToText = replacementFunction })) + TypeChecks = [] + }) -let runner (args:AstNodeRuleParams) = +let private runner (config:Config) (args:AstNodeRuleParams) = match args.AstNode with | AstNode.Identifier (identifier, range) -> - checkIfPartialIdentifier (String.concat "." identifier) range + checkIfPartialIdentifier config (String.concat "." identifier) range |> Option.toArray | _ -> Array.empty -let rule = +let rule config = { Name = "NoPartialFunctions" Identifier = Identifiers.NoPartialFunctions - RuleConfig = { AstNodeRuleConfig.Runner = runner + RuleConfig = { AstNodeRuleConfig.Runner = runner config Cleanup = ignore } } |> AstNodeRule - diff --git a/src/FSharpLint.Core/Text.resx b/src/FSharpLint.Core/Text.resx index d41b12ee5..ca6f56af4 100644 --- a/src/FSharpLint.Core/Text.resx +++ b/src/FSharpLint.Core/Text.resx @@ -313,6 +313,9 @@ Consider using '{0}' instead of partial function '{1}'. - Consider using pattern matching instead of partial function '%s'. + Consider using pattern matching instead of partial function '{0}'. + + + Consider not using partial function '{0}'. diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs index cc303683e..4d0a569cc 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs @@ -5,19 +5,19 @@ open FSharpLint.Rules [] type TestConventionsNoPartialFunctions() = - inherit TestAstNodeRuleBase.TestAstNodeRuleBase(NoPartialFunctions.rule) + inherit TestAstNodeRuleBase.TestAstNodeRuleBase(NoPartialFunctions.rule { AdditionalPartials = ["Custom.partial"]; AllowedPartials = ["List.pick"] }) [] member this.``Error for partial function which should be replaced with pattern matching``() = this.Parse("let x = Option.get None") - this.AssertErrorWithMessageExists("Consider using pattern matching instead of partial function 'Option.get'") + this.AssertErrorWithMessageExists("Consider using pattern matching instead of partial function 'Option.get'.") [] member this.``Error for partial function which should be replaced with another function``() = this.Parse("let x = List.find 1 [2; 3; 4]") - this.AssertErrorWithMessageExists( "Consider using 'List.tryFind' instead of partial function 'List.find'") + this.AssertErrorWithMessageExists("Consider using 'List.tryFind' instead of partial function 'List.find'.") [] member this.``Quickfix for partial function which should be replaced with another function``() = @@ -26,4 +26,16 @@ type TestConventionsNoPartialFunctions() = let expected = "let x = List.tryFind 1 [2; 3; 4]" Assert.AreEqual(expected, this.ApplyQuickFix source) - this.AssertErrorWithMessageExists( "Consider using 'List.tryFind' instead of partial function 'List.find'") + this.AssertErrorWithMessageExists( "Consider using 'List.tryFind' instead of partial function 'List.find'.") + + [] + member this.``Error for user-specified partial function``() = + this.Parse("let x = Custom.partial 4") + + this.AssertErrorWithMessageExists("Consider not using partial function 'Custom.partial'.") + + [] + member this.``No error for user-specified allowed partial function``() = + this.Parse("let x = List.pick id [Some 4; None]") + + this.AssertNoWarnings() From 5bb7108744c64ced4a7bc0fe50f9f77c90fcaeaf Mon Sep 17 00:00:00 2001 From: Jason Gardella Date: Sat, 13 Jun 2020 16:03:22 -0400 Subject: [PATCH 06/10] Add doc page for rule --- docs/content/how-tos/rules/FL0066.md | 36 ++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 docs/content/how-tos/rules/FL0066.md diff --git a/docs/content/how-tos/rules/FL0066.md b/docs/content/how-tos/rules/FL0066.md new file mode 100644 index 000000000..5af096213 --- /dev/null +++ b/docs/content/how-tos/rules/FL0066.md @@ -0,0 +1,36 @@ +--- +title: FL0066 +category: how-to +hide_menu: true +--- + +# NoPartialFunctions (FL0066) + +## Cause + +A partial function was used. + +## Rationale + +Partial functions are only valid for certain inputs; for invalid inputs they will throw an exception. In most cases, +it is better to use a non-partial version of the function (e.g. `List.tryFind` instead of `List.find`) or use pattern matching, +so that you need to explicitly handle those cases which would cause an exception in the partial version. + +## How To Fix + +Use non-partial function or pattern matching. + +## Rule Settings + + { + "noPartialFunctions": { + "enabled": false, + "config": { + "allowedPartials": [], + "additionalPartials": [] + } + } + } + +* *allowedPartials* - list of strings representing partial functions to allow (e.g. `"List.tryFind"`) +* *additionalPartials* - list of strings representing additional partial functions to disallow From 3b0ebd4ec16318c7cfd3b8759f33f49f2178e7dc Mon Sep 17 00:00:00 2001 From: Jason Gardella Date: Sat, 13 Jun 2020 16:04:49 -0400 Subject: [PATCH 07/10] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8830808f7..cd460939c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added +- New rule `NoPartialFunctions (FL0066)`. ## [0.16.2] - 2020-06-10 - Load config from `fsharplint.json` by default. From 3d0f41aab45f5eb0063f58b3f67d800c927e08ca Mon Sep 17 00:00:00 2001 From: Matt Mcveigh Date: Fri, 8 Jan 2021 01:22:49 +0000 Subject: [PATCH 08/10] Add Map.find and Map.tryFind --- src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs index 1fcc73983..984f99762 100644 --- a/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs +++ b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs @@ -22,6 +22,10 @@ let private partialFunctionIdentifiers = // Option ("Option.get", PatternMatch) + // Map + ("Map.find", Function "Map.tryFind") + ("Map.findKey", Function "Map.tryFindKey") + // Array ("Array.exactlyOne", Function "Array.tryExactlyOne") ("Array.get", Function "Array.tryItem") From e238bd5071c03f0c80808a5e496d4f911c69ad22 Mon Sep 17 00:00:00 2001 From: Matt Mcveigh Date: Fri, 8 Jan 2021 01:24:03 +0000 Subject: [PATCH 09/10] Bump version --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ff68a234..a8bbc8278 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [Unreleased] +## [0.18.0] ### Added - New rule `NoPartialFunctions (FL0066)`. From 032dac12d9e01bd16986c8390782ecdc56516280 Mon Sep 17 00:00:00 2001 From: Matt Mcveigh Date: Fri, 8 Jan 2021 01:44:03 +0000 Subject: [PATCH 10/10] Update list of rules in docs --- docs/content/how-tos/rule-configuration.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/content/how-tos/rule-configuration.md b/docs/content/how-tos/rule-configuration.md index 4610f7d15..75d0f40f9 100644 --- a/docs/content/how-tos/rule-configuration.md +++ b/docs/content/how-tos/rule-configuration.md @@ -98,3 +98,4 @@ The following rules can be specified for linting. - [TrailingNewLineInFile (FL0063)](rules/FL0063.html) - [NoTabCharacters (FL0064)](rules/FL0064.html) - [Hints (FL0065)](rules/FL0065.html) +- [NoPartialFunctions (FL0066)](rules/FL0066.html)