DRAFT: Favour nested functions

docs/content/how-tos/rule-configuration.md

@@ -122,3 +122,4 @@ The following rules can be specified for linting.
 - [SuggestUseAutoProperty (FL0079)](rules/FL0079.html)
 - [UnnestedFunctionNames (FL0080)](rules/FL0080.html)
 - [NestedFunctionNames (FL0081)](rules/FL0081.html)
+- [FavourNestedFunctions (FL0082)](rules/FL0082.html)

docs/content/how-tos/rules/FL0082.md

@@ -0,0 +1,31 @@
+---
+title: FL0082
+category: how-to
+hide_menu: true
+---
+
+# FavourNestedFunctions (FL0082)
+
+*Introduced in `0.23.0`*
+
+## Cause
+
+Prefer using local (nested) functions over private module-level functions.
+
+## Rationale
+
+With a nested function, one can clearly see from reading the code that there is only one consumer of the function.
+The code being this way becomes more streamlined.
+Code is also more concise because nested functions don't need accessibility modifiers.
+
+## How To Fix
+
+Move private function inside function that uses it.
+
+## Rule Settings
+
+    {
+        "FavourNestedFunctions": {
+            "enabled": false
+        }
+    }

src/FSharpLint.Core/Application/Configuration.fs

@@ -322,7 +322,8 @@ type ConventionsConfig =
       binding:BindingConfig option
       favourReRaise:EnabledConfig option
       favourConsistentThis:RuleConfig<FavourConsistentThis.Config> option
-      suggestUseAutoProperty:EnabledConfig option}
+      suggestUseAutoProperty:EnabledConfig option
+      favourNestedFunctions:EnabledConfig option }
 with
     member this.Flatten() =
         [|
@@ -344,6 +345,7 @@ with
             this.numberOfItems |> Option.map (fun config -> config.Flatten()) |> Option.toArray |> Array.concat
             this.binding |> Option.map (fun config -> config.Flatten()) |> Option.toArray |> Array.concat
             this.suggestUseAutoProperty |> Option.bind (constructRuleIfEnabled SuggestUseAutoProperty.rule) |> Option.toArray
+            this.favourNestedFunctions |> Option.bind (constructRuleIfEnabled FavourNestedFunctions.rule) |> Option.toArray
         |] |> Array.concat
 
 type TypographyConfig =
@@ -463,7 +465,8 @@ type Configuration =
       TrailingNewLineInFile:EnabledConfig option
       NoTabCharacters:EnabledConfig option
       NoPartialFunctions:RuleConfig<NoPartialFunctions.Config> option
-      SuggestUseAutoProperty:EnabledConfig option }
+      SuggestUseAutoProperty:EnabledConfig option
+      FavourNestedFunctions:EnabledConfig option }
 with
     static member Zero = {
         Global = None
@@ -551,6 +554,7 @@ with
         NoTabCharacters = None
         NoPartialFunctions = None
         SuggestUseAutoProperty = None
+        FavourNestedFunctions = None
     }
 
 // fsharplint:enable RecordFieldNames
@@ -701,6 +705,7 @@ let flattenConfig (config:Configuration) =
             config.TrailingNewLineInFile |> Option.bind (constructRuleIfEnabled TrailingNewLineInFile.rule)
             config.NoTabCharacters |> Option.bind (constructRuleIfEnabled NoTabCharacters.rule)
             config.NoPartialFunctions |> Option.bind (constructRuleWithConfig NoPartialFunctions.rule)
+            config.FavourNestedFunctions |> Option.bind (constructRuleIfEnabled FavourNestedFunctions.rule)
         |] |> Array.choose id
 
     if config.NonPublicValuesNames.IsSome &&

src/FSharpLint.Core/FSharpLint.Core.fsproj

@@ -53,6 +53,7 @@
     <Compile Include="Rules\Conventions\FavourConsistentThis.fs" />
     <Compile Include="Rules\Conventions\AvoidSinglePipeOperator.fs" />
     <Compile Include="Rules\Conventions\SuggestUseAutoProperty.fs" />
+    <Compile Include="Rules\Conventions\FavourNestedFunctions.fs" />
     <Compile Include="Rules\Conventions\RaiseWithTooManyArguments\RaiseWithTooManyArgumentsHelper.fs" />
     <Compile Include="Rules\Conventions\RaiseWithTooManyArguments\FailwithWithSingleArgument.fs" />
     <Compile Include="Rules\Conventions\RaiseWithTooManyArguments\RaiseWithSingleArgument.fs" />

src/FSharpLint.Core/Rules/Conventions/FavourNestedFunctions.fs

