Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

Configure "yes to all" per linter #516

Merged
merged 6 commits into from
Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Linters/AsyncFunctionAndMethodLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ use namespace HH\Lib\Str;
class AsyncFunctionAndMethodLinter extends FunctionNamingLinter {
const type TConfig = shape();

/**
* Does not update use sites, so this fix is only safe on
* names that you just added moments ago.
* Existing names should be updated with a migration.
*/
use UnsafeBulkAutoFixesTrait;

<<__Override>>
final public function getSuggestedNameForFunction(
string $name,
Expand Down
15 changes: 15 additions & 0 deletions src/Linters/AutoFixable.hack
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* Copyright (c) 2017-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
*/

namespace Facebook\HHAST;

<<__Sealed(AutoFixingLinter::class)>>
interface AutoFixable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AutoFixingLinter takes a generic, which means UnsafeBulkAutoFixes
can't require implements AutoFixingLinter<Terror> without being generic itself.
It doesn't use the generic either, so this small marker interface is used instead.

public function areBulkAutoFixesSafe(): bool;
}
2 changes: 1 addition & 1 deletion src/Linters/AutoFixingLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace Facebook\HHAST;
use type Facebook\HHAST\File;
use namespace Facebook\HHAST\__Private\LSP;

interface AutoFixingLinter<Terror as SingleRuleLintError> {
interface AutoFixingLinter<Terror as SingleRuleLintError> extends AutoFixable {
require extends SingleRuleLinter;

<<__Override>>
Expand Down
6 changes: 6 additions & 0 deletions src/Linters/AutoFixingLinterTrait.hack
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,10 @@ trait AutoFixingLinterTrait<Terror as SingleRuleLintError>
);
}

/**
* Explicitly non-final, expected to be overridden.
*/
public function areBulkAutoFixesSafe(): bool {
return true;
}
}
7 changes: 7 additions & 0 deletions src/Linters/CamelCasedMethodsUnderscoredFunctionsLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ use namespace HH\Lib\{C, Str};
class CamelCasedMethodsUnderscoredFunctionsLinter extends FunctionNamingLinter {
const type TConfig = shape();

/**
* Does not update use sites, so this fix is only safe on
* names that you just added moments ago.
* Existing names should be updated with a migration.
*/
use UnsafeBulkAutoFixesTrait;

<<__Override>>
final public function getSuggestedNameForFunction(
string $name,
Expand Down
15 changes: 15 additions & 0 deletions src/Linters/ConsistentLineEndingsLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,21 @@ final class ConsistentLineEndingsLinter
// Could become configurable later.
const type TConfig = shape('line ending' => LineEnding);

/**
* This linter may change the behavior of the code.
* A multiline string literal may contain a carriage return at eol.
* ```
* <<<'EOF'
*abc<carriage return>
*EOF
* ```
* Hhast will remove the trailing `\r` character.
* This changes the contents of the string.
* The programmer running the linter should make sure
* the suggested fix does not alter string literals.
*/
use UnsafeBulkAutoFixesTrait;

private function getLineEnding(): LineEnding {
return LineEnding::UNIX;
}
Expand Down
7 changes: 7 additions & 0 deletions src/Linters/DontHaveTwoEmptyLinesInARowLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ final class DontHaveTwoEmptyLinesInARowLinter extends AutoFixingASTLinter {
const type TContext = Script;
const type TNode = Token;

/**
* Unlike NoWhitespaceAtEndOfLineLinter and ConsistentLineEndingLinter
* this linter operates on the AST.
* It will therefore not change the contents of string literals.
*/
use SafeBulkAutoFixesTrait;

<<__Override>>
public function getLintErrorForNode(
this::TContext $context,
Expand Down
3 changes: 1 addition & 2 deletions src/Linters/DontUseAsioJoinLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Facebook\HHAST;

use namespace HH\Lib\{Str, Vec};

final class DontUseAsioJoinLinter extends AutoFixingASTLinter {
final class DontUseAsioJoinLinter extends ASTLinter {
const type TConfig = shape();
const type TContext = Script;
const type TNode = FunctionCallExpression;
Expand Down Expand Up @@ -49,7 +49,6 @@ final class DontUseAsioJoinLinter extends AutoFixingASTLinter {
$this,
"Don't use ".static::F_JOIN,
$node,
() ==> null,
);
}
}
1 change: 0 additions & 1 deletion src/Linters/Linter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,4 @@ interface Linter {
public function isLinterSuppressedForFile(): bool;

public function getFile(): File;

}
1 change: 0 additions & 1 deletion src/Linters/LinterTrait.hack
Original file line number Diff line number Diff line change
Expand Up @@ -69,5 +69,4 @@ trait LinterTrait {
public function isLinterSuppressedForFile(): bool {
return $this->isSuppressedForFile($this->getFile());
}

}
14 changes: 10 additions & 4 deletions src/Linters/NoEmptyStatementsLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ final class NoEmptyStatementsLinter extends AutoFixingASTLinter {
const type TConfig = shape();
const type TNode = ExpressionStatement;

/**
* This linter finds serious logic bugs.
* Each case should be considered individually
* since the intent of the programmer probably differed from
* what the code does right now.
*/
use UnsafeBulkAutoFixesTrait;

<<__Override>>
public function getTitleForFix(SingleRuleLintError $_): string {
return 'Remove statement';
Expand Down Expand Up @@ -77,10 +85,8 @@ final class NoEmptyStatementsLinter extends AutoFixingASTLinter {

// This could cause a TypeError still,
// if it does, add more patches.
return NodeList::concat(
$semicolon->getLeading(),
$semicolon->getTrailing(),
);
return
NodeList::concat($semicolon->getLeading(), $semicolon->getTrailing());
}

/**
Expand Down
15 changes: 11 additions & 4 deletions src/Linters/NoPHPEqualityLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ final class NoPHPEqualityLinter extends AutoFixingASTLinter {
const type TConfig = shape();
const type TNode = BinaryExpression;

/**
* Changing `==` to `===` and `!=` to `!==` may change
* the behavior of the code.
* `keyset[1, 2] == keyset[2, 1]` is true, but
* `keyset[1, 2] === keyset[2, 1]` is false.
* Each case should be considered individually.
*/
use UnsafeBulkAutoFixesTrait;

<<__Override>>
protected function getTitleForFix(ASTLintError $err): string {
$blame = $err->getBlameNode() as this::TNode;
Expand Down Expand Up @@ -56,10 +65,8 @@ final class NoPHPEqualityLinter extends AutoFixingASTLinter {
if ($op is EqualEqualToken) {
$op = new EqualEqualEqualToken($op->getLeading(), $op->getTrailing());
} else if ($op is ExclamationEqualToken) {
$op = new ExclamationEqualEqualToken(
$op->getLeading(),
$op->getTrailing(),
);
$op =
new ExclamationEqualEqualToken($op->getLeading(), $op->getTrailing());
} else {
invariant_violation("Shouldn't be asked to fix non-equality operators");
}
Expand Down
15 changes: 15 additions & 0 deletions src/Linters/NoWhitespaceAtEndOfLineLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,21 @@ final class NoWhitespaceAtEndOfLineLinter
extends AutoFixingLineLinter<LineLintError> {
const type TConfig = shape();

/**
* This linter may change the behavior of the code.
* A multiline string literal may contain whitespace at eol.
* ```
* <<<'EOF'
*abc<space>
*EOF
* ```
* Hhast will remove the trailing space character.
* This changes the contents of the string.
* The programmer running the linter should make sure
* the suggested fix does not alter string literals.
*/
use UnsafeBulkAutoFixesTrait;

<<__Override>>
public function getTitleForFix(LineLintError $_): string {
return 'Remove trailing whitespace';
Expand Down
17 changes: 14 additions & 3 deletions src/Linters/PreferRequireOnceLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,18 @@ final class PreferRequireOnceLinter extends AutoFixingASTLinter {
const type TContext = Script;
const type TNode = InclusionExpression;

/**
* Changing `include`, `include_once`, or `require`
* to `require_once` may change the behavior of the program.
* If `include_once`-like don't error behavior is expected,
* use `if (HH\could_include($f)) require_once $f; else your_error_here($f);`.
*
* Including a file multiple times doesn't make sense in Hack,
* it will either introduce 0 named entities, or error,
* since you are redeclaring a named entity.
*/
use UnsafeBulkAutoFixesTrait;

<<__Override>>
public function getLintErrorForNode(
Script $_context,
Expand All @@ -34,9 +46,8 @@ final class PreferRequireOnceLinter extends AutoFixingASTLinter {
public function getFixedNode(InclusionExpression $node): ?Node {
$left_trivia = $node->getRequire()->getLeading();
$right_trivia = $node->getRequire()->getTrailing();
return $node->withRequire(
new Require_onceToken($left_trivia, $right_trivia),
);
return
$node->withRequire(new Require_onceToken($left_trivia, $right_trivia));
}

<<__Override>>
Expand Down
27 changes: 27 additions & 0 deletions src/Linters/SafeBulkAutoFixesTrait.hack
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright (c) 2017-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
*/

namespace Facebook\HHAST;

/**
* This trait doesn't "do" anything when overriding
* `AutoFixingLinterTrait::allowYesToAll()`, since both
* implementations return true.
*
* This trait communicates to your readers (and future self).
* Yes, I have considered if this autofix is safe.
* It didn't "forget" to add LinterDisallowingYesToAllTrait.
*/
trait SafeBulkAutoFixesTrait {
require implements AutoFixable;

final public function allowYesToAll(): bool {
return true;
}
}
13 changes: 9 additions & 4 deletions src/Linters/ShoutCaseEnumMembersLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ final class ShoutCaseEnumMembersLinter extends AutoFixingASTLinter {
const type TContext = EnumDeclaration;
const type TNode = Enumerator;

/**
* Does not update use sites, so this fix is only safe on
* enum members that you just added moments ago.
* Existing enums should be updated with a migration.
*/
use UnsafeBulkAutoFixesTrait;

<<__Override>>
public function getLintErrorForNode(
self::TContext $enum,
Expand Down Expand Up @@ -80,10 +87,8 @@ but the autogenerated name collided, so no autofix is suggested.',
$old_names = Vec\partition($old_names, $name ==> $this->isShoutCase($name))
|> Vec\concat($$[1], $$[0]);

return Dict\from_values(
$old_names,
$name ==> $this->transformToShoutCase($name),
)
return
Dict\from_values($old_names, $name ==> $this->transformToShoutCase($name))
|> Dict\flip($$);
}

Expand Down
18 changes: 18 additions & 0 deletions src/Linters/UnsafeBulkAutoFixesTrait.hack
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* Copyright (c) 2017-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
*/

namespace Facebook\HHAST;

trait UnsafeBulkAutoFixesTrait {
require implements AutoFixable;

final public function allowYesToAll(): bool {
return false;
}
}
12 changes: 12 additions & 0 deletions src/Linters/UnusedLambdaParameterLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@ final class UnusedLambdaParameterLinter extends AutoFixingASTLinter {

use __Private\SharedCodeForUnusedParameterLinters;

/**
* This linter changes the parameter `$v` to `$_v`.
* This change is unsafe when `$_v` was previously auto captured.
* ```
* $_v = 'capture me';
* return Vec\filter_with_key($dict, ($k, $v) ==> $k === $_v);
* ```
* If the autofix rewrites the `$v` parameter, this will compare
* the keys of `$dict` to the values, not to the string 'capture me'.
*/
use UnsafeBulkAutoFixesTrait;

<<__Override>>
public function getLintErrorForNode(
this::TContext $lambda,
Expand Down
13 changes: 13 additions & 0 deletions src/Linters/UnusedParameterLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,19 @@ final class UnusedParameterLinter extends AutoFixingASTLinter {

use __Private\SharedCodeForUnusedParameterLinters;

/**
* Unlike UnusedLambdaParameterLinter, this autofix is safe.
* This method was added and returns true (the default)
* to show that it was not forgotten and carefully considered.
* Changing `$v` to `$_v` in a parameter is always safe.
* If `$_v` was already used in the body, it must have been
* unconditionally assigned before every use, else it would
* have been invalid Hack.
* This means that the parameter value can not be observed,
* nor change the meaning of existing code.
*/
use SafeBulkAutoFixesTrait;

<<__Override>>
public function getLintErrorForNode(
this::TContext $functionish,
Expand Down
16 changes: 14 additions & 2 deletions src/Linters/UnusedVariableLinter.hack
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,18 @@ final class UnusedVariableLinter extends AutoFixingASTLinter {
const type TConfig = shape();
const type TNode = VariableExpression;
const type TContext = IFunctionishDeclaration;
/**
* The autofix may change the behavior of existing code.
* It changes the name of `$a` in this example to `$_a`.
* This makes the return statement return `3`, instead of `4`.
* ```
* $_a = 4;
* list($a) = tuple(3);
* return $_a;
* ```
* Each autofix should be individually checked.
*/
use UnsafeBulkAutoFixesTrait;

<<__Override>>
public function getLintErrorForNode(
Expand Down Expand Up @@ -111,8 +123,8 @@ final class UnusedVariableLinter extends AutoFixingASTLinter {

private static function isByRefParam(ParameterDeclaration $param): bool {
$name = $param->getName();
return $name is DecoratedExpression &&
$name->getDecorator() is AmpersandToken;
return
$name is DecoratedExpression && $name->getDecorator() is AmpersandToken;
}

private static function getParamName(ParameterDeclaration $param): string {
Expand Down
Loading