From b78310364098efe0f82e29f4be430dd1192e91f9 Mon Sep 17 00:00:00 2001 From: ijanus Date: Mon, 14 Feb 2022 17:32:47 +0100 Subject: [PATCH] Add new rule AvoidTypeHintSuffixesInNames Rule to detect type member name suffixes with type hints. --- docs/content/how-tos/rules/FL0077.md | 37 ++++++++ .../Application/Configuration.fs | 5 + src/FSharpLint.Core/FSharpLint.Core.fsproj | 1 + .../AvoidTypeHintSuffixesInNames.fs | 93 +++++++++++++++++++ src/FSharpLint.Core/Rules/Identifiers.fs | 1 + src/FSharpLint.Core/Text.resx | 3 + src/FSharpLint.Core/fsharplint.json | 1 + .../FSharpLint.Core.Tests.fsproj | 1 + .../AvoidTypeHintSuffixesInNames.fs | 79 ++++++++++++++++ 9 files changed, 221 insertions(+) create mode 100644 docs/content/how-tos/rules/FL0077.md create mode 100644 src/FSharpLint.Core/Rules/Conventions/AvoidTypeHintSuffixesInNames.fs create mode 100644 tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidTypeHintSuffixesInNames.fs diff --git a/docs/content/how-tos/rules/FL0077.md b/docs/content/how-tos/rules/FL0077.md new file mode 100644 index 000000000..224618476 --- /dev/null +++ b/docs/content/how-tos/rules/FL0077.md @@ -0,0 +1,37 @@ +--- +title: FL0077 +category: how-to +hide_menu: true +--- + +# AvoidTypeHintSuffixesInNames (FL0077) + +*Introduced in `0.21.2`* + +## Cause + +Use of type hints in names suffixes. + +## Rationale + +The similar practice that predates this was the Systems Hungarian notation dialect that consisted in adding a type-hint as a +prefix for variable names. While still debatable, this practice is not being used anymore because of the redundancy that it +brings (especially in statically typed languages); and using suffixes (instead of prefixes) is no different. This rule, +however, will not flag variables/constants, just type members, because they are the ones that have higher chances to be +affected by the biggest downside from this bad practice: possible inconsistency when the type is modified (and subsequent +need to modify the member name besides the type, which increases the brittleness factor of the API, especially when referring +to public members). + +## How To Fix + +Just remove the type hint suffix; example: a member named "NamesList" of type `List` can simply be named "Names" (so +that in the future could be changed to a Seq or Array without the need to modify the name, or the risk of its name becoming +inconsistent). + +## Rule Settings + + { + "AvoidTypeHintSuffixesInNames": { + "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 7762fe4f7..dcbab6b76 100644 --- a/src/FSharpLint.Core/Application/Configuration.fs +++ b/src/FSharpLint.Core/Application/Configuration.fs @@ -319,6 +319,7 @@ type ConventionsConfig = numberOfItems:NumberOfItemsConfig option binding:BindingConfig option favourReRaise:EnabledConfig option + avoidTypeHintSuffixesInNames:EnabledConfig option favourConsistentThis:RuleConfig option } with member this.Flatten() = @@ -327,6 +328,7 @@ with this.avoidTooShortNames |> Option.bind (constructRuleIfEnabled AvoidTooShortNames.rule) |> Option.toArray this.redundantNewKeyword |> Option.bind (constructRuleIfEnabled RedundantNewKeyword.rule) |> Option.toArray this.favourReRaise |> Option.bind (constructRuleIfEnabled FavourReRaise.rule) |> Option.toArray + this.avoidTypeHintSuffixesInNames |> Option.bind (constructRuleIfEnabled AvoidTypeHintSuffixesInNames.rule) |> Option.toArray this.favourStaticEmptyFields |> Option.bind (constructRuleIfEnabled FavourStaticEmptyFields.rule) |> Option.toArray this.nestedStatements |> Option.bind (constructRuleWithConfig NestedStatements.rule) |> Option.toArray this.favourConsistentThis |> Option.bind (constructRuleWithConfig FavourConsistentThis.rule) |> Option.toArray @@ -396,6 +398,7 @@ type Configuration = AvoidTooShortNames:EnabledConfig option RedundantNewKeyword:EnabledConfig option FavourReRaise:EnabledConfig option + AvoidTypeHintSuffixesInNames:EnabledConfig option FavourStaticEmptyFields:EnabledConfig option NestedStatements:RuleConfig option FavourConsistentThis:RuleConfig option @@ -480,6 +483,7 @@ with AvoidTooShortNames = None RedundantNewKeyword = None FavourReRaise = None + AvoidTypeHintSuffixesInNames = None FavourStaticEmptyFields = None NestedStatements = None FavourConsistentThis = None @@ -627,6 +631,7 @@ let flattenConfig (config:Configuration) = config.AvoidTooShortNames |> Option.bind (constructRuleIfEnabled AvoidTooShortNames.rule) config.RedundantNewKeyword |> Option.bind (constructRuleIfEnabled RedundantNewKeyword.rule) config.FavourReRaise |> Option.bind (constructRuleIfEnabled FavourReRaise.rule) + config.AvoidTypeHintSuffixesInNames |> Option.bind (constructRuleIfEnabled AvoidTypeHintSuffixesInNames.rule) config.FavourStaticEmptyFields |> Option.bind (constructRuleIfEnabled FavourStaticEmptyFields.rule) config.NestedStatements |> Option.bind (constructRuleWithConfig NestedStatements.rule) config.FavourConsistentThis |> Option.bind (constructRuleWithConfig FavourConsistentThis.rule) diff --git a/src/FSharpLint.Core/FSharpLint.Core.fsproj b/src/FSharpLint.Core/FSharpLint.Core.fsproj index aa55c8d30..49616b191 100644 --- a/src/FSharpLint.Core/FSharpLint.Core.fsproj +++ b/src/FSharpLint.Core/FSharpLint.Core.fsproj @@ -50,6 +50,7 @@ + diff --git a/src/FSharpLint.Core/Rules/Conventions/AvoidTypeHintSuffixesInNames.fs b/src/FSharpLint.Core/Rules/Conventions/AvoidTypeHintSuffixesInNames.fs new file mode 100644 index 000000000..5172e14ca --- /dev/null +++ b/src/FSharpLint.Core/Rules/Conventions/AvoidTypeHintSuffixesInNames.fs @@ -0,0 +1,93 @@ +module FSharpLint.Rules.AvoidTypeHintSuffixesInNames + +open FSharpLint.Framework +open FSharpLint.Framework.Suggestion +open FSharp.Compiler.Syntax +open FSharp.Compiler.Text +open FSharpLint.Framework.Ast +open FSharpLint.Framework.Rules +open System + +let ruleName: string = "AvoidTypeHintSuffixesInNames" +let discouragedMemberSuffixes: List = ["Lst"; "List"; "Array"; "Opt"; "Str"] + +let private hasTypeHint (identifier: string) = + let likelySuffixes = + discouragedMemberSuffixes + |> List.filter (fun text -> not (identifier.Equals text)) + likelySuffixes |> List.exists identifier.EndsWith + +let private generateError (range: range) = + { Range = range + Message = Resources.GetString ruleName + SuggestedFix = None + TypeChecks = List.Empty } + |> Array.singleton + +let typeHintSuffixesInRecordFields (fields: List) (range: range) = + let rec traverse recordFields = + match recordFields with + | SynField(_, _, maybeVal, _, _, _, _, _)::rest -> + match maybeVal with + | Some field -> + if hasTypeHint field.idText then + generateError range + else + traverse rest + | None -> + traverse rest + | [] -> Array.empty + + traverse fields + +let typeHintSuffixesInUnionFields (fields: List) (range: range) = + let rec traverse unionCases = + match unionCases with + | SynUnionCase(_, ident, _, _, _, _)::rest -> + if hasTypeHint ident.idText then + generateError range + else + traverse rest + | [] -> Array.empty + + traverse fields + +let typeHintSuffixesInProperties (members: List) (range: range) = + let rec traverse memberDefinitions = + match memberDefinitions with + | SynMemberDefn.AutoProperty(_, _, ident, _, _, _, _, _, _expression, _, _)::rest -> + if hasTypeHint ident.idText then + generateError range + else + traverse rest + | SynMemberDefn.Member(SynBinding(_, _, _, _, _, _, _, pattern, _, _expression, _, _), _)::rest -> + match pattern with + | SynPat.LongIdent(LongIdentWithDots(ident, _), _, _, _, _, _) -> + if hasTypeHint ident.Tail.Head.idText then + generateError range + else + traverse rest + | _ -> traverse rest + | _::rest -> traverse rest + | [] -> Array.empty + + traverse members + +let runner args = + match args.AstNode with + | TypeDefinition(SynTypeDefn(_, typeRepresentation, _members, _implicitCtor, range)) -> + match typeRepresentation with + | SynTypeDefnRepr.Simple(SynTypeDefnSimpleRepr.Record(_, fields, _), _) -> + typeHintSuffixesInRecordFields fields range + | SynTypeDefnRepr.Simple(SynTypeDefnSimpleRepr.Union(_, fields, _), _) -> + typeHintSuffixesInUnionFields fields range + | SynTypeDefnRepr.ObjectModel(_, members, _) -> + typeHintSuffixesInProperties members range + | _ -> Array.empty + | _ -> Array.empty + +let rule = + { Name = ruleName + Identifier = Identifiers.AvoidTypeHintSuffixesInNames + RuleConfig = { AstNodeRuleConfig.Runner = runner; Cleanup = ignore } } + |> AstNodeRule diff --git a/src/FSharpLint.Core/Rules/Identifiers.fs b/src/FSharpLint.Core/Rules/Identifiers.fs index 95fd12259..4230ca947 100644 --- a/src/FSharpLint.Core/Rules/Identifiers.fs +++ b/src/FSharpLint.Core/Rules/Identifiers.fs @@ -81,3 +81,4 @@ let FavourReRaise = identifier 73 let FavourConsistentThis = identifier 74 let AvoidTooShortNames = identifier 75 let FavourStaticEmptyFields = identifier 76 +let AvoidTypeHintSuffixesInNames = identifier 77 diff --git a/src/FSharpLint.Core/Text.resx b/src/FSharpLint.Core/Text.resx index 1c90e1adc..eccc1068b 100644 --- a/src/FSharpLint.Core/Text.resx +++ b/src/FSharpLint.Core/Text.resx @@ -345,4 +345,7 @@ Consider using 'Array.empty' instead. + + Consider removing from the name the suffix that gives a hint about its type. + diff --git a/src/FSharpLint.Core/fsharplint.json b/src/FSharpLint.Core/fsharplint.json index 06cf0a47e..60701d2c5 100644 --- a/src/FSharpLint.Core/fsharplint.json +++ b/src/FSharpLint.Core/fsharplint.json @@ -267,6 +267,7 @@ "favourTypedIgnore": { "enabled": false }, "favourReRaise": { "enabled": true }, "favourStaticEmptyFields": { "enabled": false }, + "AvoidTypeHintSuffixesInNames": { "enabled": false }, "favourConsistentThis": { "enabled": false, "config": { diff --git a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj index abc0ec7ca..8ab5bc2b2 100644 --- a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj +++ b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj @@ -39,6 +39,7 @@ + diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidTypeHintSuffixesInNames.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidTypeHintSuffixesInNames.fs new file mode 100644 index 000000000..991045a3e --- /dev/null +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/AvoidTypeHintSuffixesInNames.fs @@ -0,0 +1,79 @@ +module FSharpLint.Core.Tests.Rules.Conventions.AvoidTypeHintSuffixesInNames + +open NUnit.Framework +open FSharpLint.Rules +open System + +[] +type TestConventionsAvoidTypeHintSuffixesInNames() = + inherit TestAstNodeRuleBase.TestAstNodeRuleBase(AvoidTypeHintSuffixesInNames.rule) + + [] + member this.``Lint flags record member(s) with type hints``() = + this.Parse """ +module Person = + type FullName = { FirstName: string; SurnamesList: List } """ + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Lint flags discriminated union member(s) with type hints``() = + this.Parse """ +type Tree = + | Scalar + | NodeList of int * Tree * Tree """ + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Lint flags autoproperty with type hints``() = + this.Parse """ +type MyClass() = + let random = new System.Random() + member val RandomOpt = (random.Next() |> Some) with get, set """ + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Lint flags property with type hints``() = + this.Parse """ +type MyClass() = + let mutable someString = "someString" + member this.SomeStr with get() = someString and set(v : string) = someString <- v """ + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Lint does not flag discriminated union member(s) without type hints``() = + this.Parse """ +type Tree = + | Tip + | Node of int * Tree * Tree """ + + Assert.IsTrue this.NoErrorsExist + + [] + member this.``Lint only flags type hints when they are suffixes``() = + this.Parse """ +type Tree = + | Opt + | Node of int * Tree * Tree """ + + Assert.IsTrue this.NoErrorsExist + + [] + member this.``Lint does not flag record member(s) without type hints``() = + this.Parse """ +module Person = + type FullName = { First: string; Last: string } """ + + Assert.IsTrue this.NoErrorsExist + + [] + member this.``Lint does not flag variable(s) even if they have type hints``() = + this.Parse """ +let getNothing () = + let valOpt = 90 + valOpt """ + + Assert.IsTrue this.NoErrorsExist