diff --git a/docs/content/how-tos/rule-configuration.md b/docs/content/how-tos/rule-configuration.md index ba7855f70..52c075709 100644 --- a/docs/content/how-tos/rule-configuration.md +++ b/docs/content/how-tos/rule-configuration.md @@ -127,3 +127,4 @@ The following rules can be specified for linting. - [FavourNonMutablePropertyInitialization (FL0084)](rules/FL0084.html) - [EnsureTailCallDiagnosticsInRecursiveFunctions (FL0085)](rules/FL0085.html) - [FavourAsKeyword (FL0086)](rules/FL0086.html) +- [FavourNestedFunctions (FL0087)](rules/FL0087.html) diff --git a/docs/content/how-tos/rules/FL0087.md b/docs/content/how-tos/rules/FL0087.md new file mode 100644 index 000000000..babd0a7b6 --- /dev/null +++ b/docs/content/how-tos/rules/FL0087.md @@ -0,0 +1,31 @@ +--- +title: FL0087 +category: how-to +hide_menu: true +--- + +# FavourNestedFunctions (FL0087) + +*Introduced in `0.24.3`* + +## 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 + } + } \ No newline at end of file diff --git a/src/FSharpLint.Core/Application/Configuration.fs b/src/FSharpLint.Core/Application/Configuration.fs index de6621336..fb41a45c4 100644 --- a/src/FSharpLint.Core/Application/Configuration.fs +++ b/src/FSharpLint.Core/Application/Configuration.fs @@ -309,7 +309,8 @@ type ConventionsConfig = favourConsistentThis:RuleConfig option suggestUseAutoProperty:EnabledConfig option usedUnderscorePrefixedElements:EnabledConfig option - ensureTailCallDiagnosticsInRecursiveFunctions:EnabledConfig option} + ensureTailCallDiagnosticsInRecursiveFunctions:EnabledConfig option + favourNestedFunctions:EnabledConfig option } with member this.Flatten() = [| @@ -335,6 +336,7 @@ with this.binding |> Option.map (fun config -> config.Flatten()) |> Option.toArray |> Array.concat this.suggestUseAutoProperty |> Option.bind (constructRuleIfEnabled SuggestUseAutoProperty.rule) |> Option.toArray this.ensureTailCallDiagnosticsInRecursiveFunctions |> Option.bind (constructRuleIfEnabled EnsureTailCallDiagnosticsInRecursiveFunctions.rule) |> Option.toArray + this.favourNestedFunctions |> Option.bind (constructRuleIfEnabled FavourNestedFunctions.rule) |> Option.toArray |] |> Array.concat type TypographyConfig = @@ -459,9 +461,10 @@ type Configuration = TrailingNewLineInFile:EnabledConfig option NoTabCharacters:EnabledConfig option NoPartialFunctions:RuleConfig option - SuggestUseAutoProperty:EnabledConfig option EnsureTailCallDiagnosticsInRecursiveFunctions:EnabledConfig option - FavourAsKeyword:EnabledConfig option } + FavourAsKeyword:EnabledConfig option + SuggestUseAutoProperty:EnabledConfig option + FavourNestedFunctions:EnabledConfig option } with static member Zero = { Global = None @@ -556,6 +559,7 @@ with SuggestUseAutoProperty = None EnsureTailCallDiagnosticsInRecursiveFunctions = None FavourAsKeyword = None + FavourNestedFunctions = None } // fsharplint:enable RecordFieldNames @@ -714,6 +718,7 @@ let flattenConfig (config:Configuration) = config.SuggestUseAutoProperty |> Option.bind (constructRuleIfEnabled SuggestUseAutoProperty.rule) config.EnsureTailCallDiagnosticsInRecursiveFunctions |> Option.bind (constructRuleIfEnabled EnsureTailCallDiagnosticsInRecursiveFunctions.rule) config.FavourAsKeyword |> Option.bind (constructRuleIfEnabled FavourAsKeyword.rule) + config.FavourNestedFunctions |> Option.bind (constructRuleIfEnabled FavourNestedFunctions.rule) |] |> Array.choose id if config.NonPublicValuesNames.IsSome && diff --git a/src/FSharpLint.Core/FSharpLint.Core.fsproj b/src/FSharpLint.Core/FSharpLint.Core.fsproj index c73bdac00..d415fa306 100644 --- a/src/FSharpLint.Core/FSharpLint.Core.fsproj +++ b/src/FSharpLint.Core/FSharpLint.Core.fsproj @@ -58,6 +58,7 @@ + diff --git a/src/FSharpLint.Core/Rules/Conventions/FavourNestedFunctions.fs b/src/FSharpLint.Core/Rules/Conventions/FavourNestedFunctions.fs new file mode 100644 index 000000000..bb82acaec --- /dev/null +++ b/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(SynLongIdent([ident], _, _), _, _, _, accessibility, _) -> + Some(ident, expr, accessibility) + | _ -> None + | _ -> None + +let runner (args: AstNodeRuleParams) = + match args.AstNode with + | AstNode.ModuleOrNamespace(SynModuleOrNamespace(_, _, _kind, declarations, _, _, _, _, _)) -> + let privateFunctionIdentifiers = + declarations + |> Seq.choose + (fun declaration -> + match declaration with + | FunctionDeclaration(ident, _body, Some(SynAccess.Private _)) -> + Some ident + | _ -> None) + |> Seq.toArray + + 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 + |> Seq.filter (fun funcBody -> + checkInfo.GetUsesOfSymbolInFile symbolUse.Symbol + |> Array.exists (fun usage -> ExpressionUtilities.rangeContainsOtherRange funcBody.Range usage.Range)) + |> Seq.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 diff --git a/src/FSharpLint.Core/Rules/Identifiers.fs b/src/FSharpLint.Core/Rules/Identifiers.fs index ecc0da8b5..4adea183a 100644 --- a/src/FSharpLint.Core/Rules/Identifiers.fs +++ b/src/FSharpLint.Core/Rules/Identifiers.fs @@ -92,3 +92,4 @@ let UnneededRecKeyword = identifier 83 let FavourNonMutablePropertyInitialization = identifier 84 let EnsureTailCallDiagnosticsInRecursiveFunctions = identifier 85 let FavourAsKeyword = identifier 86 +let FavourNestedFunctions = identifier 87 diff --git a/src/FSharpLint.Core/Text.resx b/src/FSharpLint.Core/Text.resx index 17681b968..7d0fd8604 100644 --- a/src/FSharpLint.Core/Text.resx +++ b/src/FSharpLint.Core/Text.resx @@ -372,4 +372,7 @@ Prefer using the 'as' pattern to match a constant and bind it to a variable. + + Prefer using local functions over private module-level functions + diff --git a/src/FSharpLint.Core/fsharplint.json b/src/FSharpLint.Core/fsharplint.json index 8c5ec8307..1823b8950 100644 --- a/src/FSharpLint.Core/fsharplint.json +++ b/src/FSharpLint.Core/fsharplint.json @@ -333,6 +333,7 @@ }, "ensureTailCallDiagnosticsInRecursiveFunctions": { "enabled": false }, "favourAsKeyword": { "enabled": true }, + "favourNestedFunctions": { "enabled": false }, "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 3d82af5a0..c3f59b75d 100644 --- a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj +++ b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj @@ -46,6 +46,7 @@ + diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/FavourNestedFunctions.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/FavourNestedFunctions.fs new file mode 100644 index 000000000..da408c395 --- /dev/null +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/FavourNestedFunctions.fs @@ -0,0 +1,76 @@ +module FSharpLint.Core.Tests.Rules.Conventions.FavourNestedFunctions + +open NUnit.Framework +open FSharpLint.Rules +open FSharpLint.Core.Tests + +[] +type TestFavourNestedFunctions() = + inherit TestAstNodeRuleBase.TestAstNodeRuleBase(FavourNestedFunctions.rule) + + [] + 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() + + [] + 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() + + [] + 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 + + [] + member this.``Nested functions should not give an error`` () = + this.Parse """ +let Bar () = + let Foo() = + () + + Foo() + () +""" + + this.AssertNoWarnings() + + [] + 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()