Merge pull request #3624 from nojaf/analyzers

fable-compiler · Dec 6, 2023 · bab5acc · bab5acc
2 parents ba59b1b + f550b92
commit bab5acc
Show file tree

Hide file tree

Showing 40 changed files with 441 additions and 198 deletions.
diff --git a/.config/dotnet-tools.json b/.config/dotnet-tools.json
@@ -13,6 +13,12 @@
       "commands": [
         "husky"
       ]
+    },
+    "fsharp-analyzers": {
+      "version": "0.21.0",
+      "commands": [
+        "fsharp-analyzers"
+      ]
     }
   }
 }
diff --git a/.gitignore b/.gitignore
@@ -218,3 +218,6 @@ __pycache__/
 
 # Compilation tests
 tests/**/*.actual
+
+# Analyzers
+*.sarif
diff --git a/src/Directory.Build.props b/src/Directory.Build.props
@@ -24,5 +24,13 @@
     </ItemGroup>
     <ItemGroup>
         <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All" />
+        <PackageReference Include="FSharp.Analyzers.Build" Version="0.2.0">
+            <PrivateAssets>all</PrivateAssets>
+            <IncludeAssets>build</IncludeAssets>
+        </PackageReference>
+        <PackageReference Include="G-Research.FSharp.Analyzers" Version="0.4.0">
+            <PrivateAssets>all</PrivateAssets>
+            <IncludeAssets>analyzers</IncludeAssets>
+        </PackageReference>
     </ItemGroup>
 </Project>
diff --git a/src/Directory.Build.targets b/src/Directory.Build.targets
@@ -0,0 +1,11 @@
+<Project>
+    <PropertyGroup>
+        <FSharpAnalyzersOtherFlags>--analyzers-path &quot;$(PkgG-Research_FSharp_Analyzers)/analyzers/dotnet/fs&quot;</FSharpAnalyzersOtherFlags>
+        <FSharpAnalyzersOtherFlags>$(FSharpAnalyzersOtherFlags) --analyzers-path &quot;$(PkgIonide_Analyzers)/analyzers/dotnet/fs&quot;</FSharpAnalyzersOtherFlags>
+        <CodeRoot>$([System.IO.Path]::GetDirectoryName($(DirectoryBuildTargetsPath)))</CodeRoot>
+        <SarifOutput>$(CodeRoot)/reports/</SarifOutput>
+        <FSharpAnalyzersOtherFlags>$(FSharpAnalyzersOtherFlags) --code-root &quot;$(CodeRoot)&quot;</FSharpAnalyzersOtherFlags>
+        <FSharpAnalyzersOtherFlags>$(FSharpAnalyzersOtherFlags) --report &quot;$(SarifOutput)$(MSBuildProjectName)-$(TargetFramework).sarif&quot;</FSharpAnalyzersOtherFlags>
+        <FSharpAnalyzersOtherFlags>$(FSharpAnalyzersOtherFlags) --exclude-analyzer PartialAppAnalyzer</FSharpAnalyzersOtherFlags>
+    </PropertyGroup>
+</Project>
diff --git a/src/Fable.AST/Common.fs b/src/Fable.AST/Common.fs
@@ -1,5 +1,7 @@
 namespace Fable.AST
 
