diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5ee1c01c..d94b89b1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -99,6 +99,7 @@ phutil_register_library_map(array( 'ArcanistWhichWorkflow' => 'workflow/which', 'ArcanistWorkingCopyIdentity' => 'workingcopyidentity', 'ArcanistXHPASTLintNamingHook' => 'lint/linter/xhpast/naminghook', + 'ArcanistXHPASTLintNamingHookTestCase' => 'lint/linter/xhpast/naminghook/__tests__', 'ArcanistXHPASTLinter' => 'lint/linter/xhpast', 'ArcanistXHPASTLinterTestCase' => 'lint/linter/xhpast/__tests__', 'BranchInfo' => 'branch', @@ -170,6 +171,7 @@ phutil_register_library_map(array( 'ArcanistUploadWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistUserAbortException' => 'ArcanistUsageException', 'ArcanistWhichWorkflow' => 'ArcanistBaseWorkflow', + 'ArcanistXHPASTLintNamingHookTestCase' => 'ArcanistPhutilTestCase', 'ArcanistXHPASTLinter' => 'ArcanistLinter', 'ArcanistXHPASTLinterTestCase' => 'ArcanistLinterTestCase', 'ComprehensiveLintEngine' => 'ArcanistLintEngine', diff --git a/src/lint/linter/base/test/ArcanistLinterTestCase.php b/src/lint/linter/base/test/ArcanistLinterTestCase.php index 35ad1f1f..9f75b7f5 100644 --- a/src/lint/linter/base/test/ArcanistLinterTestCase.php +++ b/src/lint/linter/base/test/ArcanistLinterTestCase.php @@ -118,13 +118,16 @@ abstract class ArcanistLinterTestCase extends ArcanistPhutilTestCase { private function compareLint($file, $expect, $result) { $seen = array(); $raised = array(); + $message_map = array(); foreach ($result->getMessages() as $message) { $sev = $message->getSeverity(); $line = $message->getLine(); $char = $message->getChar(); $code = $message->getCode(); $name = $message->getName(); - $seen[] = $sev.":".$line.":".$char; + $message_key = $sev.":".$line.":".$char; + $message_map[$message_key] = $message; + $seen[] = $message_key; $raised[] = " {$sev} at line {$line}, char {$char}: {$code} {$name}"; } $expect = trim($expect); @@ -154,11 +157,15 @@ abstract class ArcanistLinterTestCase extends ArcanistPhutilTestCase { } foreach (array_diff_key($seen, $expect) as $surprising => $ignored) { + + $message = $message_map[$surprising]; + $message_info = $message->getDescription(); + list($sev, $line, $char) = explode(':', $surprising); $this->assertFailure( "In '{$file}', ". "lint raised {$sev} on line {$line} at char {$char}, ". - "but nothing was expected. {$raised}"); + "but nothing was expected:\n\n{$message_info}\n\n{$raised}"); } } diff --git a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php index 51810fbe..89840212 100644 --- a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php +++ b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php @@ -455,16 +455,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter { $declarations = array( '$this' => 0, - '$GLOBALS' => 0, - '$_SERVER' => 0, - '$_GET' => 0, - '$_POST' => 0, - '$_FILES' => 0, - '$_COOKIE' => 0, - '$_SESSION' => 0, - '$_REQUEST' => 0, - '$_ENV' => 0, - ); + ) + array_fill_keys($this->getSuperGlobalNames(), 0); $declaration_tokens = array(); $exclude_tokens = array(); $vars = array(); @@ -711,28 +702,16 @@ class ArcanistXHPASTLinter extends ArcanistLinter { foreach ($classes as $class) { $name_token = $class->getChildByIndex(1); $name_string = $name_token->getConcreteString(); - $is_xhp = ($name_string[0] == ':'); - if ($is_xhp) { - $names[] = array( - 'xhp-class', - $name_string, - $name_token, - $this->isLowerCaseWithXHP($name_string) - ? null - : 'Follow naming conventions: XHP elements should be named using '. - 'lower case.', - ); - } else { - $names[] = array( - 'class', - $name_string, - $name_token, - $this->isUpperCamelCase($name_string) - ? null - : 'Follow naming conventions: classes should be named using '. - 'UpperCamelCase.', - ); - } + + $names[] = array( + 'class', + $name_string, + $name_token, + ArcanistXHPASTLintNamingHook::isUpperCamelCase($name_string) + ? null + : 'Follow naming conventions: classes should be named using '. + 'UpperCamelCase.', + ); } $ifaces = $root->selectDescendantsOfType('n_INTERFACE_DECLARATION'); @@ -743,7 +722,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter { 'interface', $name_string, $name_token, - $this->isUpperCamelCase($name_string) + ArcanistXHPASTLintNamingHook::isUpperCamelCase($name_string) ? null : 'Follow naming conventions: interfaces should be named using '. 'UpperCamelCase.', @@ -763,7 +742,8 @@ class ArcanistXHPASTLinter extends ArcanistLinter { 'function', $name_string, $name_token, - $this->isLowercaseWithUnderscores($name_string) + ArcanistXHPASTLintNamingHook::isLowercaseWithUnderscores( + ArcanistXHPASTLintNamingHook::stripPHPFunction($name_string)) ? null : 'Follow naming conventions: functions should be named using '. 'lowercase_with_underscores.', @@ -779,24 +759,32 @@ class ArcanistXHPASTLinter extends ArcanistLinter { 'method', $name_string, $name_token, - $this->isLowerCamelCase($name_string) + ArcanistXHPASTLintNamingHook::isLowerCamelCase( + ArcanistXHPASTLintNamingHook::stripPHPFunction($name_string)) ? null : 'Follow naming conventions: methods should be named using '. 'lowerCamelCase.', ); } + $param_tokens = array(); $params = $root->selectDescendantsOfType('n_DECLARATION_PARAMETER_LIST'); foreach ($params as $param_list) { foreach ($param_list->getChildren() as $param) { $name_token = $param->getChildByIndex(1); + if ($name_token->getTypeName() == 'n_VARIABLE_REFERENCE') { + $name_token = $name_token->getChildOfType(0, 'n_VARIABLE'); + } + $param_tokens[$name_token->getID()] = true; $name_string = $name_token->getConcreteString(); + $names[] = array( 'parameter', $name_string, $name_token, - $this->isLowercaseWithUnderscores($name_string) + ArcanistXHPASTLintNamingHook::isLowercaseWithUnderscores( + ArcanistXHPASTLintNamingHook::stripPHPVariable($name_string)) ? null : 'Follow naming conventions: parameters should be named using '. 'lowercase_with_underscores.', @@ -815,7 +803,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter { 'constant', $name_string, $name_token, - $this->isUppercaseWithUnderscores($name_string) + ArcanistXHPASTLintNamingHook::isUppercaseWithUnderscores($name_string) ? null : 'Follow naming conventions: class constants should be named '. 'using UPPERCASE_WITH_UNDERSCORES.', @@ -823,19 +811,25 @@ class ArcanistXHPASTLinter extends ArcanistLinter { } } + $member_tokens = array(); + $props = $root->selectDescendantsOfType('n_CLASS_MEMBER_DECLARATION_LIST'); foreach ($props as $prop_list) { - foreach ($prop_list->getChildren() as $prop) { + foreach ($prop_list->getChildren() as $token_id => $prop) { if ($prop->getTypeName() == 'n_CLASS_MEMBER_MODIFIER_LIST') { continue; } + $name_token = $prop->getChildByIndex(0); + $member_tokens[$name_token->getID()] = true; + $name_string = $name_token->getConcreteString(); $names[] = array( 'member', $name_string, $name_token, - $this->isLowerCamelCase($name_string) + ArcanistXHPASTLintNamingHook::isLowerCamelCase( + ArcanistXHPASTLintNamingHook::stripPHPVariable($name_string)) ? null : 'Follow naming conventions: class properties should be named '. 'using lowerCamelCase.', @@ -843,6 +837,84 @@ class ArcanistXHPASTLinter extends ArcanistLinter { } } + $superglobal_map = array_fill_keys( + $this->getSuperGlobalNames(), + true); + + + $fdefs = $root->selectDescendantsOfType('n_FUNCTION_DECLARATION'); + $mdefs = $root->selectDescendantsOfType('n_METHOD_DECLARATION'); + $defs = $fdefs->add($mdefs); + + foreach ($defs as $def) { + $globals = $def->selectDescendantsOfType('n_GLOBAL_DECLARATION_LIST'); + $globals = $globals->selectDescendantsOfType('n_VARIABLE'); + + $globals_map = array(); + foreach ($globals as $global) { + $global_string = $global->getConcreteString(); + $globals_map[$global_string] = true; + $names[] = array( + 'global', + $global_string, + $global, + + // No advice for globals, but hooks have an option to provide some. + null); + } + + // Exclude access of static properties, since lint will be raised at + // their declaration if they're invalid and they may not conform to + // variable rules. This is slightly overbroad (includes the entire + // rhs of a "Class::..." token) to cover cases like "Class:$x[0]". These + // varaibles are simply made exempt from naming conventions. + $exclude_tokens = array(); + $statics = $def->selectDescendantsOfType('n_CLASS_STATIC_ACCESS'); + foreach ($statics as $static) { + $rhs = $static->getChildByIndex(1); + $rhs_vars = $def->selectDescendantsOfType('n_VARIABLE'); + foreach ($rhs_vars as $var) { + $exclude_tokens[$var->getID()] = true; + } + } + + $vars = $def->selectDescendantsOfType('n_VARIABLE'); + foreach ($vars as $token_id => $var) { + if (isset($member_tokens[$token_id])) { + continue; + } + if (isset($param_tokens[$token_id])) { + continue; + } + if (isset($exclude_tokens[$token_id])) { + continue; + } + + $var_string = $var->getConcreteString(); + + // Awkward artifact of "$o->{$x}". + $var_string = trim($var_string, '{}'); + + if (isset($superglobal_map[$var_string])) { + continue; + } + if (isset($globals_map[$var_string])) { + continue; + } + + $names[] = array( + 'variable', + $var_string, + $var, + ArcanistXHPASTLintNamingHook::isLowercaseWithUnderscores( + ArcanistXHPASTLintNamingHook::stripPHPVariable($var_string)) + ? null + : 'Follow naming conventions: variables should be named using '. + 'lowercase_with_underscores.', + ); + } + } + $engine = $this->getEngine(); $working_copy = $engine->getWorkingCopy(); @@ -872,28 +944,6 @@ class ArcanistXHPASTLinter extends ArcanistLinter { } } - protected function isUpperCamelCase($str) { - return preg_match('/^[A-Z][A-Za-z0-9]*$/', $str); - } - - protected function isLowerCamelCase($str) { - // Allow initial "__" for magic methods like __construct; we could also - // enumerate these explicitly. - return preg_match('/^\$?(?:__)?[a-z][A-Za-z0-9]*$/', $str); - } - - protected function isUppercaseWithUnderscores($str) { - return preg_match('/^[A-Z0-9_]+$/', $str); - } - - protected function isLowercaseWithUnderscores($str) { - return preg_match('/^[&]?\$?[a-z0-9_]+$/', $str); - } - - protected function isLowercaseWithXHP($str) { - return preg_match('/^:[a-z0-9_:-]+$/', $str); - } - protected function lintSurpriseConstructors($root) { $classes = $root->selectDescendantsOfType('n_CLASS_DECLARATION'); foreach ($classes as $class) { @@ -1352,4 +1402,18 @@ class ArcanistXHPASTLinter extends ArcanistLinter { $replace); } + public function getSuperGlobalNames() { + return array( + '$GLOBALS', + '$_SERVER', + '$_GET', + '$_POST', + '$_FILES', + '$_COOKIE', + '$_SESSION', + '$_REQUEST', + '$_ENV', + ); + } + } diff --git a/src/lint/linter/xhpast/__init__.php b/src/lint/linter/xhpast/__init__.php index 934c9387..b98d8edb 100644 --- a/src/lint/linter/xhpast/__init__.php +++ b/src/lint/linter/xhpast/__init__.php @@ -7,10 +7,11 @@ phutil_require_module('arcanist', 'lint/linter/base'); +phutil_require_module('arcanist', 'lint/linter/xhpast/naminghook'); phutil_require_module('arcanist', 'lint/severity'); + phutil_require_module('phutil', 'parser/xhpast/api/tree'); phutil_require_module('phutil', 'parser/xhpast/bin'); - phutil_require_module('phutil', 'utils'); diff --git a/src/lint/linter/xhpast/__tests__/data/duplicate-key-in-array.lint-test b/src/lint/linter/xhpast/__tests__/data/duplicate-key-in-array.lint-test index cba4132d..1c517403 100644 --- a/src/lint/linter/xhpast/__tests__/data/duplicate-key-in-array.lint-test +++ b/src/lint/linter/xhpast/__tests__/data/duplicate-key-in-array.lint-test @@ -23,8 +23,8 @@ $d = array( $e = array( self::B => 'bee', self::B => 'bea', - self::B() => 'ehh', - self::$B => 'weh', + self::b() => 'ehh', + self::$b => 'weh', B => 'b', C::B => 'cb', ); diff --git a/src/lint/linter/xhpast/__tests__/data/naming-conventions.lint-test b/src/lint/linter/xhpast/__tests__/data/naming-conventions.lint-test index e03d9fad..96403246 100644 --- a/src/lint/linter/xhpast/__tests__/data/naming-conventions.lint-test +++ b/src/lint/linter/xhpast/__tests__/data/naming-conventions.lint-test @@ -25,6 +25,30 @@ function () use ($this_is_a_closure) { }; function f(&$YY) { } + +function g() { + $lowerCamelCase = 0; + $UpperCamelCase = 0; + $UPPERCASE_WITH_UNDERSCORES = 0; + $lowercase_with_underscores = 0; + $mIxEd_CaSe = 0; +} + +function h() { + global $mIxEd_CaSe; + $mIxEd_CaSe = 2; + $GLOBALS[0] = 2; + $_ENV[0] = 1; +} + +function i() { + self::$X_x; + Other::$Y_y; + parent::$Z_z; + self::$X_x[0]; + Other::$Y_y[0]; + parent::$Z_z[0]; +} ~~~~~~~~~~ warning:2:7 warning:3:9 @@ -37,4 +61,8 @@ warning:5:25 warning:8:11 warning:12:10 warning:12:13 -warning:26:12 \ No newline at end of file +warning:26:13 +warning:30:3 +warning:31:3 +warning:32:3 +warning:34:3 \ No newline at end of file diff --git a/src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php b/src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php index e526f555..8eb365a8 100644 --- a/src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php +++ b/src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php @@ -1,7 +1,7 @@ } + +/* -( Overriding Symbol Name Lint Messages )------------------------------- */ + + /** * Callback invoked for each symbol, which can override the default * determination of name validity or accept it by returning $default. The @@ -57,4 +76,85 @@ abstract class ArcanistXHPASTLintNamingHook { */ abstract public function lintSymbolName($type, $name, $default); + +/* -( Name Utilities )----------------------------------------------------- */ + + + /** + * Returns true if a symbol name is UpperCamelCase. + * + * @param string Symbol name. + * @return bool True if the symbol is UpperCamelCase. + * @task util + */ + public static function isUpperCamelCase($symbol) { + return preg_match('/^[A-Z][A-Za-z0-9]*$/', $symbol); + } + + + /** + * Returns true if a symbol name is lowerCamelCase. + * + * @param string Symbol name. + * @return bool True if the symbol is lowerCamelCase. + * @task util + */ + public static function isLowerCamelCase($symbol) { + return preg_match('/^[a-z][A-Za-z0-9]*$/', $symbol); + } + + + /** + * Returns true if a symbol name is UPPERCASE_WITH_UNDERSCORES. + * + * @param string Symbol name. + * @return bool True if the symbol is UPPERCASE_WITH_UNDERSCORES. + * @task util + */ + public static function isUppercaseWithUnderscores($symbol) { + return preg_match('/^[A-Z0-9_]+$/', $symbol); + } + + + /** + * Returns true if a symbol name is lowercase_with_underscores. + * + * @param string Symbol name. + * @return bool True if the symbol is lowercase_with_underscores. + * @task util + */ + public static function isLowercaseWithUnderscores($symbol) { + return preg_match('/^[a-z0-9_]+$/', $symbol); + } + + + /** + * Strip non-name components from PHP function symbols. Notably, this discards + * the "__" magic-method signifier, to make a symbol appropriate for testing + * with methods like @{method:isLowerCamelCase}. + * + * @param string Symbol name. + * @return string Stripped symbol. + * @task util + */ + public static function stripPHPFunction($symbol) { + // Allow initial "__" for magic methods like __construct; we could also + // enumerate these explicitly. + return preg_replace('/^__/', '', $symbol); + } + + + /** + * Strip non-name components from PHP variable symbols. Notably, this discards + * the "$", to make a symbol appropriate for testing with methods like + * @{method:isLowercaseWithUnderscores}. + * + * @param string Symbol name. + * @return bool True if the symbol is UpperCamelCase. + * @task util + */ + public static function stripPHPVariable($symbol) { + return preg_replace('/^\$/', '', $symbol); + } + } diff --git a/src/lint/linter/xhpast/naminghook/__tests__/ArcanistXHPASTLintNamingHookTestCase.php b/src/lint/linter/xhpast/naminghook/__tests__/ArcanistXHPASTLintNamingHookTestCase.php new file mode 100644 index 00000000..dd96e2cf --- /dev/null +++ b/src/lint/linter/xhpast/naminghook/__tests__/ArcanistXHPASTLintNamingHookTestCase.php @@ -0,0 +1,84 @@ + array(1, 0, 0, 0), + 'UpperCamelCaseROFL' => array(1, 0, 0, 0), + + 'lowerCamelCase' => array(0, 1, 0, 0), + 'lowerCamelCaseROFL' => array(0, 1, 0, 0), + + 'UPPERCASE_WITH_UNDERSCORES' => array(0, 0, 1, 0), + '_UPPERCASE_WITH_UNDERSCORES_' => array(0, 0, 1, 0), + '__UPPERCASE__WITH__UNDERSCORES__' => array(0, 0, 1, 0), + + 'lowercase_with_underscores' => array(0, 0, 0, 1), + '_lowercase_with_underscores_' => array(0, 0, 0, 1), + '__lowercase__with__underscores__' => array(0, 0, 0, 1), + + 'mixedCASE_NoNsEnSe' => array(0, 0, 0, 0), + ); + + foreach ($tests as $test => $expect) { + $this->assertEqual( + $expect[0], + ArcanistXHPASTLintNamingHook::isUpperCamelCase($test), + "UpperCamelCase: '{$test}'"); + $this->assertEqual( + $expect[1], + ArcanistXHPASTLintNamingHook::isLowerCamelCase($test), + "lowerCamelCase: '{$test}'"); + $this->assertEqual( + $expect[2], + ArcanistXHPASTLintNamingHook::isUppercaseWithUnderscores($test), + "UPPERCASE_WITH_UNDERSCORES: '{$test}'"); + $this->assertEqual( + $expect[3], + ArcanistXHPASTLintNamingHook::isLowercaseWithUnderscores($test), + "lowercase_with_underscores: '{$test}'"); + } + } + + public function testStripUtilities() { + // Variable stripping. + $this->assertEqual( + 'stuff', + ArcanistXHPASTLintNamingHook::stripPHPVariable('stuff')); + $this->assertEqual( + 'stuff', + ArcanistXHPASTLintNamingHook::stripPHPVariable('$stuff')); + + // Function/method stripping. + $this->assertEqual( + 'construct', + ArcanistXHPASTLintNamingHook::stripPHPFunction('construct')); + $this->assertEqual( + 'construct', + ArcanistXHPASTLintNamingHook::stripPHPFunction('__construct')); + } + +} diff --git a/src/lint/linter/xhpast/naminghook/__tests__/__init__.php b/src/lint/linter/xhpast/naminghook/__tests__/__init__.php new file mode 100644 index 00000000..adfbd132 --- /dev/null +++ b/src/lint/linter/xhpast/naminghook/__tests__/__init__.php @@ -0,0 +1,13 @@ +