1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2025-01-25 05:58:20 +01:00
Commit graph

213 commits

Author SHA1 Message Date
vrana
ca37bfb2ab Display other locations of Reused lint errors
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
2013-02-08 15:49:50 -08:00
epriestley
2c2cb3b78a Improve error message for phutil missing symbols
Summary: This is fairly confusing. Make the error message suggest the common remedy (update libphutil).

Test Plan: Eyeballed it.

Reviewers: Afaque_Hussain, btrahan, vrana

Reviewed By: Afaque_Hussain

CC: aran

Differential Revision: https://secure.phabricator.com/D4834
2013-02-06 19:11:06 -08:00
vrana
bd71ba675e Implement hook for checking switch lint
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
2013-02-05 11:46:59 -08:00
vrana
a9e316bf9c Fix dynamic string usage as safe input
Summary: This fixes some real issues.

Test Plan:
  $ arc lint

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, btrahan

Differential Revision: https://secure.phabricator.com/D4795
2013-02-02 16:28:15 -08:00
vrana
03199df925 Lint dynamic string usage in parameters treated as safe
Summary:
This disallows code like this:

  $cmd = 'ls';
  execx($cmd);

But I guess it's not that big deal?

Test Plan: Linted whole Arcanist and Phabricator codebases, most parts looks fixable.

Reviewers: epriestley

CC: nh, aran, Korvin

Differential Revision: https://secure.phabricator.com/D4794
2013-02-02 16:26:52 -08:00
vrana
8374dbc4e3 Handle $var::method() in XHPAST linter
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
2013-02-01 11:21:06 -08:00
vrana
3f747be644 Move isConstantString() to base class
Summary: This can be useful by itself, we want to use it in FB linter.

Test Plan: This diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4744
2013-01-30 12:56:59 -08:00
vrana
98e8d0c9c4 Split php-tags-bad.lint-test to two tests
Summary: D1818 effectivelly disabled this test case.

Test Plan: This diff.

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4746
2013-01-30 12:46:30 -08:00
vrana
5aa3bc6ec0 Separate Phutil specific lint rules from XHPAST
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
2013-01-29 10:54:40 -08:00
Jesse Mullan
c49997a6e8 Added paths to search for python linters
Summary: Added paths to search for python linters

Test Plan: Linted in ubuntu

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2414

Differential Revision: https://secure.phabricator.com/D4648
2013-01-26 05:32:26 -08:00
epriestley
44e25495ca Raise a lint warning on array_combine(x, x)
Summary: Suggest array_fuse().

