Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail the tool if any analysis fails #200

Merged
merged 2 commits into from
Jan 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/FSharp.Analyzers.Cli/FSharp.Analyzers.Cli.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

<ItemGroup>
<Compile Include="CustomLogging.fs" />
<Compile Include="Result.fs" />
<Compile Include="Program.fs" />
</ItemGroup>

Expand Down
90 changes: 53 additions & 37 deletions src/FSharp.Analyzers.Cli/Program.fs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ open Microsoft.CodeAnalysis.Sarif
open Microsoft.CodeAnalysis.Sarif.Writers
open Microsoft.Extensions.Logging
open Ionide.ProjInfo
open FSharp.Analyzers.Cli
open FSharp.Analyzers.Cli.CustomLogging

type Arguments =
Expand Down Expand Up @@ -136,6 +137,7 @@ let runProjectAux
(fsharpOptions: FSharpProjectOptions)
(excludeIncludeFiles: Choice<Glob list, Glob list>)
(mappings: SeverityMappings)
: Async<Result<AnalyzerMessage list, AnalysisFailure> list>
=
async {
let! checkProjectResults = fcs.ParseAndCheckProject(fsharpOptions)
Expand All @@ -157,26 +159,33 @@ let runProjectAux
true
| None -> false
)
|> Array.choose (fun fileName ->
|> Array.map (fun fileName ->
let fileContent = File.ReadAllText fileName
let sourceText = SourceText.ofString fileContent

Utils.typeCheckFile fcs logger fsharpOptions fileName (Utils.SourceOfSource.SourceText sourceText)
|> Option.map (Utils.createContext checkProjectResults fileName sourceText)
|> Result.map (Utils.createContext checkProjectResults fileName sourceText)
)
|> Array.map (fun ctx ->
logger.LogInformation("Running analyzers for {0}", ctx.FileName)
client.RunAnalyzers ctx
match ctx with
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could all have been done point-free but I thought it was clearer this way.

| Error e -> async.Return(Error e)
| Ok ctx ->
async {
logger.LogInformation("Running analyzers for {0}", ctx.FileName)
let! results = client.RunAnalyzers ctx
return Ok results
}
)
|> Async.Parallel

return
Some
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was never any chance that this was None, so again I tightened up the types to get rid of the option. Instead it's now applied where it's needed (namely in the big match in Program.fs).

[
for messages in messagesPerAnalyzer do
let mappedMessages = messages |> List.map (mapMessageToSeverity mappings)
yield! mappedMessages
]
messagesPerAnalyzer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Again, this could have been point-free.)

|> Seq.map (fun messages ->
match messages with
| Error e -> Error e
| Ok messages -> messages |> List.map (mapMessageToSeverity mappings) |> Ok
)
|> Seq.toList
}

let runProject
Expand Down Expand Up @@ -267,7 +276,7 @@ let printMessages (msgs: AnalyzerMessage list) =
let msgLogger = factory.CreateLogger("")

