From 93a45bbc25ddf29737107c9d8594caa132c94762 Mon Sep 17 00:00:00 2001 From: nojaf Date: Tue, 19 Sep 2023 17:03:42 +0200 Subject: [PATCH 01/13] Extract EndsWith functionality. --- src/FSharp.Analyzers/StringAnalyzers.fs | 66 ++++++++++++++++++------- 1 file changed, 47 insertions(+), 19 deletions(-) diff --git a/src/FSharp.Analyzers/StringAnalyzers.fs b/src/FSharp.Analyzers/StringAnalyzers.fs index 5df7004..2b2d2f3 100644 --- a/src/FSharp.Analyzers/StringAnalyzers.fs +++ b/src/FSharp.Analyzers/StringAnalyzers.fs @@ -1,6 +1,7 @@ namespace GR.FSharp.Analyzers open FSharp.Analyzers.SDK +open FSharp.Compiler.CodeAnalysis open FSharp.Compiler.Symbols open FSharp.Compiler.Syntax open FSharp.Compiler.Text @@ -18,9 +19,9 @@ module StringAnalyzers = List.tryLast lid.LongIdent |> Option.bind (fun ident -> if ident.idText = name then Some ident.idRange else None) - let rec (|SingleArgumentExpr|_|) = + let rec (|SingleStringArgumentExpr|_|) = function - | SynExpr.Paren (expr = SingleArgumentExpr) + | SynExpr.Paren (expr = SingleStringArgumentExpr) // "" | SynExpr.Const (constant = SynConst.String _) // a.b @@ -29,23 +30,28 @@ module StringAnalyzers = | SynExpr.Ident _ -> Some () | _ -> None - let findAllInvocations (functionName : string) (ast : ParsedInput) : range list = + let findAllInvocations + (parameterPredicate : SynExpr -> bool) + (functionName : string) + (ast : ParsedInput) + : range list + = let collector = ResizeArray () let walker = { new SyntaxCollectorBase() with override _.WalkExpr (expr : SynExpr) = match expr with - // "".EndsWith("a") + // "".FunctionName arg | SynExpr.App ( isInfix = false funcExpr = SynExpr.DotGet (longDotId = SingleNameInSynLongIdent functionName mFunctionName) - argExpr = SingleArgumentExpr) + argExpr = argExpr) - // w.EndsWith("a") + // w.FunctionName arg | SynExpr.App ( funcExpr = SynExpr.LongIdent (longDotId = SynLongIdentEndsWith functionName mFunctionName) - argExpr = SingleArgumentExpr) -> collector.Add mFunctionName + argExpr = argExpr) when parameterPredicate argExpr -> collector.Add mFunctionName | _ -> () } @@ -54,18 +60,26 @@ module StringAnalyzers = collector |> Seq.toList - [] - let endsWithAnalyzer (ctx : CliContext) : Async = + let invalidStringFunctionUseAnalyzer + functionName + code + message + severity + (sourceText : ISourceText) + (untypedTree : ParsedInput) + (checkFileResults : FSharpCheckFileResults) + (parameterPredicate : SynExpr -> bool) + = async { - let invocations = findAllInvocations "EndsWith" ctx.ParseFileResults.ParseTree + let invocations = findAllInvocations parameterPredicate functionName untypedTree return invocations |> List.choose (fun mEndsWith -> - let lineText = ctx.SourceText.GetLineString (mEndsWith.EndLine - 1) + let lineText = sourceText.GetLineString (mEndsWith.EndLine - 1) let symbolUseOpt = - ctx.CheckFileResults.GetSymbolUseAtLocation ( + checkFileResults.GetSymbolUseAtLocation ( mEndsWith.EndLine, mEndsWith.EndColumn, lineText, @@ -92,15 +106,29 @@ module StringAnalyzers = | _ -> None ) ) - |> List.map (fun mEndsWith -> + |> List.map (fun mFunctionName -> { - Type = "String.EndsWith analyzer" - Message = - "The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." - Code = EndsWithCode - Severity = Warning - Range = mEndsWith + Type = $"String.{functionName} analyzer" + Message = message + Code = code + Severity = severity + Range = mFunctionName Fixes = [] } ) } + + [] + let endsWithAnalyzer (ctx : CliContext) : Async = + invalidStringFunctionUseAnalyzer + "EndsWith" + EndsWithCode + "The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." + Warning + ctx.SourceText + ctx.ParseFileResults.ParseTree + ctx.CheckFileResults + (function + | SingleStringArgumentExpr _ -> true + | _ -> false + ) From cbe850346467ca8d8d36f03c33df43ab59710b6b Mon Sep 17 00:00:00 2001 From: nojaf Date: Wed, 20 Sep 2023 09:59:42 +0200 Subject: [PATCH 02/13] Add String.StartsWith analyzer --- src/FSharp.Analyzers/Codes.fs | 7 ++ src/FSharp.Analyzers/StringAnalyzers.fs | 82 ++++++++++++++----- src/FSharp.Analyzers/StringAnalyzers.fsi | 10 +++ .../Function call - Single Argument.fs | 2 + ...unction call - Single Argument.fs.expected | 1 + .../Constant String - Single Argument.fs | 1 + ...stant String - Single Argument.fs.expected | 1 + .../Function call - Single Argument.fs | 2 + ...unction call - Single Argument.fs.expected | 1 + .../Prefixed Value - Single Argument.fs | 4 + ...efixed Value - Single Argument.fs.expected | 1 + .../startswith/Value - Single Argument.fs | 2 + .../Value - Single Argument.fs.expected | 1 + .../negative/Boolean CultureInfo.fs | 3 + tests/data/string/startswith/negative/Char.fs | 1 + .../startswith/negative/StringComparison.fs | 1 + 16 files changed, 98 insertions(+), 22 deletions(-) create mode 100644 src/FSharp.Analyzers/Codes.fs create mode 100644 src/FSharp.Analyzers/StringAnalyzers.fsi create mode 100644 tests/data/string/endswith/Function call - Single Argument.fs create mode 100644 tests/data/string/endswith/Function call - Single Argument.fs.expected create mode 100644 tests/data/string/startswith/Constant String - Single Argument.fs create mode 100644 tests/data/string/startswith/Constant String - Single Argument.fs.expected create mode 100644 tests/data/string/startswith/Function call - Single Argument.fs create mode 100644 tests/data/string/startswith/Function call - Single Argument.fs.expected create mode 100644 tests/data/string/startswith/Prefixed Value - Single Argument.fs create mode 100644 tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected create mode 100644 tests/data/string/startswith/Value - Single Argument.fs create mode 100644 tests/data/string/startswith/Value - Single Argument.fs.expected create mode 100644 tests/data/string/startswith/negative/Boolean CultureInfo.fs create mode 100644 tests/data/string/startswith/negative/Char.fs create mode 100644 tests/data/string/startswith/negative/StringComparison.fs diff --git a/src/FSharp.Analyzers/Codes.fs b/src/FSharp.Analyzers/Codes.fs new file mode 100644 index 0000000..dc54e46 --- /dev/null +++ b/src/FSharp.Analyzers/Codes.fs @@ -0,0 +1,7 @@ +module ``G-Research``.FSharp.Analyzers.Codes + +[] +let StringEndsWith = "GRA-001" + +[] +let StringStartsWith = "GRA-002" diff --git a/src/FSharp.Analyzers/StringAnalyzers.fs b/src/FSharp.Analyzers/StringAnalyzers.fs index 2b2d2f3..81dcdba 100644 --- a/src/FSharp.Analyzers/StringAnalyzers.fs +++ b/src/FSharp.Analyzers/StringAnalyzers.fs @@ -7,9 +7,6 @@ open FSharp.Compiler.Syntax open FSharp.Compiler.Text module StringAnalyzers = - [] - let EndsWithCode = "GRA-001" - let (|SingleNameInSynLongIdent|_|) name (lid : SynLongIdent) = match lid with | SynLongIdent (id = [ ident ]) when ident.idText = name -> Some ident.idRange @@ -21,14 +18,35 @@ module StringAnalyzers = let rec (|SingleStringArgumentExpr|_|) = function - | SynExpr.Paren (expr = SingleStringArgumentExpr) - // "" - | SynExpr.Const (constant = SynConst.String _) - // a.b - | SynExpr.LongIdent _ - // a - | SynExpr.Ident _ -> Some () - | _ -> None + // Strip parentheses + | SynExpr.Paren (expr = SingleStringArgumentExpr) -> Some () + // Only allow "" + | SynExpr.Const (constant = constant) -> + match constant with + | SynConst.String _ -> Some () + | _ -> None + // Don't allow tuples and any other obvious non value expression + | SynExpr.Tuple _ + | SynExpr.Lambda _ + | SynExpr.MatchLambda _ + | SynExpr.MatchBang _ + | SynExpr.LetOrUseBang _ + | SynExpr.AnonRecd _ + | SynExpr.ArrayOrList _ + | SynExpr.ArrayOrListComputed _ + | SynExpr.Assert _ + | SynExpr.DoBang _ + | SynExpr.DotSet _ + | SynExpr.For _ + | SynExpr.ForEach _ + | SynExpr.Lazy _ + | SynExpr.Record _ + | SynExpr.Set _ + | SynExpr.While _ + | SynExpr.YieldOrReturn _ + | SynExpr.YieldOrReturnFrom _ -> None + // Allow pretty much any expression + | _ -> Some () let findAllInvocations (parameterPredicate : SynExpr -> bool) @@ -68,10 +86,12 @@ module StringAnalyzers = (sourceText : ISourceText) (untypedTree : ParsedInput) (checkFileResults : FSharpCheckFileResults) - (parameterPredicate : SynExpr -> bool) + (unTypedArgumentPredicate : SynExpr -> bool) + (typedArgumentPredicate : FSharpMemberOrFunctionOrValue -> bool) = async { - let invocations = findAllInvocations parameterPredicate functionName untypedTree + let invocations = + findAllInvocations unTypedArgumentPredicate functionName untypedTree return invocations @@ -92,14 +112,9 @@ module StringAnalyzers = | :? FSharpMemberOrFunctionOrValue as mfv -> if mfv.Assembly.SimpleName <> "netstandard" then None - elif mfv.CurriedParameterGroups.Count <> 1 then - None - elif mfv.CurriedParameterGroups.[0].Count <> 1 then + elif not (mfv.FullName = $"System.String.%s{functionName}") then None - elif - mfv.CurriedParameterGroups.[0].[0].Type.ErasedType.BasicQualifiedName - <> "System.String" - then + elif not (typedArgumentPredicate mfv) then None else Some mEndsWith @@ -122,7 +137,7 @@ module StringAnalyzers = let endsWithAnalyzer (ctx : CliContext) : Async = invalidStringFunctionUseAnalyzer "EndsWith" - EndsWithCode + Codes.StringEndsWith "The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." Warning ctx.SourceText @@ -130,5 +145,28 @@ module StringAnalyzers = ctx.CheckFileResults (function | SingleStringArgumentExpr _ -> true - | _ -> false + | _ -> false) + (fun mfv -> + mfv.CurriedParameterGroups.Count = 1 + && mfv.CurriedParameterGroups.[0].Count = 1 + && mfv.CurriedParameterGroups.[0].[0].Type.ErasedType.BasicQualifiedName = "System.String" + ) + + [] + let startsWithAnalyzer (ctx : CliContext) : Async = + invalidStringFunctionUseAnalyzer + "StartsWith" + Codes.StringStartsWith + "The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." + Warning + ctx.SourceText + ctx.ParseFileResults.ParseTree + ctx.CheckFileResults + (function + | SingleStringArgumentExpr _ -> true + | _ -> false) + (fun mfv -> + mfv.CurriedParameterGroups.Count = 1 + && mfv.CurriedParameterGroups.[0].Count = 1 + && mfv.CurriedParameterGroups.[0].[0].Type.ErasedType.BasicQualifiedName = "System.String" ) diff --git a/src/FSharp.Analyzers/StringAnalyzers.fsi b/src/FSharp.Analyzers/StringAnalyzers.fsi new file mode 100644 index 0000000..49bd96d --- /dev/null +++ b/src/FSharp.Analyzers/StringAnalyzers.fsi @@ -0,0 +1,10 @@ +namespace ``G-Research``.FSharp.Analyzers + +open FSharp.Analyzers.SDK + +module StringAnalyzers = + [] + val endsWithAnalyzer : ctx : CliContext -> Async + + [] + val startsWithAnalyzer : ctx : CliContext -> Async diff --git a/tests/data/string/endswith/Function call - Single Argument.fs b/tests/data/string/endswith/Function call - Single Argument.fs new file mode 100644 index 0000000..bd779b5 --- /dev/null +++ b/tests/data/string/endswith/Function call - Single Argument.fs @@ -0,0 +1,2 @@ +let f () = "f" +"g".EndsWith(f()) diff --git a/tests/data/string/endswith/Function call - Single Argument.fs.expected b/tests/data/string/endswith/Function call - Single Argument.fs.expected new file mode 100644 index 0000000..cb04bf0 --- /dev/null +++ b/tests/data/string/endswith/Function call - Single Argument.fs.expected @@ -0,0 +1 @@ +GRA-001 | Warning | (2,4 - 2,12) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Constant String - Single Argument.fs b/tests/data/string/startswith/Constant String - Single Argument.fs new file mode 100644 index 0000000..a9176fa --- /dev/null +++ b/tests/data/string/startswith/Constant String - Single Argument.fs @@ -0,0 +1 @@ +"foo".StartsWith("p") diff --git a/tests/data/string/startswith/Constant String - Single Argument.fs.expected b/tests/data/string/startswith/Constant String - Single Argument.fs.expected new file mode 100644 index 0000000..fb65237 --- /dev/null +++ b/tests/data/string/startswith/Constant String - Single Argument.fs.expected @@ -0,0 +1 @@ +GRA-002 | Warning | (1,6 - 1,16) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Function call - Single Argument.fs b/tests/data/string/startswith/Function call - Single Argument.fs new file mode 100644 index 0000000..3f28681 --- /dev/null +++ b/tests/data/string/startswith/Function call - Single Argument.fs @@ -0,0 +1,2 @@ +let f () = "f" +"g".StartsWith(f()) diff --git a/tests/data/string/startswith/Function call - Single Argument.fs.expected b/tests/data/string/startswith/Function call - Single Argument.fs.expected new file mode 100644 index 0000000..61112b2 --- /dev/null +++ b/tests/data/string/startswith/Function call - Single Argument.fs.expected @@ -0,0 +1 @@ +GRA-002 | Warning | (2,4 - 2,14) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Prefixed Value - Single Argument.fs b/tests/data/string/startswith/Prefixed Value - Single Argument.fs new file mode 100644 index 0000000..3ea636e --- /dev/null +++ b/tests/data/string/startswith/Prefixed Value - Single Argument.fs @@ -0,0 +1,4 @@ +module A = + let b = "b" + +A.b.StartsWith("b") diff --git a/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected b/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected new file mode 100644 index 0000000..35110bc --- /dev/null +++ b/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected @@ -0,0 +1 @@ +GRA-002 | Warning | (4,4 - 4,14) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Value - Single Argument.fs b/tests/data/string/startswith/Value - Single Argument.fs new file mode 100644 index 0000000..18940c3 --- /dev/null +++ b/tests/data/string/startswith/Value - Single Argument.fs @@ -0,0 +1,2 @@ +let a = "a" +a.StartsWith("a") diff --git a/tests/data/string/startswith/Value - Single Argument.fs.expected b/tests/data/string/startswith/Value - Single Argument.fs.expected new file mode 100644 index 0000000..2467d4b --- /dev/null +++ b/tests/data/string/startswith/Value - Single Argument.fs.expected @@ -0,0 +1 @@ +GRA-002 | Warning | (2,2 - 2,12) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/negative/Boolean CultureInfo.fs b/tests/data/string/startswith/negative/Boolean CultureInfo.fs new file mode 100644 index 0000000..a671a2e --- /dev/null +++ b/tests/data/string/startswith/negative/Boolean CultureInfo.fs @@ -0,0 +1,3 @@ +open System.Globalization + +"a".StartsWith("b", false, CultureInfo.InvariantCulture) diff --git a/tests/data/string/startswith/negative/Char.fs b/tests/data/string/startswith/negative/Char.fs new file mode 100644 index 0000000..2d1ef7d --- /dev/null +++ b/tests/data/string/startswith/negative/Char.fs @@ -0,0 +1 @@ +"a".StartsWith('b') diff --git a/tests/data/string/startswith/negative/StringComparison.fs b/tests/data/string/startswith/negative/StringComparison.fs new file mode 100644 index 0000000..a1df206 --- /dev/null +++ b/tests/data/string/startswith/negative/StringComparison.fs @@ -0,0 +1 @@ +"a".StartsWith("b", System.StringComparison.InvariantCulture) From 3de374db6e11ab3486d582f76e8794298655d52f Mon Sep 17 00:00:00 2001 From: nojaf Date: Wed, 20 Sep 2023 11:01:47 +0200 Subject: [PATCH 03/13] Use FSharp.Analyzers.SDK.Testing --- tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj index ce53dbd..8d4c8ce 100644 --- a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj +++ b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj @@ -27,7 +27,7 @@ - + From 519219d6bad67196428ec0640119488074a68050 Mon Sep 17 00:00:00 2001 From: nojaf Date: Wed, 20 Sep 2023 11:07:21 +0200 Subject: [PATCH 04/13] Restore project reference --- tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj index 8d4c8ce..c878e33 100644 --- a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj +++ b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj @@ -28,6 +28,7 @@ + From 430ebd5abb388956614a0decf5c6a2d77ca31cbc Mon Sep 17 00:00:00 2001 From: nojaf Date: Wed, 20 Sep 2023 11:29:49 +0200 Subject: [PATCH 05/13] Assert function signature. --- src/FSharp.Analyzers/StringAnalyzers.fs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/FSharp.Analyzers/StringAnalyzers.fs b/src/FSharp.Analyzers/StringAnalyzers.fs index 81dcdba..acee19b 100644 --- a/src/FSharp.Analyzers/StringAnalyzers.fs +++ b/src/FSharp.Analyzers/StringAnalyzers.fs @@ -133,6 +133,15 @@ module StringAnalyzers = ) } + let hasMatchingSignature (expectedSignature : string) (mfv : FSharpMemberOrFunctionOrValue) : bool = + let rec visit (fsharpType : FSharpType) = + if fsharpType.GenericArguments.Count = 0 then + fsharpType.ErasedType.BasicQualifiedName + else + fsharpType.GenericArguments |> Seq.map visit |> String.concat " -> " + + visit mfv.FullType = expectedSignature + [] let endsWithAnalyzer (ctx : CliContext) : Async = invalidStringFunctionUseAnalyzer @@ -146,11 +155,7 @@ module StringAnalyzers = (function | SingleStringArgumentExpr _ -> true | _ -> false) - (fun mfv -> - mfv.CurriedParameterGroups.Count = 1 - && mfv.CurriedParameterGroups.[0].Count = 1 - && mfv.CurriedParameterGroups.[0].[0].Type.ErasedType.BasicQualifiedName = "System.String" - ) + (hasMatchingSignature "System.String -> System.Boolean") [] let startsWithAnalyzer (ctx : CliContext) : Async = @@ -165,8 +170,4 @@ module StringAnalyzers = (function | SingleStringArgumentExpr _ -> true | _ -> false) - (fun mfv -> - mfv.CurriedParameterGroups.Count = 1 - && mfv.CurriedParameterGroups.[0].Count = 1 - && mfv.CurriedParameterGroups.[0].[0].Type.ErasedType.BasicQualifiedName = "System.String" - ) + (hasMatchingSignature "System.String -> System.Boolean") From 2acc503fc529a8f7bed58b576f17123d10a18675 Mon Sep 17 00:00:00 2001 From: nojaf Date: Wed, 20 Sep 2023 11:43:47 +0200 Subject: [PATCH 06/13] Add positive test for String.IndexOf. --- src/FSharp.Analyzers/Codes.fs | 3 +++ src/FSharp.Analyzers/StringAnalyzers.fs | 22 ++++++++++++++++--- src/FSharp.Analyzers/StringAnalyzers.fsi | 7 ++++-- .../Constant String - Single Argument.fs | 1 + ...stant String - Single Argument.fs.expected | 1 + .../Function call - Single Argument.fs | 2 ++ ...unction call - Single Argument.fs.expected | 1 + .../Prefixed Value - Single Argument.fs | 4 ++++ ...efixed Value - Single Argument.fs.expected | 1 + .../string/indexof/Value - Single Argument.fs | 2 ++ .../Value - Single Argument.fs.expected | 1 + 11 files changed, 40 insertions(+), 5 deletions(-) create mode 100644 tests/data/string/indexof/Constant String - Single Argument.fs create mode 100644 tests/data/string/indexof/Constant String - Single Argument.fs.expected create mode 100644 tests/data/string/indexof/Function call - Single Argument.fs create mode 100644 tests/data/string/indexof/Function call - Single Argument.fs.expected create mode 100644 tests/data/string/indexof/Prefixed Value - Single Argument.fs create mode 100644 tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected create mode 100644 tests/data/string/indexof/Value - Single Argument.fs create mode 100644 tests/data/string/indexof/Value - Single Argument.fs.expected diff --git a/src/FSharp.Analyzers/Codes.fs b/src/FSharp.Analyzers/Codes.fs index dc54e46..4623313 100644 --- a/src/FSharp.Analyzers/Codes.fs +++ b/src/FSharp.Analyzers/Codes.fs @@ -5,3 +5,6 @@ let StringEndsWith = "GRA-001" [] let StringStartsWith = "GRA-002" + +[] +let StringIndexOf = "GRA-003" diff --git a/src/FSharp.Analyzers/StringAnalyzers.fs b/src/FSharp.Analyzers/StringAnalyzers.fs index acee19b..cf673f4 100644 --- a/src/FSharp.Analyzers/StringAnalyzers.fs +++ b/src/FSharp.Analyzers/StringAnalyzers.fs @@ -140,9 +140,10 @@ module StringAnalyzers = else fsharpType.GenericArguments |> Seq.map visit |> String.concat " -> " - visit mfv.FullType = expectedSignature + let actualSignature = visit mfv.FullType + actualSignature = expectedSignature - [] + [] let endsWithAnalyzer (ctx : CliContext) : Async = invalidStringFunctionUseAnalyzer "EndsWith" @@ -157,7 +158,7 @@ module StringAnalyzers = | _ -> false) (hasMatchingSignature "System.String -> System.Boolean") - [] + [] let startsWithAnalyzer (ctx : CliContext) : Async = invalidStringFunctionUseAnalyzer "StartsWith" @@ -171,3 +172,18 @@ module StringAnalyzers = | SingleStringArgumentExpr _ -> true | _ -> false) (hasMatchingSignature "System.String -> System.Boolean") + + [] + let indexOfAnalyzer (ctx : CliContext) : Async = + invalidStringFunctionUseAnalyzer + "IndexOf" + Codes.StringIndexOf + "The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload." + Warning + ctx.SourceText + ctx.ParseFileResults.ParseTree + ctx.CheckFileResults + (function + | SingleStringArgumentExpr _ -> true + | _ -> false) + (hasMatchingSignature "System.String -> System.Int32") diff --git a/src/FSharp.Analyzers/StringAnalyzers.fsi b/src/FSharp.Analyzers/StringAnalyzers.fsi index 49bd96d..69cdf62 100644 --- a/src/FSharp.Analyzers/StringAnalyzers.fsi +++ b/src/FSharp.Analyzers/StringAnalyzers.fsi @@ -3,8 +3,11 @@ namespace ``G-Research``.FSharp.Analyzers open FSharp.Analyzers.SDK module StringAnalyzers = - [] + [] val endsWithAnalyzer : ctx : CliContext -> Async - [] + [] val startsWithAnalyzer : ctx : CliContext -> Async + + [] + val indexOfAnalyzer : ctx : CliContext -> Async diff --git a/tests/data/string/indexof/Constant String - Single Argument.fs b/tests/data/string/indexof/Constant String - Single Argument.fs new file mode 100644 index 0000000..61ab7fa --- /dev/null +++ b/tests/data/string/indexof/Constant String - Single Argument.fs @@ -0,0 +1 @@ +"foo".IndexOf("p") diff --git a/tests/data/string/indexof/Constant String - Single Argument.fs.expected b/tests/data/string/indexof/Constant String - Single Argument.fs.expected new file mode 100644 index 0000000..4a4c32c --- /dev/null +++ b/tests/data/string/indexof/Constant String - Single Argument.fs.expected @@ -0,0 +1 @@ +GRA-003 | Warning | (1,6 - 1,13) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Function call - Single Argument.fs b/tests/data/string/indexof/Function call - Single Argument.fs new file mode 100644 index 0000000..d9b6e16 --- /dev/null +++ b/tests/data/string/indexof/Function call - Single Argument.fs @@ -0,0 +1,2 @@ +let f () = "f" +"g".IndexOf(f()) diff --git a/tests/data/string/indexof/Function call - Single Argument.fs.expected b/tests/data/string/indexof/Function call - Single Argument.fs.expected new file mode 100644 index 0000000..c659fc3 --- /dev/null +++ b/tests/data/string/indexof/Function call - Single Argument.fs.expected @@ -0,0 +1 @@ +GRA-003 | Warning | (2,4 - 2,11) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Prefixed Value - Single Argument.fs b/tests/data/string/indexof/Prefixed Value - Single Argument.fs new file mode 100644 index 0000000..63de87d --- /dev/null +++ b/tests/data/string/indexof/Prefixed Value - Single Argument.fs @@ -0,0 +1,4 @@ +module A = + let b = "b" + +A.b.IndexOf("b") diff --git a/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected b/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected new file mode 100644 index 0000000..8425d45 --- /dev/null +++ b/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected @@ -0,0 +1 @@ +GRA-003 | Warning | (4,4 - 4,11) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Value - Single Argument.fs b/tests/data/string/indexof/Value - Single Argument.fs new file mode 100644 index 0000000..deb7b20 --- /dev/null +++ b/tests/data/string/indexof/Value - Single Argument.fs @@ -0,0 +1,2 @@ +let a = "a" +a.IndexOf("a") diff --git a/tests/data/string/indexof/Value - Single Argument.fs.expected b/tests/data/string/indexof/Value - Single Argument.fs.expected new file mode 100644 index 0000000..affccf7 --- /dev/null +++ b/tests/data/string/indexof/Value - Single Argument.fs.expected @@ -0,0 +1 @@ +GRA-003 | Warning | (2,2 - 2,9) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. From 865ec60fe3d81d8d1d919e0f2fd10574255b2125 Mon Sep 17 00:00:00 2001 From: nojaf Date: Wed, 4 Oct 2023 11:16:44 +0200 Subject: [PATCH 07/13] Update StringAnalyzer to latest changes. --- src/FSharp.Analyzers/FSharp.Analyzers.fsproj | 5 +- src/FSharp.Analyzers/StringAnalyzer.fs | 192 ++++++++++++++++++ src/FSharp.Analyzers/StringAnalyzer.fsi | 21 ++ src/FSharp.Analyzers/StringAnalyzers.fs | 189 ----------------- .../StringAnalyzerTests.fs | 114 +++++------ ...stant String - Single Argument.fs.expected | 2 +- ...unction call - Single Argument.fs.expected | 2 +- ...efixed Value - Single Argument.fs.expected | 2 +- .../Value - Single Argument.fs.expected | 2 +- 9 files changed, 276 insertions(+), 253 deletions(-) create mode 100644 src/FSharp.Analyzers/StringAnalyzer.fs create mode 100644 src/FSharp.Analyzers/StringAnalyzer.fsi delete mode 100644 src/FSharp.Analyzers/StringAnalyzers.fs diff --git a/src/FSharp.Analyzers/FSharp.Analyzers.fsproj b/src/FSharp.Analyzers/FSharp.Analyzers.fsproj index ae47b9c..3987cf8 100644 --- a/src/FSharp.Analyzers/FSharp.Analyzers.fsproj +++ b/src/FSharp.Analyzers/FSharp.Analyzers.fsproj @@ -1,4 +1,4 @@ - + net6.0 @@ -11,9 +11,10 @@ - + + diff --git a/src/FSharp.Analyzers/StringAnalyzer.fs b/src/FSharp.Analyzers/StringAnalyzer.fs new file mode 100644 index 0000000..eb49a30 --- /dev/null +++ b/src/FSharp.Analyzers/StringAnalyzer.fs @@ -0,0 +1,192 @@ +module GR.FSharp.Analyzers.StringAnalyzer + +open FSharp.Analyzers.SDK +open FSharp.Compiler.CodeAnalysis +open FSharp.Compiler.Symbols +open FSharp.Compiler.Syntax +open FSharp.Compiler.Text + +[] +let StringEndsWithCode = "GRA-STRING-001" + +[] +let StringStartsWithCode = "GRA-STRING-002" + +[] +let StringIndexOfCode = "GRA-STRING-003" + +let (|SingleNameInSynLongIdent|_|) name (lid : SynLongIdent) = + match lid with + | SynLongIdent (id = [ ident ]) when ident.idText = name -> Some ident.idRange + | _ -> None + +let (|SynLongIdentEndsWith|_|) name (lid : SynLongIdent) = + List.tryLast lid.LongIdent + |> Option.bind (fun ident -> if ident.idText = name then Some ident.idRange else None) + +let rec (|SingleStringArgumentExpr|_|) = + function + // Strip parentheses + | SynExpr.Paren (expr = SingleStringArgumentExpr) -> Some () + // Only allow "" + | SynExpr.Const (constant = constant) -> + match constant with + | SynConst.String _ -> Some () + | _ -> None + // Don't allow tuples and any other obvious non value expression + | SynExpr.Tuple _ + | SynExpr.Lambda _ + | SynExpr.MatchLambda _ + | SynExpr.MatchBang _ + | SynExpr.LetOrUseBang _ + | SynExpr.AnonRecd _ + | SynExpr.ArrayOrList _ + | SynExpr.ArrayOrListComputed _ + | SynExpr.Assert _ + | SynExpr.DoBang _ + | SynExpr.DotSet _ + | SynExpr.For _ + | SynExpr.ForEach _ + | SynExpr.Lazy _ + | SynExpr.Record _ + | SynExpr.Set _ + | SynExpr.While _ + | SynExpr.YieldOrReturn _ + | SynExpr.YieldOrReturnFrom _ -> None + // Allow pretty much any expression + | _ -> Some () + +let findAllInvocations (parameterPredicate : SynExpr -> bool) (functionName : string) (ast : ParsedInput) : range list = + let collector = ResizeArray () + + let walker = + { new SyntaxCollectorBase() with + override _.WalkExpr (expr : SynExpr) = + match expr with + // "".FunctionName arg + | SynExpr.App ( + isInfix = false + funcExpr = SynExpr.DotGet (longDotId = SingleNameInSynLongIdent functionName mFunctionName) + argExpr = argExpr) + + // w.FunctionName arg + | SynExpr.App ( + funcExpr = SynExpr.LongIdent (longDotId = SynLongIdentEndsWith functionName mFunctionName) + argExpr = argExpr) when parameterPredicate argExpr -> collector.Add mFunctionName + + | _ -> () + } + + walkAst walker ast + + collector |> Seq.toList + +let invalidStringFunctionUseAnalyzer + functionName + code + message + severity + (sourceText : ISourceText) + (untypedTree : ParsedInput) + (checkFileResults : FSharpCheckFileResults) + (unTypedArgumentPredicate : SynExpr -> bool) + (typedArgumentPredicate : FSharpMemberOrFunctionOrValue -> bool) + = + async { + let invocations = + findAllInvocations unTypedArgumentPredicate functionName untypedTree + + return + invocations + |> List.choose (fun mEndsWith -> + let lineText = sourceText.GetLineString (mEndsWith.EndLine - 1) + + let symbolUseOpt = + checkFileResults.GetSymbolUseAtLocation ( + mEndsWith.EndLine, + mEndsWith.EndColumn, + lineText, + [ "EndsWith" ] + ) + + symbolUseOpt + |> Option.bind (fun symbolUse -> + match symbolUse.Symbol with + | :? FSharpMemberOrFunctionOrValue as mfv -> + if mfv.Assembly.SimpleName <> "netstandard" then + None + elif not (mfv.FullName = $"System.String.%s{functionName}") then + None + elif not (typedArgumentPredicate mfv) then + None + else + Some mEndsWith + | _ -> None + ) + ) + |> List.map (fun mFunctionName -> + { + Type = $"String.{functionName} analyzer" + Message = message + Code = code + Severity = severity + Range = mFunctionName + Fixes = [] + } + ) + } + +let hasMatchingSignature (expectedSignature : string) (mfv : FSharpMemberOrFunctionOrValue) : bool = + let rec visit (fsharpType : FSharpType) = + if fsharpType.GenericArguments.Count = 0 then + fsharpType.ErasedType.BasicQualifiedName + else + fsharpType.GenericArguments |> Seq.map visit |> String.concat " -> " + + let actualSignature = visit mfv.FullType + actualSignature = expectedSignature + +[] +let endsWithAnalyzer (ctx : CliContext) : Async = + invalidStringFunctionUseAnalyzer + "EndsWith" + StringEndsWithCode + "The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." + Warning + ctx.SourceText + ctx.ParseFileResults.ParseTree + ctx.CheckFileResults + (function + | SingleStringArgumentExpr _ -> true + | _ -> false) + (hasMatchingSignature "System.String -> System.Boolean") + +[] +let startsWithAnalyzer (ctx : CliContext) : Async = + invalidStringFunctionUseAnalyzer + "StartsWith" + StringStartsWithCode + "The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." + Warning + ctx.SourceText + ctx.ParseFileResults.ParseTree + ctx.CheckFileResults + (function + | SingleStringArgumentExpr _ -> true + | _ -> false) + (hasMatchingSignature "System.String -> System.Boolean") + +[] +let indexOfAnalyzer (ctx : CliContext) : Async = + invalidStringFunctionUseAnalyzer + "IndexOf" + StringIndexOfCode + "The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload." + Warning + ctx.SourceText + ctx.ParseFileResults.ParseTree + ctx.CheckFileResults + (function + | SingleStringArgumentExpr _ -> true + | _ -> false) + (hasMatchingSignature "System.String -> System.Int32") diff --git a/src/FSharp.Analyzers/StringAnalyzer.fsi b/src/FSharp.Analyzers/StringAnalyzer.fsi new file mode 100644 index 0000000..716eb2f --- /dev/null +++ b/src/FSharp.Analyzers/StringAnalyzer.fsi @@ -0,0 +1,21 @@ +module GR.FSharp.Analyzers.StringAnalyzer + +open FSharp.Analyzers.SDK + +[] +val StringEndsWithCode : string = "GRA-STRING-001" + +[] +val StringStartsWithCode : string = "GRA-STRING-002" + +[] +val StringIndexOfCode : string = "GRA-STRING-003" + +[] +val endsWithAnalyzer : ctx : CliContext -> Async + +[] +val startsWithAnalyzer : ctx : CliContext -> Async + +[] +val indexOfAnalyzer : ctx : CliContext -> Async diff --git a/src/FSharp.Analyzers/StringAnalyzers.fs b/src/FSharp.Analyzers/StringAnalyzers.fs deleted file mode 100644 index cf673f4..0000000 --- a/src/FSharp.Analyzers/StringAnalyzers.fs +++ /dev/null @@ -1,189 +0,0 @@ -namespace GR.FSharp.Analyzers - -open FSharp.Analyzers.SDK -open FSharp.Compiler.CodeAnalysis -open FSharp.Compiler.Symbols -open FSharp.Compiler.Syntax -open FSharp.Compiler.Text - -module StringAnalyzers = - let (|SingleNameInSynLongIdent|_|) name (lid : SynLongIdent) = - match lid with - | SynLongIdent (id = [ ident ]) when ident.idText = name -> Some ident.idRange - | _ -> None - - let (|SynLongIdentEndsWith|_|) name (lid : SynLongIdent) = - List.tryLast lid.LongIdent - |> Option.bind (fun ident -> if ident.idText = name then Some ident.idRange else None) - - let rec (|SingleStringArgumentExpr|_|) = - function - // Strip parentheses - | SynExpr.Paren (expr = SingleStringArgumentExpr) -> Some () - // Only allow "" - | SynExpr.Const (constant = constant) -> - match constant with - | SynConst.String _ -> Some () - | _ -> None - // Don't allow tuples and any other obvious non value expression - | SynExpr.Tuple _ - | SynExpr.Lambda _ - | SynExpr.MatchLambda _ - | SynExpr.MatchBang _ - | SynExpr.LetOrUseBang _ - | SynExpr.AnonRecd _ - | SynExpr.ArrayOrList _ - | SynExpr.ArrayOrListComputed _ - | SynExpr.Assert _ - | SynExpr.DoBang _ - | SynExpr.DotSet _ - | SynExpr.For _ - | SynExpr.ForEach _ - | SynExpr.Lazy _ - | SynExpr.Record _ - | SynExpr.Set _ - | SynExpr.While _ - | SynExpr.YieldOrReturn _ - | SynExpr.YieldOrReturnFrom _ -> None - // Allow pretty much any expression - | _ -> Some () - - let findAllInvocations - (parameterPredicate : SynExpr -> bool) - (functionName : string) - (ast : ParsedInput) - : range list - = - let collector = ResizeArray () - - let walker = - { new SyntaxCollectorBase() with - override _.WalkExpr (expr : SynExpr) = - match expr with - // "".FunctionName arg - | SynExpr.App ( - isInfix = false - funcExpr = SynExpr.DotGet (longDotId = SingleNameInSynLongIdent functionName mFunctionName) - argExpr = argExpr) - - // w.FunctionName arg - | SynExpr.App ( - funcExpr = SynExpr.LongIdent (longDotId = SynLongIdentEndsWith functionName mFunctionName) - argExpr = argExpr) when parameterPredicate argExpr -> collector.Add mFunctionName - - | _ -> () - } - - walkAst walker ast - - collector |> Seq.toList - - let invalidStringFunctionUseAnalyzer - functionName - code - message - severity - (sourceText : ISourceText) - (untypedTree : ParsedInput) - (checkFileResults : FSharpCheckFileResults) - (unTypedArgumentPredicate : SynExpr -> bool) - (typedArgumentPredicate : FSharpMemberOrFunctionOrValue -> bool) - = - async { - let invocations = - findAllInvocations unTypedArgumentPredicate functionName untypedTree - - return - invocations - |> List.choose (fun mEndsWith -> - let lineText = sourceText.GetLineString (mEndsWith.EndLine - 1) - - let symbolUseOpt = - checkFileResults.GetSymbolUseAtLocation ( - mEndsWith.EndLine, - mEndsWith.EndColumn, - lineText, - [ "EndsWith" ] - ) - - symbolUseOpt - |> Option.bind (fun symbolUse -> - match symbolUse.Symbol with - | :? FSharpMemberOrFunctionOrValue as mfv -> - if mfv.Assembly.SimpleName <> "netstandard" then - None - elif not (mfv.FullName = $"System.String.%s{functionName}") then - None - elif not (typedArgumentPredicate mfv) then - None - else - Some mEndsWith - | _ -> None - ) - ) - |> List.map (fun mFunctionName -> - { - Type = $"String.{functionName} analyzer" - Message = message - Code = code - Severity = severity - Range = mFunctionName - Fixes = [] - } - ) - } - - let hasMatchingSignature (expectedSignature : string) (mfv : FSharpMemberOrFunctionOrValue) : bool = - let rec visit (fsharpType : FSharpType) = - if fsharpType.GenericArguments.Count = 0 then - fsharpType.ErasedType.BasicQualifiedName - else - fsharpType.GenericArguments |> Seq.map visit |> String.concat " -> " - - let actualSignature = visit mfv.FullType - actualSignature = expectedSignature - - [] - let endsWithAnalyzer (ctx : CliContext) : Async = - invalidStringFunctionUseAnalyzer - "EndsWith" - Codes.StringEndsWith - "The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." - Warning - ctx.SourceText - ctx.ParseFileResults.ParseTree - ctx.CheckFileResults - (function - | SingleStringArgumentExpr _ -> true - | _ -> false) - (hasMatchingSignature "System.String -> System.Boolean") - - [] - let startsWithAnalyzer (ctx : CliContext) : Async = - invalidStringFunctionUseAnalyzer - "StartsWith" - Codes.StringStartsWith - "The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." - Warning - ctx.SourceText - ctx.ParseFileResults.ParseTree - ctx.CheckFileResults - (function - | SingleStringArgumentExpr _ -> true - | _ -> false) - (hasMatchingSignature "System.String -> System.Boolean") - - [] - let indexOfAnalyzer (ctx : CliContext) : Async = - invalidStringFunctionUseAnalyzer - "IndexOf" - Codes.StringIndexOf - "The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload." - Warning - ctx.SourceText - ctx.ParseFileResults.ParseTree - ctx.CheckFileResults - (function - | SingleStringArgumentExpr _ -> true - | _ -> false) - (hasMatchingSignature "System.String -> System.Int32") diff --git a/tests/FSharp.Analyzers.Tests/StringAnalyzerTests.fs b/tests/FSharp.Analyzers.Tests/StringAnalyzerTests.fs index 4618d07..ec5bb54 100644 --- a/tests/FSharp.Analyzers.Tests/StringAnalyzerTests.fs +++ b/tests/FSharp.Analyzers.Tests/StringAnalyzerTests.fs @@ -1,58 +1,56 @@ -namespace GR.FSharp.Analyzers.Tests - -module StringAnalyzerTests = - - open System.Collections - open System.IO - open FSharp.Compiler.CodeAnalysis - open NUnit.Framework - 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 "net6.0" [] - projectOptions <- options - } - - type TestCases() = - - interface IEnumerable with - member _.GetEnumerator () : IEnumerator = - constructTestCaseEnumerator [| "string" ; "endswith" |] - - [)>] - let EndsWithTests (fileName : string) = - task { - let fileName = Path.Combine (dataFolder, fileName) - - let! messages = - File.ReadAllText fileName - |> getContext projectOptions - |> StringAnalyzers.endsWithAnalyzer - - do! assertExpected fileName messages - } - - type NegativeTestCases() = - - interface IEnumerable with - member _.GetEnumerator () : IEnumerator = - constructTestCaseEnumerator [| "string" ; "endswith" ; "negative" |] - - [)>] - let NegativeEndsWithTests (fileName : string) = - task { - let fileName = Path.Combine (dataFolder, fileName) - - let! messages = - File.ReadAllText fileName - |> getContext projectOptions - |> StringAnalyzers.endsWithAnalyzer - - Assert.IsEmpty messages - } +module GR.FSharp.Analyzers.Tests.StringAnalyzerTests + +open System.Collections +open System.IO +open FSharp.Compiler.CodeAnalysis +open NUnit.Framework +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" [] + projectOptions <- options + } + +type TestCases() = + + interface IEnumerable with + member _.GetEnumerator () : IEnumerator = + constructTestCaseEnumerator [| "string" ; "endswith" |] + +[)>] +let EndsWithTests (fileName : string) = + task { + let fileName = Path.Combine (dataFolder, fileName) + + let! messages = + File.ReadAllText fileName + |> getContext projectOptions + |> StringAnalyzer.endsWithAnalyzer + + do! assertExpected fileName messages + } + +type NegativeTestCases() = + + interface IEnumerable with + member _.GetEnumerator () : IEnumerator = + constructTestCaseEnumerator [| "string" ; "endswith" ; "negative" |] + +[)>] +let NegativeEndsWithTests (fileName : string) = + task { + let fileName = Path.Combine (dataFolder, fileName) + + let! messages = + File.ReadAllText fileName + |> getContext projectOptions + |> StringAnalyzer.endsWithAnalyzer + + Assert.IsEmpty messages + } diff --git a/tests/data/string/endswith/Constant String - Single Argument.fs.expected b/tests/data/string/endswith/Constant String - Single Argument.fs.expected index 248da2c..1ddcc6a 100644 --- a/tests/data/string/endswith/Constant String - Single Argument.fs.expected +++ b/tests/data/string/endswith/Constant String - Single Argument.fs.expected @@ -1 +1 @@ -GRA-001 | Warning | (1,6 - 1,14) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-001 | Warning | (1,6 - 1,14) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/endswith/Function call - Single Argument.fs.expected b/tests/data/string/endswith/Function call - Single Argument.fs.expected index cb04bf0..46c25dc 100644 --- a/tests/data/string/endswith/Function call - Single Argument.fs.expected +++ b/tests/data/string/endswith/Function call - Single Argument.fs.expected @@ -1 +1 @@ -GRA-001 | Warning | (2,4 - 2,12) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-001 | Warning | (2,4 - 2,12) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/endswith/Prefixed Value - Single Argument.fs.expected b/tests/data/string/endswith/Prefixed Value - Single Argument.fs.expected index 9fe373d..df4c826 100644 --- a/tests/data/string/endswith/Prefixed Value - Single Argument.fs.expected +++ b/tests/data/string/endswith/Prefixed Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-001 | Warning | (4,4 - 4,12) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-001 | Warning | (4,4 - 4,12) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/endswith/Value - Single Argument.fs.expected b/tests/data/string/endswith/Value - Single Argument.fs.expected index bf94a73..dd1ef47 100644 --- a/tests/data/string/endswith/Value - Single Argument.fs.expected +++ b/tests/data/string/endswith/Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-001 | Warning | (2,2 - 2,10) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-001 | Warning | (2,2 - 2,10) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. From 5edc7a435066618edb24254bf3c38d0a5ffe9c1d Mon Sep 17 00:00:00 2001 From: nojaf Date: Wed, 4 Oct 2023 11:42:23 +0200 Subject: [PATCH 08/13] Correctly run other string tests. --- .../StringAnalyzerTests.fs | 42 ++++++++++++++----- tests/FSharp.Analyzers.Tests/Testing.fs | 13 ++++-- ...stant String - Single Argument.fs.expected | 2 +- ...unction call - Single Argument.fs.expected | 2 +- ...efixed Value - Single Argument.fs.expected | 2 +- .../Value - Single Argument.fs.expected | 2 +- ...stant String - Single Argument.fs.expected | 2 +- ...unction call - Single Argument.fs.expected | 2 +- ...efixed Value - Single Argument.fs.expected | 2 +- .../Value - Single Argument.fs.expected | 2 +- 10 files changed, 48 insertions(+), 23 deletions(-) diff --git a/tests/FSharp.Analyzers.Tests/StringAnalyzerTests.fs b/tests/FSharp.Analyzers.Tests/StringAnalyzerTests.fs index ec5bb54..517440a 100644 --- a/tests/FSharp.Analyzers.Tests/StringAnalyzerTests.fs +++ b/tests/FSharp.Analyzers.Tests/StringAnalyzerTests.fs @@ -21,36 +21,56 @@ type TestCases() = interface IEnumerable with member _.GetEnumerator () : IEnumerator = - constructTestCaseEnumerator [| "string" ; "endswith" |] + [| "endswith" ; "indexof" ; "startswith" |] + |> Seq.collect (fun subFolder -> + let folder = Path.Combine (dataFolder, "string", subFolder) + Directory.EnumerateFiles (folder, "*.fs") + ) + |> constructTestCaseEnumeratorAux + +let findStringAnalyzerFor (fileName : string) = + fileName.Split Path.DirectorySeparatorChar + |> Array.skip 1 + |> Array.head + |> function + | "endswith" -> StringAnalyzer.endsWithAnalyzer + | "startswith" -> StringAnalyzer.startsWithAnalyzer + | "indexof" -> StringAnalyzer.indexOfAnalyzer + | unknown -> failwithf $"Unknown subfolder \"%s{unknown}\", please configure analyzer" [)>] -let EndsWithTests (fileName : string) = +let StringTests (fileName : string) = task { - let fileName = Path.Combine (dataFolder, fileName) + let fullPath = Path.Combine (dataFolder, fileName) let! messages = - File.ReadAllText fileName + File.ReadAllText fullPath |> getContext projectOptions - |> StringAnalyzer.endsWithAnalyzer + |> findStringAnalyzerFor fileName - do! assertExpected fileName messages + do! assertExpected fullPath messages } type NegativeTestCases() = interface IEnumerable with member _.GetEnumerator () : IEnumerator = - constructTestCaseEnumerator [| "string" ; "endswith" ; "negative" |] + [| "endswith" ; "indexof" ; "startswith" |] + |> Seq.collect (fun subFolder -> + let folder = Path.Combine (dataFolder, "string", subFolder, "negative") + Directory.EnumerateFiles (folder, "*.fs") + ) + |> constructTestCaseEnumeratorAux [)>] -let NegativeEndsWithTests (fileName : string) = +let NegativeStringTests (fileName : string) = task { - let fileName = Path.Combine (dataFolder, fileName) + let fullPath = Path.Combine (dataFolder, fileName) let! messages = - File.ReadAllText fileName + File.ReadAllText fullPath |> getContext projectOptions - |> StringAnalyzer.endsWithAnalyzer + |> findStringAnalyzerFor fileName Assert.IsEmpty messages } diff --git a/tests/FSharp.Analyzers.Tests/Testing.fs b/tests/FSharp.Analyzers.Tests/Testing.fs index 036227a..e469f18 100644 --- a/tests/FSharp.Analyzers.Tests/Testing.fs +++ b/tests/FSharp.Analyzers.Tests/Testing.fs @@ -2,6 +2,7 @@ open System open System.IO +open System.Collections open System.Threading.Tasks open NUnit.Framework open FSharp.Analyzers.SDK @@ -46,12 +47,16 @@ let assertExpected sourceFile messages = let dataFolder = Path.Combine (__SOURCE_DIRECTORY__, "..", "data") -let constructTestCaseEnumerator (subDataPath : string array) = - let testsDirectory = Path.Combine (dataFolder, Path.Combine subDataPath) - - Directory.EnumerateFiles (testsDirectory, "*.fs") +let constructTestCaseEnumeratorAux (files : string seq) : IEnumerator = + files |> Seq.map (fun f -> let fileName = Path.GetRelativePath (dataFolder, f) [| fileName :> obj |] ) |> fun s -> s.GetEnumerator () + +let constructTestCaseEnumerator (subDataPath : string array) = + let testsDirectory = Path.Combine (dataFolder, Path.Combine subDataPath) + + Directory.EnumerateFiles (testsDirectory, "*.fs") + |> constructTestCaseEnumeratorAux diff --git a/tests/data/string/indexof/Constant String - Single Argument.fs.expected b/tests/data/string/indexof/Constant String - Single Argument.fs.expected index 4a4c32c..f26e000 100644 --- a/tests/data/string/indexof/Constant String - Single Argument.fs.expected +++ b/tests/data/string/indexof/Constant String - Single Argument.fs.expected @@ -1 +1 @@ -GRA-003 | Warning | (1,6 - 1,13) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-003 | Warning | (1,6 - 1,13) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Function call - Single Argument.fs.expected b/tests/data/string/indexof/Function call - Single Argument.fs.expected index c659fc3..1b40e62 100644 --- a/tests/data/string/indexof/Function call - Single Argument.fs.expected +++ b/tests/data/string/indexof/Function call - Single Argument.fs.expected @@ -1 +1 @@ -GRA-003 | Warning | (2,4 - 2,11) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-003 | Warning | (2,4 - 2,11) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected b/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected index 8425d45..6f2a715 100644 --- a/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected +++ b/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-003 | Warning | (4,4 - 4,11) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-003 | Warning | (4,4 - 4,11) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Value - Single Argument.fs.expected b/tests/data/string/indexof/Value - Single Argument.fs.expected index affccf7..80ec06f 100644 --- a/tests/data/string/indexof/Value - Single Argument.fs.expected +++ b/tests/data/string/indexof/Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-003 | Warning | (2,2 - 2,9) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-003 | Warning | (2,2 - 2,9) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Constant String - Single Argument.fs.expected b/tests/data/string/startswith/Constant String - Single Argument.fs.expected index fb65237..958651d 100644 --- a/tests/data/string/startswith/Constant String - Single Argument.fs.expected +++ b/tests/data/string/startswith/Constant String - Single Argument.fs.expected @@ -1 +1 @@ -GRA-002 | Warning | (1,6 - 1,16) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-002 | Warning | (1,6 - 1,16) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Function call - Single Argument.fs.expected b/tests/data/string/startswith/Function call - Single Argument.fs.expected index 61112b2..0da28f5 100644 --- a/tests/data/string/startswith/Function call - Single Argument.fs.expected +++ b/tests/data/string/startswith/Function call - Single Argument.fs.expected @@ -1 +1 @@ -GRA-002 | Warning | (2,4 - 2,14) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-002 | Warning | (2,4 - 2,14) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected b/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected index 35110bc..5d1b402 100644 --- a/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected +++ b/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-002 | Warning | (4,4 - 4,14) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-002 | Warning | (4,4 - 4,14) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Value - Single Argument.fs.expected b/tests/data/string/startswith/Value - Single Argument.fs.expected index 2467d4b..85df706 100644 --- a/tests/data/string/startswith/Value - Single Argument.fs.expected +++ b/tests/data/string/startswith/Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-002 | Warning | (2,2 - 2,12) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-002 | Warning | (2,2 - 2,12) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. From 1871bffad1e54403c6c9d82a6ce584b3301744be Mon Sep 17 00:00:00 2001 From: nojaf Date: Wed, 4 Oct 2023 11:54:42 +0200 Subject: [PATCH 09/13] Add negative tests for String.IndexOf --- tests/data/string/indexof/negative/Char Int32 Int32.fs | 2 ++ tests/data/string/indexof/negative/Char Int32.fs | 2 ++ tests/data/string/indexof/negative/Char StringComparison.fs | 2 ++ tests/data/string/indexof/negative/Char.fs | 2 ++ .../indexof/negative/String Int32 Int32 StringComparison.fs | 2 ++ .../string/indexof/negative/String Int32 StringComparison.fs | 2 ++ tests/data/string/indexof/negative/String StringComparison.fs | 2 ++ 7 files changed, 14 insertions(+) create mode 100644 tests/data/string/indexof/negative/Char Int32 Int32.fs create mode 100644 tests/data/string/indexof/negative/Char Int32.fs create mode 100644 tests/data/string/indexof/negative/Char StringComparison.fs create mode 100644 tests/data/string/indexof/negative/Char.fs create mode 100644 tests/data/string/indexof/negative/String Int32 Int32 StringComparison.fs create mode 100644 tests/data/string/indexof/negative/String Int32 StringComparison.fs create mode 100644 tests/data/string/indexof/negative/String StringComparison.fs diff --git a/tests/data/string/indexof/negative/Char Int32 Int32.fs b/tests/data/string/indexof/negative/Char Int32 Int32.fs new file mode 100644 index 0000000..1286125 --- /dev/null +++ b/tests/data/string/indexof/negative/Char Int32 Int32.fs @@ -0,0 +1,2 @@ +open System +"foo".IndexOf('f',0,1) diff --git a/tests/data/string/indexof/negative/Char Int32.fs b/tests/data/string/indexof/negative/Char Int32.fs new file mode 100644 index 0000000..ed7b119 --- /dev/null +++ b/tests/data/string/indexof/negative/Char Int32.fs @@ -0,0 +1,2 @@ +open System +"foo".IndexOf('f',0) diff --git a/tests/data/string/indexof/negative/Char StringComparison.fs b/tests/data/string/indexof/negative/Char StringComparison.fs new file mode 100644 index 0000000..dcb1b05 --- /dev/null +++ b/tests/data/string/indexof/negative/Char StringComparison.fs @@ -0,0 +1,2 @@ +open System +"foo".IndexOf('f',StringComparison.InvariantCulture) diff --git a/tests/data/string/indexof/negative/Char.fs b/tests/data/string/indexof/negative/Char.fs new file mode 100644 index 0000000..c89e3d9 --- /dev/null +++ b/tests/data/string/indexof/negative/Char.fs @@ -0,0 +1,2 @@ +open System +"foo".IndexOf('f') diff --git a/tests/data/string/indexof/negative/String Int32 Int32 StringComparison.fs b/tests/data/string/indexof/negative/String Int32 Int32 StringComparison.fs new file mode 100644 index 0000000..480862e --- /dev/null +++ b/tests/data/string/indexof/negative/String Int32 Int32 StringComparison.fs @@ -0,0 +1,2 @@ +open System +"foo".IndexOf("f",0,1,StringComparison.InvariantCulture) diff --git a/tests/data/string/indexof/negative/String Int32 StringComparison.fs b/tests/data/string/indexof/negative/String Int32 StringComparison.fs new file mode 100644 index 0000000..5d89ce8 --- /dev/null +++ b/tests/data/string/indexof/negative/String Int32 StringComparison.fs @@ -0,0 +1,2 @@ +open System +"foo".IndexOf("f",0,StringComparison.InvariantCulture) diff --git a/tests/data/string/indexof/negative/String StringComparison.fs b/tests/data/string/indexof/negative/String StringComparison.fs new file mode 100644 index 0000000..b1a1f0f --- /dev/null +++ b/tests/data/string/indexof/negative/String StringComparison.fs @@ -0,0 +1,2 @@ +open System +"foo".IndexOf("f",StringComparison.InvariantCulture) From b4cafbca8ba5ed70e622ef1da4f4a8ba0225aef6 Mon Sep 17 00:00:00 2001 From: nojaf Date: Wed, 4 Oct 2023 12:20:56 +0200 Subject: [PATCH 10/13] Rename Testing to Common. --- tests/FSharp.Analyzers.Tests/{Testing.fs => Common.fs} | 0 tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj | 4 ++-- 2 files changed, 2 insertions(+), 2 deletions(-) rename tests/FSharp.Analyzers.Tests/{Testing.fs => Common.fs} (100%) diff --git a/tests/FSharp.Analyzers.Tests/Testing.fs b/tests/FSharp.Analyzers.Tests/Common.fs similarity index 100% rename from tests/FSharp.Analyzers.Tests/Testing.fs rename to tests/FSharp.Analyzers.Tests/Common.fs diff --git a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj index c878e33..45e5fc5 100644 --- a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj +++ b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj @@ -10,10 +10,10 @@ - - + + From 550e862ae8f7259e8414ae8780d66bd40f1ff0f3 Mon Sep 17 00:00:00 2001 From: nojaf Date: Thu, 5 Oct 2023 09:26:29 +0200 Subject: [PATCH 11/13] Remove old files --- src/FSharp.Analyzers/Codes.fs | 10 ---------- src/FSharp.Analyzers/StringAnalyzers.fsi | 13 ------------- 2 files changed, 23 deletions(-) delete mode 100644 src/FSharp.Analyzers/Codes.fs delete mode 100644 src/FSharp.Analyzers/StringAnalyzers.fsi diff --git a/src/FSharp.Analyzers/Codes.fs b/src/FSharp.Analyzers/Codes.fs deleted file mode 100644 index 4623313..0000000 --- a/src/FSharp.Analyzers/Codes.fs +++ /dev/null @@ -1,10 +0,0 @@ -module ``G-Research``.FSharp.Analyzers.Codes - -[] -let StringEndsWith = "GRA-001" - -[] -let StringStartsWith = "GRA-002" - -[] -let StringIndexOf = "GRA-003" diff --git a/src/FSharp.Analyzers/StringAnalyzers.fsi b/src/FSharp.Analyzers/StringAnalyzers.fsi deleted file mode 100644 index 69cdf62..0000000 --- a/src/FSharp.Analyzers/StringAnalyzers.fsi +++ /dev/null @@ -1,13 +0,0 @@ -namespace ``G-Research``.FSharp.Analyzers - -open FSharp.Analyzers.SDK - -module StringAnalyzers = - [] - val endsWithAnalyzer : ctx : CliContext -> Async - - [] - val startsWithAnalyzer : ctx : CliContext -> Async - - [] - val indexOfAnalyzer : ctx : CliContext -> Async From a7e6bc448398ab2f29ca55d23fcf18fe9cf49e8d Mon Sep 17 00:00:00 2001 From: nojaf Date: Thu, 5 Oct 2023 13:14:37 +0200 Subject: [PATCH 12/13] Update string analyzers to use the typed tree. --- src/FSharp.Analyzers/StringAnalyzer.fs | 249 +++++++----------- .../FSharp.Analyzers.Tests.fsproj | 1 - ...stant String - Single Argument.fs.expected | 2 +- ...unction call - Single Argument.fs.expected | 2 +- ...efixed Value - Single Argument.fs.expected | 2 +- .../Value - Single Argument.fs.expected | 2 +- ...stant String - Single Argument.fs.expected | 2 +- ...unction call - Single Argument.fs.expected | 2 +- ...efixed Value - Single Argument.fs.expected | 2 +- .../Value - Single Argument.fs.expected | 2 +- ...stant String - Single Argument.fs.expected | 2 +- ...unction call - Single Argument.fs.expected | 2 +- ...efixed Value - Single Argument.fs.expected | 2 +- .../Value - Single Argument.fs.expected | 2 +- 14 files changed, 103 insertions(+), 171 deletions(-) diff --git a/src/FSharp.Analyzers/StringAnalyzer.fs b/src/FSharp.Analyzers/StringAnalyzer.fs index eb49a30..7c695f4 100644 --- a/src/FSharp.Analyzers/StringAnalyzer.fs +++ b/src/FSharp.Analyzers/StringAnalyzer.fs @@ -1,10 +1,9 @@ module GR.FSharp.Analyzers.StringAnalyzer open FSharp.Analyzers.SDK -open FSharp.Compiler.CodeAnalysis open FSharp.Compiler.Symbols -open FSharp.Compiler.Syntax open FSharp.Compiler.Text +open GR.FSharp.Analyzers.TASTCollecting [] let StringEndsWithCode = "GRA-STRING-001" @@ -15,178 +14,112 @@ let StringStartsWithCode = "GRA-STRING-002" [] let StringIndexOfCode = "GRA-STRING-003" -let (|SingleNameInSynLongIdent|_|) name (lid : SynLongIdent) = - match lid with - | SynLongIdent (id = [ ident ]) when ident.idText = name -> Some ident.idRange - | _ -> None +let (|StringExpr|_|) (e : FSharpExpr) = + if e.Type.ErasedType.BasicQualifiedName = "System.String" then + Some () + else + None -let (|SynLongIdentEndsWith|_|) name (lid : SynLongIdent) = - List.tryLast lid.LongIdent - |> Option.bind (fun ident -> if ident.idText = name then Some ident.idRange else None) - -let rec (|SingleStringArgumentExpr|_|) = - function - // Strip parentheses - | SynExpr.Paren (expr = SingleStringArgumentExpr) -> Some () - // Only allow "" - | SynExpr.Const (constant = constant) -> - match constant with - | SynConst.String _ -> Some () - | _ -> None - // Don't allow tuples and any other obvious non value expression - | SynExpr.Tuple _ - | SynExpr.Lambda _ - | SynExpr.MatchLambda _ - | SynExpr.MatchBang _ - | SynExpr.LetOrUseBang _ - | SynExpr.AnonRecd _ - | SynExpr.ArrayOrList _ - | SynExpr.ArrayOrListComputed _ - | SynExpr.Assert _ - | SynExpr.DoBang _ - | SynExpr.DotSet _ - | SynExpr.For _ - | SynExpr.ForEach _ - | SynExpr.Lazy _ - | SynExpr.Record _ - | SynExpr.Set _ - | SynExpr.While _ - | SynExpr.YieldOrReturn _ - | SynExpr.YieldOrReturnFrom _ -> None - // Allow pretty much any expression - | _ -> Some () - -let findAllInvocations (parameterPredicate : SynExpr -> bool) (functionName : string) (ast : ParsedInput) : range list = - let collector = ResizeArray () - - let walker = - { new SyntaxCollectorBase() with - override _.WalkExpr (expr : SynExpr) = - match expr with - // "".FunctionName arg - | SynExpr.App ( - isInfix = false - funcExpr = SynExpr.DotGet (longDotId = SingleNameInSynLongIdent functionName mFunctionName) - argExpr = argExpr) - - // w.FunctionName arg - | SynExpr.App ( - funcExpr = SynExpr.LongIdent (longDotId = SynLongIdentEndsWith functionName mFunctionName) - argExpr = argExpr) when parameterPredicate argExpr -> collector.Add mFunctionName - - | _ -> () - } - - walkAst walker ast - - collector |> Seq.toList +let (|IntExpr|_|) (e : FSharpExpr) = + if e.Type.ErasedType.BasicQualifiedName = "System.Int32" then + Some () + else + None let invalidStringFunctionUseAnalyzer functionName code message severity - (sourceText : ISourceText) - (untypedTree : ParsedInput) - (checkFileResults : FSharpCheckFileResults) - (unTypedArgumentPredicate : SynExpr -> bool) - (typedArgumentPredicate : FSharpMemberOrFunctionOrValue -> bool) + (typedTree : FSharpImplementationFileContents) + (typedArgumentPredicate : FSharpExpr list -> bool) = + let invocations = ResizeArray () + + let handler : Handler = + Handler.CallHandler (fun (m : range) (mfv : FSharpMemberOrFunctionOrValue) (args : FSharpExpr list) -> + if + mfv.Assembly.SimpleName = "System.Runtime" + && mfv.FullName = $"System.String.{functionName}" + && typedArgumentPredicate args + then + invocations.Add m + ) + + for decl in typedTree.Declarations do + visitDeclaration handler decl + + invocations + |> Seq.map (fun mFunctionName -> + { + Type = $"String.{functionName} analyzer" + Message = message + Code = code + Severity = severity + Range = mFunctionName + Fixes = [] + } + ) + |> Seq.toList + +[] +let endsWithAnalyzer (ctx : CliContext) : Async = async { - let invocations = - findAllInvocations unTypedArgumentPredicate functionName untypedTree + match ctx.TypedTree with + | None -> return List.empty + | Some typedTree -> return - invocations - |> List.choose (fun mEndsWith -> - let lineText = sourceText.GetLineString (mEndsWith.EndLine - 1) - - let symbolUseOpt = - checkFileResults.GetSymbolUseAtLocation ( - mEndsWith.EndLine, - mEndsWith.EndColumn, - lineText, - [ "EndsWith" ] - ) - - symbolUseOpt - |> Option.bind (fun symbolUse -> - match symbolUse.Symbol with - | :? FSharpMemberOrFunctionOrValue as mfv -> - if mfv.Assembly.SimpleName <> "netstandard" then - None - elif not (mfv.FullName = $"System.String.%s{functionName}") then - None - elif not (typedArgumentPredicate mfv) then - None - else - Some mEndsWith - | _ -> None + invalidStringFunctionUseAnalyzer + "EndsWith" + StringEndsWithCode + "The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." + Warning + typedTree + (fun (args : FSharpExpr list) -> + match args with + | [ StringExpr ] -> true + | _ -> false ) - ) - |> List.map (fun mFunctionName -> - { - Type = $"String.{functionName} analyzer" - Message = message - Code = code - Severity = severity - Range = mFunctionName - Fixes = [] - } - ) } -let hasMatchingSignature (expectedSignature : string) (mfv : FSharpMemberOrFunctionOrValue) : bool = - let rec visit (fsharpType : FSharpType) = - if fsharpType.GenericArguments.Count = 0 then - fsharpType.ErasedType.BasicQualifiedName - else - fsharpType.GenericArguments |> Seq.map visit |> String.concat " -> " - - let actualSignature = visit mfv.FullType - actualSignature = expectedSignature - -[] -let endsWithAnalyzer (ctx : CliContext) : Async = - invalidStringFunctionUseAnalyzer - "EndsWith" - StringEndsWithCode - "The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." - Warning - ctx.SourceText - ctx.ParseFileResults.ParseTree - ctx.CheckFileResults - (function - | SingleStringArgumentExpr _ -> true - | _ -> false) - (hasMatchingSignature "System.String -> System.Boolean") - [] let startsWithAnalyzer (ctx : CliContext) : Async = - invalidStringFunctionUseAnalyzer - "StartsWith" - StringStartsWithCode - "The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." - Warning - ctx.SourceText - ctx.ParseFileResults.ParseTree - ctx.CheckFileResults - (function - | SingleStringArgumentExpr _ -> true - | _ -> false) - (hasMatchingSignature "System.String -> System.Boolean") + async { + match ctx.TypedTree with + | None -> return List.empty + | Some typedTree -> + return + invalidStringFunctionUseAnalyzer + "StartsWith" + StringStartsWithCode + "The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload." + Warning + typedTree + (fun (args : FSharpExpr list) -> + match args with + | [ StringExpr ] -> true + | _ -> false + ) + } [] let indexOfAnalyzer (ctx : CliContext) : Async = - invalidStringFunctionUseAnalyzer - "IndexOf" - StringIndexOfCode - "The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload." - Warning - ctx.SourceText - ctx.ParseFileResults.ParseTree - ctx.CheckFileResults - (function - | SingleStringArgumentExpr _ -> true - | _ -> false) - (hasMatchingSignature "System.String -> System.Int32") + async { + match ctx.TypedTree with + | None -> return List.empty + | Some typedTree -> + + return + invalidStringFunctionUseAnalyzer + "IndexOf" + StringIndexOfCode + "The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload." + Warning + typedTree + (fun args -> + match args with + | [ StringExpr ] -> true + | _ -> false + ) + } diff --git a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj index 45e5fc5..c2d2272 100644 --- a/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj +++ b/tests/FSharp.Analyzers.Tests/FSharp.Analyzers.Tests.fsproj @@ -27,7 +27,6 @@ - diff --git a/tests/data/string/endswith/Constant String - Single Argument.fs.expected b/tests/data/string/endswith/Constant String - Single Argument.fs.expected index 1ddcc6a..6cff563 100644 --- a/tests/data/string/endswith/Constant String - Single Argument.fs.expected +++ b/tests/data/string/endswith/Constant String - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-001 | Warning | (1,6 - 1,14) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-001 | Warning | (1,0 - 1,19) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/endswith/Function call - Single Argument.fs.expected b/tests/data/string/endswith/Function call - Single Argument.fs.expected index 46c25dc..b47af2c 100644 --- a/tests/data/string/endswith/Function call - Single Argument.fs.expected +++ b/tests/data/string/endswith/Function call - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-001 | Warning | (2,4 - 2,12) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-001 | Warning | (2,0 - 2,17) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/endswith/Prefixed Value - Single Argument.fs.expected b/tests/data/string/endswith/Prefixed Value - Single Argument.fs.expected index df4c826..f9319ac 100644 --- a/tests/data/string/endswith/Prefixed Value - Single Argument.fs.expected +++ b/tests/data/string/endswith/Prefixed Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-001 | Warning | (4,4 - 4,12) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-001 | Warning | (4,0 - 4,17) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/endswith/Value - Single Argument.fs.expected b/tests/data/string/endswith/Value - Single Argument.fs.expected index dd1ef47..1b9c66e 100644 --- a/tests/data/string/endswith/Value - Single Argument.fs.expected +++ b/tests/data/string/endswith/Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-001 | Warning | (2,2 - 2,10) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-001 | Warning | (2,0 - 2,15) | The usage of String.EndsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Constant String - Single Argument.fs.expected b/tests/data/string/indexof/Constant String - Single Argument.fs.expected index f26e000..478f1fd 100644 --- a/tests/data/string/indexof/Constant String - Single Argument.fs.expected +++ b/tests/data/string/indexof/Constant String - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-003 | Warning | (1,6 - 1,13) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-003 | Warning | (1,0 - 1,18) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Function call - Single Argument.fs.expected b/tests/data/string/indexof/Function call - Single Argument.fs.expected index 1b40e62..e191e8a 100644 --- a/tests/data/string/indexof/Function call - Single Argument.fs.expected +++ b/tests/data/string/indexof/Function call - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-003 | Warning | (2,4 - 2,11) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-003 | Warning | (2,0 - 2,16) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected b/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected index 6f2a715..ac7723d 100644 --- a/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected +++ b/tests/data/string/indexof/Prefixed Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-003 | Warning | (4,4 - 4,11) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-003 | Warning | (4,0 - 4,16) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Value - Single Argument.fs.expected b/tests/data/string/indexof/Value - Single Argument.fs.expected index 80ec06f..9830255 100644 --- a/tests/data/string/indexof/Value - Single Argument.fs.expected +++ b/tests/data/string/indexof/Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-003 | Warning | (2,2 - 2,9) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-003 | Warning | (2,0 - 2,14) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Constant String - Single Argument.fs.expected b/tests/data/string/startswith/Constant String - Single Argument.fs.expected index 958651d..5f20b9c 100644 --- a/tests/data/string/startswith/Constant String - Single Argument.fs.expected +++ b/tests/data/string/startswith/Constant String - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-002 | Warning | (1,6 - 1,16) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-002 | Warning | (1,0 - 1,21) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Function call - Single Argument.fs.expected b/tests/data/string/startswith/Function call - Single Argument.fs.expected index 0da28f5..2e5d90e 100644 --- a/tests/data/string/startswith/Function call - Single Argument.fs.expected +++ b/tests/data/string/startswith/Function call - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-002 | Warning | (2,4 - 2,14) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-002 | Warning | (2,0 - 2,19) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected b/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected index 5d1b402..f52b1f5 100644 --- a/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected +++ b/tests/data/string/startswith/Prefixed Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-002 | Warning | (4,4 - 4,14) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-002 | Warning | (4,0 - 4,19) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/startswith/Value - Single Argument.fs.expected b/tests/data/string/startswith/Value - Single Argument.fs.expected index 85df706..b9da5ce 100644 --- a/tests/data/string/startswith/Value - Single Argument.fs.expected +++ b/tests/data/string/startswith/Value - Single Argument.fs.expected @@ -1 +1 @@ -GRA-STRING-002 | Warning | (2,2 - 2,12) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. +GRA-STRING-002 | Warning | (2,0 - 2,17) | The usage of String.StartsWith with a single string argument is discouraged. Signal your intention explicitly by calling an overload. From 7fc6c751166aef95f8e993f81a7dc5803fff0490 Mon Sep 17 00:00:00 2001 From: nojaf Date: Thu, 5 Oct 2023 17:45:15 +0200 Subject: [PATCH 13/13] Add unit tests for overloads. --- src/FSharp.Analyzers/StringAnalyzer.fs | 4 +++- tests/data/string/indexof/Constant String - String Int Int.fs | 1 + .../indexof/Constant String - String Int Int.fs.expected | 1 + tests/data/string/indexof/Constant String - String Int.fs | 1 + .../string/indexof/Constant String - String Int.fs.expected | 1 + 5 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 tests/data/string/indexof/Constant String - String Int Int.fs create mode 100644 tests/data/string/indexof/Constant String - String Int Int.fs.expected create mode 100644 tests/data/string/indexof/Constant String - String Int.fs create mode 100644 tests/data/string/indexof/Constant String - String Int.fs.expected diff --git a/src/FSharp.Analyzers/StringAnalyzer.fs b/src/FSharp.Analyzers/StringAnalyzer.fs index 7c695f4..81e18dc 100644 --- a/src/FSharp.Analyzers/StringAnalyzer.fs +++ b/src/FSharp.Analyzers/StringAnalyzer.fs @@ -119,7 +119,9 @@ let indexOfAnalyzer (ctx : CliContext) : Async = typedTree (fun args -> match args with - | [ StringExpr ] -> true + | [ StringExpr ] + | [ StringExpr ; IntExpr ] + | [ StringExpr ; IntExpr ; IntExpr ] -> true | _ -> false ) } diff --git a/tests/data/string/indexof/Constant String - String Int Int.fs b/tests/data/string/indexof/Constant String - String Int Int.fs new file mode 100644 index 0000000..abaed65 --- /dev/null +++ b/tests/data/string/indexof/Constant String - String Int Int.fs @@ -0,0 +1 @@ +"foo".IndexOf("p", 0 ,1) diff --git a/tests/data/string/indexof/Constant String - String Int Int.fs.expected b/tests/data/string/indexof/Constant String - String Int Int.fs.expected new file mode 100644 index 0000000..57ee22c --- /dev/null +++ b/tests/data/string/indexof/Constant String - String Int Int.fs.expected @@ -0,0 +1 @@ +GRA-STRING-003 | Warning | (1,0 - 1,24) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload. diff --git a/tests/data/string/indexof/Constant String - String Int.fs b/tests/data/string/indexof/Constant String - String Int.fs new file mode 100644 index 0000000..7b2e608 --- /dev/null +++ b/tests/data/string/indexof/Constant String - String Int.fs @@ -0,0 +1 @@ +"foo".IndexOf("p", 0) diff --git a/tests/data/string/indexof/Constant String - String Int.fs.expected b/tests/data/string/indexof/Constant String - String Int.fs.expected new file mode 100644 index 0000000..e42f730 --- /dev/null +++ b/tests/data/string/indexof/Constant String - String Int.fs.expected @@ -0,0 +1 @@ +GRA-STRING-003 | Warning | (1,0 - 1,21) | The usage of String.IndexOf with a single string argument is discouraged. Signal your intention explicitly by calling an overload.