Add an optional lintMarkerAllowList

fixes hhvm#458

hhast-lint.json

@@ -4,6 +4,12 @@
   "extraLinters": [
     "Facebook\\HHAST\\HHClientLinter"
   ],
+  "lintMarkerAllowList": [
+    "Facebook\\HHAST\\DontUseAsioJoinLinter",
+    "Facebook\\HHAST\\DontAwaitInALoopLinter",
+    "Facebook\\HHAST\\PreferLambdasLinter",
+    "Facebook\\HHAST\\FinalOrAbstractClassLinter"
+  ],
   "overrides": [
     {
       "patterns": [ "codegen/*", "codegen-no-rebuild/*", "tests/examples/*" ],
@@ -32,6 +38,13 @@
         "Error",
         "Exception"
       ]
+    },
+    "Facebook\\HHAST\\HHClientLinter": {
+      "lintMarkerAllowList": [
+        5607,
+        5624,
+        5639
+      ]
     }
   }
 }

src/Linters/ASTLinter.hack

@@ -10,7 +10,7 @@
 namespace Facebook\HHAST;
 
 use namespace Facebook\HHAST\SuppressASTLinter;
-use namespace HH\Lib\Vec;
+use namespace HH\Lib\{Str, Vec};
 
 abstract class ASTLinter extends SingleRuleLinter {
   <<__Enforceable, __Reifiable>>
@@ -98,11 +98,18 @@ abstract class ASTLinter extends SingleRuleLinter {
         );
       }
 
-      if (
-        $error !== null &&
-        !SuppressASTLinter\is_linter_error_suppressed($this, $node, $ast)
-      ) {
-        $errors[] = $error;
+      if ($error is nonnull) {
+        if (!SuppressASTLinter\is_linter_error_suppressed($this, $node, $ast)) {
+          $errors[] = $error;
+        } else if (!$this->shouldAllowSuppressionComments()) {
+          $errors[] = $error->prefixDescription(
+            Str\format(
+              "You may not use a comment to suppress %s errors.\n".
+              "See lintFixmeAllowList in hhast-lint.json.\n",
+              $this->getLinterName(),
+            ),
+          );
+        }
       }
     }
     return $errors;

src/Linters/HHClientLintError.hack

@@ -68,4 +68,8 @@ final class HHClientLintError implements LintError {
     return new HHClientLintRule($this->error['code']);
   }
 
+  public function prefixDescription(string $description): this {
+    $this->error['descr'] = $description.$this->error['descr'];
+    return $this;
+  }
 }

src/Linters/HHClientLinter.hack

