diff --git a/docs/content/how-tos/rule-configuration.md b/docs/content/how-tos/rule-configuration.md index 50e819956..d06320689 100644 --- a/docs/content/how-tos/rule-configuration.md +++ b/docs/content/how-tos/rule-configuration.md @@ -137,3 +137,4 @@ The following rules can be specified for linting. - [FavourNamedMembers (FL0094)](rules/FL0094.html) - [SynchronousFunctionNames (FL0095)](rules/FL0095.html) - [AsynchronousFunctionNames (FL0096)](rules/FL0096.html) +- [NoImpureFunctions (FL0098)](rules/FL0098.html) diff --git a/docs/content/how-tos/rules/FL0098.md b/docs/content/how-tos/rules/FL0098.md new file mode 100644 index 000000000..5e74f6155 --- /dev/null +++ b/docs/content/how-tos/rules/FL0098.md @@ -0,0 +1,36 @@ +--- +title: FL0098 +category: how-to +hide_menu: true +--- + +# NoImpureFunctions (FL0098) + +## Cause + +An impure function (that mutates one or more of its parameters) was used. + +## Rationale + +Using pure functions without side-effects is preferrable in functional programming. + +## How To Fix + +Use pure function. + +## Rule Settings + + { + "noImpureFunctions": { + "enabled": false, + "config": { + "allowedImpureFunctions": [], + "additionalImpureFunctions": [] + } + } + } + +* *allowedImpureFunctions* - list of strings representing impure functions to allow (e.g. `"Array.set"`). +* *additionalImpureFunctions* - list of strings representing additional impure functions to disallow. + +If a string in either of those settings ends in `.*` (e.g. `Foo.Bar.Baz.*`), then it is treated as type `Foo.Bar.Baz` (without type parameters). In this case, usage of type's constructors is checked. diff --git a/src/FSharpLint.Core/Application/Configuration.fs b/src/FSharpLint.Core/Application/Configuration.fs index 122de462e..5be5dcb39 100644 --- a/src/FSharpLint.Core/Application/Configuration.fs +++ b/src/FSharpLint.Core/Application/Configuration.fs @@ -558,7 +558,8 @@ type Configuration = FavourNamedMembers:EnabledConfig option SynchronousFunctionNames:EnabledConfig option AsynchronousFunctionNames:RuleConfig option - SimpleAsyncComplementaryHelpers:RuleConfig option } + SimpleAsyncComplementaryHelpers:RuleConfig option + NoImpureFunctions:RuleConfig option } with // Method Zero is too big but can't be split into parts because it returns a record // and it requires all fields to be set. @@ -671,6 +672,7 @@ with SynchronousFunctionNames = None AsynchronousFunctionNames = None SimpleAsyncComplementaryHelpers = None + NoImpureFunctions = None } // fsharplint:enable MaxLinesInMember @@ -797,6 +799,7 @@ let flattenConfig (config:Configuration) = config.Hints |> Option.map (fun hintsConfig -> HintMatcher.rule { HintMatcher.Config.HintTrie = parseHints (getOrEmptyList hintsConfig.add) }) |> Option.toArray |] +// fsharplint:disable MaxLinesInValue let allPossibleRules = [| config.TypedItemSpacing |> Option.bind (constructRuleWithConfig TypedItemSpacing.rule) @@ -896,6 +899,7 @@ let flattenConfig (config:Configuration) = config.SynchronousFunctionNames |> Option.bind (constructRuleIfEnabled SynchronousFunctionNames.rule) config.AsynchronousFunctionNames |> Option.bind (constructRuleWithConfig AsynchronousFunctionNames.rule) config.SimpleAsyncComplementaryHelpers |> Option.bind (constructRuleWithConfig SimpleAsyncComplementaryHelpers.rule) + config.NoImpureFunctions |> Option.bind (constructRuleWithConfig NoImpureFunctions.rule) |] let allEnabledRules = Array.choose id allPossibleRules diff --git a/src/FSharpLint.Core/FSharpLint.Core.fsproj b/src/FSharpLint.Core/FSharpLint.Core.fsproj index 7fa0f6aeb..dd6859aab 100644 --- a/src/FSharpLint.Core/FSharpLint.Core.fsproj +++ b/src/FSharpLint.Core/FSharpLint.Core.fsproj @@ -53,6 +53,7 @@ + diff --git a/src/FSharpLint.Core/Rules/Conventions/NoImpureFunctions.fs b/src/FSharpLint.Core/Rules/Conventions/NoImpureFunctions.fs new file mode 100644 index 000000000..de7999e10 --- /dev/null +++ b/src/FSharpLint.Core/Rules/Conventions/NoImpureFunctions.fs @@ -0,0 +1,144 @@ +module FSharpLint.Rules.NoImpureFunctions + +open System +open FSharpLint.Framework +open FSharpLint.Framework.Suggestion +open FSharpLint.Framework.Ast +open FSharpLint.Framework.Rules +open FSharp.Compiler.Text +open FSharp.Compiler.CodeAnalysis +open FSharp.Compiler.Symbols +open FSharp.Compiler.Syntax + +[] +type Config = { + AllowedImpureFunctions:string list + AdditionalImpureFunctions:string list +} + +let private impureFunctionIdentifiers = + Map.ofList + [ + ("Array.sortInPlace", Some "Array.sort") + ("Array.sortInPlaceBy", Some "Array.Array.sortBy") + ("Array.sortInPlaceWith", Some "Array.sortWith") + + ("Array.set", Some "Array.updateAt") + + ("Array.fill", None) + ("Array.blit", None) + + ("Array2D.fill", None) + ("Array2D.blit", None) + + // Disallowed types + ("System.Collections.Generic.List.*", None) + ("System.Collections.Generic.Dictionary.*", None) + ("System.Collections.Generic.HashSet.*", None) + ("System.Collections.Generic.LinkedList.*", None) + ("System.Collections.Generic.SortedDictionary.*", None) + ("System.Collections.Generic.SortedSet.*", None) + ("System.Collections.Generic.Stack.*", None) + ] + +let runner (config:Config) (args:AstNodeRuleParams) = + let checkIfImpureIdentifier (identifier:string) (range:Range) = + let issueWarning maybeReplacementFunction = + match maybeReplacementFunction with + | Some replacementFunction -> + { + Range = range + Message = String.Format(Resources.GetString "RulesConventionsNoImpureFunctionsReplacementError", replacementFunction, identifier) + SuggestedFix = Some (lazy ( Some { FromText = identifier; FromRange = range; ToText = replacementFunction })) + TypeChecks = List.Empty + } + | None -> + { + Range = range + Message = String.Format(Resources.GetString "RulesConventionsNoImpureFunctionsError", identifier) + SuggestedFix = None + TypeChecks = List.Empty + } + + if List.contains identifier config.AllowedImpureFunctions then + None + elif List.contains identifier config.AdditionalImpureFunctions then + issueWarning None |> Some + else + Map.tryFind identifier impureFunctionIdentifiers + |> Option.filter (fun _ -> not (List.contains identifier config.AllowedImpureFunctions)) + |> Option.map issueWarning + + let disallowedTypes = + let builtinIdentifiers = Map.keys impureFunctionIdentifiers |> Set.ofSeq + (Set.ofList config.AdditionalImpureFunctions) + builtinIdentifiers - (Set.ofList config.AllowedImpureFunctions) + |> Seq.choose (fun definition -> + let wildcardSuffix = ".*" + if definition.EndsWith wildcardSuffix then + Some <| definition.Substring(0, definition.Length - wildcardSuffix.Length) + else + None) + |> Set.ofSeq + + let checkForDisallowedType (checkResults: FSharpCheckFileResults) (range: Range) = + let allSymbolUses = checkResults.GetAllUsesOfAllSymbolsInFile() + let maybeSymbolUse = + allSymbolUses + |> Seq.tryFind (fun symbolUse -> range = symbolUse.Range) + match maybeSymbolUse with + | Some symbolUse -> + match symbolUse.Symbol with + | :? FSharpMemberOrFunctionOrValue as value when value.IsConstructor || value.IsImplicitConstructor -> + let declaringEntity = value.DeclaringEntity + match declaringEntity with + | Some entity -> + let fullNameWithoutTypeParams = + let lestBackTickIndex = entity.FullName.LastIndexOf '`' + if lestBackTickIndex > 0 then + entity.FullName.Substring(0, lestBackTickIndex) + else + entity.FullName + + if disallowedTypes |> Set.contains fullNameWithoutTypeParams then + Array.singleton + { + Range = range + Message = String.Format(Resources.GetString "RulesConventionsNoImpureFunctionsDisallowedTypeError", entity.FullName) + SuggestedFix = None + TypeChecks = List.Empty + } + else + Array.empty + | _ -> Array.empty + | _ -> Array.empty + | None -> Array.empty + + match (args.AstNode, args.CheckInfo) with + | AstNode.Identifier (identifier, range), _ -> + match checkIfImpureIdentifier (String.concat "." identifier) range with + | Some impureIdentifierWarning -> + Array.singleton impureIdentifierWarning + | _ -> + Array.Empty() + | AstNode.Expression(SynExpr.App(_, false, funcExpr, _, _range)), Some(checkResults) when not disallowedTypes.IsEmpty -> + match funcExpr with + | SynExpr.LongIdent(_, SynLongIdent(_), _, identRange) -> + checkForDisallowedType checkResults identRange + | SynExpr.TypeApp(SynExpr.LongIdent(_, SynLongIdent(_), _, identRange),_,_,_,_,_,_) -> + checkForDisallowedType checkResults identRange + | _ -> Array.empty + | AstNode.Expression(SynExpr.New(_, targetType, _, _)), Some(checkResults) when not disallowedTypes.IsEmpty -> + checkForDisallowedType checkResults targetType.Range + | _ -> Array.empty + +let rule config = + AstNodeRule + { + Name = "NoImpureFunctions" + Identifier = Identifiers.NoImpureFunctions + RuleConfig = + { + AstNodeRuleConfig.Runner = runner config + Cleanup = ignore + } + } diff --git a/src/FSharpLint.Core/Rules/Identifiers.fs b/src/FSharpLint.Core/Rules/Identifiers.fs index b52ce8547..ad99a1f41 100644 --- a/src/FSharpLint.Core/Rules/Identifiers.fs +++ b/src/FSharpLint.Core/Rules/Identifiers.fs @@ -102,3 +102,4 @@ let FavourNamedMembers = identifier 94 let SynchronousFunctionNames = identifier 95 let AsynchronousFunctionNames = identifier 96 let SimpleAsyncComplementaryHelpers = identifier 97 +let NoImpureFunctions = identifier 98 diff --git a/src/FSharpLint.Core/Text.resx b/src/FSharpLint.Core/Text.resx index 846703ac5..2a60d8d47 100644 --- a/src/FSharpLint.Core/Text.resx +++ b/src/FSharpLint.Core/Text.resx @@ -420,4 +420,13 @@ Consider creating twin helper function/method `{0}{1}` that just calls `async {{ return Async.AwaitTask ({2}Async{3}) }}` under the hood. + + Consider not using impure function '{0}'. + + + Consider using '{0}' instead of impure function '{1}'. + + + Consider not using mutable type '{0}'. + diff --git a/src/FSharpLint.Core/fsharplint.json b/src/FSharpLint.Core/fsharplint.json index d005ee763..2cd4b7a29 100644 --- a/src/FSharpLint.Core/fsharplint.json +++ b/src/FSharpLint.Core/fsharplint.json @@ -361,6 +361,13 @@ "mode": "OnlyPublicAPIsInLibraries" } }, + "noImpureFunctions": { + "enabled": false, + "config": { + "allowedImpureFunctions": [], + "additionalImpureFunctions": [] + } + }, "hints": { "add": [ "not (a = b) ===> a <> b", diff --git a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj index 862bc1494..9d5ec17fd 100644 --- a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj +++ b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj @@ -40,6 +40,7 @@ + diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/NoImpureFunctions.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/NoImpureFunctions.fs new file mode 100644 index 000000000..c8963c149 --- /dev/null +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/NoImpureFunctions.fs @@ -0,0 +1,73 @@ +module FSharpLint.Core.Tests.Rules.Conventions.NoImpureFunctions + + +open NUnit.Framework +open FSharpLint.Rules +open FSharpLint.Core.Tests + +[] +type TestConventionsNoImpureFunctions() = + inherit TestAstNodeRuleBase.TestAstNodeRuleBase( + NoImpureFunctions.rule { + AdditionalImpureFunctions = ["Custom.impure"] + AllowedImpureFunctions = ["Array.set"; "System.Collections.Generic.Dictionary.*"] + }) + + [] + member this.``Error for impure function which should be replaced with another function``() = + this.Parse("let x = Array.sortInPlace [2; 1; 4]") + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Error for user-specified impure function``() = + this.Parse("let x = Custom.impure (ref 4)") + + Assert.IsTrue this.ErrorsExist + + [] + member this.``No error for user-specified allowed partial function``() = + this.Parse("let x = [| Some 4; None |] +Array.set x 0 None") + + this.AssertNoWarnings() + + [] + member this.``Error for creating a mutable list``() = + this.Parse("let x = System.Collections.Generic.List()") + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Error for creating a mutable list without explicit type param``() = + this.Parse("let x = System.Collections.Generic.List [ 1; 2 ]") + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Error for creating a mutable list with new keyword``() = + this.Parse("open System.Collections.Generic + +let x = new List()") + + Assert.IsTrue this.ErrorsExist + + [] + member this.``No error for using an existing mutable list``() = + this.Parse("open System.Collections.Generic + +type IHasList = + abstract List: List + +let x (hasList: IHasList) = hasList.List") + + this.AssertNoWarnings() + + [] + member this.``No error for creating a type from allowed list``() = + this.Parse("open System.Collections.Generic + +let x = new Dictionary()") + + this.AssertNoWarnings() +