Add DontDiscardNewExpressionsLinter

hhast-lint.json

@@ -25,5 +25,13 @@
         "Facebook\\HHAST\\DataProviderTypesLinter"
       ]
     }
-  ]
+  ],
+  "linterConfigs": {
+    "Facebook\\HHAST\\DontDiscardNewExpressionsLinter": {
+      "exceptionSuffixes": [
+        "Error",
+        "Exception"
+      ]
+    }
+  }
 }

src/Linters/DontDiscardNewExpressionsLinter.hack

@@ -0,0 +1,69 @@
+/*
+ *  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;
+
+use namespace HH\Lib\{C, Str};
+
+final class DontDiscardNewExpressionsLinter extends ASTLinter {
+  const type TConfig = shape(?'exceptionSuffixes' => vec<string>);
+  const type TContext = Script;
+  const type TNode = ExpressionStatement;
+
+  const string BASE_MESSAGE = <<<'TEXT'
+You are discarding the result of a `new` expression.
+If you are intentionally discarding the newly created object,
+consider assigning it to `$_` to communicate with your readers.
+
+TEXT;
+
+  const string UNTHROWN_EXCEPTION =
+    'It looks like you are constructing an Exception. Did you intend to throw it?';
+
+  const string SIDE_EFFECTS_IN_CONSTRUCTOR_SMELL = <<<'TEXT'
+If you are running this constructor for its side-effects,
+consider restructuring that class / constructor.
+A constructor is meant for creating objects, not for causing effects.
+TEXT;
+
+  <<__Override>>
+  public function getLintErrorForNode(
+    Script $_context,
+    this::TNode $node,
+  ): ?ASTLintError {
+    $expression = $node->getExpression();
+    if (!$expression is ObjectCreationExpression) {
+      return null;
+    }
+
+    $message = static::BASE_MESSAGE.
+      (
+        $this->isConstructingAnException($expression)
+          ? static::UNTHROWN_EXCEPTION
+          : static::SIDE_EFFECTS_IN_CONSTRUCTOR_SMELL
+      );
+
+    return new ASTLintError($this, $message, $node);
+  }
+
+  private function isConstructingAnException(
+    ObjectCreationExpression $expression,
+  ): bool {
+    $cls = $expression->getObject()->getType()->getLastToken()?->getText();
+    return $cls is nonnull &&
+      C\any(
+        $this->getExceptionSuffixes(),
+        $suffix ==> Str\ends_with($cls, $suffix),
+      );
+  }
+
+  private function getExceptionSuffixes(): vec<string> {
+    return $this->getConfig()['exceptionSuffixes'] ?? vec['Exception'];
+  }
+}

src/__Private/LintRunConfig.hack

@@ -68,6 +68,7 @@ final class LintRunConfig {
     HHAST\AsyncFunctionAndMethodLinter::class,
     HHAST\CamelCasedMethodsUnderscoredFunctionsLinter::class,
     HHAST\DontAwaitInALoopLinter::class,
+    HHAST\DontDiscardNewExpressionsLinter::class,
     HHAST\LicenseHeaderLinter::class,
     HHAST\NewlineAtEndOfFileLinter::class,
     HHAST\MustUseBracesForControlFlowLinter::class,

tests/DontDiscardNewExpressionsLinterTest.hack

@@ -0,0 +1,33 @@
+/*
+ *  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;
+
+final class DontDiscardNewExpressionsLinterTest extends TestCase {
+  use LinterTestTrait;
+
+  <<__Override>>
+  protected function getLinter(string $file): DontDiscardNewExpressionsLinter {
+    return DontDiscardNewExpressionsLinter::fromPathWithConfig(
+      $file,
+      shape('exceptionSuffixes' => vec['Exception', 'MyCustomSuffix']),
+    );
+  }
+
+  <<__Override>>
+  public function getCleanExamples(): vec<(string)> {
+    return vec[
+      tuple('<?hh function _(): void { $_ = new DateTime(); }'),
+      tuple('<?hh function _(): void { $x = new DateTime(); }'),
+      tuple('<?hh function _(): DateTime { return new DateTime(); }'),
+      tuple('<?hh function _(): nothing { throw new Exception(); }'),
+      tuple('<?hh function _(): void { (new MyClass())->drop(); }'),
+    ];
+  }
+}

tests/examples/DontDiscardNewExpressionsLinter/dynamic_constructors.hack.expect

@@ -0,0 +1,22 @@
+[
+    {
+        "blame": "    new $cls();\n",
+        "blame_pretty": "    new $cls();\n",
+        "description": "You are discarding the result of a `new` expression.\nIf you are intentionally discarding the newly created object,\nconsider assigning it to `$_` to communicate with your readers.\nIf you are running this constructor for its side-effects,\nconsider restructuring that class \/ constructor.\nA constructor is meant for creating objects, not for causing effects."
+    },
+    {
+        "blame": "    new static();\n",
+        "blame_pretty": "    new static();\n",
+        "description": "You are discarding the result of a `new` expression.\nIf you are intentionally discarding the newly created object,\nconsider assigning it to `$_` to communicate with your readers.\nIf you are running this constructor for its side-effects,\nconsider restructuring that class \/ constructor.\nA constructor is meant for creating objects, not for causing effects."
+    },
+    {
+        "blame": "    new parent();\n",
+        "blame_pretty": "    new parent();\n",
+        "description": "You are discarding the result of a `new` expression.\nIf you are intentionally discarding the newly created object,\nconsider assigning it to `$_` to communicate with your readers.\nIf you are running this constructor for its side-effects,\nconsider restructuring that class \/ constructor.\nA constructor is meant for creating objects, not for causing effects."
+    },
+    {
+        "blame": "    new self();\n",
+        "blame_pretty": "    new self();\n",
+        "description": "You are discarding the result of a `new` expression.\nIf you are intentionally discarding the newly created object,\nconsider assigning it to `$_` to communicate with your readers.\nIf you are running this constructor for its side-effects,\nconsider restructuring that class \/ constructor.\nA constructor is meant for creating objects, not for causing effects."
+    }
+]

tests/examples/DontDiscardNewExpressionsLinter/dynamic_constructors.hack.in

@@ -0,0 +1,14 @@
+<<__ConsistentConstruct>>
+class Consistent {
+  public function __construct() {}
+}
+
+final class Dyn extends Consistent {
+  public function _(): void {
+    $cls = Dyn::class;
+    new $cls();
+    new static();
+    new parent();
+    new self();
+  }
+}

tests/examples/DontDiscardNewExpressionsLinter/forget_to_throw.hack.expect

@@ -0,0 +1,22 @@
+[
+    {
+        "blame": "  new Exception();\n",
+        "blame_pretty": "  new Exception();\n",
+        "description": "You are discarding the result of a `new` expression.\nIf you are intentionally discarding the newly created object,\nconsider assigning it to `$_` to communicate with your readers.\nIt looks like you are constructing an Exception. Did you intend to throw it?"
+    },
+    {
+        "blame": "  new LogicException();\n",
+        "blame_pretty": "  new LogicException();\n",
+        "description": "You are discarding the result of a `new` expression.\nIf you are intentionally discarding the newly created object,\nconsider assigning it to `$_` to communicate with your readers.\nIt looks like you are constructing an Exception. Did you intend to throw it?"
+    },
+    {
+        "blame": "  new Some\\Exception();\n",
+        "blame_pretty": "  new Some\\Exception();\n",
+        "description": "You are discarding the result of a `new` expression.\nIf you are intentionally discarding the newly created object,\nconsider assigning it to `$_` to communicate with your readers.\nIt looks like you are constructing an Exception. Did you intend to throw it?"
+    },
+    {
+        "blame": "  new ClassWithMyCustomSuffix();\n",
+        "blame_pretty": "  new ClassWithMyCustomSuffix();\n",
+        "description": "You are discarding the result of a `new` expression.\nIf you are intentionally discarding the newly created object,\nconsider assigning it to `$_` to communicate with your readers.\nIt looks like you are constructing an Exception. Did you intend to throw it?"
+    }
+]

tests/examples/DontDiscardNewExpressionsLinter/forget_to_throw.hack.in

@@ -0,0 +1,6 @@
+function _(): void {
+  new Exception();
+  new LogicException();
+  new Some\Exception();
+  new ClassWithMyCustomSuffix();
+}

tests/examples/DontDiscardNewExpressionsLinter/side_effect_constructor.hack.expect

@@ -0,0 +1,12 @@
+[
+    {
+        "blame": "  new DeleteMyFiles();\n",
+        "blame_pretty": "  new DeleteMyFiles();\n",
+        "description": "You are discarding the result of a `new` expression.\nIf you are intentionally discarding the newly created object,\nconsider assigning it to `$_` to communicate with your readers.\nIf you are running this constructor for its side-effects,\nconsider restructuring that class \/ constructor.\nA constructor is meant for creating objects, not for causing effects."
+    },
+    {
+        "blame": "  new Some\\DeleteMyFiles();\n",
+        "blame_pretty": "  new Some\\DeleteMyFiles();\n",
+        "description": "You are discarding the result of a `new` expression.\nIf you are intentionally discarding the newly created object,\nconsider assigning it to `$_` to communicate with your readers.\nIf you are running this constructor for its side-effects,\nconsider restructuring that class \/ constructor.\nA constructor is meant for creating objects, not for causing effects."
+    }
+]

tests/examples/DontDiscardNewExpressionsLinter/side_effect_constructor.hack.in

@@ -0,0 +1,4 @@
+function _(): void {
+  new DeleteMyFiles();
+  new Some\DeleteMyFiles();
+}