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
Summary: Unary postfix expressions should not have a space before the operator.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13805
Summary: Add a linter rule to check that there is no whitespace surrounding the object operator, `->`.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13798
Summary: Add a linter rule to ensure that there is no space between the operator and the operand in a unary prefix expression.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13797
Summary: Repurpose `ArcanistClosingCallParenthesesXHPASTLinterRule` (and rename it to `ArcanistCallParenthesesXHPASTLinterRule`) to ensure that there is no spacing before opening parenthesis in function calls.
Test Plan: Added test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13796
Summary: Ref T8921. See similar change in D13695. This expands the scope to also coerce `setChar()`.
Test Plan: `arc unit --everything`
Reviewers: btrahan
Subscribers: epriestley
Maniphest Tasks: T8921
Differential Revision: https://secure.phabricator.com/D13737
Summary: Fixes T8921. Harbormaster is strict about types it accepts, but `ArcanistLintMessage` is more liberal. Push the strictness barrier down to the linter level, while maintaining reasonable flexibility in the API.
Test Plan: `arc unit --everything`
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8921
Differential Revision: https://secure.phabricator.com/D13695
Summary: "Advice" is not a valid severity for Checkstyle... valid severities are `ignore`, `info`, `warning` and `error`.
Test Plan: Read the [[http://checkstyle.sourceforge.net/property_types.html | documentation]].
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13684
Summary: Improve `ArcanistClosingDeclarationParenthesesXHPASTLinterRule` (and rename it to `ArcanistDeclarationParenthesesXHPASTLinterRule`) to ensure that there is no whitespace before the opening parenthesis of a function/method declaration.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13675
Summary: I think that this output was used during the early stage of `ArcanistConfigurationDrivenLintEngine`, but I question it's value nowadays. In particular, I find that this output makes the output of `arc lint --trace` significantly less useful.
Test Plan: Ran `./bin/arc lint --trace` and saw useful output.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13593
Summary: Return `$this` from a bunch of setter methods for consistency. See also D13422.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13423
Summary: For consistency, a single space should separate `catch` and the following parenthetical expression.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13389
Summary: Add some tests to ensure that `ArcanistXHPASTLinterRule` subclasses are properly implemented. This should catch issues such as two linter rules having the same `ID` value. See D13272 for a similar change.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13274
Summary: All base classes should extend from `Phobject` or some other classes. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13281
Summary: If I understand correctly, all classes should extend from `Phobject`?
Test Plan: This needs some further work
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13275
Summary:
The `ArcanistXHPASTLinter` class is becoming quite bloated. This diff separates the class into one-class-per-rule, which makes everything much more modular. One downside to this decoupling is that code reuse between linter rules is much more difficult, although this only affects a very small number of linter rules.
There is still some further work that could be done here, but I defer this until a later diff:
- Rewrite `ArcanistPhutilXHPASTLinter` using `ArcanistXHPASTLinterRule`.
- Change the unit tests so that they are truly only testing a single linter rule.
- Maybe improve the way in which linter configuration options are handled.
- Make it easier to keep track of the linter rule IDs (see T6859).
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: johnny-bit, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10541
Summary: I found a few strings that I had missed, using a mostly-broken-but-somewhat-okay custom linter ruler (https://secure.phabricator.com/differential/diff/30988/).
Test Plan: Intense eyeballing.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aurelijus, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12888
Summary:
Currently, a lot of `ArcanistExternalLinter` subclasses have something like this:
```lang=php
if ($err && !$messages) {
return false;
}
```
We can avoid this code duplication by moving this check to the `ArcanistExternalLinter` class.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11321
Summary: This allows `ArcanistBaseXHPASTLinter::getXHPASTTreeForPath` to return the parse tree for a file that wasn't explicitly linted. In my particular use case, I am writing a linter rule to determine if it is necessary to import a PHP file with `require_once` (i.e. the file consists only of class/interface definitions which are autoloadable).
Test Plan: Tested externally using a custom linter.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12271
Summary: Ref T7674. `ArcanistXHPASTLinter` has some workarounds for running in "commit hook" mode (which has since been removed). This linter should always have a working copy.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7674
Differential Revision: https://secure.phabricator.com/D12956
Summary:
Removes the deprecated method of configuring linters (via the `.arcconfig` file). There is one main caveat here:
- There is currently no convenient method by which to change the path for an external linter (T5057). This means that there is no direct replacement for the deprecated `lint.ruby.prefix` configuration. The workaround is to symlink these binaries into `arcanist/externals/bin`.
Test Plan: Wait a sufficient amount of time before landing this.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aik099, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10005
Summary: Add some linter rule to detect invalid modifiers for class methods/properties.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12922
Summary: Ref T7977. The `ArcanistTestCase` class is pointless and can be replaced by `ArcanistPhutilTestCase`. Furthermore, it sorta makes sense to just rename `ArcanistPhutilTestCase` to `PhutilTestCase`. Depends on D12664 and D12666.
Test Plan: `arc unit`
Reviewers: avivey, #blessed_reviewers, epriestley
Reviewed By: avivey, #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12665
Summary: Improve the `ArcanistXHPASTLinter::LINT_ALIAS_FUNCTION` linter rule. Currently this rule does not correctly handle alias functions which are not strictly lowercase.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12921
Summary: Fixes T7417. Adds `ArcanistXHPASTLinter::LINT_MODIFIER_ORDERING` for ensuring that method/property modifiers (`public`, `protected`, `private`, `static`, `abstract` and `final`) are consistently ordered. The modifier ordering that is enforced is consistent with [[http://www.php-fig.org/psr/psr-2 | PSR-2]].
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7417
Differential Revision: https://secure.phabricator.com/D12421
Summary:
Improve the `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR` rule to be able to autofix the following code:
```lang=php
array(
1,
2,
3, /* comment */ );
```
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12308
Summary: Depends on D9430. If we are using the system installation of `pep8`, then it probably unnecessary (and possibly wrong) to specify `python2.6` as the default interpreter.
Test Plan: Ran `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: vrusinov, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9474
Summary: Currently, we bundle a specific version of `pep8` with Arcanist. Whilst maybe this made sense historically, as pointed out by @epriestley in D9412#6, there is no longer much point in doing so.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: talshiri, vrusinov, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D9430
Summary: This linter is a relic from Facebook days. As Harbormaster becomes more useful (T1049) this linter should become obsolete. Instead of modernising this linter to be compatible with modern infrastructure, it is easier to just let it die.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10533
Summary: Move the `getFunctionCalls` method to the `ArcanistBaseXHPASTLinter` so that it can be used by subclasses.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12472
Summary: Currently all CPPLint issues are hard-coded to warning level, which prevents customising the severity in .arclint. Change to pick up the configured severity. Note that getLintMessageSeverity will call getDefaultMessageSeverity if nothing is configured for that error category.
Test Plan: Tested manually to confirm configured categories display with the correct severity and that non-configured ones return with the default severity (ERROR).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9682
Summary: The `instanceof` operator expects the first argument to be an object instance. See http://www.phpwtf.org/instanceof-smart.
Test Plan: Added test case
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12856
Summary:
The `ArcanistXHPASTLinter::LINT_TOSTRING_EXCEPTION` linter rule was introduced in D12854, but fails for the following:
- Interfaces which declare the `__toString()` method.
- Classes which mark the `__toString()` method as `abstract`.
Test Plan: Added test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12892
Summary:
See http://phpsadness.com/sad/39. Declaring a function named `__lambda_func` prevents the `create_function` function from working. This is because `create_function` eval-declares the function `__lambda_func`, then modifies the symbol table so that the function is instead named `"\0lambda_".(++$i)`, and returns that name.
NOTE: Personally, I don't think that anyone should use `create_function`. However, despite this, I think it is reasonable that no function is named `__lambda_func` in case some external library relies on `create_function`.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12870
Summary: Fixes T8207. It is not possible to throw an exception from within the `__toString()` method. Add a linter rule to prevent this from happening.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8207
Differential Revision: https://secure.phabricator.com/D12854
Summary: Currently the error message from `ArcanistXHPASTLinter::LINT_FORMATTED_STRING` is slightly wrong, mentioning `xsprintf` instead of the actual `sprintf`-style function which was used. This diff changes the error message to be more correct.
Test Plan: Linted a file containing `sprintf('%s')` and saw a reasonable lint message.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12871
Summary: This linter rule was introduced in D12420, but fails to handle a specific edge case.
Test Plan: Added test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12853
Summary: Ref T7409. This linter rule was added in D12419, but fails in some specific cases. Improve the handling of `n_DECLARATION_PARAMETER_LIST`.
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7409
Differential Revision: https://secure.phabricator.com/D12828
Summary:
The following code is invalid:
```
final class MyClass {
public function __construct() {
parent::__construct(null);
}
}
$x = new MyClass();
```
Running the above code will produce a fatal error:
```
PHP Fatal error: Cannot access parent:: when current class scope has no parent
```
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12420
Summary:
Ref T2039. Convert the `ArcanistPylintLinter` to an `ArcanistExternalLinter` and make it compatible with `.arclint`. In doing so, it was necessary to drop support of older versions of `pylint` by setting the minimum version to be 1.0.0. I think that dropping support for older versions is reasonable because version 1.0.0 was released ~18 months ago. In the case than an incompatible version is detected, an `ArcanistMissingLinterException` is thrown.
One caveat here is that support for `lint.pylint.codes.(error|warning|advice)` is dropped. Any installs that are relying on this configuration will need to migrate to using the `.arclint` file for specifying linter severity. We could potentially continue to support this deprecated configuration, but I'm not sure if this is worthwhile.
Test Plan: Wrote and executed unit tests with `arc unit`. I ran the unit tests on all `pylint` releases after (and including) 1.0.0. Specifically, the tests were run on v1.0.0, v1.1.0, v1.2.0, v1.3.0 and v1.4.0.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9109
Summary: Use `__CLASS__` instead of hardcoding class names. Depends on D12804.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12805
Summary: Add a linter rule to advise against the use of hardcoded class names. Hardcoded class names make the code harder to refactor and it is generally preferable to use the `__CLASS__` magic constant instead.
Test Plan: This works, but needs some polish.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12605
Summary: Fixes T6311 and T5124 by returning all configured linters from `buildLinter()`, and making `ArcanistExternalLinter::checkBinaryConfiguration()` not crash if there's no executable to run.
Test Plan: `arc linters` in rP shows "Configured" and "ERROR" as appropriate; Adding a broken linter to `.arclint` in rARC doesn't invoke it's not actually needed, and prints error if it is.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: joshuaspence, epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6311
Differential Revision: https://secure.phabricator.com/D10773
Summary: Support and prefer configuration from .arclint over Configuration Manager
Test Plan: arc lint with several combinations of values in .arcconfig and .arclint.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: joshuaspence, epriestley, #blessed_reviewers
Subscribers: vrusinov, jpoehls, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10704
Summary: Ref T7674. Remove support for passing data to linters via `STDIN`. This functionality exists primarily for pre-receive workflows (which don't have a working copy to act on), but these workflows are going away soon.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7674
Differential Revision: https://secure.phabricator.com/D12696
Summary: Ref T7215. This test case is consistently failing locally (with a `LIBXML_VERSION` of `20902`).
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7215
Differential Revision: https://secure.phabricator.com/D12706
Summary: Move the `getSuperGlobalNames` method to `ArcanistBaseXHPASTLinter` so that it can be used within subclasses.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: virendra20888, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12473
Summary: Ref T7892. Memoize paths returned from `ARcanistLinter::getPaths()` to improve runtime performance.
Test Plan: Wiped ~0.5 seconds off the total time to lint rARC.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7892
Differential Revision: https://secure.phabricator.com/D12520
Summary: Ref T7892. Improve the performance of `ArcanistXHPASTLinter::LINT_UNNECESSARY_SEMICOLON`.
Test Plan: Wiped 6 seconds off the total time to lint rARC.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7892
Differential Revision: https://secure.phabricator.com/D12519
Summary: Ref T7892. Avoid reading the PHP compatibility information for every file being linted.
Test Plan:
```name=Before
real 1m24.327s
user 1m19.571s
sys 0m5.239s
```
```lang=After
real 1m12.029s
user 1m5.756s
sys 0m5.502s
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7892
Differential Revision: https://secure.phabricator.com/D12518
Summary:
Fixes T5097. When linting a large list of paths (e.g., with `--everything`), do this internally:
$chunks = array_chunk($paths, 32);
foreach ($chunks as $chunk) {
$this->lintChunk($chunk);
}
This keeps the advantages of parallelism and artifact sharing for future-based linters, without having memory usage grow in an unbounded way.
These callbacks changed:
- `willLintPath()`: Useless, no meaningful implementations. Internalized the required side effect and broke the hook.
- `didRunLinters()`: Now useless, with no meaningful implementations. Broke the hook.
- `didLintPaths()`: New hook which executes opposite `willLintPaths()`.
- `lintPath()`: Linters no longer need to implement this method.
XHPAST now has an explicit way to release shared futures.
These minor changes also happened:
- Formalized the "linter ID", which is a semi-durable identifier for the cache.
- Removed linter -> exception explicit mapping, which was unused. We now just collect exceptions.
- We do the `canRun()` checks first (and separately) now.
- Share more service call profiling code.
- Fix an issue where the test harness would use the path on disk, even if configuration set a different path.
Test Plan:
- Ran `arc lint --everything` in `arcanist/`.
- With no chunking, saw **unstable** memory usage with a peak at 941 MB.
- With chunk size 32, saw **stable** memory usage with a peak at 269 MB.
- With chunk size 8, saw **stable** memory usage with a peak at 180 MB.
- Ran with `--trace` and saw profiling information.
- Created this diff.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5097
Differential Revision: https://secure.phabricator.com/D12501
