Configure "yes to all" per linter (#516)

* Configure "yes to all" per linter
fixes #86
In order to make this change backwards compatible,
allowYesToAll() defaults to true.
Unsafe linters may override this to false.
I also considered a default of false,
but that would entail altering all safe linters (including third-party).
Unsafe linters may need to suppress `must use override` linter
if they wish to remain compatible with previous versions of hhast.

* Add allowYesToAll() to unsafe linters
Some linters look like they might be unsafe, but aren't on close inspection.
I added allowYesToAll(): bool { return true; } on those.
I checked every autofixing linter.
If I could not find a bad autofix, I left the linter untouched.

* Revert allowYesToAll to true

I changed this during testing and forgot to flip it back.

* Correct mistaken variable name in comment

The example first used `$a` as a meta syntactic variable.
I later changed to `$k` and `$v` for key and value.
I didn't update every use of `$_a`.

* Move allowYesToAll() down the type hierarchy
Suggested in a code review.
This only makes sense for autofixing linters.

* Rename allowYesToAll() to areBulkAutoFixesSafe()

Author: lexidor

src/Linters/AsyncFunctionAndMethodLinter.hack

@@ -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,

src/Linters/AutoFixable.hack

@@ -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 {
+  public function areBulkAutoFixesSafe(): bool;
+}

src/Linters/AutoFixingLinter.hack

@@ -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>>

src/Linters/AutoFixingLinterTrait.hack

@@ -40,4 +40,10 @@ trait AutoFixingLinterTrait<Terror as SingleRuleLintError>
     );
   }
 
+  /**
+   * Explicitly non-final, expected to be overridden.
+   */
+  public function areBulkAutoFixesSafe(): bool {
+    return true;
+  }
 }

src/Linters/CamelCasedMethodsUnderscoredFunctionsLinter.hack

@@ -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,

src/Linters/ConsistentLineEndingsLinter.hack

@@ -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;
   }

src/Linters/DontHaveTwoEmptyLinesInARowLinter.hack

@@ -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,

src/Linters/DontUseAsioJoinLinter.hack

@@ -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;
@@ -49,7 +49,6 @@ final class DontUseAsioJoinLinter extends AutoFixingASTLinter {
       $this,
       "Don't use ".static::F_JOIN,
       $node,
-      () ==> null,
     );
   }
 }

src/Linters/Linter.hack

@@ -29,5 +29,4 @@ interface Linter {
   public function isLinterSuppressedForFile(): bool;
 
   public function getFile(): File;
-
 }

src/Linters/LinterTrait.hack

@@ -69,5 +69,4 @@ trait LinterTrait {
   public function isLinterSuppressedForFile(): bool {
     return $this->isSuppressedForFile($this->getFile());
   }
-
 }

src/Linters/NoEmptyStatementsLinter.hack

@@ -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';
@@ -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());
   }
 
   /**

src/Linters/NoPHPEqualityLinter.hack

@@ -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;
@@ -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");
     }

src/Linters/NoWhitespaceAtEndOfLineLinter.hack

@@ -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';

src/Linters/PreferRequireOnceLinter.hack

@@ -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,
@@ -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>>

src/Linters/SafeBulkAutoFixesTrait.hack

@@ -0,0 +1,28 @@
+/*
+ *  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::areBulkAutoFixesSafe()`, 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 UnsafeBulkAutoFixesTrait.
+ */
+trait SafeBulkAutoFixesTrait {
+  require implements AutoFixable;
+
+  <<__Override>>
+  final public function areBulkAutoFixesSafe(): bool {
+    return true;
+  }
+}

src/Linters/ShoutCaseEnumMembersLinter.hack

@@ -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,
@@ -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($$);
   }
 

src/Linters/UnsafeBulkAutoFixesTrait.hack

@@ -0,0 +1,19 @@
+/*
+ *  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;
+
+  <<__Override>>
+  final public function areBulkAutoFixesSafe(): bool {
+    return false;
+  }
+}

src/Linters/UnusedLambdaParameterLinter.hack

@@ -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,

src/Linters/UnusedParameterLinter.hack

@@ -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,

src/Linters/UnusedVariableLinter.hack

@@ -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(
@@ -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 {