From 578f540a92b1b78ca70b458244e39ea65f0c0ea8 Mon Sep 17 00:00:00 2001
From: Joshua Spence <josh@freelancer.com>
Date: Mon, 30 Nov 2015 07:32:43 +1100
Subject: [PATCH] Add a linter rule for `*val` functions

Summary: Type casts should be used instead of calls to the `*val` functions as function calls impose additional overhead.

Test Plan: Added unit tests.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14572
---
 src/__phutil_library_map__.php                |  4 ++
 ...onCallShouldBeTypeCastXHPASTLinterRule.php | 54 +++++++++++++++++++
 ...ouldBeTypeCastXHPASTLinterRuleTestCase.php | 11 ++++
 .../cast-functions.lint-test                  | 19 +++++++
 .../parameter-mismatch.lint-test              | 11 ++++
 5 files changed, 99 insertions(+)
 create mode 100644 src/lint/linter/xhpast/rules/ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRule.php
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRuleTestCase.php
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/function-call-should-be-type-cast/cast-functions.lint-test
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/function-call-should-be-type-cast/parameter-mismatch.lint-test

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 6d6b2f28..937551cc 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -145,6 +145,8 @@ phutil_register_library_map(array(
     'ArcanistFlake8LinterTestCase' => 'lint/linter/__tests__/ArcanistFlake8LinterTestCase.php',
     'ArcanistFormattedStringXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistFormattedStringXHPASTLinterRule.php',
     'ArcanistFormattedStringXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistFormattedStringXHPASTLinterRuleTestCase.php',
+    'ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRule.php',
+    'ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRuleTestCase.php',
     'ArcanistFutureLinter' => 'lint/linter/ArcanistFutureLinter.php',
     'ArcanistGeneratedLinter' => 'lint/linter/ArcanistGeneratedLinter.php',
     'ArcanistGeneratedLinterTestCase' => 'lint/linter/__tests__/ArcanistGeneratedLinterTestCase.php',
@@ -539,6 +541,8 @@ phutil_register_library_map(array(
     'ArcanistFlake8LinterTestCase' => 'ArcanistExternalLinterTestCase',
     'ArcanistFormattedStringXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistFormattedStringXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
+    'ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
+    'ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistFutureLinter' => 'ArcanistLinter',
     'ArcanistGeneratedLinter' => 'ArcanistLinter',
     'ArcanistGeneratedLinterTestCase' => 'ArcanistLinterTestCase',
diff --git a/src/lint/linter/xhpast/rules/ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRule.php
new file mode 100644
index 00000000..443697ce
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRule.php
@@ -0,0 +1,54 @@
+<?php
+
+final class ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRule
+  extends ArcanistXHPASTLinterRule {
+
+  const ID = 105;
+
+  public function getLintName() {
+    return pht('Function Call Should Be Type Cast');
+  }
+
+  public function getLintSeverity() {
+    return ArcanistLintSeverity::SEVERITY_ADVICE;
+  }
+
+  public function process(XHPASTNode $root) {
+    static $cast_functions = array(
+      'boolval' => 'bool',
+      'doubleval' => 'double',
+      'floatval' => 'double',
+      'intval' => 'int',
+      'strval' => 'string',
+    );
+
+    $casts = $this->getFunctionCalls($root, array_keys($cast_functions));
+
+    foreach ($casts as $cast) {
+      $function_name = $cast
+        ->getChildOfType(0, 'n_SYMBOL_NAME')
+        ->getConcreteString();
+      $cast_name = $cast_functions[$function_name];
+
+      $parameters  = $cast->getChildOfType(1, 'n_CALL_PARAMETER_LIST');
+      $replacement = null;
+
+      // Only suggest a replacement if the function call has exactly
+      // one parameter.
+      if (count($parameters->getChildren()) == 1) {
+        $parameter   = $parameters->getChildByIndex(0);
+        $replacement = '('.$cast_name.')'.$parameter->getConcreteString();
+      }
+
+      $this->raiseLintAtNode(
+        $cast,
+        pht(
+          'For consistency, use `%s` (a type cast) instead of `%s` '.
+          '(a function call). Function calls impose additional overhead.',
+          '('.$cast_name.')',
+          $function_name),
+        $replacement);
+    }
+  }
+
+}
diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRuleTestCase.php
new file mode 100644
index 00000000..8153513d
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRuleTestCase.php
@@ -0,0 +1,11 @@
+<?php
+
+final class ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRuleTestCase
+  extends ArcanistXHPASTLinterRuleTestCase {
+
+  public function testLinter() {
+    $this->executeTestsInDirectory(
+      dirname(__FILE__).'/function-call-should-be-type-cast/');
+  }
+
+}
diff --git a/src/lint/linter/xhpast/rules/__tests__/function-call-should-be-type-cast/cast-functions.lint-test b/src/lint/linter/xhpast/rules/__tests__/function-call-should-be-type-cast/cast-functions.lint-test
new file mode 100644
index 00000000..805c6200
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/function-call-should-be-type-cast/cast-functions.lint-test
@@ -0,0 +1,19 @@
+<?php
+boolval($x);
+doubleval($x);
+floatval($x);
+intval($x);
+strval($x);
+~~~~~~~~~~
+advice:2:1
+advice:3:1
+advice:4:1
+advice:5:1
+advice:6:1
+~~~~~~~~~~
+<?php
+(bool)$x;
+(double)$x;
+(double)$x;
+(int)$x;
+(string)$x;
diff --git a/src/lint/linter/xhpast/rules/__tests__/function-call-should-be-type-cast/parameter-mismatch.lint-test b/src/lint/linter/xhpast/rules/__tests__/function-call-should-be-type-cast/parameter-mismatch.lint-test
new file mode 100644
index 00000000..9af460f0
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/function-call-should-be-type-cast/parameter-mismatch.lint-test
@@ -0,0 +1,11 @@
+<?php
+// If the function call doesn't have exactly one parameter,
+// we don't provide an autofix.
+strval($x, $y);
+~~~~~~~~~~
+advice:4:1
+~~~~~~~~~~
+<?php
+// If the function call doesn't have exactly one parameter,
+// we don't provide an autofix.
+strval($x, $y);