How do we group linters?

[lexidor]

Related to: Versioning of named linter groups ('builtinLinters' option)

This discussion is both about the implementation details of how groups should work at a code level, as well as a discussion about which groups would be appropriate to split the built-in linters into.

Important design considerations:

• It should not be possible for a third-party to add itself to a built-in linter group. I don't expect that install evil-corp/yaml-parser can add an evil linter to my hhast runs. This would allow evil-corp to run arbitrary shell commands on my machine without my consent.
• The mechanism should be both forwards compatible. It should be possible for a consumer of hhast to mention a group that does not exist in the current version. This is required for libraries that support a large range of hhvm versions.
• There should be a way to communicate a version for a group as well. A constraint like "whitespace-linters": <1.4 is a useful constraint.

[fredemmott]

We currently have 'none', 'default', and 'all'.

IMO:

• default is too subjective. What belongs here?
• 'all' is based under the assumption that 'all' is a reasonable thing to want: i.e. we should never take two contradictory style linters.
• groups should also indicate likely differences in how users want to upgrade, especially combined with Versioning of named linter groups ('builtinLinters' option) #425

Concretely, I suggest:

• 'none': as-is; for projects that only want an explicit list, or are just starting to migrate an existing code base to be linted
• 'errors': actually incorrect code, e.g. 'this condition is always false'
• 'smells': not necessarily a bug, but... fishy. For example, 'dontawaitinaloop'
• 'fb-style' or 'hhast-style': this is pretty close to what 'default' actually means, but is honest about it.
• third-party projects should be able to define additional groups that can be used in the same way as built-in ones

'none' might not make sense depending on what we end up doing (or not doing) for #423

[fredemmott]

I think we need another one: 'deprecations' - stuff that will be an error in the future. For example, the 'always use strict mode' linter.

[Atry]

(Copied from #425 (comment))

I think we should separate policy from mechanism; separate interfaces from engines. Essentially, the linter engine requires the following function:

type ErrorCode = int | classname<SingleRuleLinter>;
function shouldReportErrorFromFile(ErrorCode $error_code, string $path): bool;

And we can provide some policy functions to group error codes:

function isCodeSmell(ErrorCode $error_code): bool;
function isPHPFlavored(ErrorCode $error_code): bool;
function isFacebookFlavored(ErrorCode $error_code): bool;

If we could allow users to create the isErrorCodeForFile in Hack, it should be some simple combinations of bool operators and regex matching against the path.

[fredemmott]

type ErrorCode = int | classname<SingleRuleLinter>

This is the current state, but I think we should anticipate requiring more

Essentially, the linter engine requires the following function:

In terms of observable results, yep - but practically, we do also need shouldRunLinterImplementation(classname<T>, $file): bool, regardless of error code. I'm not suggesting that as an actual API, but we do need a way to customize the answer.

The linters are as fast as we can reasonably make them, but still pretty slow. If a user doesn't want error code Foo, we shouldn't run FooLinter. HHClientLinter makes this more complicated though.

[Atry]

In terms of observable results, yep - but practically, we do also need shouldRunLinterImplementation(classname<T>, $file): bool, regardless of error code. I'm not suggesting that as an actual API, but we do need a way to customize the answer.

The linters are as fast as we can reasonably make them, but still pretty slow. If a user doesn't want error code Foo, we shouldn't run FooLinter. HHClientLinter makes this more complicated though.

That's exactly why the signature of the proposed function shouldReportErrorFromFile is function (ErrorCode, string): bool, not function (LintError): bool, because it's easy to derive shouldRunLinterImplementation from function (ErrorCode, string): bool.

[fredemmott]

because it's easy to derive shouldRunLinterImplementation from function (ErrorCode, string): bool.

This was true, but I don't think it is any more, and we should assume the problem will get more complex over time.

If this is the only interface, the only way to know if HHClientLinter should be ran is to run function(ErrorCode, string) 1000 times, for every possible value between 5000 and 5999

This would also require that the engine be aware of how HHClientLinter is special, and if anything similar is added in the future, it will also need to be aware of them.