@@ -0,0 +1,74 @@
+module FSharpLint.Rules.FavourNestedFunctions
+
+open System
+open FSharp.Compiler.Syntax
+open FSharpLint.Framework.Ast
+open FSharpLint.Framework.Rules
+open FSharpLint.Framework
+open FSharpLint.Framework.Suggestion
+
+let private (|FunctionDeclaration|_|) (declaration: SynModuleDecl) = 
+    match declaration with
+    | SynModuleDecl.Let(_, [ SynBinding(_, _, _, _, _, _, _, headPat, _, expr, _, _) ], _) ->
+        match headPat with
+        | SynPat.LongIdent(LongIdentWithDots([ident], _), _, _, _, accessibility, _) ->
+            Some(ident, expr, accessibility)
+        | _ -> None
+    | _ -> None
+
+let runner (args: AstNodeRuleParams) =
+    match args.AstNode with
+    | AstNode.ModuleOrNamespace(SynModuleOrNamespace(_, _, _kind, declarations, _, _, _, _)) ->
+        let privateFunctionIdentifiers = 
+            declarations
+            |> List.toArray
+            |> Array.choose 
+                (fun declaration ->
+                    match declaration with
+                    | FunctionDeclaration(ident, _body, Some(SynAccess.Private)) -> 
+                        Some ident
+                    | _ -> None)
+
+        match args.CheckInfo with
+        | Some checkInfo when privateFunctionIdentifiers.Length > 0 ->
+            let otherFunctionBodies =
+                declarations
+                |> List.choose 
+                    (fun declaration ->
+                        match declaration with
+                        | FunctionDeclaration(ident, body, _) 
+                            when not(Array.exists (fun (each: Ident) -> each.idText = ident.idText) privateFunctionIdentifiers) -> 
+                            Some body
+                        | _ -> None)
+
+            privateFunctionIdentifiers
+            |> Array.choose
+                (fun currFunctionIdentifier ->
+                    match ExpressionUtilities.getSymbolFromIdent args.CheckInfo (SynExpr.Ident currFunctionIdentifier) with
+                    | Some symbolUse ->
+                        let numberOfOtherFunctionsCurrFunctionIsUsedIn =
+                            otherFunctionBodies
+                            |> List.filter (fun funcBody -> 
+                                checkInfo.GetUsesOfSymbolInFile symbolUse.Symbol
+                                |> Array.exists (fun usage -> ExpressionUtilities.rangeContainsOtherRange funcBody.Range usage.Range))
+                            |> List.length
+                        if numberOfOtherFunctionsCurrFunctionIsUsedIn = 1 then
+                            Some {
+                                Range = currFunctionIdentifier.idRange
+                                WarningDetails.Message = Resources.GetString "RulesFavourNestedFunctions"
+                                SuggestedFix = None
+                                TypeChecks = List.Empty
+                            }
+                        else
+                            None
+                    | None -> None)
+        | _ -> Array.empty
+    | _ -> Array.empty
+
+let rule =
+    { Name = "FavourNestedFunctions"
+      Identifier = Identifiers.FavourNestedFunctions
+      RuleConfig =
+        { AstNodeRuleConfig.Runner = runner
+          Cleanup = ignore } }
+    |> AstNodeRule

src/FSharpLint.Core/Rules/Identifiers.fs

@@ -86,3 +86,4 @@ let AsyncExceptionWithoutReturn = identifier 78
 let SuggestUseAutoProperty = identifier 79
 let UnnestedFunctionNames = identifier 80
 let NestedFunctionNames = identifier 81
+let FavourNestedFunctions = identifier 82

src/FSharpLint.Core/Text.resx

@@ -354,4 +354,7 @@
   <data name="RulesSuggestUseAutoProperty" xml:space="preserve">
     <value>Consider using auto-properties via the 'val' keyword.</value>
   </data>
+  <data name="RulesFavourNestedFunctions" xml:space="preserve">
+    <value>Prefer using local functions over private module-level functions</value>
+  </data>
 </root>

src/FSharpLint.Core/fsharplint.json

@@ -323,6 +323,7 @@
             "additionalPartials": []
         }
     },
+    "favourNestedFunctions": { "enabled": false },
     "hints": {
         "add": [
             "not (a =  b) ===> a <> b",

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

@@ -42,6 +42,7 @@
     <Compile Include="Rules\Conventions\AvoidTooShortNames.fs" />
     <Compile Include="Rules\Conventions\AvoidSinglePipeOperator.fs" />
     <Compile Include="Rules\Conventions\SuggestUseAutoProperty.fs" />
+    <Compile Include="Rules\Conventions\FavourNestedFunctions.fs" />
     <Compile Include="Rules\Conventions\Naming\NamingHelpers.fs" />
     <Compile Include="Rules\Conventions\Naming\InterfaceNames.fs" />
     <Compile Include="Rules\Conventions\Naming\ExceptionNames.fs" />

tests/FSharpLint.Core.Tests/Rules/Conventions/FavourNestedFunctions.fs

@@ -0,0 +1,76 @@
+module FSharpLint.Core.Tests.Rules.Conventions.FavourNestedFunctions
+
+open NUnit.Framework
+open FSharpLint.Framework.Rules
+open FSharpLint.Rules
+
+[<TestFixture>]
+type TestFavourNestedFunctions() =
+    inherit TestAstNodeRuleBase.TestAstNodeRuleBase(FavourNestedFunctions.rule)
+
+    [<Test>]
+    member this.``Top level functions that are not used in another function should not give an error`` () =
+        this.Parse """
+let Foo () =
+    ()
+
+let Bar () =
+    ()
+"""
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.``Top level private functions that are not used in another function should not give an error`` () =
+        this.Parse """
+let private Foo () =
+    ()
+
+let Bar () =
+    ()
+"""
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.``Top level private function that is used in single function should give an error`` () =
+        this.Parse """
+let private Foo () =
+    ()
+
+let Bar () =
+    Foo()
+    ()
+"""
+
+        Assert.IsTrue this.ErrorsExist
+
+    [<Test>]
+    member this.``Nested functions should not give an error`` () =
+        this.Parse """
+let Bar () =
+    let Foo() =
+        ()
+
+    Foo()
+    ()
+"""
+
+        this.AssertNoWarnings()
+
+    [<Test>]
+    member this.``Private function that is used in more than one function should not give an error`` () =
+        this.Parse """
+let private Foo () =
+    ()
+
+let Bar () =
+    Foo()
+    ()
+
+let Baz () =
+    Foo ()
+    ()
+"""
+
+        this.AssertNoWarnings()