From 9bf486c7c2e6c9794ec151a8fe8d94d4c090644d Mon Sep 17 00:00:00 2001 From: webwarrior Date: Wed, 13 Dec 2023 14:10:58 +0100 Subject: [PATCH 1/3] Core.Tests: additional checks in FavourStaticEmptyFields tests Added checks for error message containing substitution that is appropriate for given data type (array or list) in FavourStaticEmptyFields tests. E.g. `Array.empty` should be suggested for replacing `[||]`. --- .../Rules/Conventions/FavourStaticEmptyFields.fs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/FSharpLint.Core.Tests/Rules/Conventions/FavourStaticEmptyFields.fs b/tests/FSharpLint.Core.Tests/Rules/Conventions/FavourStaticEmptyFields.fs index d71531029..a2cb2e6c5 100644 --- a/tests/FSharpLint.Core.Tests/Rules/Conventions/FavourStaticEmptyFields.fs +++ b/tests/FSharpLint.Core.Tests/Rules/Conventions/FavourStaticEmptyFields.fs @@ -13,30 +13,35 @@ type TestConventionsFavourStaticEmptyFields() = this.Parse "let foo = \"\"" Assert.IsTrue this.ErrorsExist + Assert.IsTrue (this.ErrorMsg.Contains "String.Empty") [] member this.FavourStaticEmptyFieldsShouldProduceError_2() = this.Parse "System.Console.WriteLine \"\"" Assert.IsTrue this.ErrorsExist + Assert.IsTrue (this.ErrorMsg.Contains "String.Empty") [] member this.FavourStaticEmptyFieldsShouldProduceError_3() = this.Parse "let aList = []" Assert.IsTrue this.ErrorsExist + Assert.IsTrue (this.ErrorMsg.Contains "List.Empty") [] member this.FavourStaticEmptyFieldsShouldProduceError_4() = this.Parse "let aList = [ ]" Assert.IsTrue this.ErrorsExist + Assert.IsTrue (this.ErrorMsg.Contains "List.Empty") [] member this.FavourStaticEmptyFieldsShouldProduceError_5() = this.Parse "System.Console.WriteLine([].Length)" Assert.IsTrue this.ErrorsExist + Assert.IsTrue (this.ErrorMsg.Contains "List.Empty") [] member this.FavourStaticEmptyFieldsShouldProduceError_6() = @@ -48,24 +53,28 @@ let foo a = "" """ Assert.IsTrue this.ErrorsExist + Assert.IsTrue (this.ErrorMsg.Contains "String.Empty") [] member this.FavourStaticEmptyFieldsShouldProduceError_7() = this.Parse "let anArray = [||]" Assert.IsTrue this.ErrorsExist + Assert.IsTrue (this.ErrorMsg.Contains "Array.empty") [] member this.FavourStaticEmptyFieldsShouldProduceError_8() = this.Parse "let anArray = [| |]" Assert.IsTrue this.ErrorsExist + Assert.IsTrue (this.ErrorMsg.Contains "Array.empty") [] member this.FavourStaticEmptyFieldsShouldProduceError_9() = this.Parse "System.Console.WriteLine([||].Length)" Assert.IsTrue this.ErrorsExist + Assert.IsTrue (this.ErrorMsg.Contains "Array.empty") [] member this.FavourStaticEmptyFieldsShouldNotProduceError_1() = From e6be2676d76387f205c716ee8cc01b213f033f8d Mon Sep 17 00:00:00 2001 From: webwarrior Date: Thu, 14 Dec 2023 10:10:15 +0100 Subject: [PATCH 2/3] FavourStaticEmptyFields: fixed bug Fixed bug in FavourStaticEmptyFields rule that caused wrong suggestion for arrays (List.Empty instead of Array.empty). Fixes https://github.com/fsprojects/FSharpLint/issues/630 --- .../Rules/Conventions/FavourStaticEmptyFields.fs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/FSharpLint.Core/Rules/Conventions/FavourStaticEmptyFields.fs b/src/FSharpLint.Core/Rules/Conventions/FavourStaticEmptyFields.fs index 73e9105ad..7adab38c6 100644 --- a/src/FSharpLint.Core/Rules/Conventions/FavourStaticEmptyFields.fs +++ b/src/FSharpLint.Core/Rules/Conventions/FavourStaticEmptyFields.fs @@ -56,8 +56,8 @@ let private runner (args: AstNodeRuleParams) = let suggestions = generateError range idText emptyLiteralType suggestions |> Array.map (fun suggestion -> { suggestion with TypeChecks = Option.toList typeCheck })) | _ -> Array.empty - | SynExpr.ArrayOrList (_, id, range) when "[]" = sprintf "%A" id || "[||]" = sprintf "%A" id -> - (range, sprintf "%A" id, None, getEmptyLiteralType (sprintf "%A" id)) + | SynExpr.ArrayOrList (isArray, id, range) when "[]" = sprintf "%A" id || "[||]" = sprintf "%A" id -> + (range, sprintf "%A" id, None, (if isArray then EmptyArrayLiteral else EmptyListLiteral)) |> Array.singleton |> Array.collect (fun (range, idText, typeCheck, emptyLiteralType) -> let suggestions = generateError range idText emptyLiteralType @@ -77,8 +77,8 @@ let private runner (args: AstNodeRuleParams) = |> Array.collect (fun (range, idText, typeCheck, emptyLiteralType) -> let suggestions = generateError range idText emptyLiteralType suggestions |> Array.map (fun suggestion -> { suggestion with TypeChecks = Option.toList typeCheck })) - | SynExpr.ArrayOrList (_, id, range) when "[]" = sprintf "%A" id || "[||]" = sprintf "%A" id -> - (range, sprintf "%A" id, None, getEmptyLiteralType (sprintf "%A" id)) + | SynExpr.ArrayOrList (isArray, id, range) when "[]" = sprintf "%A" id || "[||]" = sprintf "%A" id -> + (range, sprintf "%A" id, None, (if isArray then EmptyArrayLiteral else EmptyListLiteral)) |> Array.singleton |> Array.collect (fun (range, idText, typeCheck, emptyLiteralType) -> let suggestions = generateError range idText emptyLiteralType From 0d2172882c7b6ab565bdffad6b48a9c93361c16a Mon Sep 17 00:00:00 2001 From: webwarrior Date: Thu, 14 Dec 2023 10:13:40 +0100 Subject: [PATCH 3/3] FavourStaticEmptyFields: refactoring Simplified FavourStaticEmptyFields rule by removing unnecessary code. Fixes https://github.com/fsprojects/FSharpLint/issues/630 --- .../Conventions/FavourStaticEmptyFields.fs | 71 ++++--------------- 1 file changed, 13 insertions(+), 58 deletions(-) diff --git a/src/FSharpLint.Core/Rules/Conventions/FavourStaticEmptyFields.fs b/src/FSharpLint.Core/Rules/Conventions/FavourStaticEmptyFields.fs index 7adab38c6..5837bcc32 100644 --- a/src/FSharpLint.Core/Rules/Conventions/FavourStaticEmptyFields.fs +++ b/src/FSharpLint.Core/Rules/Conventions/FavourStaticEmptyFields.fs @@ -12,14 +12,6 @@ type private EmptyLiteralType = | EmptyListLiteral | EmptyArrayLiteral -let private getEmptyLiteralType (str: string): EmptyLiteralType = - if str.Length = 0 then - EmptyLiteralType.EmptyStringLiteral - elif str = "[]" then - EmptyLiteralType.EmptyListLiteral - else - EmptyLiteralType.EmptyArrayLiteral - let private getStaticEmptyErrorMessage (range:FSharp.Compiler.Text.Range) (emptyLiteralType: EmptyLiteralType) = let errorMessageKey = match emptyLiteralType with @@ -30,60 +22,23 @@ let private getStaticEmptyErrorMessage (range:FSharp.Compiler.Text.Range) (empt let formatError errorName = Resources.GetString errorName - errorMessageKey |> formatError |> Array.singleton + errorMessageKey |> formatError -let private generateError (range:FSharp.Compiler.Text.Range) (idText:string) (emptyLiteralType: EmptyLiteralType) = - if idText = "" || idText = "[]" then - getStaticEmptyErrorMessage range emptyLiteralType - |> Array.map (fun message -> - { Range = range - Message = message - SuggestedFix = None - TypeChecks = List.Empty }) - else - Array.empty +let private generateError (range:FSharp.Compiler.Text.Range) (emptyLiteralType: EmptyLiteralType) = + { Range = range + Message = getStaticEmptyErrorMessage range emptyLiteralType + SuggestedFix = None + TypeChecks = List.Empty } + |> Array.singleton let private runner (args: AstNodeRuleParams) = match args.AstNode with - | AstNode.Expression expr -> - match expr with - | SynExpr.App (_, _, SynExpr.Ident failwithId, expression, range) -> - match expression with - | SynExpr.Const (SynConst.String (id, _, _), _) when id = "" -> - (range, id, None, getEmptyLiteralType id) - |> Array.singleton - |> Array.collect (fun (range, idText, typeCheck, emptyLiteralType) -> - let suggestions = generateError range idText emptyLiteralType - suggestions |> Array.map (fun suggestion -> { suggestion with TypeChecks = Option.toList typeCheck })) - | _ -> Array.empty - | SynExpr.ArrayOrList (isArray, id, range) when "[]" = sprintf "%A" id || "[||]" = sprintf "%A" id -> - (range, sprintf "%A" id, None, (if isArray then EmptyArrayLiteral else EmptyListLiteral)) - |> Array.singleton - |> Array.collect (fun (range, idText, typeCheck, emptyLiteralType) -> - let suggestions = generateError range idText emptyLiteralType - suggestions |> Array.map (fun suggestion -> { suggestion with TypeChecks = Option.toList typeCheck })) - | SynExpr.Const (SynConst.String (id, _, range), _) when id = "" -> - (range, id, None, getEmptyLiteralType id) - |> Array.singleton - |> Array.collect (fun (range, idText, typeCheck, emptyLiteralType) -> - let suggestions = generateError range idText emptyLiteralType - suggestions |> Array.map (fun suggestion -> { suggestion with TypeChecks = Option.toList typeCheck })) - | _ -> Array.empty - | Binding(SynBinding(_, _, _, _, _, _, _, _, _, expression, _, _)) -> - match expression with - | SynExpr.Const (SynConst.String (id, _, range), _) when id = "" -> - (range, id, None, getEmptyLiteralType id) - |> Array.singleton - |> Array.collect (fun (range, idText, typeCheck, emptyLiteralType) -> - let suggestions = generateError range idText emptyLiteralType - suggestions |> Array.map (fun suggestion -> { suggestion with TypeChecks = Option.toList typeCheck })) - | SynExpr.ArrayOrList (isArray, id, range) when "[]" = sprintf "%A" id || "[||]" = sprintf "%A" id -> - (range, sprintf "%A" id, None, (if isArray then EmptyArrayLiteral else EmptyListLiteral)) - |> Array.singleton - |> Array.collect (fun (range, idText, typeCheck, emptyLiteralType) -> - let suggestions = generateError range idText emptyLiteralType - suggestions |> Array.map (fun suggestion -> { suggestion with TypeChecks = Option.toList typeCheck })) - | _ -> Array.empty + | AstNode.Expression(SynExpr.Const (SynConst.String ("", _, range), _)) -> + generateError range EmptyStringLiteral + | AstNode.Expression(SynExpr.ArrayOrList (isArray, [], range)) -> + let emptyLiteralType = + if isArray then EmptyArrayLiteral else EmptyListLiteral + generateError range emptyLiteralType | _ -> Array.empty