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
Summary:
Inspired by http://news.ycombinator.com/item?id=3464671 and a lot of
diffs I've seen @ FB, I've added a spell checking linter. To reduce
false positives, it's only a blacklist. Still, it catches a large
number of 'issues'.
Test Plan:
Unit tests. Ran on FB's codebase. No false positives
noticed but a lot of cases caught.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, jack
Differential Revision: https://secure.phabricator.com/D1409
This patch adds ArcanistJSHintLinter class and three new .arcconfig options:
* lint.jshint.prefix - directory where JSHint binary resides
* lint.jshint.bin - JSHint binary name (if different from 'jshint')
* lint.jshint.config - a JSON file with JSHint project-wide options
By default, this linter assumes that JSHint is installed on user's system
as an NPM package.
Test Plan:
(1)
Run `npm install jshint -g` to install JSHint and add
ArcanistJSHintLinter to your Lint Engine (I didn't see PEP8 or
PyFlakes in the PhutilLintEngine so I decided to not to put
JSHint in there). After that you can do `arc lint my.js`.
(2)
Create a new file, config.json, and add `{ "white": true }` in it.
Add `"lint.jshint.config": "/path/to/config.json"` to your .arcconfig and
run `arc lint my.js`. After that, unless your name is Douglas Crockford,
you'll see tons of JSHint warnings about minor PEP8-like things (spacing, etc.)
Summary:
If the else clause does not have braces (one-liner), XHPASTLinter eats the
newline and brings the body statement of the else clause to the same line.
Test Plan: added a new test to space-after-control-keywords.lint-test
Reviewers: epriestley
CC: aran, epriestley, kiyoto
Differential Revision: https://secure.phabricator.com/D1367
Summary: See T645. These commands take inconsistent and overly-magical arguments
right now. Instead, make them behave consistently and allow them both to operate
on "arc <workflow> path path2 path3 ...", which is a generally useful workflow.
Test Plan: Ran "arc lint <path>", "arc unit <path>", "arc lint --rev
HEAD^^^^^^", "arc unit --rev HEAD^^^^^^^^^^^^", etc. Ran "arc diff --trace" and
verified --rev argument to child workflows.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T645
Differential Revision: https://secure.phabricator.com/D1348
Summary:
- Remove XHP tests so I can remove XHP support from XHPAST.
- Update license tests so they don't break every year.
Test Plan: - Ran all unit tests.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, pad, jungejason, btrahan
Maniphest Tasks: T635
Differential Revision: https://secure.phabricator.com/D1318
Summary: Previously, we would not correct missing space before "{".
Test Plan: Ran unit tests.
Reviewers: btrahan, jungejason, kiyoto
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: https://secure.phabricator.com/D1313
Summary: Previously, we would not correct excessive space after "if", etc.
Test Plan: Ran test case.
Reviewers: btrahan, jungejason, kiyoto
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: https://secure.phabricator.com/D1311
Summary:
Creates a new hook API that can be used to interface with
SVN/Git/Mercurial in the context of a commit hook. Currently only adds a
function to read the modified file data in a Subversion commit hook.
An object of this API is created in the SvnHookPreCommitWorkflow and
passed on the Lint Engine which then uses it to access current file
data, of the way the APIs seem to be structured); linters use the
getData function which is essentially a wrapper around the engine's
call, with another layer of caching.
Task ID: #770556
Blame Rev:
Test Plan:
- Create a local svn repository and add a minimal hook to run the local
version of arc to test commits
(http://phabricator.com/docs/arcanist/article/Installing_Arcanist_SVN_Hooks.html)
- Create a temporary repository that can trigger any of the linters
available, and test against a temporary linter by committing against
the test repository: the linter should be able to access all required
files by using loadData/getData in the LintEngine and Linter.
Revert Plan:
Tags: lint, svn-hook-pre-commit
Reviewers: jungejason, asukhachev, epriestley, aran
Reviewed By: epriestley
CC: aran, jungejason, epriestley, kunalb, asukhachev
Differential Revision: https://secure.phabricator.com/D1256
Summary:
add method setXHPASTTreeForPath() so that a child class of
ArcanistXHPASTLinter can set the tree easily.
Test Plan: wrote a subclass of ArcanistXHPASTLinter and it worked.
Reviewers: pad, epriestley
Reviewed By: pad
CC: aran, pad, jungejason
Differential Revision: 1134
Summary: if we don't care and don't expect it to break all other linters then
continue
Test Plan: ##arc lint --trace## on a commit with non-ascii characters, saw other
linters continue
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, cpiro
Differential Revision: 1122
Summary: newer PyLint includes commas and a column number for every message
after the line number. ignore that if it's present.
Test Plan: ##arc lint --trace --lintall## a file with messages with the old and
new pylint (2.5), works fine with both
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1062
Summary: allow adding PYTHONPATHs directly in addition to the hardcoded few
already allowed.
Test Plan: ##arc lint## successfully picks up the paths listed in my .arcconfig,
and no longer errors on modules not found from those directories
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1050
Summary: add the ability to specify an rcfile path either absolute or relative
to the project root.
Test Plan: added one for a project, ran ##arc lint --trace## to see the expected
result
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1046
Summary:
- The top documentation is more readable, and the details about the severity
mapping regexps is moved to the top from inline -- they're important.
- array_merge() is used when appending command line args from
##lint.pylint.options## instead of '+'. The latter merges by key instead of
appending the two lists as intended.
- rely on the exit code instead of magic text that may or may not be there
depending on how ##pylint## is invoked.
These bugs were introduced in d762311a9d.
Test Plan: ##arc lint --trace##'d a bunch of Python source files in a project,
with and without PyLint messages. Added some ##lint.pylint.options## to see
proper appending behavior.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, cpiro
Differential Revision: 1045
Summary:
See T326. Allow lint rules to be selectively overridden, e.g. for Conduit
methods.
Since FB has a long history of suggesting crazy patches for this stuff I think
we're safer just adding a hook class than trying to do some kind of regexp
magic.
Test Plan: Wrote a hook for Phabricator and linted some Conduit files without
issues. Ran unit tests.
Reviewers: nh, jungejason, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 874
Summary: See D753. Let's just have lint fix this. Depends on D754 (it fails to
detect all the blocks without that patch, but doesn't do anything bad).
Test Plan: Ran unit tests.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: hunterbridges, aran, epriestley, jungejason
Differential Revision: 755
Summary:
Document the relationship between lint engines and linters. Provide an example
linter. Improve the documentation of PyLintLinter, which has a bunch of
configuration stuff which you had to dig into the code to get.
Test Plan:
Ran "arc lint --engine ExampleLintEngine --lintall derp.py" on a file with a
Python syntax error in it. Read documentation.
Reviewed By: j3kuntz
Reviewers: j3kuntz, andrewjcg
CC: aran, j3kuntz
Differential Revision: 557
Summary:
W293 reports a lint warning for the same problem that TXT6 reports a
lint error for. This isn't so terrible in and of itself.
However, when you are then prompted to apply a patch to fix TXT6, lint
doesn't realize that the W293 warning was also handled, and still
prompts you about ignoring unresolved warnings. This is misleading.
Test Plan:
> python pep8.py hasBlankLineWS.py
hasBlankLineWS.py:3:1: W293 blank line contains whitespace
> python pep8.py --ignore=W293 hasBlankLineWS.py
(no output)
Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: epriestley, aran
Differential Revision: 525
Summary:
cpojer fixed a JS instance of this in D481, but we can catch it in PHP with
XHPAST. Add a lint rule to fail if nested loops use the same iterator.
Test Plan:
Ran unit test.
Reviewed By: tomo
Reviewers: aran, pad, cpojer, jungejason, tuomaspelkonen, tomo
Commenters: cpojer
CC: aran, cpojer, epriestley, tomo
Differential Revision: 489
Summary:
This is pretty coarse and could be refined, but I often do this when testing:
lang=diff
- if (some_complicated_condition())
+ if (true || some_complicated_condition())
aran has caught me not only doing it but sending out diffs with it like 30
times. Catch it in lint instead.
Test Plan:
Unit test, added a "true || $junk" to the code and linted it.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, pad
CC: aran
Differential Revision: 447
Summary:
Raise an error if an array is initialized with the same key present more than
once.
Test Plan:
bin/arc unit
Also added some duplicates to ArcanistXHPASTLinter.php and verified the output
of bin/arc lint.
Reviewed By: epriestley
Reviewers: jungejason, epriestley, aran
CC: aran, epriestley
Revert Plan:
Tags:
Differential Revision: 346
Summary:
Provides a lint class as a wrapper around the external project PyLint.
This exposes some arc config variables to control the behavior:
lint.pylint.prefix - non-standard installation location of pylint
lint.pylint.logilab_astng.prefix - non-standard installation location
of logilab-astng, a dependency of pylint
lint.pylint.logilab_common.prefix - non-standard installation location
of logilab-common, a dependency of pylint
lint.pylint.codes.{error,warning,advice} - regexes matching against
PyLint message codes which should trigger arc errors/warnings/advice
lint.pylint.options - options to pass PyLint
Test Plan:
used to lint python code
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 343
Summary:
Provide a simple linter wrapper around pyflakes. This relies on finding
pyflakes via:
- lint.pyflakes.path - arcconfig setting of absolute path to pyflakes
- lint.pyflakes.prefix - arcconfig setting of the prefix that pyflakes
was installed under
- users path
Test Plan:
linted python code with PyFlakes warnings
Reviewed By: epriestley
Reviewers: jungejason, epriestley
Commenters: jungejason
CC: aran, epriestley, andrewjcg, jungejason
Differential Revision: 310
Summary:
This diff:
- Adds the PEP8 linter to the externals directory
- Changes the path for finding pep8.py
- Removes use of execx since pep8.py return an errors code
when it finds PEP8 violations
Test Plan:
tested linting python code
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 309
Summary:
The story for creating and maintaining libphutil libraries and modules
is pretty terrible right now: you need to know a bunch of secret scripts and
dark magic. Provide 'arc liberate' which endeavors to always do the right thing
and put a library in the correct state.
Test Plan:
Ran liberate on libphutil, arcanist, phabricator; created new
libphutil libraries, added classes to them, liberated everything, introduced
errors etc and liberated that stuff, nothing was obviously broken in a terrible
way..?
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 269
Summary:
This isn't necessarily a root-cause solution but this syntax is really really
bad. If we want to fix the root cause, I'd recommend making its use a lint
error?
Test Plan:
Unit tests failed before patch, passed afterward.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 270
Summary:
This is necessary for the Javelin linters. The libphutil and flib linters do it
implicitly, so there's no real tradeoff here.
Test Plan:
Ran javelin linters.
Reviewed By: tuomaspelkonen
Reviewers: tomo, aran, jungejason, tuomaspelkonen
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 238
Summary:
This is realistically always wrong and the author means "."
Test Plan:
lint / unit
Reviewed By: crackerjack
Reviewers: crackerjack, aran
CC: crackerjack
Differential Revision: 68
Summary:
XHPAST encounters a parse-depth problem on some files because PHP
5.2 has an un-overridable parse depth limit for JSON. The text of this error
says it is a "warning" but we currently raise an error. Make it a warning
instead.
The JSON depth is 20 until PHP 5.2.3, where it becomes 128. After PHP 5.3
it defaults to 512 and is user-configurable, which will allow us to resolve
this issue in nearly all cases.
Since I made if/else express as a list in the AST, this only actually arises
in long binary chains, most commonly string concatenation, like:
$out = 'a'.'a'.'a'.'a'...
...where each string is a variable or HTML tag and the program is constructing
a complicated document.
At some point I'll add some PHP 5.3 massaging to the XHPAST decoder itself to
raise this limit to something more huge.
Test Plan:
Ran "arc lint --lintall" on a file with a very deep binary
expression tree ("1 + 1 + 1 ...") and received a warning instead of an error.
Reviewed By: aran
Reviewers: pad, aran
CC: epriestley, aran
Differential Revision: 54
