Added NoImpureFunctions rule

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)

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.

src/FSharpLint.Core/Application/Configuration.fs

@@ -558,7 +558,8 @@ type Configuration =
       FavourNamedMembers:EnabledConfig option
       SynchronousFunctionNames:EnabledConfig option
       AsynchronousFunctionNames:RuleConfig<AsynchronousFunctionNames.Config> option
-      SimpleAsyncComplementaryHelpers:RuleConfig<SimpleAsyncComplementaryHelpers.Config> option }
+      SimpleAsyncComplementaryHelpers:RuleConfig<SimpleAsyncComplementaryHelpers.Config> option
+      NoImpureFunctions:RuleConfig<NoImpureFunctions.Config> 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

src/FSharpLint.Core/FSharpLint.Core.fsproj

@@ -53,6 +53,7 @@
     <Compile Include="Rules\Conventions\RedundantNewKeyword.fs" />
     <Compile Include="Rules\Conventions\NestedStatements.fs" />
     <Compile Include="Rules\Conventions\NoPartialFunctions.fs" />
+    <Compile Include="Rules\Conventions\NoImpureFunctions.fs" />
     <Compile Include="Rules\Conventions\CyclomaticComplexity.fs" />
     <Compile Include="Rules\Conventions\FavourReRaise.fs" />
     <Compile Include="Rules\Conventions\FavourConsistentThis.fs" />

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
+
+[<RequireQualifiedAccess>]
+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
+                }
+        }

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

src/FSharpLint.Core/Text.resx

@@ -420,4 +420,13 @@
   <data name="RulesSimpleAsyncComplementaryHelpersTask" xml:space="preserve">
     <value>Consider creating twin helper function/method `{0}{1}` that just calls `async {{ return Async.AwaitTask ({2}Async{3}) }}` under the hood.</value>
   </data>
+  <data name="RulesConventionsNoImpureFunctionsError" xml:space="preserve">
+    <value>Consider not using impure function '{0}'.</value>
+  </data>
+  <data name="RulesConventionsNoImpureFunctionsReplacementError" xml:space="preserve">
+    <value>Consider using '{0}' instead of impure function '{1}'.</value>
+  </data>
+  <data name="RulesConventionsNoImpureFunctionsDisallowedTypeError" xml:space="preserve">
+    <value>Consider not using mutable type '{0}'.</value>
+  </data>
 </root>

src/FSharpLint.Core/fsharplint.json

@@ -361,6 +361,13 @@
             "mode": "OnlyPublicAPIsInLibraries"
         }
     },
+    "noImpureFunctions": {
+        "enabled": false,
+        "config": {
+            "allowedImpureFunctions": [],
+            "additionalImpureFunctions": []
+        }
+    },
     "hints": {
         "add": [
             "not (a =  b) ===> a <> b",

tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj

@@ -40,6 +40,7 @@
     <Compile Include="Rules\Conventions\FunctionReimplementation.fs" />
     <Compile Include="Rules\Conventions\SourceLength.fs" />
     <Compile Include="Rules\Conventions\NoPartialFunctions.fs" />
+    <Compile Include="Rules\Conventions\NoImpureFunctions.fs" />
     <Compile Include="Rules\Conventions\FavourReRaise.fs" />
     <Compile Include="Rules\Conventions\DiscourageStringInterpolationWithStringFormat.fs" />
     <Compile Include="Rules\Conventions\FavourConsistentThis.fs" />

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
+
+[<TestFixture>]
+type TestConventionsNoImpureFunctions() =
+    inherit TestAstNodeRuleBase.TestAstNodeRuleBase(
+        NoImpureFunctions.rule { 
+            AdditionalImpureFunctions = ["Custom.impure"]
+            AllowedImpureFunctions = ["Array.set"; "System.Collections.Generic.Dictionary.*"]
+        })
+
+    [<Test>]
+    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
+
+    [<Test>]
+    member this.``Error for user-specified impure function``() =
+        this.Parse("let x = Custom.impure (ref 4)")
+
+        Assert.IsTrue this.ErrorsExist
+
+    [<Test>]
+    member this.``No error for user-specified allowed partial function``() =
+        this.Parse("let x = [| Some 4; None |]
+Array.set x 0 None")
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.``Error for creating a mutable list``() =
+        this.Parse("let x = System.Collections.Generic.List<int>()")
+
+        Assert.IsTrue this.ErrorsExist
+
+    [<Test>]
+    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
+
+    [<Test>]
+    member this.``Error for creating a mutable list with new keyword``() =
+        this.Parse("open System.Collections.Generic
+
+let x = new List<int>()")
+
+        Assert.IsTrue this.ErrorsExist
+
+    [<Test>]
+    member this.``No error for using an existing mutable list``() =
+        this.Parse("open System.Collections.Generic
+
+type IHasList =
+    abstract List: List<int>
+
+let x (hasList: IHasList) = hasList.List")
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.``No error for creating a type from allowed list``() =
+        this.Parse("open System.Collections.Generic
+
+let x = new Dictionary<int,int>()")
+
+        this.AssertNoWarnings()
+