Summary:
Paths are passed into linters using UNIX-style slashes (/), as returned from the version control system; however,
`Filesystem::readablePath` swaps them to Windows-style (\) on
Windows when storing the names of the files with lint messages. This causes no lint message's path to match the set of
changed files, and thus no lint warnings are ever produced.
If a lint message's file is not found using the provided filename, also try looking up the UNIX-style filename, on Windows when determining if a lint mesage is "relevant."
Fixes T11248.
Test Plan: Ran `arc lint` on Windows.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T11248
Differential Revision: https://secure.phabricator.com/D16579
Summary: Ref T11409. Add lint to detect using `[...]` to define arrays. This doesn't work in PHP 5.2/5.3, which some of our users run.
Test Plan:
- Ran `arc unit`.
- Ran `arc lint src/applications/harbormaster/conduit/HarbormasterQueryBuildsConduitAPIMethod.php` to detect the issue in T11409.
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11409
Differential Revision: https://secure.phabricator.com/D16357
Summary:
The `cp` and `mv` commands, when run from a Windows
environment, error out. Use library functions from `Filesystem`
for cross-platform compatibility.
Test Plan:
Ran `arc lint` with auto-fix lint errors on both Linux and
Windows.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Spies: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16273
Summary:
See rARC3ffed59bd7. Currently, when a unit test includes a syntax error, it is raised in an unclear way ("error at line 10, char 1: XHP1 Unknown lint message!").
This is because each test case only activates rules it wants to test, so we lose the ID/name for the syntax message. However, we always want to test this and the lint engine can always raise it.
To get a better error message, include it unconditionally. So a test for rule `X` really tests two rules: syntax, and `X`.
Test Plan:
Ran `arc unit` at HEAD, got a better test failure:
```
FAIL ArcanistCallTimePassByReferenceXHPASTLinterRuleTestCase::testLinter
In 'call-time-pass-by-reference.lint-test', expected lint to raise error on line 10 at char 8, but no error was raised. Actually raised:
error at line 10, char 1: XHP1 PHP Syntax Error!
```
NOTE: This doesn't pass tests yet, it just makes the test failure easier to understand. I'll see about fixing the test in the next change.
Reviewers: chad, richardvanvelzen
Reviewed By: richardvanvelzen
Differential Revision: https://secure.phabricator.com/D15819
Summary: See D15678. A node containing a return type hint is added right before the statement body node. This updates all relevant linter rules to now retrieve the body from the correct index.
Test Plan: Ran `arc unit --everything`, inspected every single message to make sure all xhpast test cases succeeded. Also grepped for `getChildByIndex(5` and `getChildOfType(5`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15814
Summary: It is currently not possible to apply multiple linter standards because `$value` was used instead of `$standard`.
Test Plan: Was able to apply multiple linter standards.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: eadler, thaiphv, Korvin
Differential Revision: https://secure.phabricator.com/D15212
Summary:
Flake8 extensions are allowed to use their own letter-prefixed codes. For
example, the flake8-debugger extension emits 'T002'-tagged messages.
This change relaxes getLintCodeFromLinterConfigurationKey() to also recognize
extension codes. Otherwise, attempting to configure message severities for
e.g. 'T002' would result in an exception.
Messages from extensions continue to default to ERROR severity, as they did
before this change.
Test Plan:
Successfully reduced the severity of 'T002' to a warning via .arclint:
"severity": {
"T002": "warning"
}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15810
Summary:
- Corrected typo in install instructions
- Sets the default binary to cpplint.py
Test Plan:
- Running the unit tests.
You may need to check your test environment is set up with the latest
cpplint.py available, since the default binary is being changed.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10157
Differential Revision: https://secure.phabricator.com/D15030
Summary:
This fixes the regex in "getLintCode..." so that it accepts lint
codes such as `build/c++11` which have become valid lint codes
in later versions of cpplint.
It also corrects the install instructions for the linter (Google's
style guide is no longer available on SVN and has been migrated to
Github).
Test Plan:
For the Regex:
- Create an .arclint in a project such as:
```
{
"linters": {
"cpplint": {
"type": "cpplint",
"severity": {
"build/c++11": "advice"
}
}
}
}
```
- Run `arc lint` with the existing linter. This should fail. Patch the linter, and this should now be accepted.
For the Instructions
- Verify the download location `wget https://raw.github.com/google/styleguide/gh-pages/cpplint/cpplint.py`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10118
Differential Revision: https://secure.phabricator.com/D15019
Summary: Fixes T10124.
Test Plan:
Added this "linter" to `.arclint`:
```
"thing": {
"type": "script-and-regex",
"script-and-regex.script": "echo every file is very bad #",
"script-and-regex.regex": "/^(?P<message>.*)/"
}
```
...to produce this diff. Also made a variant of it which matches lines to make sure that still works.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10124
Differential Revision: https://secure.phabricator.com/D15000
Summary: `instanceof` should be used instead of `is_a`. I need to do a bit more research here to see if there are any edge cases.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14573
Summary:
Extends `ArcanistSelfMemberReferenceXHPASTLinterRule` to warn about the use of a hardcoded class name instead of `self` when instantiating a class. Arguably, we should maybe rename the linter rule from `ArcanistSelfMemberReferenceXHPASTLinterRule` to `ArcanistSelfUsageXHPASTLinterRule`, or even maybe split this rule into three separate rules:
- `ArcanistSelfMemberReferenceXHPASTLinterRule`
- `ArcanistSelfSpacingXHPASTLinterRule`
- `ArcanistSelfClassReferenceXHPASTLinterRule`.
Test Plan: Added to existing test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D13935
Summary: Add a linter rule which advises against public class properties.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14641
Summary: Adds a linter rule which ensures that binary integers are written in lowercase (i.e. `0b1` instead of `0B1`).
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14644
Summary: Binary integers (such as `0b1`) were added to PHP 5.4 and cannot be used in earlier versions.
Test Plan: Added a test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14643
Summary: PHP doesn't handle octals very well. Basically, it seems that any numeric scalar matching `/^0\d+$/` will be treated as an octal, whereas this should be `/^0[0-7]+$/`. As a result, `08` and `09` are both treated as `0` (because they are invalid octals. This diff adds a linter rule to detect this abnormality.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14604
Summary: Demonstrates the use of `CaseInsensitiveArray`. Depends on D14624.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14625
Summary: Fix a minor issue in which changing a function call to a type cast affects the result of an expression.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14623
Summary: Hexadecimal numbers should be written as `0xFF` and not `0xff` or `0Xff`.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14612
Summary: Adds a linter rule for spacing after the `&` token, similar to `ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule`.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14602
Summary: Type casts should be used instead of calls to the `*val` functions as function calls impose additional overhead.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14572
Summary: If a `namespace` is used within a PHP script, it must be the first statement.
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14526
Summary:
`interface`s cannot contain `abstract` methods. This construct will cause a PHP fatal error:
```
Access type for interface method SomeInterface::someMethod() must be omitted
```
Test Plan: Added test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14562
Summary:
`interface` methods cannot contain a body. This construct will cause a fatal error:
```
PHP Fatal error: Interface function X::Y() cannot contain body in /home/josh/workspace/github.com/phacility/arcanist/test.php on line 4
```
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14561
Summary: A user reported to me that `arc lint` was failing with an error message along the lines of "argument 1 passed to `idx` must be of type array, bool given". I suspect that the user is running an older version of PHP, which means that `array_combine(array(), array())` is returning `false` instead of the expected `array()`. Instead, avoid calling `array_combine` on an empty array.
Test Plan: I don't have PHP 5.3 easily accessible to test this.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14567
Summary: Return values within constructors are acceptable if they are within a closure or anonymous function.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14564
Summary:
`abstract` methods cannot contain a body.
```
PHP Fatal error: Abstract function X::Y() cannot contain body in /home/josh/workspace/github.com/phacility/arcanist/test.php on line 4
```
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14560
Summary:
A class containing `abstract` methods must itself be marked as `abstract`.
```
PHP Fatal error: Class X contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (X::Y) in /home/josh/workspace/github.com/phacility/arcanist/test.php on line 5
```
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14559
Summary:
Methods cannot be marked as both `abstract` and `private`. This language construct will cause a fatal error:
```
PHP Fatal error: Abstract function X::Y() cannot be declared private in /home/josh/workspace/github.com/phacility/arcanist/test.php on line 4
```
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14558
Summary: Add a linter rule to detect symbols which are aliased with the same name, such as `use X\Y\Z as Z`. This is unnecessary and is more-simply expressed as `use X\Y\Z`.
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14557
Summary:
When importing or aliases a symbol with a `use` statement, the leading namespace separator is optional and does not modify the behavior. That is, `use \X` is equivalent to `use X`. As such, the latter syntax should be preferred because it is more concise.
According to the [[http://php.net/manual/en/language.namespaces.importing.php | PHP documentation]]:
> Note that for namespaced names (fully qualified namespace names containing namespace separator, such as `Foo\Bar` as opposed to global names that do not, such as `FooBar`), the leading backslash is unnecessary and not recommended, as import names must be fully qualified, and are not processed relative to the current namespace.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14517
Summary: This is failing locally with `cppcheck` version 1.69.
Test Plan: Ran `arc unit`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14524
Summary: Fix the `PHPCompatibilityXHPASTLinterRule` after changes to the way in which #xhpast parses `use` statements. Depends on D14518.
Test Plan: Unit tests
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14519
Summary:
Although it is technically possible to return a value from a PHP constructor, it is rather odd and should be avoided. See some discussion on [[http://stackoverflow.com/q/11904255 | StackOverflow]]. Consider the following example:
```lang=php
class SomeClass {
public function __construct() {
return 'quack';
}
}
```
This doesn't work:
```lang=php, counterexample
echo new SomeClas();
```
This, strangely, does work:
```lang=php
echo id(new SomeClass())->__construct();
```
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14516
Summary:
Add a linter rule to detect static method calls which //should// reference the `parent` class instead of a hardcoded class reference. For example, consider the following:
```lang=php, counterexample
class SomeClass extends AnotherClass {
public function someMethod() {
AnotherClass::someOtherMethod();
}
}
```
This should instead be written as:
```lang=php
class SomeClass extends AnotherClass {
public function someMethod() {
parent::someOtherMethod();
}
}
```
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D14443
Summary:
Separate XHPAST linter rules in isolation from other rules and formalize the concept of a "linter rule". As the number of XHPAST linter rules grows (we currently have around 80), it becomes increasingly difficult to manage all of the test cases because `ArcanistXHPASTLinterTestCase` currently tests the entire linter (i.e. //all// linter rules) rather than testing individual rules in isolation. See D13534 for a situation in which this is painful. This is particularly bad for third party development because unit tests could break at any time depending on upstream changes.
Basically, in order to facilitate the unit testing of XHPAST linter rules in isolation, I have made the following changes:
# Added a `setRules()` method to `ArcanistXHPASTLinter`. Currently, `ArcanistXHPASTLinter` loads all `ArcanistXHPASTLinterRule` subclasses unconditionally. The `setRules()` method provides a way to override the configured linter rules.
# Formalize the concept of a "linter rule". The `ArcanistXHPASTLinterRule` class was introduced in D10541. I feel that the modularization of `ArcanistXHPASTLinter` has made the linter much more maintainable and easily testable and I intend to extend this concept to other linters, such as `ArcanistTextLinter`.
Test Plan: Ran unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14010
