Summary: After D9057 and D9064 this class is no longer used.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9197
Summary: To me, it seems that these methods should not be overridden by subclasses
Test Plan: `arc lint` and `arc unit`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: LegNeato, aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D7960
Summary: Since this method is only used within this class, it makes sense to move it here.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9114
Summary: Although this provides less context in terms of the error message (for example, `Parameter has invalid type. Expected type 'optional regex|list<regex>', got type 'list<string>'.`), I think that it is the right approach. I think that `PhutilTypeSpec::checkMap` should be improved such that additional context is provided in the exception message.
Test Plan: Ran `arc lint`. Modified `.arclint` to contain an invalid regex and ran `arc lint` again.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9089
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. I'll update the corresponding documentation.
It feels a little awkward that this is disconnected from `getLinterConfigurationOptions()`, but I dislike returning weird ad-hoc structures more than I dislike having two methods. Most linters don't implement either of these anyway.
Test Plan: Ran `arc linters` and `arc linters --verbose`.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9062
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:
Ref T2039. Addresses two issues:
- Issues a warning for use of config which is deprecated by `.arclint`.
- We no longer require an engine to be present for these linters, so `arc linters` doesn't fatal if they aren't configured.
Test Plan:
- Ran `arc linters` in a repo with no JSHint.
- Ran `arc linters` in a repo with junk in .arcconfig and got a warning when it was read. Verified it still took effect.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9058
Summary: Currently, paths to be excluded from linting need to be specified for each linter individually. This is a pain for projects that are using even a moderate number of linters and which have common paths which should be excluded from linting completely.
Test Plan: Unfortunately, it's hard to test this sort of stuff. I cloned the `arclint-examples` repository and tested my changes there,
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9054
Test Plan: Ran `arc lint` locally. Now it doesn't exit with an error.
Reviewers: #blessed_reviewers, #arcanist, epriestley
Reviewed By: #blessed_reviewers, #arcanist, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9028
Summary: Having a `debug` key in the `.arclint` file format doesn't seem right. Instead, it would be better to just use a `PhutilConsole` and the `writeLog` method so that "debug" messages are output when using `arc --trace`.
Test Plan: Ran `arc lint --trace` in a repository using `.arclint`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9021
Summary: Fixed wrong markup which was 1 space instead of 2
Test Plan: I couldn't test it as I don't know how to build documentation
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7587
Summary:
We have some linters that trigger based on the path name
in the tree (some rules apply in some dirs and not others).
The changes in D6798 caused all the paths to appear to be outside
the tree, so allow for passing a fake through from those test cases
that are sensitive to this.
We also have a test for the copyright linter, and that needs to read
settings from the .arcconfig file. The change to faking a working
copy meant that this config option was effectively unset, so add a way
to pass the entire arcconfig through from the tests that need it.
Lastly, the logic to skip deleted files needs to be special cased
when we're faking paths like this: if we've added data for a file
in the testable engine, we should also consider that file as existing.
Test Plan:
`arc unit --everything` here, and passing our tests in
our repo over there.
Reviewers: epriestley, mareksapota
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D6841
Summary:
Ref T3186. Ref T2039. Ref T3771. A few effects here:
# Expose PHPCS as a `.arclint` linter.
# Turn PHPCS into an ArcanistExternalLinter linter.
# Add test coverage for PHPCS.
# Add a `severity.rules` option to `.arclint`. Some linters have very explicit builtin severities ("error", "warning") but their meanings are different from how arc interprets these terms. For example, PHPCS raises "wrong indentation level" as an "error". You can already use the "severity" map to adjust individual rules, but if you want to adjust an entire linter it's currently difficult. This rule map makes it easy. There's substantial precedent for this in other linters, notably all the Python linters.
For `severity.rules`, for example, this will turn all PHPCS "errors" into warnings, and all of its warnings into advice:
"severity.rules" : {
"(^PHPCS\\.E\\.)" : "warning",
"(^PHPCS\\.W\\.)" : "advice"
}
The user can use `severity` (or more rules) to get additional granularity adjustments if they desire.
Test Plan: 5bb919bc3a
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, ajtrichards
Maniphest Tasks: T2039, T3186, T3771
Differential Revision: https://secure.phabricator.com/D6830
Summary:
Ref T3186. Ref T2039. Continues work on readying linters for `.arclint`.
- **Ruby**: Make this an ExternalLinter.
- **Priority**: Currently, linters have an implicit "correct" order (notably, the "NoLint" linter needs to run before other linters). Make this explicit by introducing `getLinterPriority()`.
- **Binaries**: Currently, linters manually reject binary files. Instead, reject binary files by default (linters can override this if they do want to lint binary files).
- **Deleted Files**: Currently, linters manually reject deleted files (usually in engines). Instead, reject deleted files by default (linters can override this).
- **Severity**: Move this `.arclint` config option up to top level.
- **willLintPaths()**: This method is abstract, but almost all linters provide a trivial implementation. Provide a trivial implementation in the base class.
- **getLintSeverityMap()/getLintNameMap()**: A bunch of linters have empty implementations; these are redundant. Remove them.
- **Spelling**: clean up some dead / test-only / unconventional code.
- **`.arclint`**: Allow the filename, generated, nolint, text, spelling and ruby linters to be configured via `.arclint`.
Test Plan:
458beca3d6
Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: Firehed, aran
Maniphest Tasks: T2039, T3186
Differential Revision: https://secure.phabricator.com/D6805
Summary:
Ref T3186. We have about 50 linters which run programs and read the results, all of which have ad-hoc one-off custom config that isn't formalized anywhere.
Consolidate all this stuff into `ArcanistExternalLinter`, which is configurable through `.arclint` (although nothing supports this quite yet).
Extend CSSLint and Pep8Lint from `ArcanistExternalLinter`.
Add unit tests for both.
There are still some rough edges here, but it mostly seems to work pretty well.
Test Plan: Ran unit tests, hit some (most?) of the error cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran, Firehed
Maniphest Tasks: T3186
Differential Revision: https://secure.phabricator.com/D6800
Summary:
Ref T2039. That task has a bunch of discussion, but basically we do a poor job of serving the midrange of lint configuration right now.
If you have something simple, the default linters work.
If you have something complex, building your own engine lets you do whatever you want.
But many users want something in between, which isn't really well accommodated. The idea is to let you write a `.arclint` file, which looks something like this:
{
"linters" : {
"css" : {
"type" : "csslint",
"include" : "(\.css$)",
"exclude" : "(^externals/)",
"bin" : "/usr/local/bin/csslint"
},
"js" : {
"type" : "jshint",
"include" : "(\.js$)",
"exclude" : "(^externals/)",
"bin" : "support/bin/jshint",
"interpreter" : "/usr/local/bin/node"
}
}
}
...which will provide a bunch of common options around lint severity, interpreter and binary locaitons, included and excluded files, etc.
This implements some basics, and very rough support in the Filename linter.
Test Plan:
Generated a `.arclint` file and saw it apply filename lint correctly. Used `debug` mode and tried invalid regexps.
{
"debug" : true,
"linters" : {
"filename" : {
"type" : "filename",
"exclude" : ["@^externals/@"]
}
}
}
Next steps include:
- Provide an external linter archetype (T3186) and expose a common set of configuration here ("bin", "interpreter", "flags", "severity").
- Provide a `.arcunit` file which works similarly (it can probably be simpler).
Reviewers: btrahan, Firehed
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D6797
Summary: If one wishes to implement a linter which finds unused resources or variables the current scheme does not work as the lint engine filters all changes from lines which were not introduced in a diff. To solve that, I've added an "always show" configuration for a lint message allowing creation of such linters.
Test Plan:
1. Created a custom linter for finding unused Android resources in a project
2. Ran arc lint with linter added and received warnings as expected
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6119
Summary:
I'm guessing this was refactored somewhere down the line and it meant
that Python and Perl files were no longer considered text files.
I'm imagining the old regex was: p(hp|y|l). Therefore I blame CSS.
Test Plan:
Perform an arc lint on a Python or Perl file that has trailing whitespace
on a line. It should prompt you for an autocorrecting lint.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5637
Summary:
People constantly forget to bump the linter version and I don't see a way how to stop it.
This may bump the version even if it wouldn't be required but let's rather undercache than overcache.
Test Plan: `var_dump($version)`.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5129
Test Plan: Threw in `didRunLinters()` of one linter, still saw the result of other linters and "Some linters failed" at the end.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5124
Summary: This number seems more interesting and it includes time for resolving futures which is the main part of some linters.
Test Plan:
$ arc --trace lint
Reviewers: fdeliege, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D5075
Summary:
Make all the broad-spectrum text linters use the new binary check.
I didn't touch `ComprehensiveLintEngine` because it's a nest of vipers and no one has complained; see T2039.
Test Plan: Ran `arc lint` on text files and binaries in a libphutil project (arcanist).
Reviewers: lisp
Reviewed By: lisp
CC: aran
Differential Revision: https://secure.phabricator.com/D5040
Summary:
Add linting capability for detecting files which contain
syntax introduced by unresolved merge conflicts. The detection is
file-type-agnostic (the only requirement is that the file is a text
file).
Test Plan:
Tested in three ways.
The first way is to add all three forms of syntax to a file to
indicate a merge conflict. HPHP will pick this up as a syntax error
before this linter reaches it.
The second way is to add the syntax in a comment. In that case, this
linter will show three warnings. For example:
$ arc lint ArcanistMergeConflictLinter.php
>>> Lint for arcanist/src/lint/linter/ArcanistMergeConflictLinter.php:
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
20
21 foreach ($lines as $lineno => $line) {
22 /*
>>> 23 >>>>>>>
24
25 =======
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
22 /*
23 >>>>>>>
24
>>> 25 =======
26
27 <<<<<<<
Warning (MERGECONFLICT1) Unresolved merge conflict
This syntax indicates there is still an unresolved merge conflict.
24
25 =======
26
>>> 27 <<<<<<<
28
29 */
The last test was to test on various different file types, including
JavaScript, PHP, an animated GIF, a PNG, and a Bash file to make sure
the file type detection worked. Each of the aforementioned tests
passed.
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, epriestley, Korvin
Maniphest Tasks: T2547
Differential Revision: https://secure.phabricator.com/D4966
Summary:
Following @epriestley suggestion to use PhutilServiceProfiler to log lint as a service call
Test Plan: arc lint
Reviewers: vrana
CC: phunt, aran, epriestley
Differential Revision: https://secure.phabricator.com/D4820
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 save repository version to lint cache but ignore it when reading the cache.
Fix it.
Test Plan: Made an error for linter with repo granularity, deleted the error from the cache. Relinted, didn't see the error. Changed another file and relinted, saw the error.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4841
Summary: Remove all references to ArcanistLicenseLinter and ArcanistApacheLicenseLinter.
Test Plan: Rerun the linter and ensure nothing is broken.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4901
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
Summary:
The cache key is repository base revision and hash of all modified files.
It isn't perfect in SVN where every file might have a different revision.
It's also suboptimal as just committing or amending changes the cache key even if the files contents weren't modified.
We can improve it later, perhaps by using previous revision and files modified since it.
Test Plan:
Changed granularity of XHPAST linter to repository.
Linted the same repo twice, verified that it was read from cache the second file.
Changed a single file, verified that all files were re-linted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4608
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
Summary:
If linter with file cache granularity stops linting then we don't run it on the same path next time.
But we still run linters with non-file cache granularity causing that they run even on the paths that would be stopped otherwise.
Test Plan:
Put global granularity linter after Generated linter.
Caused an error from this linter but in ignored path.
Verified that 'stopped' is saved in `lint-cache.json`.
Linted the same files second time, verified that the path is still skipped (wasn't before).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4446
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
Summary:
FB runs some linters on background and it's a magnificent hack using `ParallelLinter` (two instances), `BackgroundLinter` and `FutureLinter`.
I want to simplify this by resolving the futures in engine instead of in some virtual linter.
It also seems like a better place to do it.
It should also fix caching problems I have with them (because the virtual linters don't know about the cache at all).
Test Plan: None yet.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4380
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
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
Summary:
Let's try this.
It may be useful to see this together in Differential overview.
Test Plan: Linted a file with TODO.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T744
Differential Revision: https://secure.phabricator.com/D4014
Summary:
There is one big decision: How to get linters version.
I've created `getCacheVersion()` which is supposed to be bumped every time engine or any linter is changed.
This is not very nice but the other alternative (detect this automatically) seems worse:
- The engine may be outside repo and may or may not be under version control so getting its version through something like `git log` may not be even possible.
- If it is in the same repo then every rebase will obsolete the whole cache.
Even though bumping the version manually is PITA I still think it's a better solution.
Test Plan:
$ time arc lint --cache 1
# verified file
$ arc arc lint --cache 1
# added some debug output to see the cached results
# also observed better time (.57 s instead of 2.19 s)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2036
Differential Revision: https://secure.phabricator.com/D4006
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