Allow disabling analyzers for line/file/etc.

[cmeeren]

Sorry if I'm missing something obvious, but I can't find a way to disable an analyzer using e.g. a code comment.

I'm after something like what FSharpLint allows, like:

• // fsharplint:disable TypePrefixing Hints
• // fsharplint:disable-next-line TypePrefixing

Again, I'm coming at this from a linter perspective. This seems to me to be a crucial feature.

[cmeeren]

I'm willing to help implement this in FSharp.Analyzers.SDK.

Here are some design notes. Let me know what you think.

Mechanism

I suggest comments.

Roslyn uses #pragma disable, but we can't use #hash_directives in F# because they seem to only be valid on the top level.

I have also seen attributes being used for this, but those are only valid on select syntax elements and are therefore too limited.

Keyword

The comments should be prefixed with a keyword. We could use analyzer (e.g., analyzer disable ...), but I'd prefer something shorter. I like az (e.g., az disable ...) because it's short, easy to type and easy to remember (z has always struck me as a prominent character in the word "analyzer", as #24 kind of points out). Keeping it short has the benefit of allowing longer reasons (as described further below).

Disabling rules using analyzer names

Users should be able to disable rules using the analyzer name (or the message Type, whatever that's used for) or similar:

// az disable Flinter:FunctionParameterNaming

(The examples in this comment follows the analyzer name namespacing mentioned in #8 (comment)).

Disabling rules using codes?

I'm not sure whether users should be able to disable rules using its Code, e.g., az disable FLN0001. It's more descriptive to use the name, and I think we should encourage that. We should at least start with that I think, because it's easy to add later if we change our minds (but extremely breaking to take away if we first implement it and then change our minds).

Scopes/features

Disabling and enabling rules for the rest of the file

// az disable Flinter:FunctionParameterNaming
// az enable Flinter:FunctionParameterNaming

Disabling the next occurrence of a rule in a file

// az disable once Flinter:FunctionParameterNaming

Disabling a rule for the scope that starts on the next line

A scope is either a module, function, class, or method, depending on what comes after it. (A scope could also be a namespace, but I have never seen multiple namespaces in an F# source file, so disabling for the entire file has the same effect. Also, disabling and then enabling is a very simple workaround.)

// az disable scope Flinter:FunctionParameterNaming

Ignore everything after the analyzer name

This enables users to give reasons:

// az disable scope Flinter:CyclomaticComplexity because the many top-level match cases don't add actual complexity

Note that this prevents disabling multiple rules in a single comment. I think that is a good trade-off. (We could alternatively support it by separating rules with commas and no spaces.)

It also requires analyzer names to not contain spaces (or whatever we choose to split on, e.g., any word break \b in regex), which I also think makes sense.

Setting config values

This is a separate feature, but I wanted to mention it since the syntax can be similar:

// az set scope Flinter:FunctionParameterNaming.style "PascalCase" because our boss said so
// az set Flinter:CyclomaticComplexity.limit 20 because the many top-level match cases don't add actual complexity
// az restore Flinter:CyclomaticComplexity.limit

The first line sets for the following scope. The second line sets for the rest of the file. The third line restores the default (configured) value.

This would interpret the value as a JSON token and merge it into the config that the analyzer gets for the relevant range.

This of course requires analyzers to provide a range in order to get a config. This is a design decision for configs that should be there from the start, to avoid breaking changes.

(Strictly speaking it requires pos, not range, but the AST gives range everywhere and the SDK can easily just pick the start of the range as the position. It's simpler to do that once in the SDK than for every analyzer to call .Start on a range and possibly mess it up by calling .End instead.)

Note that this would not work with once, e.g., az set once Flinter:CyclomaticComplexity.limit 20 won't make sense.

Implementation

The SDK can scan each file for relevant comments and keep a list of where rules are enabled/disabled. The analyzers can be none the wiser and do their work. When the SDK gets the messages from the analyzers, it just filters them according to the Range of the Message.

In the future, we can consider giving analyzers access to disabled ranges, so that they can disable expensive analysis if a rule is disabled for a certain range. But I don't think that is likely to ever become a problem, so we shouldn't add it from the start.

[TheAngryByrd]

Yeah this is definitely a missing feature.

Just off the top of my head, if we wanted to use comments to disable sections easily, we may have to use Fantomas’s parser as it’s handling of trivia has become quite. Ice.

[cmeeren]

Isn't it simpler to just File.ReadAllLines and find the relevant lines? That gives us the line numbers and the comment contents, which is all we need. For scope, we then just search for our supported scopes (modules, functions, classes, and methods) in the AST, picking the one that starts on the line after the comment, so we know its range.

[baronfel]

Comment handling via the AST would be hard, because AST nodes are individually responsible for grabbing comments of all kinds. File.ReadAllLines would be a lot of bookkeeping, but might be the only way forward for the interim.

[cmeeren]

File.ReadAllLines would be a lot of bookkeeping, but might be the only way forward for the interim.

I agree about it being the way forward. I don't think it'll be too bad in terms of bookeeping.

[cmeeren]

In any case, do you agree with my design? Are you open for a PR with those features?

Note that since it depends on analyzer names, I think it would make sense if we require analyzer names (and remove Type, as described in #48).