Fix a false negative in LoggingTemplateMissingValuesAnalyzer (#80)

* fix a false negative caused by not diving into match clauses

* add changelog entry and release 0.9.1

* Update CHANGELOG.md

Co-authored-by: Florian Verdonck <[email protected]>

* remove test file from item group

* add comment about non-tailrec function

* detect template strings constructed by functions like sprintf

* add changelog entry and release

---------

Co-authored-by: Florian Verdonck <[email protected]>

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+## 0.9.2 - 2024-02-16
+
+### Fixed
+* Fixed a false negative of LoggingTemplateMissingValuesAnalyzer. [#79](https://github.com/G-Research/fsharp-analyzers/issues/79)
+
 ## 0.9.1 - 2024-02-15
 
 ### Fixed

src/FSharp.Analyzers/LoggingTemplateMissingValuesAnalyzer.fs

@@ -11,14 +11,38 @@ open FSharp.Compiler.Text
 [<Literal>]
 let Code = "GRA-LOGTEMPLMISSVALS-001"
 
-let (|StringConst|_|) (e : FSharpExpr) =
+let rec (|StringFormat|_|) (e : FSharpExpr) =
+    match e with
+    | Call (_exprOption, _mfv, _types, _l, exprs) ->
+        match exprs with
+        | Coerce (targetType, expr) :: _ when
+            targetType.BasicQualifiedName = "Microsoft.FSharp.Core.PrintfModule+StringFormat`1"
+            ->
+            match expr with
+            | NewObject (_mfv, _types, [ StringConst s ]) -> Some s
+            | _ -> None
+        | exprs ->
+            exprs
+            |> List.tryPick (
+                function
+                | StringConst s -> Some s
+                | _ -> None
+            )
+        | _ -> None
+    | _ -> None
+
+and (|StringConst|_|) (e : FSharpExpr) =
     let name = e.Type.ErasedType.TypeDefinition.TryGetFullName ()
 
     match name, e with
     | Some "System.String", Const (o, _type) when not (isNull o) -> Some (string o)
+    | Some "System.String", Application (expr, _, _) ->
+        match expr with
+        | Let ((_mfv, StringFormat s, _debugPointAtBinding), _expr) -> Some s
+        | _ -> None
+    | _, StringFormat s -> Some s
     | _ -> None
 
-
 let analyze (typedTree : FSharpImplementationFileContents) =
     let state = ResizeArray<range * string> ()
 
@@ -51,7 +75,7 @@ let analyze (typedTree : FSharpImplementationFileContents) =
                     else
                         0
 
-                let expected =
+                let calcExpected () =
                     let logString =
                         args
                         |> List.tryPick (
@@ -80,7 +104,7 @@ let analyze (typedTree : FSharpImplementationFileContents) =
                 if
                     m.Assembly.SimpleName = assemblyName
                     && Set.contains name namesToWarnAbout
-                    && provided <> expected
+                    && provided <> calcExpected ()
                 then
                     state.Add (range, m.DisplayName)
         }

tests/FSharp.Analyzers.Tests/data/loggingtemplatemissingvalues/Warnings for function template.fs

@@ -0,0 +1,10 @@
+module M
+
+    open Microsoft.Extensions.Logging
+
+    let testlog () =
+        use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore)
+        let logger: ILogger = factory.CreateLogger("Program")
+        let timeWasteString = "time waste"
+
+        logger.LogWarning(id "Foo {Wasted}\n{Result}", timeWasteString)

tests/FSharp.Analyzers.Tests/data/loggingtemplatemissingvalues/Warnings for function template.fs.expected

@@ -0,0 +1 @@
+GRA-LOGTEMPLMISSVALS-001 | Warning | (10,8 - 10,71) | The given values in your call to ILogger.LogWarning don't match the expected templated args. | []

tests/FSharp.Analyzers.Tests/data/loggingtemplatemissingvalues/Warnings for sprintf template.fs

@@ -0,0 +1,11 @@
+module M
+
+    open Microsoft.Extensions.Logging
+
+    let testlog () =
+        use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore)
+        let logger: ILogger = factory.CreateLogger("Program")
+        let timespan = System.TimeSpan()
+        let timeWasteString = "time waste"
+
+        logger.LogWarning(sprintf "Foo %dm %ds {Wasted}\n{Result}" (int timespan.TotalMinutes) timespan.Seconds, timeWasteString)

tests/FSharp.Analyzers.Tests/data/loggingtemplatemissingvalues/Warnings for sprintf template.fs.expected

@@ -0,0 +1 @@
+GRA-LOGTEMPLMISSVALS-001 | Warning | (11,8 - 11,129) | The given values in your call to ILogger.LogWarning don't match the expected templated args. | []

tests/FSharp.Analyzers.Tests/data/loggingtemplatemissingvalues/Warnings for sprintf template2.fs

@@ -0,0 +1,10 @@
+module M
+
+    open Microsoft.Extensions.Logging
+
+    let testlog () =
+        use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore)
+        let logger: ILogger = factory.CreateLogger("Program")
+        let timeWasteString = "time waste"
+
+        logger.LogWarning(sprintf "Foo {Wasted}\n{Result}", timeWasteString)

tests/FSharp.Analyzers.Tests/data/loggingtemplatemissingvalues/Warnings for sprintf template2.fs.expected

@@ -0,0 +1 @@
+GRA-LOGTEMPLMISSVALS-001 | Warning | (10,8 - 10,76) | The given values in your call to ILogger.LogWarning don't match the expected templated args. | []

tests/FSharp.Analyzers.Tests/data/loggingtemplatemissingvalues/negative/No warnings for function template.fs

@@ -0,0 +1,12 @@
+module M
+
+    open Microsoft.Extensions.Logging
+
+    let testlog () =
+        use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore)
+        let logger: ILogger = factory.CreateLogger("Program")
+        let timespan = System.TimeSpan()
+        let timeWasteString = "time waste"
+        let result = "result"
+
+        logger.LogWarning(id "Foo {Wasted}\n{Result}", timeWasteString, result)

tests/FSharp.Analyzers.Tests/data/loggingtemplatemissingvalues/negative/No warnings for sprintf template.fs

@@ -0,0 +1,12 @@
+module M
+
+    open Microsoft.Extensions.Logging
+
+    let testlog () =
+        use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore)
+        let logger: ILogger = factory.CreateLogger("Program")
+        let timespan = System.TimeSpan()
+        let timeWasteString = "time waste"
+        let result = "result"
+
+        logger.LogWarning(sprintf "Foo %dm %ds {Wasted}\n{Result}" (int timespan.TotalMinutes) timespan.Seconds, timeWasteString, result)

tests/FSharp.Analyzers.Tests/data/loggingtemplatemissingvalues/negative/No warnings for sprintf template2.fs

@@ -0,0 +1,12 @@
+module M
+
+    open Microsoft.Extensions.Logging
+
+    let testlog () =
+        use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore)
+        let logger: ILogger = factory.CreateLogger("Program")
+        let timespan = System.TimeSpan()
+        let timeWasteString = "time waste"
+        let result = "result"
+
+        logger.LogWarning(sprintf "Foo {Wasted}\n{Result}", timeWasteString, result)