DRAFT: Add DisallowShadowing rule

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)

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
+        }
+    }

src/FSharpLint.Core/Application/Configuration.fs

@@ -324,7 +324,8 @@ type ConventionsConfig =
       favourReRaise:EnabledConfig option
       favourConsistentThis:RuleConfig<FavourConsistentThis.Config> 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<NoPartialFunctions.Config> 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 &&

src/FSharpLint.Core/FSharpLint.Core.fsproj

@@ -55,6 +55,7 @@
     <Compile Include="Rules\Conventions\AvoidSinglePipeOperator.fs" />
     <Compile Include="Rules\Conventions\SuggestUseAutoProperty.fs" />
     <Compile Include="Rules\Conventions\UsedUnderscorePrefixedElements.fs" />
+    <Compile Include="Rules\Conventions\DisallowShadowing.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/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<Ident> =
+    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<WarningDetails> =
+    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

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

src/FSharpLint.Core/fsharplint.json

@@ -290,6 +290,7 @@
         }
     },
     "suggestUseAutoProperty": { "enabled": false },
+    "disallowShadowing": { "enabled": false },
     "avoidTooShortNames": { "enabled": false },
     "asyncExceptionWithoutReturn": { "enabled": false },
     "unneededRecKeyword": { "enabled": true },

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

@@ -44,6 +44,7 @@
     <Compile Include="Rules\Conventions\AvoidSinglePipeOperator.fs" />
     <Compile Include="Rules\Conventions\SuggestUseAutoProperty.fs" />
     <Compile Include="Rules\Conventions\UsedUnderscorePrefixedElements.fs" />
+    <Compile Include="Rules\Conventions\DisallowShadowing.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/DisallowShadowing.fs

@@ -0,0 +1,148 @@
+module FSharpLint.Core.Tests.Rules.Conventions.DisallowShadowing
+
+open NUnit.Framework
+
+open FSharpLint.Rules
+
+[<TestFixture>]
+type TestConventionsDisallowShadowing() =
+    inherit TestAstNodeRuleBase.TestAstNodeRuleBase(DisallowShadowing.rule)
+
+    [<Test>]
+    member this.``Should produce error for shadowed variable``() =
+        this.Parse """
+let foo = 0
+
+module Foo =
+    let foo = 1"""
+
+        Assert.IsTrue this.ErrorsExist
+
+    [<Test>]
+    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
+
+    [<Test>]
+    member this.``Should produce error for shadowed variable (function argument)``() =
+        this.Parse """
+let foo = 0
+let bar foo = foo + 1"""
+
+        Assert.IsTrue this.ErrorsExist
+
+    [<Test>]
+    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
+
+    [<Test>]
+    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
+
+    [<Test>]
+    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
+
+    [<Test>]
+    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
+
+    [<Test>]
+    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
+
+    [<Test>]
+    member this.``Should produce error for shadowed variable (match pattern)``() =
+        this.Parse """
+let foo = 0
+match 1 with
+| foo -> foo"""
+
+        Assert.IsTrue this.ErrorsExist
+
+    [<Test>]
+    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
+
+    [<Test>]
+    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
+
+    [<Test>]
+    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
+
+    [<Test>]
+    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
+
+    [<Test>]
+    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
+
+    [<Test>]
+    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