Test Plan:
Unit tests, and:

  $ arc lint --lintall src/unit/engine/phutil/ArcanistPhutilTestCase.php
  >>> Lint for src/unit/engine/phutil/ArcanistPhutilTestCase.php:

     Warning  (XHP37) array_combine() Unreliable
      Prior to PHP 5.4, array_combine() fails when given empty arrays. Prefer
      to write array_combine(x, x) as array_fuse(x).

               252     $callable,
               253     $exception_class = 'Exception') {
               254     return $this->tryTestCases(
      >>>      255       array_combine(array_keys($map), array_keys($map)),
               256       array_values($map),
               257       $callable,
               258       $exception_class);

Reviewers: btrahan, vrana, chad

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4659
2013-01-25 17:06:49 -08:00
epriestley
7f93b7de02 Don't use xhpast binary to compute xhpastlinter cache version if the binary doesn't exist
Summary: The binary may not be built, in which case this raises a warning.

Test Plan: Will make @zeeg test.

Reviewers: zeeg, vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4569
2013-01-22 17:16:37 -08:00
vrana
98fec27752 Fix PHT linter
Summary: Variables in strings and concatenation lists.

Test Plan:
New unit tests.

  preg_match('/^((?>[^$\\\\]*)|\\\\.)*$/s', str_repeat('a', 1e6));

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4540
2013-01-19 11:48:17 -08:00
vrana
78017033bd Warn against usage of nowdoc
Summary: Also include binary hash in the version.

Test Plan: New unit test.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4535
2013-01-19 09:23:17 -08:00
epriestley
a39b591c95 Allow pht(<<<HEREDOC ... HEREDOC)
Summary: Don't raise lint on heredoc literals.

Test Plan: Linted pht() + heredoc in D4477.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4478
2013-01-16 14:57:10 -08:00
vrana
90129c5432 Include real PEP8 version and options in lint cache key
Summary: This should be done for all external and configurable linters.

Test Plan: Linted file with lint problems, changed options, relinted.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4475
2013-01-16 12:17:11 -08:00
vrana
8142aab9d3 Fix whitespace around else 2013-01-16 12:15:46 -08:00
vrana
ff73a90482 Add hook after running linters
Summary: This resolves D4380#7 by reverting https://secure.phabricator.com/D4380?vs=9112&id=9123.

Test Plan: Used it in our linters.

Reviewers: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4409
2013-01-11 14:15:04 -08:00
vrana
5c5d347544 Format whitespace in XHPAST linter
Test Plan: Linted a method named `f_a`.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4405
2013-01-11 13:23:21 -08:00
vrana
a82493a84d Allow specifying multiple locations of the same lint error
Summary:
Some errors (duplicate declaration, invalid number of arguments) have more related places.
We need to notify user if he changes any related place.
This could be currently achieved by triggering errors instead of warnings or by including both files in the range (impossible if the locations are in different files) or by issuing multiple errors.
All options are too aggressive.

Test Plan: Issued error on unmodified line with other location on modified line.

Reviewers: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4392
2013-01-10 15:35:11 -08:00
vrana
80cd881fb1 Fix checking for lint binary paths
Summary: Like D4379.

Test Plan:
  $ arc unit

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4391
2013-01-10 15:34:42 -08:00
Gregor Stocks
767f9457c1 Add some new misspellings of "hierarchy"
Summary: turns out people really like misspelling "hierarchy"

Test Plan: hope
2013-01-10 14:32:23 -08:00
vrana
983537e620 Fix checking for flake8 path
Test Plan:
  $ arc unit # without flake8 in path

Reviewers: zeeg

Reviewed By: zeeg

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D4379
2013-01-09 13:41:10 -08:00
Jack Lindamood
48f5ecb05c Fix cppcheck path finding
Summary: This fix lets you run arc lint from any directory in the repository

Test Plan: Ran arc lint from any directory

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4356
2013-01-07 12:47:00 -08:00
Jack Lindamood
cf8d445c9f CppCheck linter
Summary:
Lints cpp code using the cppcheck static linter.  This linter needs to
be downloaded and built from http://cppcheck.sourceforge.net/

Test Plan: Used it on a few files.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4353
2013-01-07 08:18:33 -08:00
Jack Lindamood
854c1b67b1 Add cpplint.py engine
Summary: Adds arc lint support for cpp files with Google's cpplint.py lint checking.

Test Plan: ran it on some cpp files. Added unit tests

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4344
2013-01-07 08:16:34 -08:00
David Cramer
0b833c2f02 Correct description and severity on Flake8 linter
Test Plan: do it live

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4274
2012-12-21 16:13:33 -08:00
David Cramer
4ca70d4e48 Add a flake8 linter
Summary: flake8 is the better maintained combination of pep8 and pyflakes

Test Plan: There's a test!

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin, jack

Differential Revision: https://secure.phabricator.com/D4082
2012-12-21 15:28:26 -08:00
vrana
c7c3f6a7f1 Update PEP8 to 1.3.4
Summary: Primarily to avoid false positives: http://pypi.python.org/pypi/pep8#id1

Test Plan:
  $ arc lint --engine ComprehensiveLintEngine externals/pep8/pep8.py # after uncommenting externals/ check

Saw 9 errors, haha.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4260
2012-12-21 11:34:40 -08:00
vrana
7133c76d37 Update PEP8
Test Plan:
Linted this Python file:

  def get_mapper_sets(
          compression=4.0,  # Currently works for most tables
          **kwargs):
      pass

Didn't see:

> E225 missing whitespace around operator

Reviewers: andrewjcg, epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4172
2012-12-14 17:50:18 -08:00
vrana
e8eacb6ae3 Allow setting lint cache granularity
Summary:
We can add `GRANULARITY_DIRECTORY` and `GRANULARITY_REPOSITORY` later.
Repository granularity may use current commit + changes.
Directory would need to use hashes of all files in dir which would be quite expensive.

Test Plan:
  $ echo '<?php class A extends B {}' > A.php
  $ arc lint --cache 1
  $ arc lint --cache 1
  $ echo '<?php class B {}' > B.php
  $ arc lint --cache 1
  $ arc lint --cache 1
  $ rm B.php
  $ arc lint --cache 1
  $ arc lint --cache 1

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4021
2012-12-03 15:30:06 -08:00
David Cramer
0d1d04e434 canRun should return false on scala linter instead of raising an exception
Summary:
This was causing the following error in environments that didnt have scala configured:

Some linters failed:
   - ArcanistScalaSBTLinter: ArcanistUsageException: This directory does not appear to be maintained by SBT, as we can't seem to find a working build file (project/Build.scala or build.sbt).

Test Plan: .

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4070
2012-12-03 15:25:28 -08:00
Ricky Elrod
b549f565c9 Add a very basic Scala (SBT) linter.
Summary:
SBT is the most common Scala buildsystem. This adds an extremely basic and
slightly horrible linter to check SBT's output for warnings and errors.

Test Plan:
Tested this with a Scala project I've been working on for some time.
It seemed relatively sane.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4064
2012-12-03 10:04:18 -08:00
vrana
cd7f86d380 Set cache version per linter
Summary: Also delete cache with `--cache 0`.

Test Plan:
  $ arc lint --lintall --cache 1
  $ cat .git/arc/lint-cache.json
  $ arc lint --lintall --cache 1 # with debug output

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2036

Differential Revision: https://secure.phabricator.com/D4013
2012-11-21 17:22:06 -08:00
epriestley
d3afa9cc31 Fix some arcanist warnings
Summary:
  - Rename some very old variables.
  - Wrap some contributed lines.

Test Plan: `arc lint` / `arc unit`. Viewed a diff in an uncacheable mode to verify intraline behavior.

Reviewers: vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4018
2012-11-21 17:16:52 -08:00
vrana
ac92f9bdfc Remove getLink() from ArcanistLinterTestCase
Summary:
Installations extend this.

Another solution would be to extend `ArcanistLinterTestCase` from `ArcanistArcanistLinterTestCase` and return null in `getLink()` to avoid code duplication but I prefer clean class hierarchy.

Test Plan:
  $ arc unit

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3878
2012-11-07 10:12:24 -08:00
vrana
2e6dcf0fbb Ignore duplicate PEP8 lint errors in comprehensive engine
Test Plan:
  $ arc lint a.py # with too long line

Reviewers: zeeg, epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3882
2012-11-06 11:48:00 -08:00
vrana
d53dcbe952 Trigger bad charset lint warning only once per line
Summary:
Makes lots of noise:
{F22758}

Test Plan:
Linted file with several bad characters per line.
Linted file with one bad character per line.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3896
2012-11-05 16:36:19 -08:00
vrana
0938091a99 Fix ArcanistLinterTestCase
Summary: We could also inject the value from the test case config but this is simpler.

Test Plan:
  $ arc unit src/lint/linter/ArcanistLicenseLinter.php

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2035

Differential Revision: https://secure.phabricator.com/D3895
2012-11-05 16:36:01 -08:00
vrana
21530fa459 Change severity of PEP8 errors to warnings
Summary: None of these are that serious that I would like to be informed about them on unmodified lines.

Test Plan: Linted Python file with lots of PEP8 errors, now warnings.

Reviewers: zeeg, epriestley

Reviewed By: zeeg

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3884
2012-11-05 13:05:13 -08:00
vrana
66d204be81 Delete license headers from files
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).

