Feature: Passing options through the context to named analyzers

[Zaid-Ajaj]

Is your feature request related to a problem? Please describe.
The end user does not have any way to control the behavior the used analyzer within a project. Just like with code linters, it would great if the user could provide options for the analyzers on a per-project basis. One use-case would be to control the strictness of an analyzer and changing the severity of certain errors from compile errors into warning.

Describe the solution you'd like
For this to work, the SDK must have a way to identify the used analyzers. One way to do that is by giving analyzers an optional name from the Analyzer attribute:

[<Analyzer "MyAnalyzerName">]

Then with the help of ionide, the SDK would pick up a configuration file called fsharp-analyzers.json that has the following structure:

{
  "MyAnalyzerName": {
     "Mode": "Strict",
     "ErrorsAsWarnings": "true" 
  }, 

  "OtherAnalyzer": { 
     "ErrorsAsWarnings": "true" 
   }
}

The SDK can then read this JSON file and parse the options as Map<string, string> per analyzer where this Map<string, string> would be available through the context input of the analyzer. For example, for the "MyAnalyzerName", the options would be

Map.ofList [ "Mode", "Strict"; "ErrorsAsWarnings", "true" ]

Describe alternatives you've considered
N/A

Additional context
N/A

[cmeeren]

I am experimenting with implementing an F# linter. This is an absolute must-have for me if I am to implement it using FSharp.Analyzers.SDK (a decision that also depends on some other aspects).

I think that ideally, configs should be searched for in the current analyzed file's directory and all parent directories, with deeper levels overriding settings from higher levels. Perhaps Microsoft.Extensions.Configuration can be used for a lot of the boilerplate logic here (it's what's used for appsettings.json in e.g. ASP.NET Core projects, but is not dependent on ASP.NET Core). Note that just like .editorconfig, the config should support a top-level isRoot flag where no higher-level files are considered (to avoid your linter settings being inadvertently modified by configs outside your repo root).

Note that depending on how an analyzer library implements analyzers, the analyzers may need to be namespaced using e.g. the analyzer assembly name in the config and elsewhere, to avoid conflicts.

As a side note, I would also love if YAML was supported as I find that easier to write and read, but this may of course require more logic (if nothing else, at least an initial translation step from YAML to JSON using some library).

[cmeeren]

Note that some things should be clarified regarding analyzer names. For example, I'm thinking that my linter, Flinter, would have one analyzer per rule. It could also end up with very many rules/analyzers. In order to avoid collisions, should the names be e.g. Flinter:FunctionParameterNaming?

Note that if using this prefix as a config key with Microsoft.Extensions.Configuration, both of the following would actually be supported (entirely up to the user):

{
  "Flinter:FunctionParameterNaming": {
    "Rule": "camelCase"
  }
}

{
  "Flinter": {
    "FunctionParameterNaming": {
      "Rule": "camelCase"
    }
  }
}

Which is nice.

[cmeeren]

Note that according to the "Setting config values" section in #47 (comment), which describes overriding config values per scope/file, analyzers must pass a range in order to get a config.

[baronfel]

For prior art I'd look at Myriad and how it handles options, but this all sound reasonable enough. I would suggest adding a comment about analyzer configuration to the RFC for analyzers (https://github.com/fsharp/fslang-design/blob/main/tooling/FST-1033-analyzers.md), since that's not currently taken into account at all.

Also consider using .editorconfig - the existing C# analyzer system controls all analyzer configuration through that file (which is already hierarchical), and having the same control mechanism would be easier for mixed solutions.

[cmeeren]

I added a comment to the RFC discussion: fsharp/fslang-design#508 (comment)

Regarding editorconfig: Good point. Fantomas uses it. But would it be flexible enough? It can only set a flat list of keys and values. Each analyzer would have several standard keys that should be handled by the SDK for enabling/disabling and setting the level (info/warning/error), in addition to whatever it needs itself. I'm sure we could come up with a naming scheme that handles this automatically, but I'm not convinced it's the best solution. (Not saying it isn't, though.)

[cmeeren]

I will look more into the .editorconfig solution and how Roslyn uses it. If it could work for us, I think it's a good idea to lean on prior art and do something similar.

In any case, I thought of something more that configs should ideally support: The ability to enable a predefined set of analyzers. For example, in Flinter, many rules would apply to all F# projects and would be enabled by default, but there would be many that only apply to F# libraries, and still more that only apply to libraries intended to be consumed from other .NET languages. Ideally, users should be able to enable (per project/folder, e.g. using .editorconfig) all rules that are relevant for e.g. F# libraries. Then, if Flinter is updated with more rules that belong to that category, they would be automatically enabled when the user updates.

[cmeeren]

For completeness, an alternative to categories (which, by the way, Roslyn analyzers also seem to support), would be to split the analyzers into several NuGet packages. So one package for generally applicable rules, one for rules that are applicable to F# libraries, and so on. These could then be installed into different projects. (This assumes that analyzers only apply to the projects they are installed into, as mentioned in #32 (comment).) I'm not sure I like that approach; it seems a bit heavy-weight and rigid. Also, it prevents one rule from belonging to multiple categories, which might be useful (though I don't have a specific use-case in mind right now).

[cmeeren]

For the record, Roslyn uses .editorconfig like this:

[*.cs]
dotnet_diagnostic.CA1822.severity = error
dotnet_analyzer_diagnostic.category-performance.severity = warning
dotnet_analyzer_diagnostic.severity = suggestion

• You can configure the severity of a specific rule, a category, or all rules. If there are conflicts, they take precedence in that order. Note that multiple-rule configurations (category/all) only apply to rules that are enabled by default.
• It uses the error code, e.g. CA1822. I could go that way, but for reasons mentioned in Allow disabling analyzers for line/file/etc. #47 (comment), I like descriptive names better. If I see dotnet_diagnostic.CA1822.severity (or a similar inline rule suppression as described in Allow disabling analyzers for line/file/etc. #47), I have no idea what that rule does without looking it up. However, should we support one of them, or both? It's always a bit iffy when something has two "public" IDs. Error codes are great for searching, but when used in configs, would require you to search to see what it is.
• We could easily support custom analyzer config values, e.g. fsharp_analyzer.Flinter:CyclomaticComplexity.limit = 7. We would need to reserve severity and potentially other configs handled by the SDK (I can't think of any, unless we need enabled, but that can easily be handled by severity as described in Use same severity levels as Roslyn analyzers #59). Alternatively, we can use a different prefix for SDK options (like severity) and rule options. Roslyn analyzers seem to do so. For example, Code quality uses dotnet_code_quality, and there are also examples of completely custom names without prefixes. I'm not sure how relevant that is to us.

Just like Roslyn analyzers, we should support marking files as generated code and suppressing all messages for those files:

[*.MyGenerated.cs]
generated_code = true

Note that configuration common to all analyzers, such as severity and generated code, should be handled by the SDK, not the individual analyzers.

---

On another note, I think that the SDK, if possible, should respect <TreatWarningsAsErrors> in project files. Ideally also <WarnOn> and <NoWarn>.

I notice that Roslyn analyzers do this, and have a separate <CodeAnalysisTreatWarningsAsErrors> that can be set to false to override the normal <TreatWarningsAsErrors> for analysis messages. We could consider using that (or something similar).

---

Regarding what I said in a previous comment:

Note that according to the "Setting config values" section in #47 (comment), which describes overriding config values per scope/file, analyzers must pass a range in order to get a config.

You must of course also pass a full path to a file, not just a range (unless the range contains the full path).

Furthermore, this is also needed for any kind of non-global configuration like .editorconfig, where the path of the file is relevant for the effective config that applies to it.