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: 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
Summary: `pht`ize a bunch of strings in `ArcanistXHPASTLinter`.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12392
Summary: Ref T7409. If `::` is surrounded by whitespace tokens and the whitespace token contains a newline character, allow it.
Test Plan: Added test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7409
Differential Revision: https://secure.phabricator.com/D12391
Summary: Improve `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR` in handling multi-line arrays, see D12280 and D12281 for example. Depends on D12295.
Test Plan: Updated unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12296
Summary: Fixes T7464. JSHint is unable to exclude files from linting (via the `jshintignore` file) if the data is piped through `STDIN`. An alternative would be to pass `$options[] = '--filename='.$this->getActivePath()`, but `$this->getActivePath()` is not set when the command arguments are constructed.
Test Plan: Created a file and linted it with `ArcanistJSHintLinter`. Was able to exclude the file from linting with a `jshintignore` file.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7464
Differential Revision: https://secure.phabricator.com/D12298
Summary: The format of this file has changed. Depends on D12278.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12279
Summary:
Ref T7409. Adds a rule to detect unnecessary semicolons. The most common scenario I've seen in the wild is the use of semicolons after a class definition:
```lang=php
class MyClass {
// ...
};
```
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/D12139
Test Plan:
Add the following JSON to your `.arclint`:
```lang=json
"rubocop": {
"type": "rubocop",
"include": "(\\.rb$)"
}
```
Then, add a ruby file with errors, for instance:
```lang=ruby
def hello()
puts 'world'
end
```
Run `arc lint`. It should come up with something along the line of: "Omit the parentheses in defs when the method doesn't accept any arguments."
Reviewers: joshuaspence, remon, #blessed_reviewers, epriestley
Reviewed By: joshuaspence, #blessed_reviewers, epriestley
Subscribers: reu, calfzhou, jjooss, cburroughs, chad, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10738
Summary: After all that, I forgot to change this back.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11735
Summary:
pep8 has used both 2 (`1.2`) and 3 (`1.2.1`) digit versions. Losen
the version check to allow for both.
NOTE: This is the same regex as flake8.
Test Plan: `arc unit` with a 2 and 3 digit pep8 version on `$PATH`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, epriestley
Differential Revision: https://secure.phabricator.com/D11728
Summary: Fixes T7046. Adds a linter rule to detect mismatched parameters for formatted strings. Originally I had considered putting this rule in `ArcanistPhutilXHPASTLinter`, but I later decided to move it to `ArcanistXHPASTLinter` as I think that it is general enough to be more widely useful.
Test Plan: This seems to work but needs some polish.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7046
Differential Revision: https://secure.phabricator.com/D11661
Summary: It should be safe to remove this now.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11714
Summary: I don't think that this provides too much value. I think that we should rework this to be inferred from the `.arcconfig` file perhaps?
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11662
Summary: There is no need to check if the XHPAST binary is available here because this linter does not actually parse PHP, it only considers the library map.
Test Plan: Removed the XHPAST binary and ran `arc lint` on a bunch of files.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11528
Summary: Use `PhutilXHPASTBinary` methods instead of `xhpast_parse` functions. Depends on D11517.
Test Plan: N/A, this is a direct swap.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11613
Summary: Rely on the output of `xhpast --version` when determing the lint cache key.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11611
Summary: These tests are failing with the latest version of `jscs` (v1.10.0).
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11520
Summary: A bunch of unit tests are failing with the latest version of `lessc` (v2.3.0). I decided to delete a bunch of test cases for this linter as we have far too many at the moment.
Test Plan: `arc unit`
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11521
Summary: This requires PHP 5.4. Use `newv()` instead. I don't think the "without constructor" part is meaningful (or desirable). Simplify slightly.
Test Plan: `arc unit --everything`
Reviewers: hach-que, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11450
Summary: With a special guest appearance from `ArcanistGeneratedLinter`.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11345
Summary: Fixes T5639. Allows the detection of undefined magic symbols, such as the use of `__DIR__` in a codebase that targets < PHP 5.3.0.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T5639
Differential Revision: https://secure.phabricator.com/D11386
Summary: Fixes T6830. Don't require `n_STATEMENT` nested under `n_DECLARE` to be written using braces.
Test Plan: Added a test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6830
Differential Revision: https://secure.phabricator.com/D11350
Summary: The expectation is that this linter only raises errors.
Test Plan: This is mostly theoretical.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11331
Summary: Using `--reporter=raw` exposes raw JSON output rather than a limited XML output. This allows us to pull more context from the `coffeelint` output.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11320
Summary: It is more common for linters to exit with a non-zero status than it is for linters to return with a zero exit status, Really this function serves very little purposes, it simply determines whether or not to throw an exception if a non-zero status is returned by the external linter.
Test Plan: `arc unit`
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11322
Summary: `false` is the default return value for this method.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11314
Summary: `csslint` returns an `evidence` field which contains the line of the "original text" which produced the linter message (see 5dd84b259b/src/core/Reporter.js (L64)). We can use this data instead of trying to achieve the same result using `$this->getData($path)`.
Test Plan: {F265449}
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11311
Summary: Similar to D11198. This flag doesn't do anything when combined with `--format=lint-xml`.
Test Plan:
```lang=bash
> csslint *.css
csslint *.css
csslint: There are 1 problems in /home/joshua/workspace/github.com/phacility/arcanist/fail.css.
fail.css
1: error at line 1, col 1
Unexpected token '~' at line 1, col 1.
~
csslint: No errors in /home/joshua/workspace/github.com/phacility/arcanist/pass.css.
> csslint --quiet *.css
csslint --quiet *.css
csslint: There are 1 problems in /home/joshua/workspace/github.com/phacility/arcanist/fail.css.
fail.css
1: error at line 1, col 1
Unexpected token '~' at line 1, col 1.
~
> csslint --format=lint-xml *.css
<?xml version="1.0" encoding="utf-8"?><lint>
<file name="/home/joshua/workspace/github.com/phacility/arcanist/fail.css"><issue line="1" char="1" severity="error" reason="Unexpected token '~' at line 1, col 1." evidence="~"/></file>
</lint>
> csslint --format=lint-xml --quiet *.css
<?xml version="1.0" encoding="utf-8"?><lint>
<file name="/home/joshua/workspace/github.com/phacility/arcanist/fail.css"><issue line="1" char="1" severity="error" reason="Unexpected token '~' at line 1, col 1." evidence="~"/></file>
</lint>
```
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11308
Summary: The `src/lint/linter` directory is a bit cluttered at the moment.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11309
Summary: This seems to be off, at least for `puppet-lint` versions greater than 1.0.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11312
Summary: This test case is currently failing with various different versions of `puppet-lint`. I was considering fixing this test case, but I feel that we have more than enough test cases for `ArcanistPuppetLintLinter` anyway. Since this is an `ArcanistExternalLinter`, we only really need to test that the bindings are working as expected (rather than the linter functionality).
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11313
Summary: Without explicitly passing `--error-level=all` to `puppet-lint`, the configuration file for `puppet-lint` could contain `--error-level=error`. This would limit the ability of `arc lint` to detect linter issues and therefore we should override this flag from the command line explicitly.
Test Plan: This is mostly theoretical.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11316
Summary: We can use `php --run "echo phpversion();"` to return a nice, compact representation of the PHP version.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11264
Summary: I'm not sure whether this is in any coding standards, but it seems to be an unspoken rule that methods should have a visiblity modifier (`public`, `protected` or `private`) explicitly specified. According to the [[http://php.net/manual/en/language.oop5.visibility.php#language.oop5.visiblity-methods | PHP documentation]], methods declared without any explicit visibility keyword are defined as public.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10687
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `ArcanistExternalLinter` and `ArcanistLinter` subclasses.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11237
Summary: I feel that we are abusing `ArcanistUsageException`. Throw a more tailed exception instead. Depends on D11197.
Test Plan: `arc lint`, I guess.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: avivey, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11205
Summary: Creates a new base class for unit testing `ArcanistExternalLinter` subclasses. Specifically, add a test case for verifying that we are correctly parsing the output of `$external_linter --version`.
Test Plan: `arc unit`
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11197
Summary: According to `coffeelint --help`, `--noconfig` ignores the environment variable `COFFEELINT_CONFIG`. This means that `arc lint` will behave more consistently across developer workstations by forcing configuration to be specified with `coffeelint.config` instead of with environment variables.
Test Plan: This is mostly theoretical.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11196
Summary: According to `coffeelint --help` (with an up-to-date version of `coffeelint`, version 1.8.1) , `--nocolor` is deprecated in favor of `--color=never`. According to the [[http://www.coffeelint.org/#changelog | changelog]], `--nocolor` was deprecated in v1.6.0.
Test Plan: `arc lint`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11195
Summary: This is a bit magical and is maybe a terrible idea, but it seems okayish.
Test Plan: `arc unit`
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11172
Summary: Ref T5141. Utilize the `'max'` key from the `php_compat_info.json` compatibility map to lint for the use of classes/functions/symbols which have been removed from the target PHP version.
Test Plan: Added a test case for `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5141
Differential Revision: https://secure.phabricator.com/D10538
Summary: I don't feel that this linter rule belongs in the `ArcanistTextLinter`. In fact, this linter rule is quite similar to the rules provided by `ArcanistGeneratedLinter` and `ArcanistNoLintLinter` and these classes could possibly be consolidated. I have moved this linter rule to a standalone `ArcanistCommitLinter` class (which could possibly do additional lints in the future).
Test Plan: Moved existing test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10473
Summary: Self-explanatory. This is implied anyway as we iterate over it with a `foreach` loop.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11174
Summary: This doesn't make too much sense anymore as all modern linters are configured via the `.arclint` file rather than `.arcconfig`.
Test Plan: `grep`ped to make sure this option isn't used.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11173
Summary:
- The "exclude static variable access" branch in XHPAST incorrectly excluded all variables in a function or method which used any static access (we were selecting the wrong root node for exclusion).
- The "PHPLinter" regexp did not work with 5.4.30. Make it more permissive in what it parses.
Test Plan:
- Added a failing unit test for the variable case and made it pass.
- PHPLinter tests now pass under 5.4.30
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11087
Summary: This unit test is failing locally for me, using v1.1.0 of `puppet-lint`.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11057
Summary: Extend the `ArcanistXHPASTLinter::LINT_BRACE_FORMATTING` rule to support a bunch of extra cases.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10572
Summary: Extend the `ArcanistXHPASTLinter::LINT_BRACE_FORMATTING` rule to raise a warning when `n_STATEMENT` is used without having `n_STATEMENT_LIST` as a parent. Essentially, this means that `if ($x) { do_y(); }` is preferred over `if ($x) do_y();`.
Test Plan: Added some test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10571
Summary: Extend `ArcanistXHPASTLinter::LINT_KEYWORD_CASING` to include the `class` keyword.
Test Plan: I can't really added a test case for this without getting "XHP19 Class-Filename Mismatch".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10653
Summary: Minor change, self-explanatory... this seems to make sense.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10476
Summary: Adds an `ArcanistXHPASTLinter::LINT_CONSTRUCTOR_PARENTHESES` rule which ensures that (possibly empty) parentheses are used when calling a constructor.
Test Plan: Added test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10597
Summary: In PHP 5.4, the `break` and `continue` statements no longer accept variable arguments (e.g., `break 1 + foo() * $bar;`). Static arguments still work, such as break 2;. As a side effect of this change `break 0;` and `continue 0;` are no longer allowed.
Test Plan: Added some test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10567
Summary: It is not correct to add a trailing comma if the `n_ARRAY_VALUE` node is a heredoc string.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10569
Summary: The `LINT_ARRAY_SEPARATOR` rule was added to `ArcanistXHPASTLinter` in D10535. Unfortunately, it does not correctly handle nested arrays correctly. This patch fixes this. Depends on D10554.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10558
Summary:
Adds a rule to `ArcanistXHPASTLinter` which ensures that:
# Single-lined arrays //do not// contain an unnecessary trailing comma separator after the final array value.
# Multi-lined arrays //do// contain a trailing comma seperator after the final value value.
Depends on D10534.
Test Plan: Wrote and executed unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10535
Summary: I had assumed that we would always be interested in `n_STATEMENT_LIST`, which is fine for `if (...) { some_statements(); }` but not for `if (...) one_statement();`.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10551
Summary: Improve the behavior of `ArcanistTextLinter` in dealing with `LINT_DOS_NEWLINE`. In particular, provide replacement text so that these linter issues can be autofixed.
Test Plan: Added a unit test.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10472
Summary: Explicitly list PHP magic methods (methods beginning with `__`) instead of assuming that //all// methods beginning with `__` are okay in terms of naming conventions. These magic methods were obtained from http://us1.php.net/manual/en/language.oop5.magic.php.
Test Plan:
```
Warning (XHP9) Naming Conventions
Follow naming conventions: methods should be named using lowerCamelCase.
1 <?php
2
3 class Foo {
>>> 4 function __foo() {}
5
6 function _bar() {}
7 }
Warning (XHP9) Naming Conventions
Follow naming conventions: methods should be named using lowerCamelCase.
3 class Foo {
4 function __foo() {}
5
>>> 6 function _bar() {}
7 }
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10539
Summary: Self-explanatory, improve test coverage.
Test Plan: Wrote and executed unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10474
Summary: It seems that `severity.rules` does not override the default linter severities set by the `ArcanistLinter` itself. This seems incorrect and counter-intuitive.
Test Plan: Created a `"severity.rules": { "(.*)": "disabled" }"` configuration and verified that it was being applied.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10475
Summary: D10132 improved this check by eliminating some false postives, but assumes an `n_FUNCTION_CALL` always starts with `n_SYMBOL_NAME`. This is true for `f()`, but not true for `C::m()` or `$v()`.
Test Plan: Added and ran unit tests.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10467
Summary:
Fixes T5299. Currently, linting `src/future/http/HTTPSFuture.php` from rPHU will raise the following error:
```
Error (XHP31) Use Of PHP 5.3 Features
This codebase targets PHP 5.2.3, but `curlfile` was not introduced until
PHP 5.5.0.
532 // use this "@" stuff.
533
534 if (class_exists('CURLFile')) {
>>> 535 $file_value = new CURLFile((string)$tmp, $info['mime'], $info['name']);
536 } else {
537 $file_value = '@'.(string)$tmp;
538 }
```
However, since this class is being used conditionally, it //should// be fine and no linter errors should be being raised.
Test Plan: Added a test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5299
Differential Revision: https://secure.phabricator.com/D10132
Summary: Fix linting for spacing around default parameter assignment in function/method declaration so that `function foo($x = null) {}` is preferred in favor of `function foo($x=null) {}`.
Test Plan: Modified existing unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10429
Summary: Adds an XHPAST linter rule for empty block statements. Basically, if a block statement is empty then it is much neater if the opening (`{`) and closing (`}`) braces are adjacent. Maybe this is just my own personal preference, in which case we could reduce the default severity to `ArcanistLintSeverity::SEVERITY_DISABLED`.
Test Plan: Wrote unit tests. I had to modify a bunch of existing unit tests accordingly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10434
Summary: Add some missing function parameter typehints to the `ArcanistPhutilXHPASTLinter` class.
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10430
Summary: `pht`ize some string in `ArcanistPhutilXHPASTLinter` so that they are translatable.
Test Plan: Not 100% sure that I am doing it right...
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10431
Summary: Update the `puppet-lint` linter to support the latest version (v1.0.1). This version allows a custom configuration file to be passed via `--config`, which brings this linter into line with other linters.
Test Plan: Update test cases to pass.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10384
Summary: Adds a linter that checks php files for syntax errors using php -l
Test Plan:
Add a section to your .arclint file similar to:
```
"php": {
"type": "php",
"include": "(\\.php$)"
}
```
Then run arc lint on a php file with a syntax error.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, jacobwalker0814, Korvin
Differential Revision: https://secure.phabricator.com/D10370
Summary: flake8 v2.2.3 (released Aug 25) broke this. Just drop the stdin stuff.
Test Plan: User confirmed in IRC that this resolves the issue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10358
Summary:
This adds a lint engine for `hlint`, which is the standard and most general Haskell lint tool around these days.
Signed-off-by: Austin Seipp <aseipp@pobox.com>
Test Plan: Install `hlint`, and run `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Projects: #arcanist
Differential Revision: https://secure.phabricator.com/D10250
Summary:
We currently require an exact number of "~" characters, but this is needless and error-prone.
Instead, allow any number larger than 3.
Test Plan:
- Added meta test-case.
- Ran `arc unit --everything`.
Reviewers: btrahan, thoughtpolice
Reviewed By: thoughtpolice
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10251
Summary: Rename `ArcanistPHPCSLinterTestCase` to `ArcanistPhpcsLinterTestCase` for consistency with the class being tested.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10145
