diff --git a/docs/content/how-tos/rule-configuration.md b/docs/content/how-tos/rule-configuration.md index b0af74e66..7870db243 100644 --- a/docs/content/how-tos/rule-configuration.md +++ b/docs/content/how-tos/rule-configuration.md @@ -130,3 +130,4 @@ The following rules can be specified for linting. - [InterpolatedStringWithNoSubstitution (FL0087)](rules/FL0087.html) - [IndexerAccessorStyleConsistency (FL0088)](rules/FL0088.html) - [FavourSingleton (FL0089)](rules/FL0089.html) +- [NoAsyncRunSynchronouslyInLibrary (FL0090)](rules/FL0090.html) diff --git a/docs/content/how-tos/rules/FL0090.md b/docs/content/how-tos/rules/FL0090.md new file mode 100644 index 000000000..eb15db0a9 --- /dev/null +++ b/docs/content/how-tos/rules/FL0090.md @@ -0,0 +1,68 @@ +--- +title: FL0090 +category: how-to +hide_menu: true +--- + +# NoAsyncRunSynchronouslyInLibrary (FL0090) + +*Introduced in `0.26.10`* + +## Cause + +`Async.RunSynchronously` method is used to run async computation in library code. + +The rule assumes the code is in the library if none of the following is true: +- The code is inside NUnit or MSTest test. +- Namespace or project name contains "test" or "console". +- Assembly has `[]` attribute one one of the functions/methods. + +## Rationale + +Your library code might be consumed by certain type of programs which have strict threading requirements (e.g. a long running operation shouldn't be run on the main thread of a desktop app, or it will make the app look like it's hanging for a while), so it's better to expose asynchronous code with `Async<'TResult>` or `Task`/`Task<'TResult>` return types, so that the consumer of your library can decide how/when to start the operation. + +## How To Fix + +Remove `Async.RunSynchronously` and wrap the code that uses `async` computations in `async` computation, using `let!`, `use!`, `match!`, or `return!` keyword to get the result. + +Example: + +```fsharp +type SomeType() = + member self.SomeMethod someParam = + let foo = + asyncSomeFunc someParam + |> Async.RunSynchronously + processFoo foo +``` + +The function can be modified to be asynchronous. In that case it might be better to prefix its name with Async: + +```fsharp +type SomeType() = + member self.AsyncSomeMethod someParam = async { + let! foo = asyncSomeFunc someParam + return processFoo foo + } +``` + +In case the method/function is public, a nice C#-friendly overload that returns `Task<'T>` could be provided, suffixed with Async, that just calls the previous method with `Async.StartAsTask`: + +```fsharp +type SomeType() = + member self.AsyncSomeMethod someParam = async { + let! foo = asyncSomeFunc someParam + return processFoo foo + } + member self.SomeMethodAsync someParam = + self.AsyncSomeMethod someParam + |> Async.StartAsTask +``` + +## Rule Settings + + { + "noAsyncRunSynchronouslyInLibrary": { + "enabled": true + } + } diff --git a/src/FSharpLint.Console/Program.fs b/src/FSharpLint.Console/Program.fs index df33fa137..d62691ed3 100644 --- a/src/FSharpLint.Console/Program.fs +++ b/src/FSharpLint.Console/Program.fs @@ -158,9 +158,9 @@ let private lint try let lintResult = match fileType with - | FileType.File -> Lint.lintFile lintParams target - | FileType.Source -> Lint.lintSource lintParams target - | FileType.Solution -> Lint.lintSolution lintParams target toolsPath + | FileType.File -> Lint.asyncLintFile lintParams target |> Async.RunSynchronously + | FileType.Source -> Lint.asyncLintSource lintParams target |> Async.RunSynchronously + | FileType.Solution -> Lint.asyncLintSolution lintParams target toolsPath |> Async.RunSynchronously | FileType.Wildcard -> output.WriteInfo "Wildcard detected, but not recommended. Using a project (slnx/sln/fsproj) can detect more issues." let files = expandWildcard target @@ -169,9 +169,9 @@ let private lint LintResult.Success List.empty else output.WriteInfo $"Found %d{List.length files} file(s) matching pattern '%s{target}'." - Lint.lintFiles lintParams files + Lint.asyncLintFiles lintParams files |> Async.RunSynchronously | FileType.Project - | _ -> Lint.lintProject lintParams target toolsPath + | _ -> Lint.asyncLintProject lintParams target toolsPath |> Async.RunSynchronously handleLintResult lintResult with | exn -> diff --git a/src/FSharpLint.Core/Application/Configuration.fs b/src/FSharpLint.Core/Application/Configuration.fs index 0d2efba81..f02086d1e 100644 --- a/src/FSharpLint.Core/Application/Configuration.fs +++ b/src/FSharpLint.Core/Application/Configuration.fs @@ -554,7 +554,8 @@ type Configuration = EnsureTailCallDiagnosticsInRecursiveFunctions:EnabledConfig option FavourAsKeyword:EnabledConfig option InterpolatedStringWithNoSubstitution:EnabledConfig option - FavourSingleton:EnabledConfig option } + FavourSingleton:EnabledConfig option + NoAsyncRunSynchronouslyInLibrary:EnabledConfig option} with static member Zero = { Global = None @@ -656,6 +657,7 @@ with FavourAsKeyword = None InterpolatedStringWithNoSubstitution = None FavourSingleton = None + NoAsyncRunSynchronouslyInLibrary = None } // fsharplint:enable RecordFieldNames @@ -859,6 +861,7 @@ let flattenConfig (config:Configuration) = config.FavourAsKeyword |> Option.bind (constructRuleIfEnabled FavourAsKeyword.rule) config.InterpolatedStringWithNoSubstitution |> Option.bind (constructRuleIfEnabled InterpolatedStringWithNoSubstitution.rule) config.FavourSingleton |> Option.bind (constructRuleIfEnabled FavourSingleton.rule) + config.NoAsyncRunSynchronouslyInLibrary |> Option.bind (constructRuleIfEnabled NoAsyncRunSynchronouslyInLibrary.rule) |] findDeprecation config deprecatedAllRules allRules diff --git a/src/FSharpLint.Core/Application/Lint.fs b/src/FSharpLint.Core/Application/Lint.fs index da988cc96..eca7b1fd3 100644 --- a/src/FSharpLint.Core/Application/Lint.fs +++ b/src/FSharpLint.Core/Application/Lint.fs @@ -124,6 +124,7 @@ module Lint = Rules: RuleMetadata[] GlobalConfig: Rules.GlobalRuleConfig TypeCheckResults: FSharpCheckFileResults option + ProjectCheckResults: FSharpCheckProjectResults option FilePath: string FileContent: string Lines: string[] @@ -147,6 +148,7 @@ module Lint = FileContent = config.FileContent Lines = config.Lines CheckInfo = config.TypeCheckResults + ProjectCheckInfo = config.ProjectCheckResults GlobalConfig = config.GlobalConfig } // Build state for rules with context. @@ -260,6 +262,7 @@ module Lint = Rules = enabledRules.AstNodeRules GlobalConfig = enabledRules.GlobalConfig TypeCheckResults = fileInfo.TypeCheckResults + ProjectCheckResults = fileInfo.ProjectCheckResults FilePath = fileInfo.File FileContent = fileInfo.Text Lines = lines @@ -285,12 +288,6 @@ module Lint = |> Array.iter trySuggest if cancelHasNotBeenRequested () then - let runSynchronously work = - let timeoutMs = 2000 - match lintInfo.CancellationToken with - | Some(cancellationToken) -> Async.RunSynchronously(work, timeoutMs, cancellationToken) - | None -> Async.RunSynchronously(work, timeoutMs) - try let typeChecksSuccessful (typeChecks:(unit -> bool) list) = (true, typeChecks) @@ -420,6 +417,8 @@ module Lint = Source:string /// Optional results of inferring the types on the AST (allows for a more accurate lint). TypeCheckResults:FSharpCheckFileResults option + /// Optional results of project-wide type info (allows for a more accurate lint). + ProjectCheckResults:FSharpCheckProjectResults option } /// Gets a FSharpLint Configuration based on the provided ConfigurationParam. @@ -443,7 +442,7 @@ module Lint = /// Lints an entire F# project by retrieving the files from a given /// path to the `.fsproj` file. - let lintProject (optionalParams:OptionalLintParameters) (projectFilePath:string) (toolsPath:Ionide.ProjInfo.Types.ToolsPath) = + let asyncLintProject (optionalParams:OptionalLintParameters) (projectFilePath:string) (toolsPath:Ionide.ProjInfo.Types.ToolsPath) = async { if IO.File.Exists projectFilePath then let projectFilePath = Path.GetFullPath projectFilePath let lintWarnings = LinkedList() @@ -459,7 +458,7 @@ module Lint = let checker = FSharpChecker.Create(keepAssemblyContents=true) - let parseFilesInProject files projectOptions = + let parseFilesInProject files projectOptions = async { let lintInformation = { Configuration = config CancellationToken = optionalParams.CancellationToken @@ -473,39 +472,50 @@ module Lint = Configuration.IgnoreFiles.shouldFileBeIgnored parsedIgnoreFiles filePath) |> Option.defaultValue false - let parsedFiles = + let! parsedFiles = files |> List.filter (not << isIgnoredFile) - |> List.map (fun file -> ParseFile.parseFile file checker (Some projectOptions) |> Async.RunSynchronously) + |> List.map (fun file -> ParseFile.parseFile file checker (Some projectOptions)) + |> Async.Sequential + + let failedFiles = Array.choose getFailedFiles parsedFiles - let failedFiles = List.choose getFailedFiles parsedFiles + if Array.isEmpty failedFiles then + let! projectCheckResults = checker.ParseAndCheckProject projectOptions - if List.isEmpty failedFiles then parsedFiles - |> List.choose getParsedFiles - |> List.iter (lint lintInformation) + |> Array.choose getParsedFiles + |> Array.iter (fun fileParseResult -> + lint + lintInformation + { fileParseResult with ProjectCheckResults = Some projectCheckResults }) - Success () + return Success () else - Failure (FailedToParseFilesInProject failedFiles) + return Failure (FailedToParseFilesInProject (Array.toList failedFiles)) + } match getProjectInfo projectFilePath toolsPath with | Ok projectOptions -> - match parseFilesInProject (Array.toList projectOptions.SourceFiles) projectOptions with - | Success _ -> lintWarnings |> Seq.toList |> LintResult.Success - | Failure lintFailure -> LintResult.Failure lintFailure + match! parseFilesInProject (Array.toList projectOptions.SourceFiles) projectOptions with + | Success _ -> return lintWarnings |> Seq.toList |> LintResult.Success + | Failure lintFailure -> return LintResult.Failure lintFailure | Error error -> - MSBuildFailedToLoadProjectFile (projectFilePath, BuildFailure.InvalidProjectFileMessage error) - |> LintResult.Failure + return + MSBuildFailedToLoadProjectFile (projectFilePath, BuildFailure.InvalidProjectFileMessage error) + |> LintResult.Failure | Error err -> - RunTimeConfigError err - |> LintResult.Failure + return RunTimeConfigError err |> LintResult.Failure else - FailedToLoadFile projectFilePath - |> LintResult.Failure + return FailedToLoadFile projectFilePath |> LintResult.Failure + } + + [] + let lintProject optionalParams projectFilePath toolsPath = + asyncLintProject optionalParams projectFilePath toolsPath |> Async.RunSynchronously /// Lints an entire F# solution by linting all projects specified in the `.sln`, `slnx` or `.slnf` file. - let lintSolution (optionalParams:OptionalLintParameters) (solutionFilePath:string) (toolsPath:Ionide.ProjInfo.Types.ToolsPath) = + let asyncLintSolution (optionalParams:OptionalLintParameters) (solutionFilePath:string) (toolsPath:Ionide.ProjInfo.Types.ToolsPath) = async { if IO.File.Exists solutionFilePath then let solutionFilePath = Path.GetFullPath solutionFilePath let solutionFolder = Path.GetDirectoryName solutionFilePath @@ -532,9 +542,13 @@ module Lint = projectPath.Replace("\\", "/")) |> Seq.toArray - let (successes, failures) = + let! lintResults = projectsInSolution - |> Array.map (fun projectFilePath -> lintProject optionalParams projectFilePath toolsPath) + |> Array.map (fun projectFilePath -> asyncLintProject optionalParams projectFilePath toolsPath) + |> Async.Sequential + + let (successes, failures) = + lintResults |> Array.fold (fun (successes, failures) result -> match result with | LintResult.Success warnings -> @@ -544,17 +558,21 @@ module Lint = match failures with | [] -> - LintResult.Success successes + return LintResult.Success successes | firstErr :: _ -> - LintResult.Failure firstErr + return LintResult.Failure firstErr with | ex -> - LintResult.Failure (MSBuildFailedToLoadProjectFile (solutionFilePath, BuildFailure.InvalidProjectFileMessage ex.Message)) + return LintResult.Failure (MSBuildFailedToLoadProjectFile (solutionFilePath, BuildFailure.InvalidProjectFileMessage ex.Message)) - | Error err -> LintResult.Failure (RunTimeConfigError err) + | Error err -> return LintResult.Failure (RunTimeConfigError err) else - FailedToLoadFile solutionFilePath - |> LintResult.Failure + return FailedToLoadFile solutionFilePath |> LintResult.Failure + } + + [] + let lintSolution optionalParams solutionFilePath toolsPath = + asyncLintSolution optionalParams solutionFilePath toolsPath |> Async.RunSynchronously /// Lints F# source code that has already been parsed using `FSharp.Compiler.Services` in the calling application. let lintParsedSource optionalParams parsedFileInfo = @@ -576,6 +594,7 @@ module Lint = { ParseFile.Text = parsedFileInfo.Source ParseFile.Ast = parsedFileInfo.Ast ParseFile.TypeCheckResults = parsedFileInfo.TypeCheckResults + ParseFile.ProjectCheckResults = parsedFileInfo.ProjectCheckResults ParseFile.File = "" } lint lintInformation parsedFileInfo @@ -594,13 +613,14 @@ module Lint = let parsedFileInfo = { Source = parseFileInformation.Text Ast = parseFileInformation.Ast - TypeCheckResults = parseFileInformation.TypeCheckResults } + TypeCheckResults = parseFileInformation.TypeCheckResults + ProjectCheckResults = None } return lintParsedSource optionalParams parsedFileInfo | ParseFile.Failed failure -> return LintResult.Failure(FailedToParseFile failure) } - /// Lints F# source code. + [] let lintSource optionalParams source = asyncLintSource optionalParams source |> Async.RunSynchronously @@ -625,6 +645,7 @@ module Lint = { ParseFile.Text = parsedFileInfo.Source ParseFile.Ast = parsedFileInfo.Ast ParseFile.TypeCheckResults = parsedFileInfo.TypeCheckResults + ParseFile.ProjectCheckResults = parsedFileInfo.ProjectCheckResults ParseFile.File = filePath } lint lintInformation parsedFileInfo @@ -633,52 +654,70 @@ module Lint = | Error err -> LintResult.Failure (RunTimeConfigError err) /// Lints an F# file from a given path to the `.fs` file. - let lintFile optionalParams filePath = + let asyncLintFile optionalParams filePath = async { if IO.File.Exists filePath then let checker = FSharpChecker.Create(keepAssemblyContents=true) - match ParseFile.parseFile filePath checker None |> Async.RunSynchronously with + match! ParseFile.parseFile filePath checker None with | ParseFile.Success astFileParseInfo -> let parsedFileInfo = { Source = astFileParseInfo.Text Ast = astFileParseInfo.Ast - TypeCheckResults = astFileParseInfo.TypeCheckResults } + TypeCheckResults = astFileParseInfo.TypeCheckResults + ProjectCheckResults = astFileParseInfo.ProjectCheckResults } - lintParsedFile optionalParams parsedFileInfo filePath - | ParseFile.Failed failure -> LintResult.Failure(FailedToParseFile failure) + return lintParsedFile optionalParams parsedFileInfo filePath + | ParseFile.Failed failure -> return LintResult.Failure(FailedToParseFile failure) else - FailedToLoadFile filePath - |> LintResult.Failure + return FailedToLoadFile filePath |> LintResult.Failure + } + + [] + let lintFile optionalParams filePath = + asyncLintFile optionalParams filePath |> Async.RunSynchronously /// Lints multiple F# files from given file paths. - let lintFiles optionalParams filePaths = + let asyncLintFiles optionalParams filePaths = async { let checker = FSharpChecker.Create(keepAssemblyContents=true) match getConfig optionalParams.Configuration with | Ok config -> let optionalParams = { optionalParams with Configuration = ConfigurationParam.Configuration config } - let lintSingleFile filePath = + let lintSingleFile filePath = async { if IO.File.Exists filePath then - match ParseFile.parseFile filePath checker None |> Async.RunSynchronously with + match! ParseFile.parseFile filePath checker None with | ParseFile.Success astFileParseInfo -> let parsedFileInfo = { Source = astFileParseInfo.Text Ast = astFileParseInfo.Ast - TypeCheckResults = astFileParseInfo.TypeCheckResults } - lintParsedFile optionalParams parsedFileInfo filePath + TypeCheckResults = astFileParseInfo.TypeCheckResults + ProjectCheckResults = astFileParseInfo.ProjectCheckResults } + return lintParsedFile optionalParams parsedFileInfo filePath | ParseFile.Failed failure -> - LintResult.Failure (FailedToParseFile failure) + return LintResult.Failure (FailedToParseFile failure) else - LintResult.Failure (FailedToLoadFile filePath) + return LintResult.Failure (FailedToLoadFile filePath) + } - let results = filePaths |> Seq.map lintSingleFile |> Seq.toList + let! results = filePaths |> Seq.map lintSingleFile |> Async.Sequential - let failures = results |> List.choose (function | LintResult.Failure failure -> Some failure | _ -> None) - let warnings = results |> List.collect (function | LintResult.Success warning -> warning | _ -> List.empty) + let failures = + results + |> Seq.choose (function | LintResult.Failure failure -> Some failure | _ -> None) + |> Seq.toList + let warnings = + results + |> Seq.collect (function | LintResult.Success warning -> warning | _ -> List.empty) + |> Seq.toList match failures with - | firstFailure :: _ -> LintResult.Failure firstFailure - | [] -> LintResult.Success warnings + | firstFailure :: _ -> return LintResult.Failure firstFailure + | [] -> return LintResult.Success warnings | Error err -> - LintResult.Failure (RunTimeConfigError err) + return LintResult.Failure (RunTimeConfigError err) + } + + [] + let lintFiles optionalParams filePaths = + asyncLintFiles optionalParams filePaths |> Async.RunSynchronously diff --git a/src/FSharpLint.Core/Application/Lint.fsi b/src/FSharpLint.Core/Application/Lint.fsi index 29ddd3332..618d48317 100644 --- a/src/FSharpLint.Core/Application/Lint.fsi +++ b/src/FSharpLint.Core/Application/Lint.fsi @@ -74,6 +74,9 @@ module Lint = /// Optional results of inferring the types on the AST (allows for a more accurate lint). TypeCheckResults: FSharpCheckFileResults option + + /// Optional results of project-wide type info (allows for a more accurate lint). + ProjectCheckResults:FSharpCheckProjectResults option } type BuildFailure = | InvalidProjectFileMessage of string @@ -124,6 +127,7 @@ module Lint = Rules: RuleMetadata[] GlobalConfig: Rules.GlobalRuleConfig TypeCheckResults: FSharpCheckFileResults option + ProjectCheckResults: FSharpCheckProjectResults option FilePath: string FileContent: string Lines: string[] @@ -147,26 +151,38 @@ module Lint = val runLineRules : RunLineRulesConfig -> Suggestion.LintWarning [] /// Lints an entire F# solution by linting all projects specified in the `.sln`, `slnx` or `.slnf` file. + val asyncLintSolution : optionalParams:OptionalLintParameters -> solutionFilePath:string -> toolsPath:Ionide.ProjInfo.Types.ToolsPath -> Async + + /// [Obsolete] Lints an entire F# solution by linting all projects specified in the `.sln`, `slnx` or `.slnf` file. val lintSolution : optionalParams:OptionalLintParameters -> solutionFilePath:string -> toolsPath:Ionide.ProjInfo.Types.ToolsPath -> LintResult /// Lints an entire F# project by retrieving the files from a given /// path to the `.fsproj` file. - val lintProject : optionalParams:OptionalLintParameters -> projectFilePath:string -> toolsPath:Ionide.ProjInfo.Types.ToolsPath -> LintResult + val asyncLintProject : optionalParams:OptionalLintParameters -> projectFilePath:string -> toolsPath:Ionide.ProjInfo.Types.ToolsPath -> Async - /// Lints F# source code. - val lintSource : optionalParams:OptionalLintParameters -> source:string -> LintResult + /// [Obsolete] Lints an entire F# project by retrieving the files from a given path to the `.fsproj` file. + val lintProject : optionalParams:OptionalLintParameters -> projectFilePath:string -> toolsPath:Ionide.ProjInfo.Types.ToolsPath -> LintResult /// Lints F# source code async. val asyncLintSource : optionalParams:OptionalLintParameters -> source:string -> Async + /// [Obsolete] Lints F# source code. + val lintSource : optionalParams:OptionalLintParameters -> source:string -> LintResult + /// Lints F# source code that has already been parsed using /// `FSharp.Compiler.Services` in the calling application. val lintParsedSource : optionalParams:OptionalLintParameters -> parsedFileInfo:ParsedFileInformation -> LintResult /// Lints an F# file from a given path to the `.fs` file. + val asyncLintFile : optionalParams:OptionalLintParameters -> filePath:string -> Async + + /// [Obsolete] Lints an F# file from a given path to the `.fs` file. val lintFile : optionalParams:OptionalLintParameters -> filePath:string -> LintResult /// Lints multiple F# files from given file paths. + val asyncLintFiles : optionalParams:OptionalLintParameters -> filePaths:string seq -> Async + + /// [Obsolete] Lints multiple F# files from given file paths. val lintFiles : optionalParams:OptionalLintParameters -> filePaths:string seq -> LintResult /// Lints an F# file that has already been parsed using diff --git a/src/FSharpLint.Core/FSharpLint.Core.fsproj b/src/FSharpLint.Core/FSharpLint.Core.fsproj index daa002b30..e0810b760 100644 --- a/src/FSharpLint.Core/FSharpLint.Core.fsproj +++ b/src/FSharpLint.Core/FSharpLint.Core.fsproj @@ -31,7 +31,6 @@ - @@ -47,7 +46,6 @@ - @@ -58,16 +56,8 @@ - - - - - - - - @@ -125,6 +115,17 @@ + + + + + + + + + + + diff --git a/src/FSharpLint.Core/Framework/ParseFile.fs b/src/FSharpLint.Core/Framework/ParseFile.fs index b8e844401..86b0586f7 100644 --- a/src/FSharpLint.Core/Framework/ParseFile.fs +++ b/src/FSharpLint.Core/Framework/ParseFile.fs @@ -23,6 +23,9 @@ module ParseFile = /// Optional results of inferring the types on the AST (allows for a more accurate lint). TypeCheckResults:FSharpCheckFileResults option + /// Optional results of project-wide type info (allows for a more accurate lint). + ProjectCheckResults:FSharpCheckProjectResults option + /// Path to the file. File:string } @@ -49,6 +52,7 @@ module ParseFile = Text = source Ast = parseResults.ParseTree TypeCheckResults = Some(typeCheckResults) + ProjectCheckResults = None File = file } | FSharpCheckFileAnswer.Aborted -> return Failed(AbortedTypeCheck) diff --git a/src/FSharpLint.Core/Framework/Rules.fs b/src/FSharpLint.Core/Framework/Rules.fs index 2877244ab..29057abe8 100644 --- a/src/FSharpLint.Core/Framework/Rules.fs +++ b/src/FSharpLint.Core/Framework/Rules.fs @@ -30,6 +30,7 @@ type AstNodeRuleParams = FileContent:string Lines:string [] CheckInfo:FSharpCheckFileResults option + ProjectCheckInfo:FSharpCheckProjectResults option GlobalConfig:GlobalRuleConfig } type LineRuleParams = diff --git a/src/FSharpLint.Core/Rules/Identifiers.fs b/src/FSharpLint.Core/Rules/Identifiers.fs index 910e968d6..01889c1bc 100644 --- a/src/FSharpLint.Core/Rules/Identifiers.fs +++ b/src/FSharpLint.Core/Rules/Identifiers.fs @@ -94,3 +94,4 @@ let FavourAsKeyword = identifier 86 let InterpolatedStringWithNoSubstitution = identifier 87 let IndexerAccessorStyleConsistency = identifier 88 let FavourSingleton = identifier 89 +let NoAsyncRunSynchronouslyInLibrary = identifier 90 diff --git a/src/FSharpLint.Core/Rules/Smells/NoAsyncRunSynchronouslyInLibrary.fs b/src/FSharpLint.Core/Rules/Smells/NoAsyncRunSynchronouslyInLibrary.fs new file mode 100644 index 000000000..601d476de --- /dev/null +++ b/src/FSharpLint.Core/Rules/Smells/NoAsyncRunSynchronouslyInLibrary.fs @@ -0,0 +1,157 @@ +module FSharpLint.Rules.NoAsyncRunSynchronouslyInLibrary + +open FSharp.Compiler.Syntax +open FSharp.Compiler.Symbols +open FSharp.Compiler.Text +open FSharp.Compiler.CodeAnalysis +open FSharpLint.Framework +open FSharpLint.Framework.Suggestion +open FSharpLint.Framework.Ast +open FSharpLint.Framework.Rules +open FSharpLint.Framework.Utilities + +type LibraryHeuristicResultByProjectName = + | Likely + | Unlikely + | Uncertain + +let hasEntryPoint (checkFileResults: FSharpCheckFileResults) (maybeProjectCheckResults: Option) = + let hasEntryPointInTheSameFile = + match checkFileResults.ImplementationFile with + | Some implFile -> implFile.HasExplicitEntryPoint + | None -> false + + hasEntryPointInTheSameFile + || + match maybeProjectCheckResults with + | Some projectCheckResults -> + projectCheckResults.AssemblyContents.ImplementationFiles + |> Seq.exists (fun implFile -> implFile.HasExplicitEntryPoint) + | None -> + false + +let excludedProjectNames = [ "test"; "console" ] + +let howLikelyProjectIsLibrary (projectFileName: string): LibraryHeuristicResultByProjectName = + let nameSegments = Helper.Naming.QuickFixes.splitByCaseChange projectFileName + if nameSegments |> Seq.contains "Lib" then + Likely + elif excludedProjectNames |> List.exists (fun name -> projectFileName.ToLowerInvariant().Contains name) then + Unlikely + elif projectFileName.ToLowerInvariant().EndsWith "lib" then + Likely + else + Uncertain + +let extractAttributeNames (attributes: SynAttributes) = + seq { + for attr in extractAttributes attributes do + match attr.TypeName with + | SynLongIdent([ident], _, _) -> yield ident.idText + | _ -> () + } + +let testMethodAttributes = [ "Test"; "TestMethod" ] +let testClassAttributes = [ "TestFixture"; "TestClass" ] + +let areThereTestsInSameFileOrProject (nodes: array) (maybeProjectCheckResults: FSharpCheckProjectResults option) = + let isTestMethodOrClass node = + match node with + | AstNode.MemberDefinition(SynMemberDefn.Member(SynBinding(_, _, _, _, attributes, _, _, _, _, _, _, _, _), _)) -> + attributes + |> extractAttributeNames + |> Seq.exists (fun name -> testMethodAttributes |> List.contains name) + | AstNode.TypeDefinition(SynTypeDefn.SynTypeDefn(SynComponentInfo(attributes, _, _, _, _, _, _, _), _, _, _, _, _)) -> + attributes + |> extractAttributeNames + |> Seq.exists (fun name -> testClassAttributes |> List.contains name) + | _ -> false + + let containsTests declaration = + match declaration with + | FSharpImplementationFileDeclaration.Entity(entity, declarations) when entity.IsClass -> + entity.Attributes + |> Seq.exists (fun attr -> testClassAttributes |> List.contains attr.AttributeType.DisplayName) + || + declarations + |> Seq.exists + (fun memberDecl -> + match memberDecl with + | FSharpImplementationFileDeclaration.MemberOrFunctionOrValue(method, _, _) when method.IsMethod -> + method.Attributes + |> Seq.exists (fun attr -> testMethodAttributes |> List.contains attr.AttributeType.DisplayName) + | _ -> false) + | _ -> false + + match maybeProjectCheckResults with + | Some projectCheckResults -> + projectCheckResults.AssemblyContents.ImplementationFiles + |> Seq.exists (fun implFile -> + implFile.Declarations + |> Seq.exists containsTests + ) + | None -> + nodes |> Array.exists (fun node -> isTestMethodOrClass node.Actual) + +let isInObsoleteMethodOrFunction parents = + let isObsolete node = + match node with + | AstNode.MemberDefinition(SynMemberDefn.Member(SynBinding(_, _, _, _, attributes, _, _, _, _, _, _, _, _), _)) -> + attributes + |> extractAttributeNames + |> Seq.contains "Obsolete" + | AstNode.Binding(SynBinding(_, _, _, _, attributes, _, _, _, _, _, _, _, _)) -> + attributes + |> extractAttributeNames + |> Seq.contains "Obsolete" + | _ -> false + + parents |> List.exists isObsolete + +let checkIfInLibrary (args: AstNodeRuleParams) (range: range) : array = + let ruleNotApplicable = + isInObsoleteMethodOrFunction (args.GetParents args.NodeIndex) + || + match (args.CheckInfo, args.ProjectCheckInfo) with + | Some checkFileResults, Some checkProjectResults -> + let projectFile = System.IO.FileInfo checkProjectResults.ProjectContext.ProjectOptions.ProjectFileName + match howLikelyProjectIsLibrary projectFile.Name with + | Likely -> false + | Unlikely -> true + | Uncertain -> + hasEntryPoint checkFileResults args.ProjectCheckInfo + || areThereTestsInSameFileOrProject args.SyntaxArray args.ProjectCheckInfo + | Some checkFileResults, None -> + hasEntryPoint checkFileResults None + || areThereTestsInSameFileOrProject args.SyntaxArray args.ProjectCheckInfo + | _ -> + areThereTestsInSameFileOrProject args.SyntaxArray args.ProjectCheckInfo + + if ruleNotApplicable then + Array.empty + else + Array.singleton + { + Range = range + Message = Resources.GetString "NoAsyncRunSynchronouslyInLibrary" + SuggestedFix = None + TypeChecks = List.Empty + } + +let runner args = + match args.AstNode with + | AstNode.Identifier(["Async"; "RunSynchronously"], range) -> + checkIfInLibrary args range + | _ -> Array.empty + +let rule = + AstNodeRule + { + Name = "NoAsyncRunSynchronouslyInLibrary" + Identifier = Identifiers.NoAsyncRunSynchronouslyInLibrary + RuleConfig = + { + AstNodeRuleConfig.Runner = runner + Cleanup = ignore + } + } diff --git a/src/FSharpLint.Core/Text.resx b/src/FSharpLint.Core/Text.resx index 92b713b3b..a6f38a3f4 100644 --- a/src/FSharpLint.Core/Text.resx +++ b/src/FSharpLint.Core/Text.resx @@ -390,4 +390,7 @@ Consider using List.singleton/Array.singleton instead of single member list/array. + + Async.RunSynchronously should not be used in libraries. + diff --git a/src/FSharpLint.Core/fsharplint.json b/src/FSharpLint.Core/fsharplint.json index 3f7e1c858..7b32a6b17 100644 --- a/src/FSharpLint.Core/fsharplint.json +++ b/src/FSharpLint.Core/fsharplint.json @@ -341,6 +341,7 @@ } }, "favourSingleton": { "enabled": false }, + "noAsyncRunSynchronouslyInLibrary": { "enabled": true }, "hints": { "add": [ "not (a = b) ===> a <> b", diff --git a/tests/FSharpLint.Benchmarks/Benchmark.fs b/tests/FSharpLint.Benchmarks/Benchmark.fs index 63f5e703c..4e8b536ef 100644 --- a/tests/FSharpLint.Benchmarks/Benchmark.fs +++ b/tests/FSharpLint.Benchmarks/Benchmark.fs @@ -30,7 +30,7 @@ type Benchmark () = let (fileInfo, lines) = let text = File.ReadAllText sourceFile let tree = generateAst text sourceFile - ({ Ast = tree; Source = text; TypeCheckResults = None }, String.toLines text |> Array.toList) + ({ Ast = tree; Source = text; TypeCheckResults = None; ProjectCheckResults = None }, String.toLines text |> Array.toList) [] member this.LintParsedFile () = diff --git a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj index b376875d1..d57f7fff5 100644 --- a/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj +++ b/tests/FSharpLint.Core.Tests/FSharpLint.Core.Tests.fsproj @@ -14,6 +14,7 @@ + diff --git a/tests/FSharpLint.Core.Tests/Rules/Smells/NoAsyncRunSynchronouslyInLibrary.fs b/tests/FSharpLint.Core.Tests/Rules/Smells/NoAsyncRunSynchronouslyInLibrary.fs new file mode 100644 index 000000000..65e95c40a --- /dev/null +++ b/tests/FSharpLint.Core.Tests/Rules/Smells/NoAsyncRunSynchronouslyInLibrary.fs @@ -0,0 +1,170 @@ +module FSharpLint.Core.Tests.Rules.Smells.NoAsyncRunSynchronouslyInLibrary + +open NUnit.Framework +open FSharpLint.Framework.Rules +open FSharpLint.Rules +open FSharpLint.Rules.NoAsyncRunSynchronouslyInLibrary + +[] +type TestNoAsyncRunSynchronouslyInLibrary() = + inherit FSharpLint.Core.Tests.TestAstNodeRuleBase.TestAstNodeRuleBase(NoAsyncRunSynchronouslyInLibrary.rule) + + [] + member this.``Async.RunSynchronously should not be used in library code``() = + this.Parse(""" +module Program + +async { + return () +} +|> Async.RunSynchronously""") + + Assert.IsTrue this.ErrorsExist + + [] + member this.``Async.RunSynchronously may be used in code that declares entry point``() = + this.Parse(""" +module Program + +[] +let main () = + async { + return () + } + |> Async.RunSynchronously""") + + this.AssertNoWarnings() + + [] + member this.``Async.RunSynchronously may be used in code module that has function with entry point``() = + this.Parse(""" +module Program + +let foo () = + async { + return () + } + |> Async.RunSynchronously + +[] +let main () = + 0""") + + this.AssertNoWarnings() + + [] + member this.``Async.RunSynchronously may be used in NUnit test code``() = + this.Parse(""" +module Program + +[] +type FooTest () = + [] + member this.Foo() = + async { + return () + } + |> Async.RunSynchronously""") + + this.AssertNoWarnings() + + [] + member this.``Async.RunSynchronously may be used in MSTest test code``() = + this.Parse(""" +module Program + +[] +type FooTest () = + [] + member this.Foo() = + async { + return () + } + |> Async.RunSynchronously""") + + this.AssertNoWarnings() + + [] + member this.``Async.RunSynchronously may be used in module with tests``() = + this.Parse(""" +module Program + +let foo () = + async { + return () + } + |> Async.RunSynchronously + +[] +type FooTest () = + [] + member this.Foo() = + ()""") + + this.AssertNoWarnings() + + [] + member this.``Async.RunSynchronously may be used in methods with Obsolete attribute``() = + this.Parse(""" +module Program + +type FooTest () = + [] + member this.Foo() = + async { + return () + } + |> Async.RunSynchronously""") + + this.AssertNoWarnings() + + [] + member this.``Async.RunSynchronously may be used in functions with Obsolete attribute``() = + this.Parse(""" +module Program + +[] +let Foo() = + async { + return () + } + |> Async.RunSynchronously""") + + this.AssertNoWarnings() + +[] +type TestNoAsyncRunSynchronouslyInLibraryHeuristic() = + [] + member this.``Unlikely to be library if contains "test" in name``() = + Assert.AreEqual( + howLikelyProjectIsLibrary "TestProject", + LibraryHeuristicResultByProjectName.Unlikely + ) + + [] + member this.``Unlikely to be library if contains "console" in name``() = + Assert.AreEqual( + howLikelyProjectIsLibrary "FooConsole", + LibraryHeuristicResultByProjectName.Unlikely + ) + + [] + member this.``Likely to be library if contains Contains "Lib" as a PascalCase segment``() = + Assert.AreEqual( + howLikelyProjectIsLibrary "LibFoo", + LibraryHeuristicResultByProjectName.Likely + ) + + [] + member this.``Uncertain if contains contains "Lib" but not as a PascalCase segment``() = + Assert.AreEqual( + howLikelyProjectIsLibrary "LibreOfficeProg", + LibraryHeuristicResultByProjectName.Uncertain + ) + + [] + member this.``Likely to be library if contains ends with "lib" (case-insensitive)``() = + Assert.AreEqual( + howLikelyProjectIsLibrary "FooLib", + LibraryHeuristicResultByProjectName.Likely + ) diff --git a/tests/FSharpLint.Core.Tests/Rules/TestAstNodeRule.fs b/tests/FSharpLint.Core.Tests/Rules/TestAstNodeRule.fs index 0f78b6f19..425a3e5bd 100644 --- a/tests/FSharpLint.Core.Tests/Rules/TestAstNodeRule.fs +++ b/tests/FSharpLint.Core.Tests/Rules/TestAstNodeRule.fs @@ -42,6 +42,7 @@ type TestAstNodeRuleBase (rule:Rule) = Rules = Array.singleton rule GlobalConfig = globalConfig TypeCheckResults = checkResult + ProjectCheckResults = None FilePath = (Option.defaultValue String.Empty fileName) FileContent = input Lines = (input.Split("\n")) diff --git a/tests/FSharpLint.Core.Tests/Rules/TestHintMatcherBase.fs b/tests/FSharpLint.Core.Tests/Rules/TestHintMatcherBase.fs index 5f2222309..6fe9dbe11 100644 --- a/tests/FSharpLint.Core.Tests/Rules/TestHintMatcherBase.fs +++ b/tests/FSharpLint.Core.Tests/Rules/TestHintMatcherBase.fs @@ -64,6 +64,7 @@ type TestHintMatcherBase () = Rules = Array.singleton rule GlobalConfig = globalConfig TypeCheckResults = checkResult + ProjectCheckResults = None FilePath = (Option.defaultValue String.Empty fileName) FileContent = input Lines = (input.Split("\n")) diff --git a/tests/FSharpLint.Core.Tests/Rules/TestIndentationRule.fs b/tests/FSharpLint.Core.Tests/Rules/TestIndentationRule.fs index 8abd41702..e2d2fd68b 100644 --- a/tests/FSharpLint.Core.Tests/Rules/TestIndentationRule.fs +++ b/tests/FSharpLint.Core.Tests/Rules/TestIndentationRule.fs @@ -37,6 +37,7 @@ type TestIndentationRuleBase (rule:Rule) = Rules = Array.empty GlobalConfig = globalConfig TypeCheckResults = None + ProjectCheckResults = None FilePath = fileName FileContent = input Lines = lines diff --git a/tests/FSharpLint.Core.Tests/Rules/TestLineRule.fs b/tests/FSharpLint.Core.Tests/Rules/TestLineRule.fs index c9c46cf02..480e55f82 100644 --- a/tests/FSharpLint.Core.Tests/Rules/TestLineRule.fs +++ b/tests/FSharpLint.Core.Tests/Rules/TestLineRule.fs @@ -37,6 +37,7 @@ type TestLineRuleBase (rule:Rule) = Rules = Array.empty GlobalConfig = globalConfig TypeCheckResults = None + ProjectCheckResults = None FilePath = fileName FileContent = input Lines = lines diff --git a/tests/FSharpLint.Core.Tests/Rules/TestNoTabCharactersRule.fs b/tests/FSharpLint.Core.Tests/Rules/TestNoTabCharactersRule.fs index 2807d7ad8..ccfcaccf2 100644 --- a/tests/FSharpLint.Core.Tests/Rules/TestNoTabCharactersRule.fs +++ b/tests/FSharpLint.Core.Tests/Rules/TestNoTabCharactersRule.fs @@ -37,6 +37,7 @@ type TestNoTabCharactersRuleBase (rule:Rule) = Rules = Array.empty GlobalConfig = globalConfig TypeCheckResults = None + ProjectCheckResults = None FilePath = fileName FileContent = input Lines = lines diff --git a/tests/FSharpLint.FunctionalTest/TestApi.fs b/tests/FSharpLint.FunctionalTest/TestApi.fs index d7fe3282b..635113f10 100644 --- a/tests/FSharpLint.FunctionalTest/TestApi.fs +++ b/tests/FSharpLint.FunctionalTest/TestApi.fs @@ -38,7 +38,7 @@ module TestApi = member _.``Performance of linting an existing file``() = let text = File.ReadAllText sourceFile let tree = generateAst text - let fileInfo = { Ast = tree; Source = text; TypeCheckResults = None } + let fileInfo = { Ast = tree; Source = text; TypeCheckResults = None; ProjectCheckResults = None } let stopwatch = Stopwatch.StartNew() let times = ResizeArray() @@ -64,7 +64,7 @@ module TestApi = let projectPath = basePath "tests" "FSharpLint.FunctionalTest.TestedProject" "FSharpLint.FunctionalTest.TestedProject.NetCore" let projectFile = projectPath "FSharpLint.FunctionalTest.TestedProject.NetCore.fsproj" - let result = lintProject OptionalLintParameters.Default projectFile toolsPath + let result = asyncLintProject OptionalLintParameters.Default projectFile toolsPath |> Async.RunSynchronously match result with | LintResult.Success warnings -> @@ -77,7 +77,7 @@ module TestApi = let projectPath = basePath "tests" "FSharpLint.FunctionalTest.TestedProject" "FSharpLint.FunctionalTest.TestedProject.NetCore" let projectFile = projectPath "FSharpLint.FunctionalTest.TestedProject.NetCore.fsproj" - let result = lintProject OptionalLintParameters.Default projectFile toolsPath + let result = asyncLintProject OptionalLintParameters.Default projectFile toolsPath |> Async.RunSynchronously match result with | LintResult.Success warnings -> @@ -92,7 +92,7 @@ module TestApi = let tempConfigFile = TestContext.CurrentContext.TestDirectory "fsharplint.json" File.WriteAllText (tempConfigFile, """{ "ignoreFiles": ["*"] }""") - let result = lintProject OptionalLintParameters.Default projectFile toolsPath + let result = asyncLintProject OptionalLintParameters.Default projectFile toolsPath |> Async.RunSynchronously File.Delete tempConfigFile match result with @@ -108,7 +108,7 @@ module TestApi = let projectPath = basePath "tests" "FSharpLint.FunctionalTest.TestedProject" let solutionFile = projectPath solutionFileName - let result = lintSolution OptionalLintParameters.Default solutionFile toolsPath + let result = asyncLintSolution OptionalLintParameters.Default solutionFile toolsPath |> Async.RunSynchronously match result with | LintResult.Success warnings -> @@ -126,7 +126,9 @@ module TestApi = let relativePathToSolutionFile = Path.GetRelativePath (Directory.GetCurrentDirectory(), solutionFile) - let result = lintSolution OptionalLintParameters.Default relativePathToSolutionFile toolsPath + let result = + asyncLintSolution OptionalLintParameters.Default relativePathToSolutionFile toolsPath + |> Async.RunSynchronously match result with | LintResult.Success warnings ->