hhast
hhast copied to clipboard
hhvm
Add an optional lintMarkerAllowList
Asking for feedback for HHClientLinter. This linter is special, because it is an amalgamation of hundreds of rules. The option "allow comments for HHClientLinter" could not be a simple binary toggle. I have added the lintMarkerAllowList option to HHClientLinter to allow for granular tuning.
I completely ignore the global lintMarkerAllowList in HHClientLinter. So when "lintMarkerAllowList": ["UnusedUseClause"] is configured HHClientLinter will still honor its own suppression comments.
I did this, because of the following confusing example.
{
"roots": ["src/"],
"lintMarkerAllowList": [],
"linterConfigs": {
"HHClientLinter": {
"lintMarkerAllowList": [5555]
}
}
}
This is conflicting, because the global list says, no markers allowed, but HHClientLinter allows marker 5555. I would expect HHClientLinter to respect its own config here. But this means, if you want to disallow all markers, you need to set two empty lists. One for HHClientLinter and one at the global level.
I also considered to error out when HHClientLinter receives a lintMarkerAllowList in its own config when shouldAllowSuppressionComments() is false. But this requires you to repeat yourself. First add HHClientLinter linter to the global list and then fine-tune the HHClientLinter config.
Which of these two would be right for the hhast project? Which would be the least surprising?
Do we really need the lintMarkerAllowList setting for HHClientLinter if there is such the lintMarkerAllowList global setting?
Do we really need the
lintMarkerAllowListsetting forHHClientLinterif there is such thelintMarkerAllowListglobal setting?
The global list contains only classnames. HHClientLinter has many sublinters:
- SketchyNullCheckLinter
- BooleanExpressionAlwaysFalseLinter
- DontUseTheCloneOperatorLinter
- DontUseSafeMemberAccessOnNonnullLinter
- DontImplicitlyConvertStringToBoolLinter
These are not classnames. The sublinters of HHClientLinter are unnamed. They can't be included in the global list.
There is no "sublinter" concept in HHAST. A marker is for a lint error code, whose name could be either a class name without the -Linter suffix or an integer code. Given the setting name lintMarkerAllowList, I would expect it contains both class names without the -Linter suffix and integer codes.
Given that the proposed setting is for markers, not linters, it would be straightforward to evaluate the allow list when parsing markers globally, instead of implementing the feature in each linter.
- Parsing line based markers at https://github.com/hhvm/hhast/blob/5bc52f7af5df17b65a513227483e56d75fa7cafc/src/File.hack#L42
- Parsing AST based markers at https://github.com/hhvm/hhast/blob/5bc52f7af5df17b65a513227483e56d75fa7cafc/src/Linters/suppress_ast_linter_error.hack#L19
There is no "sublinter" concept in HHAST. A marker is for a lint error code, whose name could be either a class name or an integer code. Given the setting name
lintMarkerAllowList, I would expect it contains both class names and integer codes.
That would be worth it. I'll type it as keyset<arraykey>.
arraykey might be better. I typed it as string because it was string long before we introduced HHClientLinter and changing the type would break existing code.
Given that the proposed setting is for markers, not linters, it would be straightforward to evaluate the allow list when parsing markers globally, instead of implementing the feature in each linter.
* Parsing line based markers at https://github.com/hhvm/hhast/blob/5bc52f7af5df17b65a513227483e56d75fa7cafc/src/File.hack#L42 * Parsing AST based markers at https://github.com/hhvm/hhast/blob/5bc52f7af5df17b65a513227483e56d75fa7cafc/src/Linters/suppress_ast_linter_error.hack#L19
I am assuming you are proposing to add a new function besides is_linter_error_suppressed that returns a tri-state.
enum SuppressionState : int {
NOT_SUPPRESSED = 0;
SUPPRESSED = 1;
DISALLOWED_SUPPRESSION = 2;
}
I could then reimplement the is_linter_error_suppressed as get_suppression_state($linter, $node, $root, /*is suppression allowed*/ true) === SuppressionState::SUPPRESSED.
I am not so sure about how I would change the signature of erroCodesToSuppress. Any suggestions?
I would create a new linter to raise lint errors for markers not in the allow list, and let all other linters simply ignore markers not in the allow list.
I would create a new linter to raise lint errors for markers not in the allow list, and let all other linters simply ignore markers not in the allow list.
That is a very interesting approach I had not considered before. That is a great idea! Thanks for your input. I will rework my PR.
If that new linter would be impossible to suppress, we'd already be done. That could be done by automatically adding DontUseSuppressionMarkersThatAreNotDeclaredInLintMarkerAllowListLinter to the lintMarkerAllowList, because that would cause and infinite suppression regression.
Then we barely have to change anything about suppression comment logic. We just need to defang HHAST_IGNORE_ALL for DontUseSuppressionMarkersThatAreNotDeclaredInLintMarkerAllowList.
// We just need to defang this IGNORE_ALL, since this would suppress itself.
/*HHAST_IGNORE_ALL[DontUseSuppressionMarkersThatAreNotDeclaredInLintMarkerAllowList]*/
// After that...
/*HHAST_FIXME[DontUseSuppressionMarkersThatAreNotDeclaredInLintMarkerAllowList]
Oh wait, I can't, since it is turtles all the way down. */
/*HHAST_FIXME[DontUseSuppressionMarkersThatAreNotDeclaredInLintMarkerAllowList]
Suppressing the don't use markers linter*/
/*HHAST_FIXME[DontUseSuppressionMarkersThatAreNotDeclaredInLintMarkerAllowList]
Suppressing the don't suppress MustUseBracesForControlFlow linter */
/*HHAST_FIXME[MustUseBracesForControlFlow]*/
if ($xyzzy) return;
Converting to draft, will redo using a new linter. DontUseSuppressionMarkersThatAreNotDeclaredInLintMarkerAllowListLinter is a placeholder name btw.