Summary:
We want to have a class of lint problems which are displayed to author and attached to revision but don't require excuse in diff workflow.
Lint advices already serve this purpose but no linters emit them because they need `--advice` flag to be processed.
By always enabling advices, we can switch more linters from warnings to advices and don't stop the diff workflow for them.
This diff also bumps down default severity of TODO rule.
Test Plan: Made lint advice mistake, ran `arc lint`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, ide, Korvin
Differential Revision: https://secure.phabricator.com/D3364
Summary: See D3296#1.
Test Plan:
New test.
Linted Arcanist, libphutil, Phabricator repositories, found no false positive and one real positive.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3297
Summary:
I'm not sure if we are interested in this kind of linters ("Don't use slow function if there is a fast alternative").
I didn't find a case where `strpos() === 0` could be useful except [[ http://www.php.net/manual/en/mbstring.overload.php | mbstring.func_overload ]] craziness.
Test Plan:
New unit test.
Linted Arcanist, libphutil, Phabricator repositories, found no false positive and one real positive.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3296
Summary: XHPAST doesn't currently parse most PHP 5.4 stuff, but it does parse this. Warn about it.
Test Plan:
Unit tests, and:
Error (XHP35) Use Of PHP 5.4 Features
The f()[...] syntax was not introduced until PHP 5.4, but this codebase
targets an earlier version of PHP. You can rewrite this expression using
idx().
365 public function lintPHP54Features($root) {
366
367 if (false) {
>>> 368 id()[0];
^
369 }
370
Reviewers: alanh, vrana
Reviewed By: alanh
CC: aran
Differential Revision: https://secure.phabricator.com/D3291
Summary: On windows there is no 'which', only 'where'
Test Plan: Run JSHint linter on windows and unix-like system.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3096
Summary:
I use `//~` for marking places which shouldn't be committed.
It also looks ugly.
Maybe it can be just a warning but we provide a patch so error is OK?
Test Plan: New test.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2824
Summary:
Delete all code related to v1 libraries in arcanist.
When users liberate a v1 library, prompt them to upgrade.
Test Plan: Reverted phabricator/ to a couple of months ago and liberated it. Got prompted to upgrade. Upgraded.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2861
Summary: We will extract them at some point, lint before it's too late.
Test Plan:
New test.
Linted all callsites.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2825
Summary:
If a message has only "line", or "line" and "char", we don't point out the location in the context block.
When a message includes "line", show chevrons on the line.
When a message includes "line" and "char", show chevrons on the line and a caret on the next line.
Test Plan: Ran regex linters to capture line, line+char, and normal linters to capture everything. Output appeared sane.
Reviewers: csilvers, vrana, btrahan
Reviewed By: csilvers
CC: aran
Differential Revision: https://secure.phabricator.com/D2642
Summary: I wonder if this is not by purpose?
Test Plan: Modified two files, ran `arc lint`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2629
Summary: Mechanical changes from D2588. No "Class.php" moves yet because they aren't necessary for libraries to function.
Test Plan: See D2588.
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2589
Summary:
- "arc liberate" now works for v1 or v2 libraries.
- "arc liberate --upgrade" converts a v1 to a v2.
- We delete __init__.php files but do not move Class.php files, since this is vastly simpler. Authors can do this on their own if they want. We'll do it separately.
- v2 has less lint support than v1, but I think we can move first and migrate lint support later. Much of the v1 lint is bad anyway.
Test Plan: Upgraded libphutil, arcanist and phabricator to v2
Reviewers: vrana, btrahan, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2588
Summary:
Adds a linter for v2 libraries which raises the relevant errors.
NOTE: Not hooked up anywhere yet, so this diff has no effect.
Test Plan:
Switched the ModuleLinter to LibraryLinter and ran it with a junk block to trigger errors:
>>> Lint for src/lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php:
Error (PHL3) One Class Per File
File 'lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php' mixes
function (id) and class/interface (ArcanistPhutilLibraryLinter)
definitions in the same file. A file which declares a class or an
interface MUST declare nothing else.
190 }
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Error (PHL2) Duplicate Symbol
Definition of function 'id' in
'lint/linter/phutillibrary/ArcanistPhutilLibraryLinter.php' in library
'arcanist' duplicates prior definition in 'utils/utils.php' in library
'phutil'.
190 }
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Error (PHL1) Unknown Symbol
Use of unknown class 'XYZ'. This symbol is not defined in any loaded
libphutil library.
191
192 if (false) {
193 function id() { }
194 new XYZ();
195 }
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2597
Summary:
Simple wrapper for PHP_CodeSniffer.
You need to have PHP_CodeSniffer and it's dependencies installed.
Test Plan: - Try running it with your custom lint engine
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2560
Summary: On windows paths are separated with \.
Test Plan: - Run filename linter on windows (or some path with \)
Reviewers: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2559
Summary:
Xcode (a popular code editor on Mac OS X) has no facility
to trim trailing whitespace automatically.
This adds a new lint severity "AUTOFIX" that's between
WARNING and ERROR. When running the linter, any lint message
whose severity is AUTOFIX will automatically be patched.
Furthermore, if all lint messages returned from the engine are
AUTOFIX, we'll automatically amend HEAD with the patch.
Test Plan:
arc lint on files with and without trailing whitespace,
with and without UTF-8 contents to confirm those still error
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2125
Summary: Saw this in a diff somewhere; complain about it.
Test Plan: Unit coverage.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1060
Differential Revision: https://secure.phabricator.com/D2153
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2084
Summary:
See D2049, D2050. Identify reuses of locals as iterator variables. Before raising an error, we require:
- Variable is declared before the loop.
- Variable is used after the loop, ignoring uses as an iterator variable.
I think this identifies all problems with a very low false positive rate (the false positives are suspicious/unconventional code, but not necessarily errors).
Also fix an issue identified by the linter.
Test Plan:
- Verified this identified the bugs in D2049 and D2050.
- Ran linter against libphutil/, arcanist/ and phabricator/ (see D2051, this, and next diff).
- Ran unit tests.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley, jungejason
Differential Revision: https://secure.phabricator.com/D2052
Summary:
Addresses concerns in rARCefb8219196abf047f14b505959e54d078e1df6d3:
- As I recall, the intent of "generateFile" was that these warnings would replace all the other warnings for that file, under the theory that if one warning caused regeneration of
the file the other warnings were irrelevant.
- However, this code never had any effect and I haven't seen any issues with the behavior.
- So, just remove it.
Addresses concerns in rARC070e963d1c26879e47eab19a2377e388c2f166c5:
- Ran "arc liberate --all".
Test Plan: Ran "arc lint" after removing an "__init__.php" with and without a "fixed" generateFile block, there was no behavioral change. So I just nuked it.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2049
Summary:
- Add a lint check for PHP 5.3 features (namespaces, anonymous functions).
- Add unit tests.
- Disable it by default.
- Enable it for us.
Test Plan: Ran unit tests.
Reviewers: btrahan, edward
Reviewed By: edward
CC: aran, epriestley
Maniphest Tasks: T962
Differential Revision: https://secure.phabricator.com/D1846
Summary:
This allows for disabling certain PEP8 linter errors by calling
setCustomSeverityMap on an ArcanistPEP8Linter. However, any custom severities
besides disabled will be ignored.
Test Plan: arc lint
Reviewers: epriestley, andrewjcg
Reviewed By: epriestley
CC: jungejason, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1839
Summary: If a case does not end with break, continue, throw, exit or return and does not have a "fallthrough" comment, raise a warning.
Test Plan: Ran test case; ran linter against all of Phabricator (no hits).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1824
Summary: Linter flipped out on D1817; reign it in.
Test Plan: Ran "arc lint" on D1817, got one message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1818
Summary:
Some linters return absolute path.
It causes separate lint messages for the same file.
Also lint messages aren't properly bound in Phabricator.
I've preferred changing addLintMessage() to fixing all linters because it is
more robust.
Test Plan: arc lint of a file with lint problem from linter using absolute paths
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1589
Summary:
Mark all applicable Arcanist classes as "final", except PhutilLintEngine, which
needs a little finesse.
@jungejason / @nh, does this break any Facebook stuff?
Test Plan: Linter no longer raises warnings. Ran "testEverythingImplemented" in
Phabricator.
Reviewers: btrahan, jungejason, nh
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1519
"@concrete-extensible"
Summary:
See T795. I think ~all classes in the classtree should be "abstract" or "final",
but I provided "@concrete-extensible" if you really have "Rectangle extends
Square extends Shape" or something.
I'm not totally sure this should be enabled globally by default, maybe I should
default it to DISABLED and then enable it for libphutil/arcanist/Phabricator? It
feels like it might be a little overbearing to push on everyone by defualt.
Test Plan: See unit tests.
Reviewers: btrahan, aran, nh, arudolph, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1510
Summary:
- Move name helper functions to ArcanistXHPASTLintNamingHook to make it easier
to write custom linters.
- Add test coverage for name functions.
- Add 'variable' and 'global' naming convention tests.
- Expand test cases.
- Improve lint message error when an unexpected message is raised during a
test.
- Remove a defunct XHP lint message.
Test Plan:
- Ran unit tests.
- Ran "arc lint --lintall" on arcanist/.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: johnduhart, aran, epriestley, arudolph
Differential Revision: https://secure.phabricator.com/D1506
Summary:
Split ArcanistXHPASTLinter::LINT_FORMATTING_CONVENTIONS into seperate
lint names so that each linting rule can be controled sperately
Test Plan:
This shouldn't break anything, if it does we'll know soon enough.
<epriestley> Better things break loudly/obviously I think.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1495
Summary:
Julien built a really cool static analysis database of our codebase. One
capability is that it can suggest typehints that are not in the code. The
analysis to do this is very expensive, so it can't reasonably be run locally.
But it can remain indexed on a server.
The idea here is to provide a familiar interface to it through arc lint, via a
generic Conduit service call.
In our lint engine, this will probably be gated on --advice for performance.
This will introduce a slight awkwardness in that running with --advice can add
new non-advice lint if the server chooses, but this isn't likely to cause a
practical problem.
Test Plan:
Construct a fake Conduit lint endpoint, attach this linter to it, and see bogus
lint
appear with --advice.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1462
Summary:
This allows engines to check the canRun method on linters,
which should determine if a linter is configured and can
be run in the current environment.
Test Plan:
Implement a linter with canRun as false, and ensure
it doesnt run.
Ensure all existing linters still run by default.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1445
Summary:
This cleans up the PEP8 linter to bring it inline
with the new JSHint Linter's level of quality.
It adds a getPEP8Path method which gives the ability
for users to override the pep8 binary with two new
options in .arcconfig:
* lint.pep8.prefix
* lint.pep8.bin
* lint.pep8.options
Test Plan:
Adjust your engine to use the 'ArcanistPEP8Linter' and run
arc lint against Python files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1440
Summary:
This adds a new lint engine, ComprehensiveLintEngine, which
includes sane defaults for some generic languages.
Ideally this would include *all* available language linters,
but that can be enhanced at a later point. Right now it's mostly
the base linter with additional JavaScript and Python linters.
Test Plan:
Adjust the lint_engine to be "ComprehensiveLinterEngine". You'll
also need jshint, pyflakes, and pep8 to all be available on PATH.
Run arc lint against files which contain .php, .py, and .js.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1439
