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

WIP - [RFC FST-1033] analyzers draft #11057

Closed
wants to merge 57 commits into from
Closed

WIP - [RFC FST-1033] analyzers draft #11057

wants to merge 57 commits into from

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Feb 8, 2021

Draft of support for analyzers integrated into FCS, see tooling RFC https://github.com/fsharp/fslang-design/blob/master/tooling/FST-1033-analyzers.md (which needs to be updated)

  • Inspired by https://github.com/ionide/FSharp.Analyzers.SDK#fsharpanalyzerssdk and (obviously) Rosyln analyzers

  • Relies on FSharp.Compiler.Service binary compatibility

  • Analyzers specified by "/compilertool:....analyzer.dll" command line flag, automatically fed in via compiler tools in nuget packages

  • Analyzers loaded via reflection into whatever is hosting FSharp.Compiler.Service.dll

  • Analyzers provide diagnostics and tooltips and code fixes like https://github.com/ionide/FSharp.Analyzers.SDK#fsharpanalyzerssdk

  • Analyzers are run as a separate FCS call after CheckFileInProject in the process of collecting diagnostics.

  • Analyzers also run by F# compiler

  • Analyzers not run by F# Interactive

  • Analyzers get access to the FSharpParseFileResults and FSharpCheckFileResults

  • Analyzers accept cancellation tokens

  • Analyzers can request access to full implementation details (FSharpExpr etc.) in the check results they are given. Some need this. (Note: Storing full implementation details is turned on for a project automatically if any analyzers request it, but there is a memory cost associated with this - so using any one analyzer that requests this pumps up memory costs.)

Feedback welcome, I'll update the RFC once I've tested this actually does something useful.

TODOS

  • Test and use it with DiffSharp shape checking

  • Test and use it with FSharpLint or others

  • Analyzer diagnostics are currently collected in the same activity (DocumentDiagnosticAnalyzer) as parsing and type checking.
    They should be collected using a separate Roslyn diagnostic analyzer (perhaps even one for each analyzer present) so that parsing and type checking diagnostics are never delayed when waiting on analyser diagnostics

  • Currently analyzers have no way to persist results between invocations (except leaky mutable state held in the analyzer objects). The Roslyn way to do this is to allow analyzers to register artifacts associated with one analysis for reuse in later analysis. We will likely need this.

  • Code fixes are not yet hooked up

  • Analyzers should be netstandard only

Limitations:

  • Analyzer quick info is collected when preparing any quick info at all. This is probably ok - asynchronously updating the quick info as each analyzer reports could be painful, and relatively few analyzers will want to report quick info beyond diagnostics.

  • Project-wide analyzers are not included and it's not clear we need to support it in an initial version, and we've had trouble hooking up project-wide analysis into Roslyn in any case

Example

There's an example TestAnalyzer in the PR. e.g.

C:\GitHub\dsyme\fsharp>artifacts\bin\fsc\Debug\net472\fsc --compilertool:C:\GitHub\dsyme\fsharp\artifacts\bin\TestAnalyzer\Release\net472\TestAnalyzer.dll a.fs
Microsoft (R) F# Compiler version 11.3.2.0 for F# 5.0
Copyright (c) Microsoft Corporation. All Rights Reserved.

a.fs(6,1): warning FS0045: this diagnostic is always on line 6 until the magic word WIBBLE appears

a.fs(3,1): warning FS0045: this diagnostic is always on line 3 until the magic word WAZZAM appears

C:\GitHub\dsyme\fsharp>code a.fs (edit to add // WIBBLE 

C:\GitHub\dsyme\fsharp>artifacts\bin\fsc\Debug\net472\fsc --compilertool:C:\GitHub\dsyme\fsharp\artifacts\bin\TestAnalyzer\Release\net472\TestAnalyzer.dll a.fs
Microsoft (R) F# Compiler version 11.3.2.0 for F# 5.0
Copyright (c) Microsoft Corporation. All Rights Reserved.

a.fs(3,1): warning FS0045: this diagnostic is always on line 3 until the magic word WAZZAM appears

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>net472</TargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

a key part of the roslyn analyzer business is they are netstandard only. it's worth requiring here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure yup makes sense

@dsyme
Copy link
Contributor Author

dsyme commented Jun 24, 2021

Failures after integrating main

2021-06-24T17:57:48.3332842Z   Failed Regression.ModulesFromExternalLibrariesBug5785 [1 s]
2021-06-24T17:59:02.1400907Z   Failed Regression.NoError.Timestamps.Bug3368b [1 s]

@dsyme dsyme mentioned this pull request Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants