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

Commit

Permalink
add NoReferencesLinter (#223)
Browse files Browse the repository at this point in the history
  • Loading branch information
jjergus authored Oct 7, 2019
1 parent 5881eab commit 0548b5b
Show file tree
Hide file tree
Showing 12 changed files with 456 additions and 0 deletions.
110 changes: 110 additions & 0 deletions src/Linters/NoByRefCallArgumentsLinter.hack
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/*
* 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, Vec};

/**
* Warns people to stop passing function arguments by &$reference. Auto-fixes to
* `inout` unless it's one of the built-in functions that need special
* migration.
*/
final class NoByRefCallArgumentsLinter extends AutoFixingASTLinter {
const type TNode = PrefixUnaryExpression;
const type TContext = Script;

const keyset<string> SPECIAL_FUNCTIONS = keyset[
'array_multisort',
'current',
'headers_sent',
'icu_match',
'is_callable',
'key',
'openssl_encrypt',
'pos',
'preg_match',
'preg_match_all',
'preg_replace',
'str_ireplace',
'str_replace',
'xml_set_object',
];

<<__Override>>
public function getLintErrorForNode(
Script $root,
PrefixUnaryExpression $ref_node,
): ?ASTLintError {
$ampersand_token = $ref_node->getOperator();
$var_name_token = $ref_node->getOperand()->getFirstToken();

if (
!$ampersand_token is AmpersandToken || !$var_name_token is VariableToken
) {
// not a &$reference
return null;
}

$call_node = $root
->getClosestAncestorOfDescendantOfType<FunctionCallExpression>($ref_node);

if (
$call_node is null ||
!C\contains(
$call_node->getArgumentList()?->getChildrenOfItems() ?? vec[],
$ref_node,
)
) {
// This &$reference is not a call argument. References in any other
// position are a Hack error, no need to return a redundant Lint error.
return null;
}

$function_name = $call_node->getReceiver()
// not using ->getCode() because there might be Trivia inside
->getDescendantsOfType(Token::class)
|> Vec\map($$, $token ==> $token->getText())
|> Str\join($$, '')
// get full function name including namespace
|> resolve_function($$, $root, $call_node);

if (C\contains_key(self::SPECIAL_FUNCTIONS, $function_name)) {
// Needs special handling, no auto-fix here.
return new ASTLintError(
$this,
'References are deprecated and will be disallowed soon. Use '.
'`hhast-migrate --ref-to-inout` to migrate this function call.',
$ref_node,
);
}

$trailing = $ampersand_token->getTrailing();
if ($trailing->isEmpty() && $var_name_token->getLeading()->isEmpty()) {
$trailing = NodeList::createMaybeEmptyList(vec[new WhiteSpace(' ')]);
}

return new ASTLintError(
$this,
'References are deprecated and will be disallowed soon. Note that it '.
'is OK to update this function call separately from the function '.
'declaration, because & and inout are compatible during the reference '.
'deprecation period.',
$ref_node,
() ==> $ref_node->withOperator(
new InoutToken($ampersand_token->getLeading(), $trailing),
),
);
}

<<__Override>>
public function getTitleForFix(LintError $_): string {
return 'Replace with inout';
}
}
107 changes: 107 additions & 0 deletions src/Linters/NoByRefParameterDeclarationsLinter.hack
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
* 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;


/**
* Warns people to stop declaring functions with &$reference parameters.
* Auto-fixes to `inout` if there is no default value.
*/
final class NoByRefParameterDeclarationsLinter extends AutoFixingASTLinter {
const type TNode = ParameterDeclaration;
const type TContext = Script;

<<__Override>>
public function getLintErrorForNode(
Script $_root,
ParameterDeclaration $param_node,
): ?ASTLintError {
// Check if this is a by-ref parameter.
$name_node = $param_node->getName();
if (!$name_node is DecoratedExpression) {
return null;
}

$ampersand_token = $name_node->getDecorator();
$var_name_token = $name_node->getExpression()->getFirstToken();
if (
!$ampersand_token is AmpersandToken || !$var_name_token is VariableToken
) {
return null;
}

// Can't auto-fix if it has a default value.
if ($param_node->hasDefaultValue()) {
return new ASTLintError(
$this,
'References are deprecated and will be disallowed soon. Use inout '.
'instead (this will require removing the default value).',
$param_node,
);
}

$first_token = $param_node->getFirstTokenx();

// We need different whitespace/Trivia handling based on whether there is a
// typehint or not.
if ($first_token === $ampersand_token) {
// Missing typehint.
$trailing = $ampersand_token->getTrailing();
if ($trailing->isEmpty() && $var_name_token->getLeading()->isEmpty()) {
$trailing = NodeList::createMaybeEmptyList(vec[new WhiteSpace(' ')]);
}
$autofix = () ==> $param_node
->withCallConvention(
new InoutToken($first_token->getLeading(), $trailing),
)
->withName($name_node->getExpression());
} else {
$autofix = () ==> $param_node
->withCallConvention(
new InoutToken(
$first_token->getLeading(),
NodeList::createMaybeEmptyList(vec[new WhiteSpace(' ')]),
),
)
->withName($name_node->getExpression())
// the declaration's leading trivia was moved before the inout token
->replace($first_token, $first_token->withLeading(null))
// add the removed & token's trivia to the $name token so we don't lose
// comments etc.
->replace(
$var_name_token,
$var_name_token->withLeading(
NodeList::concat(
NodeList::concat(
$ampersand_token->getLeading(),
$ampersand_token->getTrailing(),
),
$var_name_token->getLeading(),
),
),
);
}

return new ASTLintError(
$this,
'References are deprecated and will be disallowed soon. Note that it '.
'is OK to update this function declaration separately from all its '.
'calls, because & and inout are compatible during the reference '.
'deprecation period.',
$param_node,
$autofix,
);
}

<<__Override>>
public function getTitleForFix(LintError $_): string {
return 'Replace with inout';
}
}
2 changes: 2 additions & 0 deletions src/__Private/LintRunConfig.hack
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ final class LintRunConfig {
HHAST\PocketAtomExpressionLinter::class,
HHAST\PocketIdentifierExpressionLinter::class,
HHAST\PocketEnumDeclarationLinter::class,
HHAST\NoByRefCallArgumentsLinter::class,
HHAST\NoByRefParameterDeclarationsLinter::class,
];

const vec<classname<BaseLinter>> NON_DEFAULT_LINTERS = vec[
Expand Down
13 changes: 13 additions & 0 deletions src/nodes/Node.hack
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,19 @@ abstract class Node implements IMemoizeParam {
invariant_violation('unreachable');
}

final public function getClosestAncestorOfDescendantOfType<
<<__Enforceable>> reify TAncestor as Node,
>(
Node $node,
): ?TAncestor {
foreach (Vec\reverse($this->getAncestorsOfDescendant($node)) as $ancestor) {
if ($ancestor is TAncestor) {
return $ancestor;
}
}
return null;
}

final public function getParentOfDescendant(Node $node): Node {
invariant($node !== $this, "Asked to find parent of self");
invariant($this->isAncestorOf($node), "Node is not a descendant");
Expand Down
26 changes: 26 additions & 0 deletions tests/NoByRefCallArgumentsLinterTest.hack
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* 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 NoByRefCallArgumentsLinterTest extends TestCase {
use AutoFixingLinterTestTrait<ASTLintError>;

protected function getLinter(string $file): NoByRefCallArgumentsLinter {
return NoByRefCallArgumentsLinter::fromPath($file);
}

public function getCleanExamples(): vec<(string)> {
return vec[
tuple('<?hh foo($bar);'),
tuple('<?hh function foo(int &$bar): void {}'),
tuple('<?hh $foo = &$bar;'),
];
}
}
28 changes: 28 additions & 0 deletions tests/NoByRefParameterDeclarationsLinterTest.hack
Original file line number Diff line number Diff line change
@@ -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;

final class NoByRefParameterDeclarationsLinterTest extends TestCase {
use AutoFixingLinterTestTrait<ASTLintError>;

protected function getLinter(
string $file,
): NoByRefParameterDeclarationsLinter {
return NoByRefParameterDeclarationsLinter::fromPath($file);
}

public function getCleanExamples(): vec<(string)> {
return vec[
tuple('<?hh foo(&$bar);'),
tuple('<?hh function foo(int $bar): void {}'),
tuple('<?hh $foo = &$bar;'),
];
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* 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.
*
*/

function byref_arguments(): void {
foo(inout $bar, $baz, $bar &$baz, inout $bar);
foo(/* weird */ inout /* formatting */ $ref);

// special function, no auto-fix
key(&$foo);
\key(&$foo);

// not a function argument, no auto-fix, no Lint error
$ref = &$foo;
$ref2 =& $ref;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
[
{
"blame": "&$bar",
"blame_pretty": "&$bar",
"description": "References are deprecated and will be disallowed soon. Note that it is OK to update this function call separately from the function declaration, because & and inout are compatible during the reference deprecation period."
},
{
"blame": "& $bar",
"blame_pretty": "& $bar",
"description": "References are deprecated and will be disallowed soon. Note that it is OK to update this function call separately from the function declaration, because & and inout are compatible during the reference deprecation period."
},
{
"blame": "& \/* formatting *\/ $ref",
"blame_pretty": "& \/* formatting *\/ $ref",
"description": "References are deprecated and will be disallowed soon. Note that it is OK to update this function call separately from the function declaration, because & and inout are compatible during the reference deprecation period."
},
{
"blame": "&$foo",
"blame_pretty": "&$foo",
"description": "References are deprecated and will be disallowed soon. Use `hhast-migrate --ref-to-inout` to migrate this function call."
},
{
"blame": "&$foo",
"blame_pretty": "&$foo",
"description": "References are deprecated and will be disallowed soon. Use `hhast-migrate --ref-to-inout` to migrate this function call."
}
]
21 changes: 21 additions & 0 deletions tests/examples/NoByRefCallArgumentsLinter/byref_arguments.hack.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* 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.
*
*/

function byref_arguments(): void {
foo(&$bar, $baz, $bar &$baz, & $bar);
foo(/* weird */ & /* formatting */ $ref);

// special function, no auto-fix
key(&$foo);
\key(&$foo);

// not a function argument, no auto-fix, no Lint error
$ref = &$foo;
$ref2 =& $ref;
}
Loading

0 comments on commit 0548b5b

Please sign in to comment.