From bf88f4616cdfc8a391012f768e5e226aa0fb9a52 Mon Sep 17 00:00:00 2001
From: Joshua Spence <josh@joshuaspence.com>
Date: Fri, 14 Aug 2015 07:37:48 +1000
Subject: [PATCH] Add a linter rule for global variables

Summary: Ref T7409. Global variables are gross and should be avoided like the plague.

Test Plan: Added test cases.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T7409

Differential Revision: https://secure.phabricator.com/D13882
---
 src/__phutil_library_map__.php                |  2 ++
 .../xhpast/global-variables.lint-test         |  8 ++++++
 .../xhpast/naming-conventions.lint-test       |  1 +
 .../xhpast/undeclared-variables.lint-test     |  2 ++
 ...ArcanistGlobalVariableXHPASTLinterRule.php | 28 +++++++++++++++++++
 5 files changed, 41 insertions(+)
 create mode 100644 src/lint/linter/__tests__/xhpast/global-variables.lint-test
 create mode 100644 src/lint/linter/xhpast/rules/ArcanistGlobalVariableXHPASTLinterRule.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index c4b3d8b1..70f87173 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -110,6 +110,7 @@ phutil_register_library_map(array(
     'ArcanistGeneratedLinterTestCase' => 'lint/linter/__tests__/ArcanistGeneratedLinterTestCase.php',
     'ArcanistGetConfigWorkflow' => 'workflow/ArcanistGetConfigWorkflow.php',
     'ArcanistGitAPI' => 'repository/api/ArcanistGitAPI.php',
+    'ArcanistGlobalVariableXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistGlobalVariableXHPASTLinterRule.php',
     'ArcanistGoLintLinter' => 'lint/linter/ArcanistGoLintLinter.php',
     'ArcanistGoLintLinterTestCase' => 'lint/linter/__tests__/ArcanistGoLintLinterTestCase.php',
     'ArcanistGoTestResultParser' => 'unit/parser/ArcanistGoTestResultParser.php',
@@ -390,6 +391,7 @@ phutil_register_library_map(array(
     'ArcanistGeneratedLinterTestCase' => 'ArcanistLinterTestCase',
     'ArcanistGetConfigWorkflow' => 'ArcanistWorkflow',
     'ArcanistGitAPI' => 'ArcanistRepositoryAPI',
+    'ArcanistGlobalVariableXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistGoLintLinter' => 'ArcanistExternalLinter',
     'ArcanistGoLintLinterTestCase' => 'ArcanistExternalLinterTestCase',
     'ArcanistGoTestResultParser' => 'ArcanistTestResultParser',
diff --git a/src/lint/linter/__tests__/xhpast/global-variables.lint-test b/src/lint/linter/__tests__/xhpast/global-variables.lint-test
new file mode 100644
index 00000000..863cf5fd
--- /dev/null
+++ b/src/lint/linter/__tests__/xhpast/global-variables.lint-test
@@ -0,0 +1,8 @@
+<?php
+global $BASE_URL;
+function foo() {
+  global $x, $y;
+}
+~~~~~~~~~~
+warning:2:1
+warning:4:3
diff --git a/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test b/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test
index cc78e403..2cd47e36 100644
--- a/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test
+++ b/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test
@@ -72,4 +72,5 @@ warning:29:3
 warning:30:3
 warning:31:3
 warning:33:3
+warning:37:3
 warning:55:3
diff --git a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test
index c6cd1b40..1bedf0fa 100644
--- a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test
+++ b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test
@@ -180,6 +180,7 @@ function some_func($x, $y) {
   };
 }
 ~~~~~~~~~~
+warning:9:3
 error:28:3
 error:30:3
 error:36:3
@@ -188,6 +189,7 @@ error:43:7
 error:44:5
 error:45:5
 error:46:10
+warning:51:3
 error:51:10 worst ever
 warning:61:3
 error:87:3 This stuff is basically testing the lexer/parser for function decls.
diff --git a/src/lint/linter/xhpast/rules/ArcanistGlobalVariableXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistGlobalVariableXHPASTLinterRule.php
new file mode 100644
index 00000000..7a68bf17
--- /dev/null
+++ b/src/lint/linter/xhpast/rules/ArcanistGlobalVariableXHPASTLinterRule.php
@@ -0,0 +1,28 @@
+<?php
+
+final class ArcanistGlobalVariableXHPASTLinterRule
+  extends ArcanistXHPASTLinterRule {
+
+  const ID = 79;
+
+  public function getLintName() {
+    return pht('Global Variables');
+  }
+
+  public function getLintSeverity() {
+    return ArcanistLintSeverity::SEVERITY_WARNING;
+  }
+
+  public function process(XHPASTNode $root) {
+    $nodes = $root->selectDescendantsOfType('n_GLOBAL_DECLARATION_LIST');
+
+    foreach ($nodes as $node) {
+      $this->raiseLintAtNode(
+        $node,
+        pht(
+          'Limit the use of global variables. Global variables are '.
+          'generally a bad idea and should be avoided when possible.'));
+    }
+  }
+
+}