From f6e0fba6a9bc8cf61a92c03570130e2cedbeea5a Mon Sep 17 00:00:00 2001 From: webwarrior Date: Thu, 21 Dec 2023 13:36:34 +0100 Subject: [PATCH 1/3] Core(Tests): add FavourNestedFunctions rule Added FavourNestedFunctions rule and tests for it. --- src/FSharpLint.Core/FSharpLint.Core.fsproj | 1 + .../Conventions/FavourNestedFunctions.fs | 19 +++++ src/FSharpLint.Core/Rules/Identifiers.fs | 1 + .../FSharpLint.Core.Tests.fsproj | 1 + .../Conventions/FavourNestedFunctions.fs | 76 +++++++++++++++++++ 5 files changed, 98 insertions(+) create mode 100644 src/FSharpLint.Core/Rules/Conventions/FavourNestedFunctions.fs create mode 100644 tests/FSharpLint.Core.Tests/Rules/Conventions/FavourNestedFunctions.fs diff --git a/src/FSharpLint.Core/FSharpLint.Core.fsproj b/src/FSharpLint.Core/FSharpLint.Core.fsproj index e0eea381c..ba5ae52de 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/FavourNestedFunctions.fs b/src/FSharpLint.Core/Rules/Conventions/FavourNestedFunctions.fs new file mode 100644 index 000000000..67f13141e --- /dev/null +++ b/src/FSharpLint.Core/Rules/Conventions/FavourNestedFunctions.fs @@ -0,0 +1,19 @@ +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 runner (args: AstNodeRuleParams) = + failwith "Not yet implemeted" + +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 0999cd8e9..474afd5e7 100644 --- a/src/FSharpLint.Core/Rules/Identifiers.fs +++ b/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 diff --git a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj index ccd3b58c4..dea4a3e7c 100644 --- a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj +++ b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj @@ -42,6 +42,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..785683e5f --- /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.Framework.Rules +open FSharpLint.Rules + +[] +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() From 62375ea81a4d35868c870b9721d19817b2dd020e Mon Sep 17 00:00:00 2001 From: webwarrior Date: Thu, 21 Dec 2023 13:37:23 +0100 Subject: [PATCH 2/3] FavourNestedFunctions: implement rule Implemented FavourNestedFunctions rule. Added rule text message to Text.resx. Fixes #638 --- .../Conventions/FavourNestedFunctions.fs | 57 ++++++++++++++++++- src/FSharpLint.Core/Text.resx | 3 + 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/FSharpLint.Core/Rules/Conventions/FavourNestedFunctions.fs b/src/FSharpLint.Core/Rules/Conventions/FavourNestedFunctions.fs index 67f13141e..1a65b76b1 100644 --- a/src/FSharpLint.Core/Rules/Conventions/FavourNestedFunctions.fs +++ b/src/FSharpLint.Core/Rules/Conventions/FavourNestedFunctions.fs @@ -7,8 +7,63 @@ 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) = - failwith "Not yet implemeted" + 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" diff --git a/src/FSharpLint.Core/Text.resx b/src/FSharpLint.Core/Text.resx index 7a6d15976..8ca1c4d4b 100644 --- a/src/FSharpLint.Core/Text.resx +++ b/src/FSharpLint.Core/Text.resx @@ -354,4 +354,7 @@ Consider using auto-properties via the 'val' keyword. + + Prefer using local functions over private module-level functions + From 8d130ab071ed1237da106670b9bf34789e4b18e6 Mon Sep 17 00:00:00 2001 From: webwarrior Date: Thu, 21 Dec 2023 13:51:50 +0100 Subject: [PATCH 3/3] FavourNestedFunctions: hook cfg & add docs Updated Configuration.fs and fsharplint.json to include FavourNestedFunctions rule. Added docs. --- docs/content/how-tos/rule-configuration.md | 1 + docs/content/how-tos/rules/FL0082.md | 31 +++++++++++++++++++ .../Application/Configuration.fs | 9 ++++-- src/FSharpLint.Core/fsharplint.json | 1 + 4 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 docs/content/how-tos/rules/FL0082.md diff --git a/docs/content/how-tos/rule-configuration.md b/docs/content/how-tos/rule-configuration.md index 2906e3f95..264d24e3c 100644 --- a/docs/content/how-tos/rule-configuration.md +++ b/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) diff --git a/docs/content/how-tos/rules/FL0082.md b/docs/content/how-tos/rules/FL0082.md new file mode 100644 index 000000000..f78153356 --- /dev/null +++ b/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 + } + } \ No newline at end of file diff --git a/src/FSharpLint.Core/Application/Configuration.fs b/src/FSharpLint.Core/Application/Configuration.fs index d010da390..3e591f445 100644 --- a/src/FSharpLint.Core/Application/Configuration.fs +++ b/src/FSharpLint.Core/Application/Configuration.fs @@ -322,7 +322,8 @@ type ConventionsConfig = binding:BindingConfig option favourReRaise:EnabledConfig option favourConsistentThis:RuleConfig 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 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 && diff --git a/src/FSharpLint.Core/fsharplint.json b/src/FSharpLint.Core/fsharplint.json index 45ff93395..503c9e187 100644 --- a/src/FSharpLint.Core/fsharplint.json +++ b/src/FSharpLint.Core/fsharplint.json @@ -323,6 +323,7 @@ "additionalPartials": [] } }, + "favourNestedFunctions": { "enabled": false }, "hints": { "add": [ "not (a = b) ===> a <> b",