DRAFT: Add new rule PreferStringInterpolationWithSprintf

docs/content/how-tos/rule-configuration.md

@@ -117,3 +117,4 @@ The following rules can be specified for linting.
 - [FavourConsistentThis (FL0074)](rules/FL0074.html)
 - [AvoidTooShortNames (FL0075)](rules/FL0075.html)
 - [FavourStaticEmptyFields (FL0076)](rules/FL0076.html)
+- [PreferStringInterpolationWithSprintf (FL0077)](rules/FL0077.html)

docs/content/how-tos/rules/FL0077.md

@@ -0,0 +1,29 @@
+---
+title: FL0077
+category: how-to
+hide_menu: true
+---
+
+# PreferStringInterpolationWithSprintf (FL0077)
+
+*Introduced in `0.21.3`*
+
+## Cause
+
+String interpolation is done with String.Format.
+
+## Rationale
+
+sprintf is statically type checked and with sprintf F# compiler will complain when too few arguments are supplied (unlike with String.Format).
+
+## How To Fix
+
+Use sprintf instead of String.Format.
+
+## Rule Settings
+
+    {
+        "preferStringInterpolationWithSprintf": {
+            "enabled": false
+        }
+    }

src/FSharpLint.Core/Application/Configuration.fs

@@ -307,6 +307,7 @@ with
 type ConventionsConfig =
     { recursiveAsyncFunction:EnabledConfig option
       avoidTooShortNames:EnabledConfig option
+      preferStringInterpolationWithSprintf:EnabledConfig option
       redundantNewKeyword:EnabledConfig option
       favourStaticEmptyFields:EnabledConfig option
       nestedStatements:RuleConfig<NestedStatements.Config> option
@@ -325,6 +326,7 @@ with
         [|
             this.recursiveAsyncFunction |> Option.bind (constructRuleIfEnabled RecursiveAsyncFunction.rule) |> Option.toArray
             this.avoidTooShortNames |> Option.bind (constructRuleIfEnabled AvoidTooShortNames.rule) |> Option.toArray           
+            this.preferStringInterpolationWithSprintf |> Option.bind (constructRuleIfEnabled PreferStringInterpolationWithSprintf.rule) |> Option.toArray           
             this.redundantNewKeyword |> Option.bind (constructRuleIfEnabled RedundantNewKeyword.rule) |> Option.toArray
             this.favourReRaise |> Option.bind (constructRuleIfEnabled FavourReRaise.rule) |> Option.toArray
             this.favourStaticEmptyFields |> Option.bind (constructRuleIfEnabled FavourStaticEmptyFields.rule) |> Option.toArray
@@ -394,6 +396,7 @@ type Configuration =
       PatternMatchExpressionIndentation:EnabledConfig option
       RecursiveAsyncFunction:EnabledConfig option
       AvoidTooShortNames:EnabledConfig option
+      PreferStringInterpolationWithSprintf:EnabledConfig option
       RedundantNewKeyword:EnabledConfig option
       FavourReRaise:EnabledConfig option
       FavourStaticEmptyFields:EnabledConfig option
@@ -478,6 +481,7 @@ with
         PatternMatchExpressionIndentation = None
         RecursiveAsyncFunction = None
         AvoidTooShortNames = None
+        PreferStringInterpolationWithSprintf = None
         RedundantNewKeyword = None
         FavourReRaise = None
         FavourStaticEmptyFields = None
@@ -625,6 +629,7 @@ let flattenConfig (config:Configuration) =
             config.PatternMatchExpressionIndentation |> Option.bind (constructRuleIfEnabled PatternMatchExpressionIndentation.rule)
             config.RecursiveAsyncFunction |> Option.bind (constructRuleIfEnabled RecursiveAsyncFunction.rule)
             config.AvoidTooShortNames |> Option.bind (constructRuleIfEnabled AvoidTooShortNames.rule)
+            config.PreferStringInterpolationWithSprintf |> Option.bind (constructRuleIfEnabled PreferStringInterpolationWithSprintf.rule)         
             config.RedundantNewKeyword |> Option.bind (constructRuleIfEnabled RedundantNewKeyword.rule)
             config.FavourReRaise |> Option.bind (constructRuleIfEnabled FavourReRaise.rule)
             config.FavourStaticEmptyFields |> Option.bind (constructRuleIfEnabled FavourStaticEmptyFields.rule)

src/FSharpLint.Core/FSharpLint.Core.fsproj

@@ -50,6 +50,7 @@
     <Compile Include="Rules\Conventions\CyclomaticComplexity.fs" />
     <Compile Include="Rules\Conventions\FavourReRaise.fs" />
     <Compile Include="Rules\Conventions\FavourConsistentThis.fs" />
+    <Compile Include="Rules\Conventions\PreferStringInterpolationWithSprintf.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/PreferStringInterpolationWithSprintf.fs

@@ -0,0 +1,88 @@
+module FSharpLint.Rules.PreferStringInterpolationWithSprintf
+
+open System
+open FSharpLint.Framework
+open FSharpLint.Framework.Suggestion
+open FSharp.Compiler.Syntax
+open FSharpLint.Framework.Ast
+open FSharpLint.Framework.Rules
+
+let mutable moduleIdentifiers = Set.empty
+let mutable letIdentifiers = Set.empty
+
+let private isStringFormat (identifiers: List<Ident>) =
+    "String" = identifiers.[0].idText && "Format" = identifiers.[1].idText
+
+let private genereateErrorMessage range =
+    { Range = range
+      Message = Resources.GetString "RulesPreferStringInterpolationWithSprintf"
+      SuggestedFix = None
+      TypeChecks = List.empty }
+    |> Array.singleton
+
+let runner args =
+    match args.AstNode with
+    | AstNode.Expression(SynExpr.App(_, _, SynExpr.LongIdent(_, LongIdentWithDots(ids, _), _, _), paren, range)) when ids.Length = 2 && isStringFormat ids ->
+        let isTopMember (text: string) =
+            moduleIdentifiers.Contains text
+        match paren with
+        | SynExpr.Paren(SynExpr.Tuple(_, [SynExpr.Const(SynConst.String(_, _, _), _); _], _, _), _, _, _) ->
+            genereateErrorMessage range
+        | SynExpr.Paren(SynExpr.Tuple(_, [SynExpr.Ident identifier; _], _, _), _, _, range)  ->
+
+            if isTopMember identifier.idText then
+                genereateErrorMessage range
+            else
+                let isNamedBinding binding =
+                    match binding with
+                    | SynBinding(_, _, _, _, _, _, _, SynPat.Named(_, ident, _, _, _), _, _, _, _) ->
+                        identifier.idText = ident.idText
+                    | _ -> false
+
+                let isVisible asts =
+                    let rec loop asts =
+                        match asts with
+                        | AstNode.Expression (SynExpr.LetOrUse (_, _, [binding], _, _)) :: rest ->
+                            isNamedBinding binding || loop rest
+                        | _ :: rest -> loop rest
+                        | [] -> false
+                    loop asts
+
+                let getTopLevelParent index =
+                    let rec loop index =
+                        let parents = List.rev (args.GetParents index)
+                        match parents with
+                        | AstNode.ModuleDeclaration (SynModuleDecl.Let _) :: rest -> rest
+                        | _ -> loop (index + 1)
+                    loop index
+
+                if letIdentifiers.Contains identifier.idText && isVisible (getTopLevelParent 2) then
+                    genereateErrorMessage range
+                else
+                    Array.empty
+        | _ -> Array.empty
+    | AstNode.Binding(SynBinding(_, _, _, _, _, _, _, SynPat.Named(_, identifier, _, _, _), _, SynExpr.Const(SynConst.String(value, _, _), _), range, _)) when value.Contains "{0}" ->
+        let parents = args.GetParents 1
+        match parents with
+        | AstNode.ModuleDeclaration(SynModuleDecl.Let _) :: _ ->
+            moduleIdentifiers <- moduleIdentifiers.Add(identifier.idText)
+        | _ -> letIdentifiers <- letIdentifiers.Add(identifier.idText)
+        Array.empty
+    | AstNode.ModuleDeclaration(SynModuleDecl.Let _) ->
+        letIdentifiers <- Set.empty
+        Array.empty
+    | AstNode.ModuleDeclaration(SynModuleDecl.NestedModule _) ->
+        moduleIdentifiers <- Set.empty
+        letIdentifiers <- Set.empty
+        Array.empty
+    | _ -> Array.empty
+
+let cleanup () =
+    moduleIdentifiers <- Set.empty
+    letIdentifiers <- Set.empty
+
+let rule =
+    { Name = "PreferStringInterpolationWithSprintf"
+      Identifier = Identifiers.PreferStringInterpolationWithSprintf
+      RuleConfig = { AstNodeRuleConfig.Runner = runner; Cleanup = cleanup } }
+    |> AstNodeRule

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 PreferStringInterpolationWithSprintf = identifier 77

src/FSharpLint.Core/Text.resx

@@ -345,4 +345,7 @@
   <data name="RulesFavourStaticEmptyFieldsForArray" xml:space="preserve">
     <value>Consider using 'Array.empty' instead.</value>
   </data>
+  <data name="RulesPreferStringInterpolationWithSprintf" xml:space="preserve">
+    <value>Use sprintf instead of String.Format.</value>
+  </data>
 </root>

src/FSharpLint.Core/fsharplint.json

@@ -277,6 +277,9 @@
     "indentation": {
         "enabled": false
     },
+    "preferStringInterpolationWithSprintf": {
+        "enabled": false
+    },
     "maxCharactersOnLine": {
         "enabled": false,
         "config": {

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

@@ -37,6 +37,7 @@
     <Compile Include="Rules\Conventions\SourceLength.fs" />
     <Compile Include="Rules\Conventions\NoPartialFunctions.fs" />
     <Compile Include="Rules\Conventions\FavourReRaise.fs" />
+    <Compile Include="Rules\Conventions\PreferStringInterpolationWithSprintf.fs" />
     <Compile Include="Rules\Conventions\FavourConsistentThis.fs" />
     <Compile Include="Rules\Conventions\AvoidTooShortNames.fs" />
     <Compile Include="Rules\Conventions\Naming\NamingHelpers.fs" />

tests/FSharpLint.Core.Tests/Rules/Conventions/PreferStringInterpolationWithSprintf.fs

@@ -0,0 +1,80 @@
+module FSharpLint.Core.Tests.Rules.Conventions.PreferStringInterpolationWithSprintf
+
+open NUnit.Framework
+open FSharpLint.Rules
+
+[<TestFixture>]
+type TestConventionsPreferStringInterpolationWithSprintf() =
+    inherit TestAstNodeRuleBase.TestAstNodeRuleBase(PreferStringInterpolationWithSprintf.rule)
+
+    [<Test>]
+    member this.StringInterpolationWithSprintfShouldNotProduceError() =
+        this.Parse """
+let someString = sprintf "Hello %s" world"""
+
+        Assert.IsTrue this.NoErrorsExist
+
+    [<Test>]
+    member this.StringInterpolationWithStringFormatShouldProduceError() =
+        this.Parse """
+let someString = String.Format("Hello {0}", world)"""
+
+        Assert.IsTrue this.ErrorsExist
+
+
+    [<Test>]
+    member this.StringInterpolationWithStringFormatAndExternalTemplateShouldNotProduceError() =
+        this.Parse """
+let someFunction someTemplate =
+    Console.WriteLine(String.Format(someTemplate, world))"""
+
+        Assert.IsTrue this.NoErrorsExist
+
+
+    [<Test>]
+    member this.StringInterpolationWithStringFormatAndLocalVariableShouldProduceError() =
+        this.Parse """
+let someTemplate = "Hello {0}"
+let someString = String.Format(someTemplate, world)"""
+
+        Assert.IsTrue this.ErrorsExist
+
+
+    [<Test>]
+    member this.StringInterpolationWithMultipleModuleWithSameVariableNameShouldNotProduceError() =
+        this.Parse """
+module Foo =
+    let someTemplate = "Hello, this is not for String.Format actually"
+module Bar =
+    let someFunction someTemplate =
+        Console.WriteLine(String.Format(someTemplate, "world"))"""
+
+        Assert.IsTrue this.NoErrorsExist
+
+
+    [<Test>]
+    member this.StringInterpolationWithSameVariableNameInInnerLetShouldNotProduceError() =
+        this.Parse """
+module Bar =
+    let exampleFunction () =
+        let someTemplate = "Hello, this is not for String.Format actually"
+        someTemplate
+    let someFunction someTemplate =
+        let returnConstInt () =
+            89
+        Console.WriteLine(String.Format(someTemplate, "world"))"""
+
+        Assert.IsTrue this.NoErrorsExist
+
+
+    [<Test>]
+    member this.StringInterpolationWithSameVariableNameWithLocalLetShouldNotProduceError() =
+        this.Parse """
+module Bar =
+    let exampleFunction someTemplate =
+        let someResults =
+            let someTemplate = "Hello, this is not for String.Format actually"
+            someTemplate
+        Console.WriteLine(String.Format(someTemplate, "world"))"""
+
+        Assert.IsTrue this.NoErrorsExist