Summary:
When importing or aliases a symbol with a `use` statement, the leading namespace separator is optional and does not modify the behavior. That is, `use \X` is equivalent to `use X`. As such, the latter syntax should be preferred because it is more concise.
According to the [[http://php.net/manual/en/language.namespaces.importing.php | PHP documentation]]:
> Note that for namespaced names (fully qualified namespace names containing namespace separator, such as `Foo\Bar` as opposed to global names that do not, such as `FooBar`), the leading backslash is unnecessary and not recommended, as import names must be fully qualified, and are not processed relative to the current namespace.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14517
Summary: This is failing locally with `cppcheck` version 1.69.
Test Plan: Ran `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14524
Summary: Fix the `PHPCompatibilityXHPASTLinterRule` after changes to the way in which #xhpast parses `use` statements. Depends on D14518.
Test Plan: Unit tests
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14519
Summary:
Although it is technically possible to return a value from a PHP constructor, it is rather odd and should be avoided. See some discussion on [[http://stackoverflow.com/q/11904255 | StackOverflow]]. Consider the following example:
```lang=php
class SomeClass {
public function __construct() {
return 'quack';
}
}
```
This doesn't work:
```lang=php, counterexample
echo new SomeClas();
```
This, strangely, does work:
```lang=php
echo id(new SomeClass())->__construct();
```
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14516
Summary:
Add a linter rule to detect static method calls which //should// reference the `parent` class instead of a hardcoded class reference. For example, consider the following:
```lang=php, counterexample
class SomeClass extends AnotherClass {
public function someMethod() {
AnotherClass::someOtherMethod();
}
}
```
This should instead be written as:
```lang=php
class SomeClass extends AnotherClass {
public function someMethod() {
parent::someOtherMethod();
}
}
```
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D14443
Summary:
Separate XHPAST linter rules in isolation from other rules and formalize the concept of a "linter rule". As the number of XHPAST linter rules grows (we currently have around 80), it becomes increasingly difficult to manage all of the test cases because `ArcanistXHPASTLinterTestCase` currently tests the entire linter (i.e. //all// linter rules) rather than testing individual rules in isolation. See D13534 for a situation in which this is painful. This is particularly bad for third party development because unit tests could break at any time depending on upstream changes.
Basically, in order to facilitate the unit testing of XHPAST linter rules in isolation, I have made the following changes:
# Added a `setRules()` method to `ArcanistXHPASTLinter`. Currently, `ArcanistXHPASTLinter` loads all `ArcanistXHPASTLinterRule` subclasses unconditionally. The `setRules()` method provides a way to override the configured linter rules.
# Formalize the concept of a "linter rule". The `ArcanistXHPASTLinterRule` class was introduced in D10541. I feel that the modularization of `ArcanistXHPASTLinter` has made the linter much more maintainable and easily testable and I intend to extend this concept to other linters, such as `ArcanistTextLinter`.
Test Plan: Ran unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14010
Summary: Adjusts `ArcanistBraceFormattingXHPASTLinterRule` after changes to the way in which XHPAST parses namespaces. Depends on D14498.
Test Plan: Unit tests are now passing.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14499
Summary:
csslint offers some diagnostics related to all the document without any
relevant line number.
In such case, arc lint failed, as setLine expects null or an integer,
but not an empty string.
The 'char' and 'line' attributes in XML report are now optional.
Fixes T9804.
Test Plan: test case to repro the issue added
Reviewers: chad, joshuaspence, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9804
Differential Revision: https://secure.phabricator.com/D14497
Summary: I've been thinking about this for a while... why not just fold `ArcanistPhutilXHPASTLinter` into `ArcanistXHPASTLinter`?
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13867
Summary: Ref T8742. As mentioned in D13512. This still needs some work, but looks roughly how I expect it to. Mainly, I want to move the standards stuff to the linter itself rather than the linter rule. I wanted to push this out for some initial feedback though.
Test Plan: This still needs work.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8742
Differential Revision: https://secure.phabricator.com/D13942
Summary:
Linters can now use the `version` configuration value to specify the required
version of the external binary. The version number may be prefixed with <, <=,
>, >=, or = to specify the version comparison operator (default: =).
PHP's native `version_compare()` function is used to perform the comparison.
Fixes T4954.
Test Plan: Tested against a sample of external linters.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, joshuaspence
Projects: #lint
Maniphest Tasks: T4954
Differential Revision: https://secure.phabricator.com/D14298
Summary:
See discussion in D13737. If you're using this linter to match messages which //sometimes// have a character, you can get `""` (empty string) matches when the expression doesn't match. We'll complain about these later.
Instead, cast the matches the expected types.
Test Plan: @csilvers confirmed fix, see D13737.
Reviewers: chad, csilvers
Reviewed By: csilvers
Subscribers: spicyj, csilvers
Differential Revision: https://secure.phabricator.com/D14307
Summary:
This is in a similar vein as D14220 and sets a name on linter
messages. This should handle issues from D14165.
Test Plan: Run as many of the changed linters as possible + existing linter tests.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14225
Summary: Fixes T9498
Test Plan: Run `arc lint` with errors that get caught and reported by `ArcanistClosureLinter`
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T9498
Differential Revision: https://secure.phabricator.com/D14220
Summary: Ref T9145. Fixes T9316. We now require "name" and "code" has a maximum length (currently, this is 32, but the next diff will raise it to 128).
Test Plan:
- Installed PHPCS.
- Hit both the "name" and "code" issues.
- Applied this patch.
- Got better errors sooner.
Reviewers: chad
Reviewed By: chad
Subscribers: aik099
Maniphest Tasks: T9145, T9316
Differential Revision: https://secure.phabricator.com/D14165
Summary: Closes T9353. I think this makes all descriptions at least basically informative.
Test Plan: arc linters - I can now understand what each one is about.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9353
Differential Revision: https://secure.phabricator.com/D14106
Summary: Ref T7215. This test is occasionally failing for me locally. Remove it as per D13992.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T7215
Differential Revision: https://secure.phabricator.com/D14027
Summary: Adds two different linter rules (one general purpose and another PHP specific) to prevent empty files from being added to a repository. For some unknown reason, people seem to like to do this.
Test Plan: Added test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: avivey, cburroughs, Korvin
Differential Revision: https://secure.phabricator.com/D13881
Summary: `<?php\n\n...` is much easier to read than `<?php\n...`. Depends on D13889 and D13890.
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13534
Summary: Ref T9134. It looks like this functionality was removed in D13848. Depends on D13869.
Test Plan: Ran `arc diff`, `arc lint` and `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T9134
Differential Revision: https://secure.phabricator.com/D13868
Summary: Fixes T9257. For some messages, PyLint can raise at "character -1", which we don't allow since we don't consider it to make sense.
Test Plan:
- Added failing unit test from T9257.
- Applied patch.
- Test now passes.
Reviewers: joshuaspence, chad
Reviewed By: joshuaspence, chad
Maniphest Tasks: T9257
Differential Revision: https://secure.phabricator.com/D13991
Summary:
Fixes T7215. See D13991. These test cases have been failing intermittently for a while.
I think the XML stuff (which we don't control) changed where it raises these warnings: an old version raised them at the end of the attribute, while the new version raises them at the beginning of the attribute. Not totally 100% sure about this since installing multiple versions is fairly inconvenient, but as far as I know both versions raise the warnings, just at different character offsets.
We could do various things to fix these tests (allow the warning to raise at any character, skip the tests based on versions, etc) but I think it's easier to just remove the tests. They don't seem valuable.
Test Plan: Tests failed prior to change; now pass.
Reviewers: chad, joshuaspence
Reviewed By: joshuaspence
Maniphest Tasks: T7215
Differential Revision: https://secure.phabricator.com/D13992
Summary:
Improves upon D13795 to correctly handle variables within strings. Specifically, the following code currently (incorrectly) warns about `$x` being undeclared:
```lang=php
function some_func() {
return function ($x) {
echo "$x";
};
}
```
It's worth noting that the situation would be improved if XHPAST properly parsed strings (see T8049).
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13938
Summary: This was taken from D13569.
Test Plan: `arc lint` still works.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13943
Summary: Removed excess quotations on the `--msg-template` option as it was interfering with the string-int coercion
Test Plan: Unsure
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: joshuaspence, epriestley, #blessed_reviewers
Subscribers: joshuaspence, e-m-albright, Korvin
Maniphest Tasks: T9214
Differential Revision: https://secure.phabricator.com/D13931
Summary: Adds a basic linter for ensuring that `composer.lock` files are up-to-date.
Test Plan: We have been using this in a private project for around a month.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: johnny-bit, Korvin
Differential Revision: https://secure.phabricator.com/D13883
Summary: Currently, linting PHP short array syntax (i.e. `[...]`) throws an exception ("Expected open parentheses"). This diff relaxes some restrictions which prevent short array syntax from linting with `ArcanistParenthesesSpacingXHPASTLinterRule`.
Test Plan: Modified test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: agenticarus, Korvin
Differential Revision: https://secure.phabricator.com/D13895
Summary: Improve `ArcanistInlineHTMLXHPASTLinterRule` such that it doesn't raise duplicate warnings. Also be a bit more lax with whitespace.
Test Plan: Unit tests now pass.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13896
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
Summary: Add a linter rule to prevent trailing commas in a list assignment. `list($x, $y,)` is equivalent to `list($x, $y)`, but the latter is cleaner and should be preferred.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13870
Summary: This is a test case for `ArcanistXHPASTLinter`, not `ArcanistPhutilXHPASTLinter`.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13866
Summary: Modify `ArcanistParenthesesSpacingXHPASTLinterRule` and `ArcanistCallParenthesesXHPASTLinterRule` to apply to `array()` and `list()` as well. Depends on D13858.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13859
Summary: This test case is failing on JSHint v2.8.0.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13860
Summary:
This linter rule fails on multi-line arrays with no whitespace before the first array value. Specifically, the following exception is thrown:
```
Fatal error: Call to a member function isAnyWhitespace() on boolean in arcanist/src/lint/linter/xhpast/rules/ArcanistArrayValueXHPASTLinterRule.php on line 40
```
Test Plan: Added another test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13856
Summary: Add a linter rule to ensure that array elements occupy a single line each.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13842
Summary: Use `PhutilClassMapQuery` instead of `PhutilSymbolLoader`, mainly for consistency. Depends on D13572.
Test Plan: This hasn't been tested very comphrehensively as of yet.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13573
Summary:
Fixes T8674. Currently, `ArcanistSelfMemberReferenceXHPASTLinterRule` warns if a fully-qualified class name is used in place of `self`. This is fine in most cases, but in some specific scenarios fails for PHP 5.3 because `self` (and also `$this`) cannot be used in an anonymous closure (see [[http://php.net/manual/en/functions.anonymous.php | anonymous functions]] and [[https://wiki.php.net/rfc/closures/removal-of-this | removal of `$this` in closures]]).
In order to do this, I also had to modify the manner in which configuration was passed to individual linter rule. Previously, it was only possible or an individual linter rule to be set with a configuration value. Once the linter rule had set this value, there was no means by which to allow this value to be shared amongst linter rules.
Depends on D13819.
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T8674
Differential Revision: https://secure.phabricator.com/D13820
Summary:
Improve `ArcanistUselessOverridingMethodXHPASTLinterRule` by allowing overriding methods which set default values. For example, the following scenario is perfectly valid:
```lang=php
class SomeClass {
public function __construct($x) {}
}
class SomeOtherClass extends Class {
public function __construct($x = null) {
parent::__construct($x);
}
}
```
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13840
Summary: Ref T8674. Adds to `ArcanistPHPCompatibilityXHPASTLinterRule` such that an error is raised whenever `self` or `$this` is used in an anonymous closure prior to PHP 5.4.
Test Plan: Added test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T8674
Differential Revision: https://secure.phabricator.com/D13841
Summary: Ref T8742. Anonymous functions should have a space after the `function` keyword. Additionally, ensure that there is a space before and after the `use` token.
Test Plan: Modified existing test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8742
Differential Revision: https://secure.phabricator.com/D13804