We are removing the headers for these reasons:

- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.

This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).

Test Plan: Verified that the license survived only in unit tests and LICENSE file.

Reviewers: epriestley, btrahan, edward

Reviewed By: epriestley

CC: aran, Korvin, davidrecordon

Maniphest Tasks: T2035

Differential Revision: https://secure.phabricator.com/D3881
2012-11-05 11:16:24 -08:00
vrana
90417fbda8 Advice user to set up stripping trailing white space in editor
Test Plan: Triggered it.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2022

Differential Revision: https://secure.phabricator.com/D3864
2012-11-01 11:23:31 -07:00
vrana
77af6fb35d Set all lint paths at once
Summary:
This code is much more readable to me.
It should also be faster as building the array at once should be faster than one by one.

Test Plan: Made license and XHPAST errors in PHP file, made spelling error in JS file.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3758
2012-10-20 06:49:54 -07:00
vrana
262423b5a8 Fix lint tests after D3756
Test Plan:
Threw normal exception, saw failed test.
Threw usage exception, saw skipped test.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3761
2012-10-20 06:19:00 -07:00
epriestley
3853483c64 Fix ArcanistSingleLintEngine for deleted paths
Summary:
Currently, ArcanisSingleLintEngine lints deleted paths and directories. These are sometimes appropriate, but SingleLintEngine is a less-sophisticated linter and should have more safe defaults.

