From 3b39b533af597f9712e45b5cab7d2c2862e303b6 Mon Sep 17 00:00:00 2001 From: dawe Date: Wed, 10 Jan 2024 11:29:47 +0100 Subject: [PATCH] add the LoggingTemplateMissingValuesAnalyzer --- CHANGELOG.md | 1 + .../LoggingTemplateMissingValuesAnalyzer.md | 30 +++++ src/FSharp.Analyzers/FSharp.Analyzers.fsproj | 1 + .../LoggingTemplateMissingValuesAnalyzer.fs | 121 ++++++++++++++++++ .../FSharp.Analyzers.Tests.fsproj | 1 + ...ggingTemplateMissingValuesAnalyzerTests.fs | 71 ++++++++++ .../Warnings for one missing.fs | 9 ++ .../Warnings for one missing.fs.expected | 1 + .../No warnings for escaped parens.fs | 9 ++ .../negative/No warnings for interpolated.fs | 9 ++ .../negative/No warnings for none expected.fs | 9 ++ .../negative/No warnings for none missing.fs | 9 ++ .../negative/No warnings for obj values.fs | 9 ++ 13 files changed, 280 insertions(+) create mode 100644 docs/analyzers/LoggingTemplateMissingValuesAnalyzer.md create mode 100644 src/FSharp.Analyzers/LoggingTemplateMissingValuesAnalyzer.fs create mode 100644 tests/FSharp.Analyzers.Tests/LoggingTemplateMissingValuesAnalyzerTests.fs create mode 100644 tests/data/loggingtemplatemissingvalues/Warnings for one missing.fs create mode 100644 tests/data/loggingtemplatemissingvalues/Warnings for one missing.fs.expected create mode 100644 tests/data/loggingtemplatemissingvalues/negative/No warnings for escaped parens.fs create mode 100644 tests/data/loggingtemplatemissingvalues/negative/No warnings for interpolated.fs create mode 100644 tests/data/loggingtemplatemissingvalues/negative/No warnings for none expected.fs create mode 100644 tests/data/loggingtemplatemissingvalues/negative/No warnings for none missing.fs create mode 100644 tests/data/loggingtemplatemissingvalues/negative/No warnings for obj values.fs diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c6b4f9..e619281 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * Add editor support to all analyzers. [#50](https://github.com/G-Research/fsharp-analyzers/pull/50) * Add fix to VirtualCall Analyzer. [#51](https://github.com/G-Research/fsharp-analyzers/pull/51) * Add fix to UnionCaseAnalyzer. [#51](https://github.com/G-Research/fsharp-analyzers/pull/51) +* Add new LoggingTemplateMissingValuesAnalyzer. [#53](https://github.com/G-Research/fsharp-analyzers/pull/53) ### Changed * Update FSharp.Analyzers.SDK to `0.23.0`. [#51](https://github.com/G-Research/fsharp-analyzers/pull/45) diff --git a/docs/analyzers/LoggingTemplateMissingValuesAnalyzer.md b/docs/analyzers/LoggingTemplateMissingValuesAnalyzer.md new file mode 100644 index 0000000..b3f9bee --- /dev/null +++ b/docs/analyzers/LoggingTemplateMissingValuesAnalyzer.md @@ -0,0 +1,30 @@ +--- +title: LoggingTemplateMissingValuesAnalyzer +category: analyzers +categoryindex: 1 +index: 10 +--- + +# LoggingTemplateMissingValuesAnalyzer + +## Problem + +As param arrays are loosely typed it's easy to miss an expected templated value or even give too many. + +```fsharp +do + use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore) + let logger: ILogger = factory.CreateLogger("Program") + logger.Log(LogLevel.Information, "first {one} second {two}", 23) +``` + +## Fix + +Provide the correct number of values: + +```fsharp +do + use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore) + let logger: ILogger = factory.CreateLogger("Program") + logger.Log(LogLevel.Information, "first {one} second {two}", 23, 42) +``` \ No newline at end of file diff --git a/src/FSharp.Analyzers/FSharp.Analyzers.fsproj b/src/FSharp.Analyzers/FSharp.Analyzers.fsproj index f81d7a5..5d0b962 100644 --- a/src/FSharp.Analyzers/FSharp.Analyzers.fsproj +++ b/src/FSharp.Analyzers/FSharp.Analyzers.fsproj @@ -24,6 +24,7 @@ + diff --git a/src/FSharp.Analyzers/LoggingTemplateMissingValuesAnalyzer.fs b/src/FSharp.Analyzers/LoggingTemplateMissingValuesAnalyzer.fs new file mode 100644 index 0000000..6922f26 --- /dev/null +++ b/src/FSharp.Analyzers/LoggingTemplateMissingValuesAnalyzer.fs @@ -0,0 +1,121 @@ +module GR.FSharp.Analyzers.LoggingTemplateMissingValuesAnalyzer + +open System +open System.Text.RegularExpressions +open FSharp.Analyzers.SDK +open FSharp.Analyzers.SDK.TASTCollecting +open FSharp.Compiler.Symbols +open FSharp.Compiler.Symbols.FSharpExprPatterns +open FSharp.Compiler.Text + +[] +let Code = "GRA-LOGTEMPLMISSVALS-001" + +let (|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) + | _ -> None + + +let analyze (typedTree : FSharpImplementationFileContents) = + let state = ResizeArray () + + let namesToWarnAbout = + set + [ + "Microsoft.Extensions.Logging.LoggerExtensions.Log" + "Microsoft.Extensions.Logging.LoggerExtensions.LogCritical" + "Microsoft.Extensions.Logging.LoggerExtensions.LogDebug" + "Microsoft.Extensions.Logging.LoggerExtensions.LogError" + "Microsoft.Extensions.Logging.LoggerExtensions.LogInformation" + "Microsoft.Extensions.Logging.LoggerExtensions.LogTrace" + "Microsoft.Extensions.Logging.LoggerExtensions.LogWarning" + ] + + let pattern = @"(?{+)[a-zA-Z0-9]*(?}+)" + let regex = Regex (pattern) + + let walker = + { new TypedTreeCollectorBase() with + override _.WalkCall _ (m : FSharpMemberOrFunctionOrValue) _ _ (args : FSharpExpr list) (range : range) = + let name = String.Join (".", m.DeclaringEntity.Value.FullName, m.DisplayName) + let assemblyName = "Microsoft.Extensions.Logging.Abstractions" + + let provided = + if args.Length >= 2 then + match List.tryLast args with + | Some (NewArray (_type, exprs)) -> List.length exprs + | _ -> 0 + else + 0 + + let expected = + let logString = + args + |> List.tryPick ( + function + | StringConst (s) -> Some s + | _ -> None + ) + + match logString with + | Some s -> + let matches = regex.Matches (s) + + let escapedMatches = + Seq.sumBy + (fun (matchItem : Match) -> + let opening = matchItem.Groups["opening"] + let closing = matchItem.Groups["closing"] + let isEscaped = opening.Value.Length % 2 = 0 && closing.Value.Length % 2 = 0 + if isEscaped then 1 else 0 + ) + matches + + matches.Count - escapedMatches + | None -> 0 + + if + m.Assembly.SimpleName = assemblyName + && Set.contains name namesToWarnAbout + && provided <> expected + then + state.Add range + } + + walkTast walker typedTree + + [ + for range in state do + { + Type = "LoggingTemplateMissingValuesAnalyzer" + Message = + "The given values in your call to ILogger.Log{Warning, Error, ...} don't match the expected templated args." + Code = Code + Severity = Warning + Range = range + Fixes = [] + } + ] + + +[] +let Name = "LoggingTemplateMissingValuesAnalyzer" + +[] +let ShortDescription = + "Checks if all templated args in a log message have been given values or too many values have been given" + +[] +let HelpUri = + "https://g-research.github.io/fsharp-analyzers/analyzers/LoggingTemplateMissingValuesAnalyzer.html" + +[] +let loggingTemplateMissingValuesCliAnalyzer : Analyzer = + fun (ctx : CliContext) -> async { return ctx.TypedTree |> Option.map analyze |> Option.defaultValue [] } + +[] +let loggingTemplateMissingValuesEditorAnalyzer : Analyzer = + fun (ctx : EditorContext) -> async { return ctx.TypedTree |> Option.map analyze |> Option.defaultValue [] } diff --git a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj index 7ebd439..98b6c40 100644 --- a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj +++ b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj @@ -15,6 +15,7 @@ + diff --git a/tests/FSharp.Analyzers.Tests/LoggingTemplateMissingValuesAnalyzerTests.fs b/tests/FSharp.Analyzers.Tests/LoggingTemplateMissingValuesAnalyzerTests.fs new file mode 100644 index 0000000..59fa186 --- /dev/null +++ b/tests/FSharp.Analyzers.Tests/LoggingTemplateMissingValuesAnalyzerTests.fs @@ -0,0 +1,71 @@ +namespace GR.FSharp.Analyzers.Tests + +module LoggingTemplateMissingValuesAnalyzerTests = + + open System.Collections + open System.IO + open NUnit.Framework + open FSharp.Compiler.CodeAnalysis + open FSharp.Analyzers.SDK.Testing + open GR.FSharp.Analyzers + open GR.FSharp.Analyzers.Tests.Common + + let mutable projectOptions : FSharpProjectOptions = FSharpProjectOptions.zero + + [] + let Setup () = + task { + let! options = + mkOptionsFromProject + "net7.0" + [ + { + Name = "Microsoft.Extensions.Logging" + Version = "8.0.0" + } + { + Name = "Microsoft.Extensions.Logging.Console" + Version = "8.0.0" + } + ] + + projectOptions <- options + } + + type TestCases() = + + interface IEnumerable with + member _.GetEnumerator () : IEnumerator = + constructTestCaseEnumerator [| "loggingtemplatemissingvalues" |] + + [)>] + let Tests (fileName : string) = + task { + let fileName = Path.Combine (dataFolder, fileName) + + let! messages = + File.ReadAllText fileName + |> getContext projectOptions + |> LoggingTemplateMissingValuesAnalyzer.loggingTemplateMissingValuesCliAnalyzer + + do! assertExpected fileName messages + } + + type NegativeTestCases() = + + interface IEnumerable with + member _.GetEnumerator () : IEnumerator = + constructTestCaseEnumerator [| "loggingtemplatemissingvalues" ; "negative" |] + + [)>] + let NegativeTests (fileName : string) = + task { + let fileName = Path.Combine (dataFolder, fileName) + + let! messages = + File.ReadAllText fileName + |> getContext projectOptions + |> LoggingTemplateMissingValuesAnalyzer.loggingTemplateMissingValuesCliAnalyzer + + Assert.That (messages, Is.Empty) + } diff --git a/tests/data/loggingtemplatemissingvalues/Warnings for one missing.fs b/tests/data/loggingtemplatemissingvalues/Warnings for one missing.fs new file mode 100644 index 0000000..dbf8272 --- /dev/null +++ b/tests/data/loggingtemplatemissingvalues/Warnings for one missing.fs @@ -0,0 +1,9 @@ +module M + + open Microsoft.Extensions.Logging + + let testlog () = + use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore) + let logger: ILogger = factory.CreateLogger("Program") + + logger.Log(LogLevel.Information, "xxx {one} yyy {two}", 23) diff --git a/tests/data/loggingtemplatemissingvalues/Warnings for one missing.fs.expected b/tests/data/loggingtemplatemissingvalues/Warnings for one missing.fs.expected new file mode 100644 index 0000000..7910ecc --- /dev/null +++ b/tests/data/loggingtemplatemissingvalues/Warnings for one missing.fs.expected @@ -0,0 +1 @@ +GRA-LOGTEMPLMISSVALS-001 | Warning | (9,8 - 9,67) | The given values in your call to ILogger.Log{Warning, Error, ...} don't match the expected templated args. | [] diff --git a/tests/data/loggingtemplatemissingvalues/negative/No warnings for escaped parens.fs b/tests/data/loggingtemplatemissingvalues/negative/No warnings for escaped parens.fs new file mode 100644 index 0000000..f3978c0 --- /dev/null +++ b/tests/data/loggingtemplatemissingvalues/negative/No warnings for escaped parens.fs @@ -0,0 +1,9 @@ +module M + + open Microsoft.Extensions.Logging + + let testlog () = + use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore) + let logger: ILogger = factory.CreateLogger("Program") + + logger.Log(LogLevel.Information, "xxx {{esc1}} {{{{esc2}}}} {{{{{{esc3}}}}}} yyy {two}", 2) diff --git a/tests/data/loggingtemplatemissingvalues/negative/No warnings for interpolated.fs b/tests/data/loggingtemplatemissingvalues/negative/No warnings for interpolated.fs new file mode 100644 index 0000000..41e74a8 --- /dev/null +++ b/tests/data/loggingtemplatemissingvalues/negative/No warnings for interpolated.fs @@ -0,0 +1,9 @@ +module M + + open Microsoft.Extensions.Logging + + let testlog () = + use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore) + let logger: ILogger = factory.CreateLogger("Program") + + logger.Log(LogLevel.Information, $"xxx {123}") diff --git a/tests/data/loggingtemplatemissingvalues/negative/No warnings for none expected.fs b/tests/data/loggingtemplatemissingvalues/negative/No warnings for none expected.fs new file mode 100644 index 0000000..3ec3f6f --- /dev/null +++ b/tests/data/loggingtemplatemissingvalues/negative/No warnings for none expected.fs @@ -0,0 +1,9 @@ +module M + + open Microsoft.Extensions.Logging + + let testlog () = + use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore) + let logger: ILogger = factory.CreateLogger("Program") + + logger.Log(LogLevel.Information, "xxx") diff --git a/tests/data/loggingtemplatemissingvalues/negative/No warnings for none missing.fs b/tests/data/loggingtemplatemissingvalues/negative/No warnings for none missing.fs new file mode 100644 index 0000000..97b20f0 --- /dev/null +++ b/tests/data/loggingtemplatemissingvalues/negative/No warnings for none missing.fs @@ -0,0 +1,9 @@ +module M + + open Microsoft.Extensions.Logging + + let testlog () = + use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore) + let logger: ILogger = factory.CreateLogger("Program") + + logger.Log(LogLevel.Information, "xxx {one} yyy {two}", 23, 42) diff --git a/tests/data/loggingtemplatemissingvalues/negative/No warnings for obj values.fs b/tests/data/loggingtemplatemissingvalues/negative/No warnings for obj values.fs new file mode 100644 index 0000000..62fb1a1 --- /dev/null +++ b/tests/data/loggingtemplatemissingvalues/negative/No warnings for obj values.fs @@ -0,0 +1,9 @@ +module M + + open Microsoft.Extensions.Logging + + let testlog () = + use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore) + let logger: ILogger = factory.CreateLogger("Program") + + logger.Log(LogLevel.Information, "xxx {one}", obj())