diff --git a/hhast-lint.json b/hhast-lint.json index d53175bff..e8cf70b92 100644 --- a/hhast-lint.json +++ b/hhast-lint.json @@ -25,5 +25,13 @@ "Facebook\\HHAST\\DataProviderTypesLinter" ] } - ] + ], + "linterConfigs": { + "Facebook\\HHAST\\DontDiscardNewExpressionsLinter": { + "exceptionSuffixes": [ + "Error", + "Exception" + ] + } + } } diff --git a/src/Linters/DontDiscardNewExpressionsLinter.hack b/src/Linters/DontDiscardNewExpressionsLinter.hack new file mode 100644 index 000000000..167741e16 --- /dev/null +++ b/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); + 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 { + return $this->getConfig()['exceptionSuffixes'] ?? vec['Exception']; + } +} diff --git a/src/__Private/LintRunConfig.hack b/src/__Private/LintRunConfig.hack index a9aeefed2..98f63055b 100644 --- a/src/__Private/LintRunConfig.hack +++ b/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, diff --git a/tests/DontDiscardNewExpressionsLinterTest.hack b/tests/DontDiscardNewExpressionsLinterTest.hack new file mode 100644 index 000000000..9cd6ac3d6 --- /dev/null +++ b/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('drop(); }'), + ]; + } +} diff --git a/tests/examples/DontDiscardNewExpressionsLinter/dynamic_constructors.hack.expect b/tests/examples/DontDiscardNewExpressionsLinter/dynamic_constructors.hack.expect new file mode 100644 index 000000000..715b6b876 --- /dev/null +++ b/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." + } +] diff --git a/tests/examples/DontDiscardNewExpressionsLinter/dynamic_constructors.hack.in b/tests/examples/DontDiscardNewExpressionsLinter/dynamic_constructors.hack.in new file mode 100644 index 000000000..81950322d --- /dev/null +++ b/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(); + } +} diff --git a/tests/examples/DontDiscardNewExpressionsLinter/forget_to_throw.hack.expect b/tests/examples/DontDiscardNewExpressionsLinter/forget_to_throw.hack.expect new file mode 100644 index 000000000..cb7d2839f --- /dev/null +++ b/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?" + } +] diff --git a/tests/examples/DontDiscardNewExpressionsLinter/forget_to_throw.hack.in b/tests/examples/DontDiscardNewExpressionsLinter/forget_to_throw.hack.in new file mode 100644 index 000000000..588393bdd --- /dev/null +++ b/tests/examples/DontDiscardNewExpressionsLinter/forget_to_throw.hack.in @@ -0,0 +1,6 @@ +function _(): void { + new Exception(); + new LogicException(); + new Some\Exception(); + new ClassWithMyCustomSuffix(); +} diff --git a/tests/examples/DontDiscardNewExpressionsLinter/side_effect_constructor.hack.expect b/tests/examples/DontDiscardNewExpressionsLinter/side_effect_constructor.hack.expect new file mode 100644 index 000000000..dbe0a7f41 --- /dev/null +++ b/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." + } +] diff --git a/tests/examples/DontDiscardNewExpressionsLinter/side_effect_constructor.hack.in b/tests/examples/DontDiscardNewExpressionsLinter/side_effect_constructor.hack.in new file mode 100644 index 000000000..6a8db4cae --- /dev/null +++ b/tests/examples/DontDiscardNewExpressionsLinter/side_effect_constructor.hack.in @@ -0,0 +1,4 @@ +function _(): void { + new DeleteMyFiles(); + new Some\DeleteMyFiles(); +}