From da6d4f85eefb0ae9c945e41659b678549fbb8c8a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 17 Oct 2019 08:27:29 -0700
Subject: [PATCH] Add a lint check for deprecated argument order to "implode()"

Summary:
Ref T13428. Historically, "implode()" accepts arguments in either order. PHP 7.4+ warns about glue not being first.

Add a lint check when the second parameter is a static scalar, implying it is the glue parameter.

Test Plan: Ran unit tests. See next change.

Maniphest Tasks: T13428

Differential Revision: https://secure.phabricator.com/D20857
---
 src/__phutil_library_map__.php                |  4 ++
 ...stImplodeArgumentOrderXHPASTLinterRule.php | 39 +++++++++++++++++++
 ...eArgumentOrderXHPASTLinterRuleTestCase.php | 11 ++++++
 .../implode-argument-order/implode.lint-test  | 18 +++++++++
 4 files changed, 72 insertions(+)
 create mode 100644 src/lint/linter/xhpast/rules/ArcanistImplodeArgumentOrderXHPASTLinterRule.php
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistImplodeArgumentOrderXHPASTLinterRuleTestCase.php
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/implode-argument-order/implode.lint-test

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 695fd867..63a4ee80 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -185,6 +185,8 @@ phutil_register_library_map(array(
     'ArcanistImplicitFallthroughXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistImplicitFallthroughXHPASTLinterRuleTestCase.php',
     'ArcanistImplicitVisibilityXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistImplicitVisibilityXHPASTLinterRule.php',
     'ArcanistImplicitVisibilityXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistImplicitVisibilityXHPASTLinterRuleTestCase.php',
+    'ArcanistImplodeArgumentOrderXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistImplodeArgumentOrderXHPASTLinterRule.php',
+    'ArcanistImplodeArgumentOrderXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistImplodeArgumentOrderXHPASTLinterRuleTestCase.php',
     'ArcanistInlineHTMLXHPASTLinterRule' => 'lint/linter/ArcanistInlineHTMLXHPASTLinterRule.php',
     'ArcanistInlineHTMLXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistInlineHTMLXHPASTLinterRuleTestCase.php',
     'ArcanistInnerFunctionXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistInnerFunctionXHPASTLinterRule.php',
@@ -608,6 +610,8 @@ phutil_register_library_map(array(
     'ArcanistImplicitFallthroughXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistImplicitVisibilityXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistImplicitVisibilityXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
+    'ArcanistImplodeArgumentOrderXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
+    'ArcanistImplodeArgumentOrderXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistInlineHTMLXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistInlineHTMLXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistInnerFunctionXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
diff --git a/src/lint/linter/xhpast/rules/ArcanistImplodeArgumentOrderXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistImplodeArgumentOrderXHPASTLinterRule.php
new file mode 100644
index 00000000..db881813
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/ArcanistImplodeArgumentOrderXHPASTLinterRule.php
@@ -0,0 +1,39 @@
+<?php
+
+final class ArcanistImplodeArgumentOrderXHPASTLinterRule
+  extends ArcanistXHPASTLinterRule {
+
+  const ID = 129;
+
+  public function getLintName() {
+    return pht('Implode With Glue First');
+  }
+
+  public function getLintSeverity() {
+    return ArcanistLintSeverity::SEVERITY_ERROR;
+  }
+
+  public function process(XHPASTNode $root) {
+    $implosions = $this->getFunctionCalls($root, array('implode'));
+    foreach ($implosions as $implosion) {
+      $parameters = $implosion->getChildOfType(1, 'n_CALL_PARAMETER_LIST');
+
+      if (count($parameters->getChildren()) != 2) {
+        continue;
+      }
+
+      $parameter = $parameters->getChildByIndex(1);
+      if (!$parameter->isStaticScalar()) {
+        continue;
+      }
+
+      $this->raiseLintAtNode(
+        $implosion,
+        pht(
+          'When calling "implode()", pass the "glue" argument first. (The '.
+          'other parameter order is deprecated in PHP 7.4 and raises a '.
+          'warning.)'));
+    }
+  }
+
+}
diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistImplodeArgumentOrderXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistImplodeArgumentOrderXHPASTLinterRuleTestCase.php
new file mode 100644
index 00000000..2ad6b620
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistImplodeArgumentOrderXHPASTLinterRuleTestCase.php
@@ -0,0 +1,11 @@
+<?php
+
+final class ArcanistImplodeArgumentOrderXHPASTLinterRuleTestCase
+  extends ArcanistXHPASTLinterRuleTestCase {
+
+  public function testLinter() {
+    $this->executeTestsInDirectory(
+      dirname(__FILE__).'/implode-argument-order/');
+  }
+
+}
diff --git a/src/lint/linter/xhpast/rules/__tests__/implode-argument-order/implode.lint-test b/src/lint/linter/xhpast/rules/__tests__/implode-argument-order/implode.lint-test
new file mode 100644
index 00000000..18995143
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/__tests__/implode-argument-order/implode.lint-test
@@ -0,0 +1,18 @@
+<?php
+
+// This is the correct argument order.
+implode(' ', $x);
+
+// This is the legacy argument order which warns in PHP 7.4+.
+implode($x, ' ');
+
+// No warning: we can't statically tell which one is the glue.
+implode($x, $y);
+
+// No warning: these are likely wrong, but not a glue order problem.
+implode();
+implode($x);
+implode($x, $y, $z);
+
+~~~~~~~~~~
+error:7:1:XHP129:Implode With Glue First