@@ -18,8 +18,13 @@ final class HHClientLinter implements Linter {
   use LinterTrait;
   use SuppressibleTrait;
 
-  const type TConfig =
-    shape(?'ignore_except' => vec<int>, ?'ignore' => vec<int>, ...);
+  const type TConfig = shape(
+    ?'ignore' => vec<int>,
+    ?'ignore_except' => vec<int>,
+    ?'ignoreExcept' => vec<int>,
+    ?'lintMarkerAllowList' => vec<this::TErrorCode>,
+    ...
+  );
 
   const type TErrorCode = int;
 
@@ -39,11 +44,20 @@ final class HHClientLinter implements Linter {
     5639 /* potential co(tra)variant marker, which is highly opinionated */,
   ];
 
+  private function mayErrorCodeBeSuppressedWithComments(
+    this::TErrorCode $code,
+  ): bool {
+    return $this->config is null ||
+      !Shapes::keyExists($this->config, 'lintMarkerAllowList') ||
+      C\contains($this->config['lintMarkerAllowList'], $code);
+  }
+
   <<__Memoize>>
   private function isErrorCodeConfiguredToIgnore(): (function(
     this::TErrorCode,
   ): bool) {
-    $ignore_except = $this->config['ignore_except'] ?? null;
+    $ignore_except =
+      $this->config['ignore_except'] ?? $this->config['ignoreExcept'] ?? null;
     $ignore = $this->config['ignore'] ?? null;
     if ($ignore is null) {
       if ($ignore_except is null) {
@@ -56,7 +70,7 @@ final class HHClientLinter implements Linter {
       return $error_code ==> C\contains($ignore, $error_code);
     }
     throw new \InvalidOperationException(
-      "Must not set both of the 'ignore_except' and 'ignore' fields in the configuration of HHClientLinter",
+      "Must not set both of the 'ignoreExcept' and 'ignore' fields in the configuration of HHClientLinter",
     );
   }
 
@@ -109,30 +123,46 @@ final class HHClientLinter implements Linter {
           $this::blameCode($file_lines, $error),
         ),
       )
-      |> Vec\filter($$, $error ==> {
+      |> Vec\map($$, $error ==> {
+        $error_code = (int)$error->getLintRule()->getErrorCode();
         if ($error->getLintRule()->isSuppressedForFile($this->file)) {
-          return false;
+          if ($this->mayErrorCodeBeSuppressedWithComments($error_code)) {
+            return null;
+          }
+          return $error->prefixDescription(Str\format(
+            "You may not use a comment to suppress %s(%d) errors.\n".
+            "See lintFixmeAllowList in linterConfigs in hhast-lint.json.\n",
+            $this->getLinterName(),
+            $error_code,
+          ));
         }
-        $error_code = (int)$error->getLintRule()->getErrorCode();
         if (C\contains(self::ALWAYS_IGNORE_ERRORS, $error_code)) {
-          return false;
+          return null;
         }
         if ($this->isErrorCodeConfiguredToIgnore()($error_code)) {
-          return false;
+          return null;
         }
         $range = $error->getRange();
         list(list($line_number, $_), $_) = $range;
         $previous_line_number = $line_number - 1;
-        if ($this->isSuppressedForLine($this->file, $previous_line_number)) {
-          return false;
-        }
-        if (
+
+        $is_suppressed =
+          $this->isSuppressedForLine($this->file, $previous_line_number) ||
           $error->getLintRule()
-            ->isSuppressedForLine($this->file, $previous_line_number)
-        ) {
-          return false;
+            ->isSuppressedForLine($this->file, $previous_line_number);
+
+        if (!$is_suppressed) {
+          return $error;
+        } else if (!$this->mayErrorCodeBeSuppressedWithComments($error_code)) {
+          return $error->prefixDescription(Str\format(
+            "You may not use a comment to suppress %s(%d) errors.\n".
+            'See lintFixmeAllowList in linterConfigs in hhast-lint.json.',
+            $this->getLinterName(),
+            $error_code,
+          ));
         }
-        return true;
-      });
+        return null;
+      })
+      |> Vec\filter_nulls($$);
   }
 }

src/Linters/LineLinter.hack

@@ -10,7 +10,7 @@
 namespace Facebook\HHAST;
 
 use type Facebook\HHAST\{SingleRuleLinter};
-use namespace HH\Lib\{C, Str, Vec};
+use namespace HH\Lib\{Dict, Str, Vec};
 
 abstract class LineLinter<+Terror as LineLintError> extends SingleRuleLinter {
 
@@ -26,25 +26,36 @@ abstract class LineLinter<+Terror as LineLintError> extends SingleRuleLinter {
 
   <<__Override>>
   public async function getLintErrorsAsync(): Awaitable<vec<Terror>> {
-    $lines = $this->getLinesFromFile();
-    $errs = vec[];
+    list($suppressed, $unconditional) = Dict\map_with_key(
+      $this->getLinesFromFile(),
+      ($ln, $line) ==> vec($this->getLintErrorsForLine($line, $ln)),
+    )
+      |> Dict\filter($$)
+      |> Dict\partition_with_key(
+        $$,
+        ($ln, $_errs) ==>
+          $ln > 0 && $this->isSuppressedForLine($this->getFile(), $ln),
+      );
 
-    foreach ($lines as $ln => $line) {
-      $line_errors = vec($this->getLintErrorsForLine($line, $ln));
-
-      if (C\is_empty($line_errors)) {
-        continue;
-      }
-
-      // We got an error. Let's check the previous line to see if it is marked as ignorable
-      if ($ln - 1 >= 0 && $this->isSuppressedForLine($this->getFile(), $ln)) {
-        continue;
-      }
-
-      $errs = Vec\concat($errs, $line_errors);
+    if ($this->shouldAllowSuppressionComments()) {
+      return Vec\flatten($unconditional);
     }
 
-    return $errs;
+    return Dict\map(
+      $suppressed,
+      $errs ==> Vec\concat(
+        vec[
+          $errs[0]->prefixDescription(Str\format(
+            "You may not use a comment to suppress %s errors.\n".
+            "See lintFixmeAllowList in hhast-lint.json.\n",
+            $this->getLinterName(),
+          )),
+        ],
+        $errs,
+      ),
+    )
+      |> Vec\concat($suppressed, $unconditional)
+      |> Vec\flatten($$);
   }
 
   /** Parse a single line of code and attempts to find lint errors */

src/Linters/LintError.hack

@@ -31,4 +31,6 @@ interface LintError {
   public function getPrettyBlame(): ?string;
 
   public function getLintRule(): LintRule;
+
+  public function prefixDescription(string $description): this;
 }

src/Linters/Linter.hack

@@ -24,10 +24,15 @@ interface Linter {
 
   public static function shouldLintFile(File $_): bool;
 
-  public static function newInstance(File $file, ?this::TConfig $config): this;
+  public static function newInstance(
+    File $file,
+    ?this::TConfig $config,
+    bool $should_allow_suppression_comments,
+  ): this;
 
   public function isLinterSuppressedForFile(): bool;
 
   public function getFile(): File;
 
+  public function shouldAllowSuppressionComments(): bool;
 }

src/Linters/LinterTrait.hack

@@ -22,14 +22,24 @@ trait LinterTrait {
 
   private File $file;
   private ?this::TConfig $config;
+  private bool $shouldAllowSuppressionComments;
 
-  private function __construct(File $file, ?this::TConfig $config) {
+  private function __construct(
+    File $file,
+    ?this::TConfig $config,
+    bool $should_allow_suppression_comments,
+  ) {
     $this->file = $file;
     $this->config = $config;
+    $this->shouldAllowSuppressionComments = $should_allow_suppression_comments;
   }
 
-  public static function newInstance(File $file, ?this::TConfig $config): this {
-    return new static($file, $config);
+  public static function newInstance(
+    File $file,
+    ?this::TConfig $config,
+    bool $should_allow_suppression_comments,
+  ): this {
+    return new static($file, $config, $should_allow_suppression_comments);
   }
 
   protected function getConfig(): ?this::TConfig {
@@ -44,7 +54,23 @@ trait LinterTrait {
     string $path,
     ?this::TConfig $config,
   ): this {
-    return new static(File::fromPath($path), $config);
+    return static::fromPathWithConfigAndShouldAllowSupressionComments(
+      $path,
+      $config,
+      true,
+    );
+  }
+
+  final public static function fromPathWithConfigAndShouldAllowSupressionComments(
+    string $path,
+    ?this::TConfig $config,
+    bool $should_allow_suppression_comments,
+  ): this {
+    return new static(
+      File::fromPath($path),
+      $config,
+      $should_allow_suppression_comments,
+    );
   }
 
   final public function getFile(): File {
@@ -70,4 +96,7 @@ trait LinterTrait {
     return $this->isSuppressedForFile($this->getFile());
   }
 
+  public function shouldAllowSuppressionComments(): bool {
+    return $this->shouldAllowSuppressionComments;
+  }
 }

src/Linters/SingleRuleLintError.hack

@@ -66,4 +66,8 @@ class SingleRuleLintError implements LintError {
     return $this->getLinter();
   }
 
+  final public function prefixDescription(string $description): this {
+    $this->description = $description.$this->description;
+    return $this;
+  }
 }

src/__Private/LintRun.hack

@@ -109,7 +109,14 @@ final class LintRun {
       return LintRunResult::NO_ERRORS;
     }
 
-    $linter = $linter::newInstance($file, $linter_config);
+    $lint_marker_allow_list = $config['lintMarkerAllowList'];
+    $linter = $linter::newInstance(
+      $file,
+      $linter_config,
+      $lint_marker_allow_list is nonnull
+        ? C\contains_key($lint_marker_allow_list, $linter)
+        : true,
+    );
 
     if ($linter->isLinterSuppressedForFile()) {
       return LintRunResult::NO_ERRORS;