Summary: Ref T7977. The `ArcanistTestCase` class is pointless and can be replaced by `ArcanistPhutilTestCase`. Furthermore, it sorta makes sense to just rename `ArcanistPhutilTestCase` to `PhutilTestCase`. Depends on D12664 and D12666.
Test Plan: `arc unit`
Reviewers: avivey, #blessed_reviewers, epriestley
Reviewed By: avivey, #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12665
Summary: Explicitly list PHP magic methods (methods beginning with `__`) instead of assuming that //all// methods beginning with `__` are okay in terms of naming conventions. These magic methods were obtained from http://us1.php.net/manual/en/language.oop5.magic.php.
Test Plan:
```
Warning (XHP9) Naming Conventions
Follow naming conventions: methods should be named using lowerCamelCase.
1 <?php
2
3 class Foo {
>>> 4 function __foo() {}
5
6 function _bar() {}
7 }
Warning (XHP9) Naming Conventions
Follow naming conventions: methods should be named using lowerCamelCase.
3 class Foo {
4 function __foo() {}
5
>>> 6 function _bar() {}
7 }
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10539
Summary: I'm pretty sure that `@group` annotations are useless now... I believe that they were originally used by Diviner?
Test Plan: Eye-balled it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, aurelijus
Differential Revision: https://secure.phabricator.com/D9855
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:
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
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
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: 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:
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:
- 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: 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:
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:
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:
- 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:
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:
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:
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:
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 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
Summary: The multi-line comment regexp was potentially too greedy. See
"greedy.lint-test".
- Made it less greedy.
- Added test coverage.
- Fixed an issue with the Apache license getting applied with too much
whitespace against C files.
Test Plan: Ran unit tests.
Reviewers: aran
CC:
Differential Revision: 36