+open System
+
 /// Each Position object consists of a line number (1-indexed) and a column number (0-indexed):
 type Position =
     {
@@ -24,7 +26,7 @@ type SourceLocation =
     member this.DisplayName =
         this.identifierName
         |> Option.bind (fun name ->
-            match name.IndexOf(";file:") with
+            match name.IndexOf(";file:", StringComparison.Ordinal) with
             | -1 -> Some name
             | 0 -> None
             | i -> name.Substring(0, i) |> Some
@@ -33,7 +35,7 @@ type SourceLocation =
     member this.File =
         this.identifierName
         |> Option.bind (fun name ->
-            match name.IndexOf(";file:") with
+            match name.IndexOf(";file:", StringComparison.Ordinal) with
             | -1 -> None
             | i -> name.Substring(i + ";file:".Length) |> Some
         )

diff --git a/src/Fable.Cli/CHANGELOG.md b/src/Fable.Cli/CHANGELOG.md
@@ -6,6 +6,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Added
+
+#### All
+
+* Overall performance improvements
+    * [PR 3620](https://github.com/fable-compiler/Fable/pull/3620) Removed double-dictionary lookups (by @Thorium)
+    * [PR 3624](https://github.com/fable-compiler/Fable/pull/3624) Add G-Research analyzers and fix reported issues (by @nojaf)
+
 ### Fixed
 
 #### Python

diff --git a/src/Fable.Cli/Entry.fs b/src/Fable.Cli/Entry.fs
@@ -9,17 +9,17 @@ type CliArgs(args: string list) =
         let args =
             // Assume last arg has true value in case it's a flag
             match List.tryLast args with
-            | Some key when key.StartsWith("-") -> args @ [ "true" ]
+            | Some key when key.StartsWith('-') -> args @ [ "true" ]
             | _ -> args
 
         (Map.empty, List.windowed 2 args)
         ||> List.fold (fun map pair ->
             match pair with
-            | [ key; value ] when key.StartsWith("-") ->
+            | [ key; value ] when key.StartsWith('-') ->
                 let key = key.ToLower()
 
                 let value =
-                    if value.StartsWith("-") then
+                    if value.StartsWith('-') then
                         "true"
                     else
                         value
@@ -279,11 +279,15 @@ type Runner =
                         |> Seq.toList
 
                     files
-                    |> List.filter (fun file -> file.EndsWith(".fsproj"))
+                    |> List.filter (fun file ->
+                        file.EndsWith(".fsproj", StringComparison.Ordinal)
+                    )
                     |> function
                         | [] ->
                             files
-                            |> List.filter (fun file -> file.EndsWith(".fsx"))
+                            |> List.filter (fun file ->
+                                file.EndsWith(".fsx", StringComparison.Ordinal)
+                            )
                         | candidates -> candidates
                     |> function
                         | [] ->
@@ -388,7 +392,7 @@ type Runner =
             let fileExt =
                 args.Value("-e", "--extension")
                 |> Option.map (fun e ->
-                    if e.StartsWith(".") then
+                    if e.StartsWith('.') then
                         e
                     else
                         "." + e
@@ -537,7 +541,7 @@ let clean (args: CliArgs) language rootDir =
             "No files have been deleted. If Fable output is in another directory, pass it as argument."
         )
     else
-        Log.always ("Clean completed! Files deleted: " + string fileCount)
+        Log.always ("Clean completed! Files deleted: " + string<int> fileCount)
 
 let getStatus =
     function
@@ -563,7 +567,9 @@ let main argv =
         let! argv, runProc =
             argv
             |> List.ofArray
-            |> List.splitWhile (fun a -> not (a.StartsWith("--run")))
+            |> List.splitWhile (fun a ->
+                not (a.StartsWith("--run", StringComparison.Ordinal))
+            )
             |> function
                 | argv, flag :: runArgs ->
                     match flag, runArgs with
@@ -584,7 +590,7 @@ let main argv =
             | ("help" | "--help" | "-h") :: _ -> [ "--help" ], []
             | "--version" :: _ -> [ "--version" ], []
             | argv ->
-                argv |> List.splitWhile (fun x -> x.StartsWith("-") |> not)
+                argv |> List.splitWhile (fun x -> x.StartsWith('-') |> not)
 
         let! args = parseCliArgs args
         let! language = argLanguage args

diff --git a/src/Fable.Cli/Globbing.fs b/src/Fable.Cli/Globbing.fs
@@ -1,5 +1,6 @@
 namespace Fable.Cli
 
+open System
 open System.Collections.Generic
 open System.IO
 
@@ -141,7 +142,7 @@ module Globbing =
 
             let globRoot =
                 // If we dropped "/" from the beginning of the path in the 'Split' call, put it back!
-                if normPattern.StartsWith("/") then
+                if normPattern.StartsWith('/') then
                     "/" + globRoot
                 else
                     globRoot
@@ -163,9 +164,9 @@ module Globbing =
                     // names (as one folder name could be a substring of the other)
                     let start =
                         baseDir.TrimEnd([| Path.DirectorySeparatorChar |])
-                        + string Path.DirectorySeparatorChar
+                        + string<char> Path.DirectorySeparatorChar
                     // See https://github.com/fsharp/FAKE/issues/1925
-                    if input.StartsWith start then
+                    if input.StartsWith(start, StringComparison.Ordinal) then
                         input.Substring start.Length
                     else
                         input
@@ -183,7 +184,10 @@ module Globbing =
 
             let baseItems =
                 let start, rest =
-                    if input.StartsWith "\\\\" && splits.Length >= 4 then
+                    if
+                        input.StartsWith("\\\\", StringComparison.Ordinal)
+                        && splits.Length >= 4
+                    then
                         let serverName = splits.[2]
                         let share = splits.[3]
 
@@ -198,12 +202,12 @@ module Globbing =
                     elif
                         splits.Length >= 2
                         && Path.IsPathRooted input
-                        && input.StartsWith "/"
+                        && input.StartsWith '/'
                     then
                         [ Directory("/") ], splits |> Array.toSeq
                     else
                         if Path.IsPathRooted input then
-                            if input.StartsWith "\\" then
+                            if input.StartsWith '\\' then
                                 failwithf
                                     "Please remove the leading '\\' or '/' and replace them with \
                                            '.\\' or './' if you want to use a relative path. Leading \
@@ -415,13 +419,13 @@ module Globbing =
 
                 let included =
                     this.Includes
-                    |> Seq.exists (fun fileInclude ->
+                    |> List.exists (fun fileInclude ->
                         Glob.isMatch (fullDir fileInclude) fullPath
                     )
 
                 let excluded =
                     this.Excludes
-                    |> Seq.exists (fun fileExclude ->
+                    |> List.exists (fun fileExclude ->
                         Glob.isMatch (fullDir fileExclude) fullPath
                     )
 
@@ -468,7 +472,10 @@ module Globbing =
             |> Seq.filter (fun d ->
                 directoryIncludes
                 |> Seq.exists (fun p ->
-                    d.StartsWith(p + string Path.DirectorySeparatorChar)
+                    d.StartsWith(
+                        p + string<char> Path.DirectorySeparatorChar,
+                        StringComparison.Ordinal
+                    )
                     && p <> d
                 )
                 |> not

diff --git a/src/Fable.Cli/Main.fs b/src/Fable.Cli/Main.fs
@@ -31,7 +31,8 @@ module private Util =
             }
 
     let isImplementationFile (fileName: string) =
-        fileName.EndsWith(".fs") || fileName.EndsWith(".fsx")
+        fileName.EndsWith(".fs", StringComparison.Ordinal)
+        || fileName.EndsWith(".fsx", StringComparison.Ordinal)
 
     let caseInsensitiveSet (items: string seq) : ISet<string> =
         let s = HashSet(items)
@@ -312,7 +313,7 @@ module FileWatcherUtil =
         // See https://github.com/fable-compiler/Fable/pull/2725#issuecomment-1015123642
         |> List.filter (fun file ->
             not (
-                file.EndsWith(".fsproj.fsx")
+                file.EndsWith(".fsproj.fsx", StringComparison.Ordinal)
                 // It looks like latest F# compiler puts generated files for resolution of packages
                 // in scripts in $HOME/.packagemanagement. See #3222
                 || file.Contains(".packagemanagement")
@@ -334,7 +335,8 @@ module FileWatcherUtil =
                     if
                         restDirs
                         |> List.forall (fun d ->
-                            (withTrailingSep d).StartsWith dir'
+                            (withTrailingSep d)
+                                .StartsWith(dir', StringComparison.Ordinal)
                         )
                     then
                         dir
@@ -800,8 +802,14 @@ and FableCompiler
                                 let filesToCompile =
                                     state.FilesToCompile
                                     |> Array.filter (fun file ->
-                                        file.EndsWith(".fs")
-                                        || file.EndsWith(".fsx")
+                                        file.EndsWith(
+                                            ".fs",
+                                            StringComparison.Ordinal
+                                        )
+                                        || file.EndsWith(
+                                            ".fsx",
+                                            StringComparison.Ordinal
+                                        )
                                     )
 
                                 (state, filesToCompile)
@@ -1084,7 +1092,8 @@ let private areCompiledFilesUpToDate (state: State) (filesToCompile: string[]) =
         let upToDate =
             filesToCompile
             |> Array.filter (fun file ->
-                file.EndsWith(".fs") || file.EndsWith(".fsx")
+                file.EndsWith(".fs", StringComparison.Ordinal)
+                || file.EndsWith(".fsx", StringComparison.Ordinal)
             )
             |> Array.forall (fun source ->
                 let outPath = getOutPath state.CliArgs pathResolver source
@@ -1208,7 +1217,10 @@ let private compilationCycle (state: State) (changes: ISet<string>) =
             | Some(projCracked, fableCompiler) ->
                 // For performance reasons, don't crack .fsx scripts for every change
                 let fsprojChanged =
-                    changes |> Seq.exists (fun c -> c.EndsWith(".fsproj"))
+                    changes
+                    |> Seq.exists (fun c ->
+                        c.EndsWith(".fsproj", StringComparison.Ordinal)
+                    )
 
                 if fsprojChanged then
                     let oldProjCracked = projCracked

diff --git a/src/Fable.Cli/Pipeline.fs b/src/Fable.Cli/Pipeline.fs
@@ -102,7 +102,7 @@ module Js =
         let fileExt =
             let fileExt = cliArgs.CompilerOptions.FileExtension
 
-            if fileExt.EndsWith(".ts") then
+            if fileExt.EndsWith(".ts", StringComparison.Ordinal) then
                 Path.ChangeExtension(fileExt, ".js")
             else
                 fileExt
@@ -172,7 +172,7 @@ module Js =
                         cliArgs.OutDir
                         path
 
-                if path.EndsWith(".fs") then
+                if path.EndsWith(".fs", StringComparison.Ordinal) then
                     let isInFableModules =
                         Path.Combine(targetDir, path) |> Naming.isInFableModules
 
@@ -488,7 +488,7 @@ module Php =
                         cliArgs.OutDir
                         path
 
-                if path.EndsWith(".fs") then
+                if path.EndsWith(".fs", StringComparison.Ordinal) then
                     Path.ChangeExtension(path, fileExt)
                 else
                     path
@@ -546,7 +546,7 @@ module Dart =
                         cliArgs.OutDir
                         path
 
-                if path.EndsWith(".fs") then
+                if path.EndsWith(".fs", StringComparison.Ordinal) then
                     Path.ChangeExtension(path, fileExt)
                 else
                     path
@@ -607,7 +607,7 @@ module Rust =
                         cliArgs.OutDir
                         path
 
-                if path.EndsWith(".fs") then
+                if path.EndsWith(".fs", StringComparison.Ordinal) then
                     Path.ChangeExtension(path, fileExt)
                 else
                     path