Summary: See inline comments on D9576. Also replace `json_decode` with `phutil_json_decode`, for better error handling.
Test Plan: Make sure `arc unit` still works.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9656
Summary: Various tidying up of linting code.
Test Plan: `arc lint` and `arc unit` still pass.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9625
Summary: Fixes T5385. Provide a flexible means of setting a minimum PHP version for the `ArcanistXHPASTLinter`, instead of relying on `ArcanistXHPASTLinter::LINT_PHP_53_FEATURES` and `ArcanistXHPASTLinter::LINT_PHP_54_FEATURES`.
Test Plan: Fixed up and ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5385
Differential Revision: https://secure.phabricator.com/D9576
Summary: This linter rule is very specific to the Phabricator and it is unlikely that it is being used elsewhere. We should move it to `ArcanistPhutilXHPASTLinter`, where other Phabricator-specific linting code lives.
Test Plan: Moved the corresponding test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9590
Summary: Allows the `ArcanistXHPASTLinter` to determine whether constants can be used, based on the target PHP version. Also added class methods to the compatibility information, although this isn't used yet (it is small anyway).
Test Plan: Created a test file that contained the `JSON_PRETTY_PRINT` constant. Verified that a linter error was raised.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9571
Summary:
Fixes T5377. The current `scripts/update_compat_info.php` script works for PHP CompatInfo version 2, but doesn't work with the newer version 3.
There are a few breaking changes in version 3 that had to be addressed:
- PHP 5.3 is required. Whilst Arcanist is generally compatible with PHP 5.2, I don't think that having this dependency presents any real issues because it is purely a development tool that is rarely updated.
- [[https://getcomposer.org/ | Composer]] is used for packaging, which makes including the library slightly more complicated. Basically, I had to install `PHP_CompatInfo` globally with Composer (`composer global require "bartlett/php-compatinfo"` and then symlink `~/.composer/vendor` into `externals/includes`.
Test Plan: Compared the `resources/php_compat_info.json` file. There are a bunch of functions/classes that //were// in this file but are no longer, but I think that I've covered the most popular extensions.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5377
Differential Revision: https://secure.phabricator.com/D9568
Summary: Currently, class and interface names are converted to lowercase when comparing minimum and maximum versions. This is necessary in order to compare with the data from `php_compat_info.json` but causes the lint message to be slightly misleading.
Test Plan:
Linted a test file.
**Before**
```
Error (XHP31) Use Of PHP 5.3 Features
This codebase targets PHP 5.2.3, but `iterator` was not introduced until
PHP 5.3.0.
1 <?php
2
>>> 3 final class FooBar implements Iterator {}
```
**After**
```
Error (XHP31) Use Of PHP 5.3 Features
This codebase targets PHP 5.2.3, but `Iterator` was not introduced until
PHP 5.3.0.
1 <?php
2
>>> 3 final class FooBar implements Iterator {}
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9471
Summary: Applied various linter fixes. Also make the `.editorconfig` file a bit more specific. Unfortunately, `arc lint --apply-patches` currently modifies some test data that it shouldn't, but this should be fixed after T5105.
Test Plan: Ran `arc unit` to make sure things weren't broken.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9440
Summary: Currently, `'foo'. // Some comment` is not allowed by the `ArcanistXHPASTLinter::LINT_BINARY_EXPRESSION_SPACING` rule. I believe that in the case of a trailing comment, we //should// allow whitespace after the `.` operator.
Test Plan: Wrote and executed a unit test for this case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9438
Summary: Fixes T5298. This bug was probably introduced in D9248. It looks like I forgot to update some references to `$version`.
Test Plan: Ran `arc lint --everything` in rPHU.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5298
Differential Revision: https://secure.phabricator.com/D9434
Summary: Fixes T5209. It makes sense for the `ArcanistXHPASTLinter` to implement the `getVersion` method in order to be able to enforce version requirements (as in T4954).
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5209
Differential Revision: https://secure.phabricator.com/D9317
Summary: We have coverage for normal binary operators, but not these unusual cases.
Test Plan: Added and executed unit tests.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9314
Summary: Ran `arc lint --apply-patches --everything` over rARC, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed //most// of the diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9269
Summary: Ref T5141. In order to be able to warn when deprecated functions are used, we need to be aware of from which version the functions were deprecated.
Test Plan: Ran `arc lint` and made sure no unexpected warnings were raised.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5141
Differential Revision: https://secure.phabricator.com/D9248
Summary: Ref T5141. Currently, `php_compat_info.json` is hardcoded to support PHP 5.2.3. Instead, store as much version information as possible in `php_compat_info.json` and filter accordingly in `ArcanistXHPASTLinter.`
Test Plan: Ran `arc lint` and made sure no additional warnings were raised.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5141
Differential Revision: https://secure.phabricator.com/D9247
Summary:
Currently, when code has a block like:
} catch (Exception $ex) {
...we attempt to mark "$ex" as declared. However, we incorrectly mark every variable used anywhere in the block as declared. This means we'll never raise this warning in a `catch` block.
Instead //only// mark the caught exception as declared.
Test Plan: Added a failing unit test and made it pass.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: lpriestley, epriestley
Differential Revision: https://secure.phabricator.com/D9239
Summary: The XHPAST double quotes rule autofixes strings which use double quotes where single quotes would suffice. In the case in which the double-quoted string contained an escaped double quote, the autofix string can be improved by removing the escape symbol.
Test Plan: Modified existing unit tests to cover this case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9196
Summary: Previously, the `ArcanistXHPASTLinter` was suggesting that `"\1"` be written as `'\1'`, which is incorrect. Therefore, I ensured that all of the cases listed in the [[http://www.php.net/manual/en/language.types.string.php | PHP documentation]] were handled correctly.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9182
Summary: Personally, I am a strong fan of this rule. There is currently a similar rule provided by PHP_CodeSniffer.
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/D9117
Summary: Ref T2039. The type specs are right in theory but not quite correct in practice, since we pass strings in rather than objects.
Test Plan: While tweaking `phabricator/`, adjusted these to get desirable results.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9071
Summary: Ref T2039. It looks like "phutil-library" didn't make it over, I'll add that to the other two (unless I'm wrong and this isn't an oversight?).
Test Plan: Used `arc linters` to read help.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9070
Summary: It seems that there is a lot of overlap between `getConfig` / `setConfig` and `getLinterConfigurationOptions` / `setLinterConfigurationValue` respectively.
Test Plan: `arc lint` and `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9067
Summary: Ref T2039. The `.arclint` file is reasonably complete now and we should start using it if possible, since we are trying to recommend it to others.
Test Plan: `arc lint`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9057
Summary:
Currently, the Phutil XHPAST Linter and vanilla XHPAST Linter reuse the same parse tree, but do this by having explicit knowledge of one another.
Instead, let them synchronize by writing to a glorified array of globals on the Engine. They no longer require knowledge of one another, so this can work under `.arclint`.
(This could probably be a little cleaner by putting more logic in the shared base class, but Facebook has some kind of goofy subclass of this thing and //this// patch won't disrupt it, while a cleaner one might.)
This should unblock D9057.
Test Plan: Ran unit tests and normal lint, got accurate looking results without duplicate invocations showing up in `--trace`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9059
Summary: Allow `@todo` comments to be linted as TODOs as well as `TODO` comments.
Test Plan: I added a new test case (`todo.lint-test`)
Reviewers: #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8389
Summary:
Lingers on from D7271; Rename `ArcanistWorkingCopyIdentity.getConfig()`.
Changed all linters (Except one) to use `getConfigFromAnySource()`, because it seems to make sense.
Test Plan: arc unit --everything; arc lint in github.com:epriestley/arclint-examples.git (Except for phpcs, flake8, cpplint and csslint which I don't have installed).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: chad, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7382
Summary:
Create a new class for them, pass instance around as need.
This looks like it's mostly working, but I'd like to replace the various `new ArcanistConfigurationManager()`
calls with something more suitable.
And maybe get a better name for ArcanistConfigurationManager ("Configuration" is already taken).
Test Plan: arc unit --everything, and then some.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, chad
Differential Revision: https://secure.phabricator.com/D7271
Summary: Automatically correct `New` to `new` in lint.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6948
Summary: Also warn against functions not available on Windows at all.
Test Plan: Compared old and new file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5975
Summary: Detect and fix unconventional spellings of `true`, `false`, `null` and `array` (these are the only keywords I've seen spelled unconventionally in the wild).
Test Plan: Unit tests.
Reviewers: DurhamGoode, btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2985
Differential Revision: https://secure.phabricator.com/D5686
Summary:
Added a lint rule that warns about reusing iterator reference
variables.
Test Plan:
- Add a file with examples found in with https://secure.phabricator.com/T2536
- Did not make a unit test yet
Reviewers: vrana, bill, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Maniphest Tasks: T2536
Differential Revision: https://secure.phabricator.com/D5179
Summary:
The perf fix actually catches some real problems.
I didn't find anything in libphutil, Arcanist and Phabricator though.
Also bump version.
Also allow configuring the hook.
Test Plan:
Added a test, saw it fail with the old code.
Repeat for hook.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5097
Summary: At least on my sample file.
Test Plan: Saw time 0.073 s instead of 12.606 s.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5086
Summary:
The message is too defensive.
Test Plan: Tested in the fork.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin, s.o.butler
Differential Revision: https://secure.phabricator.com/D5043
Summary: This is a little bit tricky - if both XHPAST and PhutilXHPAST linters lint the same path then they get the same future wrapped in two different Future iterators.
Test Plan:
$ arc unit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5015
Summary: This is technically documented, but not currently enforced and we aren't consistent about it in the codebase.
Test Plan: See D5002.
Reviewers: chad, vrana
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5003
Summary:
We always generate all messages and then filter them out based on minimum severity.
It's lots of useless work, especially in commit hook mode where we are interested only in errors.
Test Plan:
$ arc lint --cache 0 --severity error ArcanistXHPASTLinter.php
0.406 s before, 0.074 after
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4963
Summary: We only caught half of this.
Test Plan: Unit test.
Reviewers: edward
Reviewed By: edward
CC: aran
Maniphest Tasks: T1261
Differential Revision: https://secure.phabricator.com/D4920
Summary: It's not trivial to find them inside 700+ lines long functions.
Test Plan:
Linted `reused-iterators.lint-test` renamed to `_.php`, saw other locations.
Repeated for `reused-local.lint-test`.
Repeated for `duplicate-key-in-array.lint-test`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4871
Summary:
We want to use it for `yield` and `invariant_violation()` which throws.
Having node instead of token would be better but this would be enough.
Test Plan: Implemented a hook in FB repo and added a test case there.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4821
Test Plan: Didn't see a fatal in new test case.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4783
Summary: Also provides an example how to build custom linter using XHPAST.
Test Plan: Added debug output to `willLintPaths()`, verified that each path is parsed only once.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4718
