diff --git a/docs/content/how-tos/rule-configuration.md b/docs/content/how-tos/rule-configuration.md index 41c276352..2405a2acf 100644 --- a/docs/content/how-tos/rule-configuration.md +++ b/docs/content/how-tos/rule-configuration.md @@ -124,3 +124,4 @@ The following rules can be specified for linting. - [NestedFunctionNames (FL0081)](rules/FL0081.html) - [UsedUnderscorePrefixedElements (FL0082)](rules/FL0082.html) - [UnneededRecKeyword (FL0083)](rules/FL0083.html) +- [DisallowShadowing (FL0084)](rules/FL0084.html) diff --git a/docs/content/how-tos/rules/FL0084.md b/docs/content/how-tos/rules/FL0084.md new file mode 100644 index 000000000..6283a6364 --- /dev/null +++ b/docs/content/how-tos/rules/FL0084.md @@ -0,0 +1,29 @@ +--- +title: FL0084 +category: how-to +hide_menu: true +--- + +# DisallowShadowing (FL0084) + +*Introduced in `0.23.8`* + +## Cause + +A variable or parameter shadows another one with the same name. + +## Rationale + +Sometimes shadowing can cause confusion. + +## How To Fix + +Rename varaible or parameter in question so it has unique name in its scope. + +## Rule Settings + + { + "disallowShadowing": { + "enabled": false + } + } diff --git a/src/FSharpLint.Core/Application/Configuration.fs b/src/FSharpLint.Core/Application/Configuration.fs index 20c30cc4c..534da1c2d 100644 --- a/src/FSharpLint.Core/Application/Configuration.fs +++ b/src/FSharpLint.Core/Application/Configuration.fs @@ -324,7 +324,8 @@ type ConventionsConfig = favourReRaise:EnabledConfig option favourConsistentThis:RuleConfig option suggestUseAutoProperty:EnabledConfig option - usedUnderscorePrefixedElements:EnabledConfig option } + usedUnderscorePrefixedElements:EnabledConfig option + disallowShadowing:EnabledConfig option } with member this.Flatten() = [| @@ -348,6 +349,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.disallowShadowing |> Option.bind (constructRuleIfEnabled DisallowShadowing.rule) |> Option.toArray |] |> Array.concat type TypographyConfig = @@ -469,7 +471,8 @@ type Configuration = TrailingNewLineInFile:EnabledConfig option NoTabCharacters:EnabledConfig option NoPartialFunctions:RuleConfig option - SuggestUseAutoProperty:EnabledConfig option } + SuggestUseAutoProperty:EnabledConfig option + DisallowShadowing:EnabledConfig option } with static member Zero = { Global = None @@ -559,6 +562,7 @@ with NoTabCharacters = None NoPartialFunctions = None SuggestUseAutoProperty = None + DisallowShadowing = None } // fsharplint:enable RecordFieldNames @@ -711,6 +715,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.DisallowShadowing |> Option.bind (constructRuleIfEnabled DisallowShadowing.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 c3a415544..e568ae68e 100644 --- a/src/FSharpLint.Core/FSharpLint.Core.fsproj +++ b/src/FSharpLint.Core/FSharpLint.Core.fsproj @@ -55,6 +55,7 @@ + diff --git a/src/FSharpLint.Core/Rules/Conventions/DisallowShadowing.fs b/src/FSharpLint.Core/Rules/Conventions/DisallowShadowing.fs new file mode 100644 index 000000000..6093cd182 --- /dev/null +++ b/src/FSharpLint.Core/Rules/Conventions/DisallowShadowing.fs @@ -0,0 +1,136 @@ +module FSharpLint.Rules.DisallowShadowing + +open System +open FSharp.Compiler.Syntax +open FSharpLint.Framework +open FSharpLint.Framework.Suggestion +open FSharpLint.Framework.Ast +open FSharpLint.Framework.Rules + +let rec private extractIdentifiersFromSimplePats (simplePats: SynSimplePats) : List = + let rec extractIdentifier (pattern: SynSimplePat) = + match pattern with + | SynSimplePat.Id(ident, _, _, _, _, _) -> + ident + | SynSimplePat.Attrib(pat, _, _) + | SynSimplePat.Typed(pat, _, _) -> + extractIdentifier pat + + match simplePats with + | SynSimplePats.SimplePats(patterns, _) -> + patterns |> List.map extractIdentifier + | SynSimplePats.Typed(pats, _, _) -> + extractIdentifiersFromSimplePats pats + + +let private checkIdentifier (args: AstNodeRuleParams) (identifier: Ident) : array = + let name = identifier.idText + match args.CheckInfo with + | Some checkResults when not (name.StartsWith '_') -> + let allUsages = checkResults.GetAllUsesOfAllSymbolsInFile() + let definitionsWithSameName = + allUsages + |> Seq.filter (fun usage -> usage.IsFromDefinition && usage.Symbol.DisplayName = name) + + let definitionsBeforeCurrent = + definitionsWithSameName + |> Seq.filter + (fun usage -> + (usage.Range.StartLine, usage.Range.StartColumn) + < (identifier.idRange.StartLine, identifier.idRange.StartColumn) ) + |> Seq.toArray + + let rangeIncludedsDefinitions range = + definitionsBeforeCurrent + |> Array.exists (fun usage -> ExpressionUtilities.rangeContainsOtherRange range usage.Range) + + let processBinding binding = + match binding with + | SynBinding(_, _, _, _, _, _, _, _, _, _, range, _, _) -> + rangeIncludedsDefinitions range + + let processArgs (args: SynSimplePats) = + args + |> extractIdentifiersFromSimplePats + |> List.exists (fun ident -> rangeIncludedsDefinitions ident.idRange) + + let rec processExpression (expression: SynExpr) = + match expression with + | SynExpr.LetOrUse(_, _, bindings, _, _, _) -> + bindings |> List.exists processBinding + | SynExpr.Sequential(_, _, expr1, expr2, _) -> + processExpression expr1 || processExpression expr2 + | SynExpr.Lambda(_, _, args, body, _, _, _) -> + processExpression body || processArgs args + | _ -> false + + let rec processPattern (pattern: SynPat) = + match pattern with + | SynPat.Named(SynIdent(ident, _), _, _, _) -> rangeIncludedsDefinitions ident.idRange + | SynPat.Ands(pats, _) -> pats |> List.exists processPattern + | SynPat.Or(lhs, rhs, _, _) -> processPattern lhs || processPattern rhs + | SynPat.ArrayOrList(_, pats, _) -> pats |> List.exists processPattern + | SynPat.As(_lhs, rhs, _) -> processPattern rhs + | SynPat.OptionalVal(ident, _) -> rangeIncludedsDefinitions ident.idRange + | SynPat.Paren(pat, _) -> processPattern pat + | SynPat.Record(fieldPats, _) -> fieldPats |> List.exists (fun (_, _, pat) -> processPattern pat) + | SynPat.Tuple(_, pats, _) -> pats |> List.exists processPattern + | SynPat.Typed(pat, _, _) -> processPattern pat + | _ -> false + + let processModuleDeclaration (moduleDecl: SynModuleDecl) = + match moduleDecl with + | SynModuleDecl.Let(_, bindings, _) -> + bindings + |> List.exists processBinding + | _ -> false + + let processAstNode (node: AstNode) = + match node with + | ModuleOrNamespace(SynModuleOrNamespace(_, _, _, declarations, _, _, _, _, _)) -> + declarations |> List.exists processModuleDeclaration + | Expression(expression) -> + processExpression (ExpressionUtilities.removeParens expression) + | Lambda(lambda, _) -> + processExpression lambda.Body + || (lambda.Arguments |> List.exists processArgs) + | Match(SynMatchClause(pattern, _, _, _, _, _)) -> + processPattern pattern + | MemberDefinition(SynMemberDefn.Member(memberDefn, _)) -> + processBinding memberDefn + | TypeDefinition(SynTypeDefn(_, _, _, Some(SynMemberDefn.ImplicitCtor(_, _, ctorArgs, _, _, _)), _, _)) -> + processArgs ctorArgs + | _ -> false + + let parents = args.GetParents args.NodeIndex + let isShadowing = + parents |> List.exists processAstNode + + if isShadowing then + Array.singleton { + Range = identifier.idRange + Message = "RulesDisallowShadowing" + SuggestedFix = None + TypeChecks = List.Empty } + else + Array.empty + + | _ -> Array.empty + +let runner (args: AstNodeRuleParams) = + match args.AstNode with + | Pattern(SynPat.Named(SynIdent(ident, _), _, _, _)) -> + checkIdentifier args ident + | Lambda(lambda, _) -> + lambda.Arguments + |> List.collect extractIdentifiersFromSimplePats + |> List.toArray + |> Array.collect (fun each -> checkIdentifier args each) + | _ -> + Array.empty + +let rule = + { Name = "DisallowShadowing" + Identifier = Identifiers.DisallowShadowing + RuleConfig = { AstNodeRuleConfig.Runner = runner; Cleanup = ignore } } + |> AstNodeRule diff --git a/src/FSharpLint.Core/Rules/Identifiers.fs b/src/FSharpLint.Core/Rules/Identifiers.fs index cd56d66e2..96be7a8ef 100644 --- a/src/FSharpLint.Core/Rules/Identifiers.fs +++ b/src/FSharpLint.Core/Rules/Identifiers.fs @@ -88,3 +88,4 @@ let UnnestedFunctionNames = identifier 80 let NestedFunctionNames = identifier 81 let UsedUnderscorePrefixedElements = identifier 82 let UnneededRecKeyword = identifier 83 +let DisallowShadowing = identifier 84 diff --git a/src/FSharpLint.Core/fsharplint.json b/src/FSharpLint.Core/fsharplint.json index 70156d75e..cb3574c7f 100644 --- a/src/FSharpLint.Core/fsharplint.json +++ b/src/FSharpLint.Core/fsharplint.json @@ -290,6 +290,7 @@ } }, "suggestUseAutoProperty": { "enabled": false }, + "disallowShadowing": { "enabled": false }, "avoidTooShortNames": { "enabled": false }, "asyncExceptionWithoutReturn": { "enabled": false }, "unneededRecKeyword": { "enabled": true }, diff --git a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj index a7ed95820..fef585767 100644 --- a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj +++ b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj @@ -44,6 +44,7 @@ + diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/DisallowShadowing.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/DisallowShadowing.fs new file mode 100644 index 000000000..64e719735 --- /dev/null +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/DisallowShadowing.fs @@ -0,0 +1,148 @@ +module FSharpLint.Core.Tests.Rules.Conventions.DisallowShadowing + +open NUnit.Framework + +open FSharpLint.Rules + +[] +type TestConventionsDisallowShadowing() = + inherit TestAstNodeRuleBase.TestAstNodeRuleBase(DisallowShadowing.rule) + + [] + member this.``Should produce error for shadowed variable``() = + this.Parse """ +let foo = 0 + +module Foo = + let foo = 1""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable, inside function``() = + this.Parse """ +let bar () = + let foo = 0 + let foo = 1 + foo""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable (function argument)``() = + this.Parse """ +let foo = 0 +let bar foo = foo + 1""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable (function argument, inside function)``() = + this.Parse """ +let baz () = + let foo = 0 + let bar foo = foo + 1 + ()""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable (function argument, nested)``() = + this.Parse """ +let baz foo = + let bar foo = foo + 1 + ()""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable (function argument, tuple)``() = + this.Parse """ +let foo = 0 +let bar (foo, baz) = foo + baz""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable (lambda function argument)``() = + this.Parse """ +let foo = 0 +(fun foo -> foo + 1) 0 |> ignore""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable inside lambda function``() = + this.Parse """ +(fun foo -> + let foo = foo + 1 + foo) 0 |> ignore""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable (match pattern)``() = + this.Parse """ +let foo = 0 +match 1 with +| foo -> foo""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable (as match pattern)``() = + this.Parse """ +let foo = 0 +match (1, 2) with +| (x, y) as foo -> foo""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable inside match pattern``() = + this.Parse """ +match 1 with +| foo -> + let foo = foo + 1 + foo""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable inside type definition``() = + this.Parse """ +type Foo(foo) = + let foo = foo + 1 + + member this.Bar = foo""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should produce error for shadowed variable inside member definition``() = + this.Parse """ +type Foo() = + member this.Bar(foo) = + let foo = foo + 1 + foo""" + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Should not produce error when variable with same name exists in another module``() = + this.Parse """ +module Foo = + let foo = 0 +let foo = 1""" + + Assert.IsTrue this.NoErrorsExist + + [] + member this.``Should not produce error when variable name starts with underscore``() = + this.Parse """ +let _foo = 0 + +module Foo = + let _foo = 1""" + + Assert.IsTrue this.NoErrorsExist