msgs
|> Seq.iter (fun analyzerMessage ->
|> List.iter (fun analyzerMessage ->
let m = analyzerMessage.Message

msgLogger.Log(
Expand All @@ -284,7 +293,7 @@ let printMessages (msgs: AnalyzerMessage list) =

()

let writeReport (results: AnalyzerMessage list option) (codeRoot: string option) (report: string) =
let writeReport (results: AnalyzerMessage list) (codeRoot: string option) (report: string) =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was never actually optional; we always statically knew that this option was Some. I tightened up the domain by representing that fact in the types.

try
let codeRoot =
match codeRoot with
Expand Down Expand Up @@ -319,7 +328,7 @@ let writeReport (results: AnalyzerMessage list option) (codeRoot: string option)

sarifLogger.AnalysisStarted()

for analyzerResult in (Option.defaultValue List.empty results) do
for analyzerResult in results do
let reportDescriptor = ReportingDescriptor()
reportDescriptor.Id <- analyzerResult.Message.Code
reportDescriptor.Name <- analyzerResult.Message.Message
Expand Down Expand Up @@ -375,20 +384,6 @@ let writeReport (results: AnalyzerMessage list option) (codeRoot: string option)
logger.LogError(ex, "Could not write sarif to {report}", report)
logger.LogInformation("{0}", ex)

let calculateExitCode (msgs: AnalyzerMessage list option) : int =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now extracted to the bottom of the file, where the logic is inlined. That's because our exit code now depends on knowledge we only have down there, and I didn't really fancy passing more args into this function.

match msgs with
| None -> -1
| Some msgs ->
let check =
msgs
|> List.exists (fun analyzerMessage ->
let message = analyzerMessage.Message

message.Severity = Severity.Error
)

if check then -2 else 0

/// If multiple MSBuild properties are given in one -p flag like -p:prop1="val1a;val1b;val1c";prop2="1;2;3";prop3=val3
/// argu will think it means prop1 has the value: "val1a;val1b;val1c";prop2="1;2;3";prop3=val3
/// so this function expands the value into multiple key-value properties
Expand Down Expand Up @@ -623,6 +618,7 @@ let main argv =
| [], Some fscArgs ->
runFscArgs client fscArgs exclInclFiles severityMapping
|> Async.RunSynchronously
|> Some
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Fallout from the "remove the spurious option type" in FSharp.Analyzers.SDK.fs)

| projects, None ->
for projPath in projects do
if not (File.Exists(projPath)) then
Expand All @@ -635,19 +631,39 @@ let main argv =
)
|> Async.Sequential
|> Async.RunSynchronously
|> Array.choose id
|> List.concat
|> Some

results |> Option.iter printMessages
report |> Option.iter (writeReport results codeRoot)
match results with
| None -> -1
| Some results ->
let results, hasError =
match Result.allOkOrError results with
| Ok results -> results, false
| Error(results, _errors) -> results, true

let results = results |> List.concat

if failedAssemblies > 0 then
logger.LogError(
"Because we failed to load some assemblies to obtain analyzers from them, exiting (failure count: {FailedAssemblyLoadCount})",
failedAssemblies
)
printMessages results

report |> Option.iter (writeReport results codeRoot)

let check =
results
|> List.exists (fun analyzerMessage ->
let message = analyzerMessage.Message

message.Severity = Severity.Error
)

if failedAssemblies > 0 then
logger.LogError(
"Because we failed to load some assemblies to obtain analyzers from them, exiting (failure count: {FailedAssemblyLoadCount})",
failedAssemblies
)

exit -3
exit -3

calculateExitCode results
if check then -2
elif hasError then -4
else 0
15 changes: 15 additions & 0 deletions src/FSharp.Analyzers.Cli/Result.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
module FSharp.Analyzers.Cli.Result

let allOkOrError<'ok, 'err> (results: Result<'ok, 'err> list) : Result<'ok list, 'ok list * 'err list> =
let oks, errs =
(([], []), results)
||> List.fold (fun (oks, errs) result ->
match result with
| Ok ok -> ok :: oks, errs
| Error err -> oks, err :: errs
)

let oks = List.rev oks
let errs = List.rev errs

if List.isEmpty errs then Ok oks else Error(oks, errs)
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ let getContextFor (opts: FSharpProjectOptions) isSignature source =
fileName
(Utils.SourceOfSource.DiscreteSource source)
with
| Some(parseFileResults, checkFileResults) ->
| Ok(parseFileResults, checkFileResults) ->
let diagErrors =
checkFileResults.Diagnostics
|> Array.filter (fun d -> d.Severity = FSharpDiagnosticSeverity.Error)
Expand All @@ -267,7 +267,7 @@ let getContextFor (opts: FSharpProjectOptions) isSignature source =

let sourceText = SourceText.ofString source
Utils.createContext checkProjectResults fileName sourceText (parseFileResults, checkFileResults)
| None -> failwith "typechecking file failed"
| Error e -> failwith $"typechecking file failed: %O{e}"

let getContext (opts: FSharpProjectOptions) source = getContextFor opts false source
let getContextForSignature (opts: FSharpProjectOptions) source = getContextFor opts true source
Expand Down
7 changes: 5 additions & 2 deletions src/FSharp.Analyzers.SDK/FSharp.Analyzers.SDK.fs
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@ type AnalyzerMessage =
HelpUri: string option
}

type AnalysisFailure = | Aborted

module Utils =
let currentFSharpAnalyzersSDKVersion =
Assembly.GetExecutingAssembly().GetName().Version
Expand Down Expand Up @@ -230,6 +232,7 @@ module Utils =
(options: FSharpProjectOptions)
(fileName: string)
(source: SourceOfSource)
: Result<FSharpParseFileResults * FSharpCheckFileResults, AnalysisFailure>
=

let sourceText =
Expand All @@ -247,5 +250,5 @@ module Utils =
match checkAnswer with
| FSharpCheckFileAnswer.Aborted ->
logger.LogError("Checking of file {0} aborted", fileName)
None
| FSharpCheckFileAnswer.Succeeded result -> Some(parseRes, result)
Error AnalysisFailure.Aborted
| FSharpCheckFileAnswer.Succeeded result -> Ok(parseRes, result)
8 changes: 7 additions & 1 deletion src/FSharp.Analyzers.SDK/FSharp.Analyzers.SDK.fsi
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,12 @@ type AnalyzerMessage =
HelpUri: string option
}

/// Represents a failure to run FSC analysis.
[<RequireQualifiedAccess>]
type AnalysisFailure =
/// The F# compiler service aborted during analysis.
| Aborted

module Utils =

[<RequireQualifiedAccess>]
Expand All @@ -171,7 +177,7 @@ module Utils =
options: FSharpProjectOptions ->
fileName: string ->
source: SourceOfSource ->
option<FSharpParseFileResults * FSharpCheckFileResults>
Result<FSharpParseFileResults * FSharpCheckFileResults, AnalysisFailure>

val createContext:
checkProjectResults: FSharpCheckProjectResults ->
Expand Down
Loading