Also fix an error where JSHint reported useless messages on failure.

Test Plan:
Reproduced the problem:

  $ git show
  commit d71efe2b13770c8861bcd3415c15503fc377339f
  Author: epriestley <git@epriestley.com>
  Date:   Fri Oct 19 12:22:50 2012 -0700

      WIP

  diff --git a/test.js b/test.js
  deleted file mode 100644
  index 8bd6648..0000000
  --- a/test.js
  +++ /dev/null
  @@ -1 +0,0 @@
  -asdf
  $ arc set-config --local lint.engine.single.linter ArcanistJSHintLinter
  Set key 'lint.engine.single.linter' = "ArcanistJSHintLinter" in local config (was null).
  $ arc lint --engine ArcanistSingleLintEngine --rev HEAD^
  Usage Exception: JSHint returned output we can't parse. Check that your JSHint installation.
  Output:

Applied the error message fix:

  $ arc lint --engine ArcanistSingleLintEngine --rev HEAD^
  Usage Exception: JSHint returned unparseable output.
  stdout:

  stderr:

  node.js:181
          throw e; // process.nextTick error, or 'error' event on first tick
          ^
  Error: ENOENT, No such file or directory '/INSECURE/repos/git-working-copy/test.js'
      at Object.statSync (fs.js:400:18)
      at _collect (/usr/lib/node_modules/jshint/lib/hint.js:77:12)
      at /usr/lib/node_modules/jshint/lib/hint.js:93:13
      at Array.forEach (native)
      at Object.hint (/usr/lib/node_modules/jshint/lib/hint.js:92:17)
      at Object.interpret (/usr/lib/node_modules/jshint/lib/cli.js:137:21)
      at Object.<anonymous> (/usr/lib/node_modules/jshint/bin/hint:2:25)
      at Module._compile (module.js:420:26)
      at Object..js (module.js:426:10)
      at Module.load (module.js:336:31)

Applied the remove paths fix:

  $ arc lint --engine ArcanistSingleLintEngine --rev HEAD^
  Usage Exception: No paths are lintable.

Reviewers: magazovski, btrahan, vrana

Reviewed By: btrahan

CC: aran, Korvin, vissi

Differential Revision: https://secure.phabricator.com/D3735
2012-10-20 06:12:24 -07:00
vrana
d0ed67ab1d Decrease severity of preg_quote() warning
Summary: I use it more often without second parameter than with it.

Test Plan:
Lint of:

  preg_quote('');
  preg_quote('', '/');

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3647
2012-10-05 18:05:11 -07:00
Bob Trahan
baa64a5c83 fix a bug in ArcanistXHPLinter
Summary: $param is null here. it should be $node.

Test Plan: arc lint no longer barfed on me!

Reviewers: vrana, epriestley

Reviewed By: vrana

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3540
2012-09-21 14:59:04 -07:00
vrana
c8687a0c79 Lint functions not available on Windows on PHP 5.2
Summary: Also use absolute paths.

Test Plan: Linted Arcanist, libphutil, Phabricator, found no false positives and one real error in [[ https://secure.phabricator.com/diffusion/PHU/browse/master/src/channel/PhutilSocketChannel.php;42d8e8447c8b5d6a$92 | PhutilSocketChannel ]].

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3504
2012-09-17 13:51:06 -07:00
vrana
7ee0f3e3b3 Lint short ternary as PHP 5.3 feature
Summary: I don't offer a replacement because `f() ?: 1` converted to `f() ? f() : 1` can cause side effects and whitespace issues.

Test Plan: New test.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3489
2012-09-13 11:24:32 -07:00
vrana
a5e2f81928 Skip linter tests with usage problems
Summary: D3480#comment-1

Test Plan: This diff.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D3481
2012-09-12 10:56:00 -07:00