diff --git a/CHANGELOG.md b/CHANGELOG.md index 64fe4577a..a8bbc8278 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ 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). +## [0.18.0] +### Added +- New rule `NoPartialFunctions (FL0066)`. + ## [0.17.1] - 2020-11-25 - Fix for records being counted as a nested statement #464 [@davidtgillard] 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) 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 diff --git a/src/FSharpLint.Core/Application/Configuration.fs b/src/FSharpLint.Core/Application/Configuration.fs index 82808ddd8..38df71ca7 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:RuleConfig 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 (constructRuleWithConfig NoPartialFunctions.rule) |] |> Array.choose id let astNodeRules = ResizeArray() diff --git a/src/FSharpLint.Core/DefaultConfiguration.json b/src/FSharpLint.Core/DefaultConfiguration.json index 431ea7ae0..660e91f51 100644 --- a/src/FSharpLint.Core/DefaultConfiguration.json +++ b/src/FSharpLint.Core/DefaultConfiguration.json @@ -268,6 +268,13 @@ }, "trailingNewLineInFile": { "enabled": false }, "noTabCharacters": { "enabled": true }, + "noPartialFunctions": { + "enabled": false, + "config": { + "allowedPartials": [], + "additionalPartials": [] + } + }, "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 fedfbcceb..cc6e4b02c 100644 --- a/src/FSharpLint.Core/Framework/AbstractSyntaxArray.fs +++ b/src/FSharpLint.Core/Framework/AbstractSyntaxArray.fs @@ -164,7 +164,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 07532fd8a..e0162f4ba 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 | LambdaBody of SynExpr | LambdaArg of SynSimplePats @@ -326,9 +328,9 @@ module Ast = | SynExpr.LetOrUse(_, _, bindings, expression, _) -> add <| Expression expression bindings |> List.revIter (Binding >> add) - | SynExpr.Ident(ident) -> add <| Identifier([ident.idText]) - | SynExpr.LongIdent(_, LongIdentWithDots(ident, _), _, _) -> - add <| Identifier(ident |> List.map (fun x -> x.idText)) + | SynExpr.Ident(ident) -> add <| Identifier([ident.idText], ident.idRange) + | SynExpr.LongIdent(_, LongIdentWithDots(ident, _), _, range) -> + add <| Identifier(ident |> List.map (fun x -> x.idText), range) | SynExpr.IfThenElse(cond, body, Some(elseExpr), _, _, _, _) -> add <| Else elseExpr add <| Expression body @@ -374,7 +376,7 @@ module Ast = add <| Type synType add <| SimplePattern simplePattern | SynSimplePat.Attrib(simplePattern, _, _) -> add <| SimplePattern simplePattern - | SynSimplePat.Id(identifier, _, _, _, _, _) -> add <| Identifier([identifier.idText]) + | SynSimplePat.Id(identifier, _, _, _, _, _) -> add <| Identifier([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..984f99762 --- /dev/null +++ b/src/FSharpLint.Core/Rules/Conventions/NoPartialFunctions.fs @@ -0,0 +1,112 @@ +module FSharpLint.Rules.NoPartialFunctions + +open System +open FSharp.Compiler.Range +open FSharpLint.Framework +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 + +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") + ("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 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 private runner (config:Config) (args:AstNodeRuleParams) = + match args.AstNode with + | AstNode.Identifier (identifier, range) -> + checkIfPartialIdentifier config (String.concat "." identifier) range + |> Option.toArray + | _ -> + Array.empty + +let rule config = + { Name = "NoPartialFunctions" + Identifier = Identifiers.NoPartialFunctions + RuleConfig = { AstNodeRuleConfig.Runner = runner config + 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/src/FSharpLint.Core/Text.resx b/src/FSharpLint.Core/Text.resx index 2a562bf04..66d5bb018 100644 --- a/src/FSharpLint.Core/Text.resx +++ b/src/FSharpLint.Core/Text.resx @@ -309,4 +309,13 @@ 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 '{0}'. + + + Consider not using partial function '{0}'. + diff --git a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj index 05623dee5..293582a99 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..4d0a569cc --- /dev/null +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/NoPartialFunctions.fs @@ -0,0 +1,41 @@ +module FSharpLint.Core.Tests.Rules.Conventions.NoPartialFunctions + +open NUnit.Framework +open FSharpLint.Rules + +[] +type TestConventionsNoPartialFunctions() = + 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'.") + + [] + 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'.") + + [] + 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'.") + + [] + 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() diff --git a/tests/FSharpLint.Core.Tests/Rules/Hints/HintMatcher.fs b/tests/FSharpLint.Core.Tests/Rules/Hints/HintMatcher.fs index 92d777617..c1b41bf33 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Hints/HintMatcher.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Hints/HintMatcher.fs @@ -626,7 +626,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.``() = @@ -638,7 +638,7 @@ module Goat do ()""") - this.ErrorWithMessageExists("`()`; suggestion: Message.") |> Assert.IsTrue + this.AssertErrorWithMessageExists("`()`; suggestion: Message.") [] member this.``Hints matches null in an expression correctly.``() = @@ -651,7 +651,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)