-
Notifications
You must be signed in to change notification settings - Fork 75
Provide project options in lint calls #872
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| namespace FSharpLint.Framework | ||
|
|
||
| #nowarn "FS0057" // 'FSharpProjectSnapshot' is considered experimental. Note: Could suppress this more locally if building with the .NET 10 compiler | ||
|
|
||
| /// Provides functionality to parse F# files using `FSharp.Compiler.Service`. | ||
| module ParseFile = | ||
|
|
||
|
|
@@ -11,6 +13,58 @@ module ParseFile = | |
| open FSharp.Compiler.Text | ||
| open Utilities | ||
|
|
||
| /// Options related to the project being linted. | ||
| /// Based on https://github.com/ionide/FSharp.Analyzers.SDK/blob/f323144f0a4db51be564a3187838f2328f0e9182/src/FSharp.Analyzers.SDK/FSharp.Analyzers.SDK.fsi#L66 | ||
| [<NoEquality; NoComparison>] | ||
| type LinterProjectOptions = | ||
| | ProjectOptions of options: FSharpProjectOptions | ||
| | ProjectSnapshot of snapshot: FSharpProjectSnapshot | ||
|
|
||
| member this.ProjectFileName = | ||
| match this with | ||
| | ProjectOptions(options) -> options.ProjectFileName | ||
| | ProjectSnapshot(snapshot) -> snapshot.ProjectFileName | ||
|
|
||
| #if false | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Numpsy why is this wrapped in #if false? put the explanation in a comment please
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what the
comment above was about - It could keep the full set of functions available in FsAutoComplete/AnalyzersSDK, or trim it down to just what's presently being used (the others may or may not be useful later)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I meant that it has to be a code comment, not a PR comment. PR comments are not visible even with git blame.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, the question should be to confirm whether you want to keep the ifdefed out parts with comments, or just remove them altogether |
||
| member x.ProjectId = | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Numpsy don't use x please, but the convention
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Names just cloned in directly from https://github.com/ionide/FSharp.Analyzers.SDK/blob/f323144f0a4db51be564a3187838f2328f0e9182/src/FSharp.Analyzers.SDK/FSharp.Analyzers.SDK.fs#L261, will change them if you want to keep the ifdefed parts rather than just deleting them |
||
| match x with | ||
| | BackgroundCompilerOptions(options) -> options.ProjectId | ||
| | TransparentCompilerOptions(snapshot) -> snapshot.ProjectId | ||
|
|
||
| member x.SourceFiles = | ||
| match x with | ||
| | BackgroundCompilerOptions(options) -> | ||
| options.SourceFiles | ||
| |> Array.toList | ||
| | TransparentCompilerOptions(snapshot) -> | ||
| snapshot.SourceFiles | ||
| |> List.map (fun f -> f.FileName) | ||
| |> List.map System.IO.Path.GetFullPath | ||
|
|
||
| member x.ReferencedProjectsPath = | ||
| match x with | ||
| | BackgroundCompilerOptions(options) -> | ||
| options.ReferencedProjects | ||
| |> Array.choose (fun p -> p.ProjectFilePath) | ||
| |> Array.toList | ||
| | TransparentCompilerOptions(snapshot) -> | ||
| snapshot.ReferencedProjects | ||
| |> List.choose (fun p -> p.ProjectFilePath) | ||
|
|
||
| member x.LoadTime = | ||
| match x with | ||
| | BackgroundCompilerOptions(options) -> options.LoadTime | ||
| | TransparentCompilerOptions(snapshot) -> snapshot.LoadTime | ||
|
|
||
| member x.OtherOptions = | ||
| match x with | ||
| | BackgroundCompilerOptions(options) -> | ||
| options.OtherOptions | ||
| |> Array.toList | ||
| | TransparentCompilerOptions(snapshot) -> snapshot.OtherOptions | ||
|
|
||
| #endif | ||
|
|
||
| /// Information for a file to be linted that is given to the analysers. | ||
| [<NoEquality; NoComparison>] | ||
| type FileParseInfo = { | ||
|
|
@@ -26,6 +80,9 @@ module ParseFile = | |
| /// Optional results of project-wide type info (allows for a more accurate lint). | ||
| ProjectCheckResults:FSharpCheckProjectResults option | ||
|
|
||
| /// Optional project options. Allows rules to operate on project options. | ||
| ProjectOptions: LinterProjectOptions option | ||
|
|
||
| /// Path to the file. | ||
| File:string | ||
| } | ||
|
|
@@ -53,6 +110,7 @@ module ParseFile = | |
| Ast = parseResults.ParseTree | ||
| TypeCheckResults = Some(typeCheckResults) | ||
| ProjectCheckResults = None | ||
| ProjectOptions = Some (ProjectOptions options) | ||
| File = file | ||
| } | ||
| | FSharpCheckFileAnswer.Aborted -> return Failed(AbortedTypeCheck) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <TargetFramework>net8.0</TargetFramework> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Include="LibAsyncNames.fs" /> | ||
| </ItemGroup> | ||
| </Project> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| module Foo | ||
|
|
||
| let Bar(): Async<int> = | ||
| async { return 1 } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,10 @@ module TestApi = | |
|
|
||
| let sourceFile = basePath </> "tests" </> "TypeChecker.fs" | ||
|
|
||
| // Test project used for transparent/background compiler project options tests | ||
| let asyncTestProjectPath = basePath </> "tests" </> "FSharpLint.FunctionalTest.TestedProject" </> "LibAsync" | ||
| let asyncTestProjectFile = asyncTestProjectPath </> "LibAsync.fsproj" | ||
|
|
||
| [<TestFixture(Category = "Acceptance Tests")>] | ||
| type TestApi() = | ||
| let generateAst source = | ||
|
|
@@ -38,7 +42,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; ProjectCheckResults = None } | ||
| let fileInfo = { Ast = tree; Source = text; TypeCheckResults = None; ProjectCheckResults = None; ProjectOptions = None } | ||
|
|
||
| let stopwatch = Stopwatch.StartNew() | ||
| let times = ResizeArray() | ||
|
|
@@ -59,6 +63,22 @@ module TestApi = | |
| Assert.Less(result, 250) | ||
| fprintf TestContext.Out "Average runtime of linter on parsed file: %d (milliseconds)." result | ||
|
|
||
| // Test linting the async-name test project with the default linting functions, which use the background compiler | ||
| // This should tokenize "LibAsync.fsproj" tokenizes to ["Lib"; "Async"; ".fsproj"] -> Likely a library. | ||
| [<Test>] | ||
| member _.``Lint async naming test project with background compiler``() = | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added this test here to test the project options being set up correctly inside asyncLintProject |
||
| task { | ||
| let! result = asyncLintProject OptionalLintParameters.Default asyncTestProjectFile toolsPath | ||
|
|
||
| match result with | ||
| | LintResult.Success warnings -> | ||
| Assert.AreEqual(1, warnings.Length) | ||
| Assert.AreEqual(FSharpLint.Rules.Identifiers.AsynchronousFunctionNames, warnings.[0].RuleIdentifier) | ||
| StringAssert.Contains("This function returns Async. Consider renaming it to AsyncBar.", warnings.[0].Details.Message) | ||
| | LintResult.Failure err -> | ||
| Assert.Fail(string err) | ||
| } | ||
|
|
||
| [<Test>] | ||
| member _.``Lint project via absolute path``() = | ||
| let projectPath = basePath </> "tests" </> "FSharpLint.FunctionalTest.TestedProject" </> "FSharpLint.FunctionalTest.TestedProject.NetCore" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this should live in this file or elsewhere, just put it here for somewhere high up the file order.
If ifdefed out part is the full set of properties present in the analyzers sdk, but only the file name is used here at present - the other parts could be deleted to minimize the change, or left in case they might be useful later.