1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-09 00:12:40 +01:00
Commit graph

2075 commits

Author SHA1 Message Date
Michael Akinde
22af67c1c0 Minor fixes to arcanist cpplint
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
2016-01-14 06:59:10 -08:00
epriestley
05c12eb9d9 If the Script-and-Regex linter captures no "line" text, treat the message as affecting the entire file
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
2016-01-11 17:37:34 -08:00
John Allen
aeb374b333 Fix cpplint message regex
Summary:
ref T8404

The issue is caused, not by the non-zero return code, but by the fact that
$messages is coming back empty due to the incorrect regex. tyhoff pointed out
that the regex matched correctly when we used STDIN, but now it is failing.

https://secure.phabricator.com/diffusion/ARC/browse/master/src/lint/linter/ArcanistExternalLinter.php;8c589f1f759f0913135b8cc6959a6c1589e14ae4$357

Test Plan:
arc lint cpp file containing lint error. run cpplint on the
file directly to confirm that there are errors and that the return code
is non-zero

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T8404

Differential Revision: https://secure.phabricator.com/D14960
2016-01-06 13:15:09 -08:00
epriestley
98d71571e4 Fix arc diff --raw with "onto" target properties
Summary:
Currently, `git show | arc diff --raw` and similar doesn't work because we try to figure out what the "Branch: feature (branched from whatever)" value is, which doesn't make sense.

```
$ git show | arc diff --raw --trace
 ARGV  '/Users/epriestley/dev/core/lib/arcanist/bin/../scripts/arcanist.php' 'diff' '--raw' '--trace'
 LOAD  Loaded "phutil" from "/Users/epriestley/dev/core/lib/libphutil/src".
 LOAD  Loaded "arcanist" from "/Users/epriestley/dev/core/lib/arcanist/src".
Config: Reading user configuration file "/Users/epriestley/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/Users/epriestley/dev/core/lib/arcanist/.arcconfig".
Working Copy: Path "/Users/epriestley/dev/core/lib/arcanist" is part of `git` working copy "/Users/epriestley/dev/core/lib/arcanist".
Working Copy: Project root is at "/Users/epriestley/dev/core/lib/arcanist".
Config: Reading local configuration file "/Users/epriestley/dev/core/lib/arcanist/.git/arc/config"...
Loading phutil library from '/Users/epriestley/dev/core/lib/arcanist/src'...
>>> [0] <conduit> conduit.connect() <bytes = 489>
>>> [1] <http> https://secure.phabricator.com/api/conduit.connect
<<< [1] <http> 211,217 us
<<< [0] <conduit> 212,001 us
>>> [2] <event> diff.didCollectChanges <listeners = 0>
<<< [2] <event> 140 us
>>> [3] <event> diff.didBuildMessage <listeners = 0>
<<< [3] <event> 46 us
Reading diff from stdin...
>>> [4] <conduit> differential.creatediff() <bytes = 10,542>
>>> [5] <http> https://secure.phabricator.com/api/differential.creatediff
<<< [5] <http> 120,215 us
<<< [4] <conduit> 120,411 us
>>> [6] <event> diff.wasCreated <listeners = 0>
<<< [6] <event> 41 us
 SKIP STAGING  Raw changes can not be pushed to a staging area.
>>> [7] <conduit> harbormaster.queryautotargets() <bytes = 290>
>>> [8] <http> https://secure.phabricator.com/api/harbormaster.queryautotargets
<<< [8] <http> 217,717 us
<<< [7] <conduit> 217,944 us
>>> [9] <conduit> harbormaster.sendmessage() <bytes = 274>
>>> [10] <http> https://secure.phabricator.com/api/harbormaster.sendmessage
>>> [11] <conduit> harbormaster.sendmessage() <bytes = 274>
>>> [12] <http> https://secure.phabricator.com/api/harbormaster.sendmessage
<<< [10] <http> 123,821 us
<<< [9] <conduit> 134,329 us
<<< [12] <http> 227,580 us
<<< [11] <conduit> 227,787 us

[2016-01-05 10:08:58] EXCEPTION: (Exception) This workflow ('ArcanistDiffWorkflow') requires a Repository API, override requiresRepositoryAPI() to return true. at [<arcanist>/src/workflow/ArcanistWorkflow.php:804]
arcanist(head=master, ref.master=b3e68c9f1793), phutil(head=stable, ref.master=adb8a9c074ba, ref.stable=7b8d38cd2d4e)
  #0 ArcanistWorkflow::getRepositoryAPI() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2421]
  #1 ArcanistDiffWorkflow::getDiffOntoTargets() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2411]
  #2 ArcanistDiffWorkflow::updateOntoDiffProperty() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:534]
  #3 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:392]
```

Test Plan: Ran `arc diff --raw` in `phabricator/`.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14946
2016-01-05 10:16:39 -08:00
Joshua Spence
b3e68c9f17 Add a linter rule for use of is_a
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
2015-12-23 08:44:39 +11:00
Joshua Spence
9ee14d2849 Improve the "self member reference" linter rule
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
2015-12-23 08:39:44 +11:00
epriestley
8762e3f367 Use "--whitespace nowarn" in arc patch to respect trailing whitespace
Summary: Fixes T10008. Git tries to fix some issues by default (apparently? empirically; not consistent with documentation, I think?), but patches from `arc patch` are "always" accurate (disregarding other bugs we might have -- basically, they haven't been emailed or copy/pasted or anything like that) so we can just tell it to apply the patch exactly as-is.

Test Plan: {F1029182}

Reviewers: chad, joshuaspence

Reviewed By: chad, joshuaspence

Maniphest Tasks: T10008

Differential Revision: https://secure.phabricator.com/D14816
2015-12-17 17:36:26 -08:00
epriestley
a183c2c4ba When arc is run with --trace, print out argv explicitly
Summary:
Ref T9993. Users may have shell aliases or wrapper scripts that they forget about.

Print out the arguments we received to make it obvious that something went through an indirection layer.

Test Plan: Ran `arc version --help`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9993

Differential Revision: https://secure.phabricator.com/D14797
2015-12-17 16:15:00 -08:00
epriestley
9a373c88d7 Explain how to move forward or backward from arc land --hold
Summary: Fixes T9973. When users run `arc land --hold`, give them the commands to move forward (push) or backward (checkout the branch/tag/commit they were on) and be explicit that branches have not changed.

Test Plan: Ran `arc land --hold`, got lots of explanatory text.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9973

Differential Revision: https://secure.phabricator.com/D14762
2015-12-13 08:19:04 -08:00
Dimitry Andric
7d25fcdbdb Simplify diff exit code handling.
Summary:
This fixes T9970 in an alternate manner, with the same effect: the
binary_safe_diff.sh script returns 0 if the diff succeeds, 1 in all
other cases.

Test Plan:
Tested locally with a fixed binary_safe_diff.sh, resulting in this
correct review:

https://reviews.freebsd.org/D4542

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: eadler, Korvin, stevenh

Maniphest Tasks: T9970

Differential Revision: https://secure.phabricator.com/D14759
2015-12-13 02:35:07 -08:00
Dimitry Andric
97056a3b85 Preserve the exit code of the diff command in binary_safe_diff.sh
Summary:
Some versions of Subversion (1.9 in any case, maybe others) will
duplicate diff headers, if the diff command run through --diff-cmd
returns 0.

This lead to T9970, where the addition of a new file with properties
only shows the properties themselves in the review, not the content of
the new file.

Test Plan: This is a trivial change, is a test needed at all?

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: stevenh, Korvin, eadler

Differential Revision: https://secure.phabricator.com/D14755
2015-12-13 02:21:43 -08:00
epriestley
74c7495b1a Clarify that "arc land" means it is merging changes, not branch refences
Summary: Ref T9973. Make this language unambiguously clear about the underlying operations.

Test Plan: Ran `arc help land`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9973

Differential Revision: https://secure.phabricator.com/D14754
2015-12-12 10:25:12 -08:00
epriestley
dae2f0073f In arc diff, try to guess where a change should land
Summary:
Ref T9952. Ref T3462. My primary goal is to improve prefilling of the "Onto Branch:" field in the "Land Revision" dialog.

When uploading a diff with `arc diff`, add a property with some information about which branch to target. In particular:

  - If the local branch tracks an upstream branch (or tracks something which tracks something which tracks the upstream), target that.
  - If not, but "arc.land.onto.default" is set, target that.

This doesn't try to guess in other cases, since they're more involved. I'll add some context about this in T3462.

I don't //love// using "diff properties" for this, but it doesn't make cleaning them up any harder since we already use it for other stuff which isn't going away (lint/unit excuses).

Test Plan:
  - Added some `var_dump()` and used `arc diff --only` to generate diffs.
  - Saw upstream tracking and config-based rules generate reasonable values and submit them.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T3462, T9952

Differential Revision: https://secure.phabricator.com/D14736
2015-12-10 15:24:38 -08:00
epriestley
d0e73bb656 Don't use "-c" flag in "git:branch-unique()" revision range rule
Summary:
Fixes T9953.

  - "-c" was introduced in 1.7.2.
  - "--no-color" has existed forever as far as I can tell.
  - "--no-column" was introducd in 1.7.11, but there was nothing that needed to be disabled before that (hopefully).

Test Plan:
  - Ran `arc which --trace` and observed a reasonable `git branch` command with correct output.
  - Ran `arc which --trace` with a faked older Git version, observed command omit `--no-column`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9953

Differential Revision: https://secure.phabricator.com/D14735
2015-12-10 14:19:39 -08:00
Joshua Spence
0c8124a272 Fix handling of empty array index
Fix handling of empty array index by `ArcanistCurlyBraceArrayIndexXHPASTLinterRule`.

Auditors: epriestley
2015-12-09 08:09:02 +11:00
Joshua Spence
d2e7785497 Add a linter rule for curly brace array indexes
Summary: In PHP, both `$x['key']` and `$x{'key'}` can be used to access an array (see  http://stackoverflow.com/questions/8092248/php-curly-braces-in-array-notation), but the former should be preferred.

Test Plan: Added test cases.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14603
2015-12-09 07:16:12 +11:00
Joshua Spence
a6e81daad1 Improve brace formatting linter rule
Summary: Allow the brace formatting linter rule to complain about `class X{}`.

Test Plan: Updated unit tests.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14690
2015-12-07 20:20:57 +00:00
Joshua Spence
b52e9dc702 Linter fixes
Summary: Minor linter fixes

Test Plan: N/A

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14633
2015-12-07 21:35:34 +11:00
epriestley
4a680c762b Tailor CLI feedback about "arc alias" to describe shell command aliases
Summary:
Fixes T9873. Instead of printing something like this:

> Aliased "arc ls" to "arc !ls".

...print this:

> Aliased "arc ls" to shell command "ls".

Test Plan:
  - Added shell commands and internal aliases.
  - Removed aliases.
  - Listed aliases.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9873

Differential Revision: https://secure.phabricator.com/D14651
2015-12-03 18:31:48 -08:00
Joshua Spence
cdaad096fa Add a linter rule for public properties
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
2015-12-03 09:48:48 +11:00
Joshua Spence
3c193984da Add a "binary integer casing" linter rule
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
2015-12-03 09:47:27 +11:00
Joshua Spence
ae3c2cb0e1 Add binary integers to ArcanistPHPCompatibilityXHPASTLinterRule
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
2015-12-03 08:39:11 +11:00
Joshua Spence
560e4ae491 Add a linter rule for invalid octals
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
2015-12-03 08:27:51 +11:00
Joshua Spence
8ed3c45c82 Use CaseInsensitiveArray in ArcanistFunctionCallShouldBeTypeCastXHPASTLinterRule
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
2015-12-03 08:10:30 +11:00
Joshua Spence
76ae02325d Fix another edge case for "function call should be type cast" linter rule
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
2015-12-02 14:13:11 +11:00
Joshua Spence
09052d5247 Minor fix for typecast linter rule
Summary: `intval($x, $y)` cannot be safely changed to `(int)$x` if `$y != 10`.

Test Plan: Added test cases.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14611
2015-12-02 14:12:53 +11:00
Joshua Spence
00efcd294f Add a linter rule for hexadecimal number casing
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
2015-12-02 07:47:16 +11:00
Joshua Spence
5218ec357c Add a table showing all XHPAST linter rules to the output of arc linters xhpast
Summary: Now that we have 91 subclasses of `ArcanistXHPASTLinterRule`, it is becoming difficult to manage the IDs. This diff adds a table to `arc linters xhpast` workflow to facilitate this.

Test Plan:
```
> arc xhpast-linter-rules

+=====+=======================================================+===================================================================+
| ID  | Class                                                 | Name                                                              |
+=====+=======================================================+===================================================================+
| 1   | ArcanistSyntaxErrorXHPASTLinterRule                   | PHP Syntax Error!                                                 |
| 2   | ArcanistUnableToParseXHPASTLinterRule                 | Unable to Parse                                                   |
| 3   | ArcanistVariableVariableXHPASTLinterRule              | Use of Variable Variable                                          |
| 4   | ArcanistExtractUseXHPASTLinterRule                    | Use of `extract`                                                  |
| 5   | ArcanistUndeclaredVariableXHPASTLinterRule            | Use of Undeclared Variable                                        |
| 6   | ArcanistPHPShortTagXHPASTLinterRule                   | Use of Short Tag `<?`                                             |
| 7   | ArcanistPHPEchoTagXHPASTLinterRule                    | Use of Echo Tag `<?=`                                             |
| 8   | ArcanistPHPCloseTagXHPASTLinterRule                   | Use of Close Tag `?>`                                             |
| 9   | ArcanistNamingConventionsXHPASTLinterRule             | Naming Conventions                                                |
| 10  | ArcanistImplicitConstructorXHPASTLinterRule           | Implicit Constructor                                              |
| 12  | ArcanistDynamicDefineXHPASTLinterRule                 | Dynamic `define`                                                  |
| 13  | ArcanistStaticThisXHPASTLinterRule                    | Use of `$this` in Static Context                                  |
| 14  | ArcanistPregQuoteMisuseXHPASTLinterRule               | Misuse of `preg_quote`                                            |
| 15  | ArcanistPHPOpenTagXHPASTLinterRule                    | Expected Open Tag                                                 |
| 16  | ArcanistTodoCommentXHPASTLinterRule                   | TODO Comment                                                      |
| 17  | ArcanistExitExpressionXHPASTLinterRule                | `exit` Used as Expression                                         |
| 18  | ArcanistCommentStyleXHPASTLinterRule                  | Comment Style                                                     |
| 19  | ArcanistClassFilenameMismatchXHPASTLinterRule         | Class-Filename Mismatch                                           |
| 20  | ArcanistTautologicalExpressionXHPASTLinterRule        | Tautological Expression                                           |
| 21  | ArcanistPlusOperatorOnStringsXHPASTLinterRule         | Not String Concatenation                                          |
| 22  | ArcanistDuplicateKeysInArrayXHPASTLinterRule          | Duplicate Keys in Array                                           |
| 23  | ArcanistReusedIteratorXHPASTLinterRule                | Reuse of Iterator Variable                                        |
| 24  | ArcanistBraceFormattingXHPASTLinterRule               | Brace Placement                                                   |
| 25  | ArcanistParenthesesSpacingXHPASTLinterRule            | Spaces Inside Parentheses                                         |
| 26  | ArcanistControlStatementSpacingXHPASTLinterRule       | Space After Control Statement                                     |
| 27  | ArcanistBinaryExpressionSpacingXHPASTLinterRule       | Space Around Binary Operator                                      |
| 28  | ArcanistArrayIndexSpacingXHPASTLinterRule             | Spacing Before Array Index                                        |
| 30  | ArcanistImplicitFallthroughXHPASTLinterRule           | Implicit Fallthrough                                              |
| 32  | ArcanistReusedAsIteratorXHPASTLinterRule              | Variable Reused As Iterator                                       |
| 34  | ArcanistCommentSpacingXHPASTLinterRule                | Comment Spaces                                                    |
| 36  | ArcanistSlownessXHPASTLinterRule                      | Slow Construct                                                    |
| 37  | ArcanistCallParenthesesXHPASTLinterRule               | Call Formatting                                                   |
| 38  | ArcanistDeclarationParenthesesXHPASTLinterRule        | Declaration Formatting                                            |
| 39  | ArcanistReusedIteratorReferenceXHPASTLinterRule       | Reuse of Iterator References                                      |
| 40  | ArcanistKeywordCasingXHPASTLinterRule                 | Keyword Conventions                                               |
| 41  | ArcanistDoubleQuoteXHPASTLinterRule                   | Unnecessary Double Quotes                                         |
| 42  | ArcanistElseIfUsageXHPASTLinterRule                   | `elseif` Usage                                                    |
| 43  | ArcanistSemicolonSpacingXHPASTLinterRule              | Semicolon Spacing                                                 |
| 44  | ArcanistConcatenationOperatorXHPASTLinterRule         | Concatenation Spacing                                             |
| 45  | ArcanistPHPCompatibilityXHPASTLinterRule              | PHP Compatibility                                                 |
| 46  | ArcanistLanguageConstructParenthesesXHPASTLinterRule  | Language Construct Parentheses                                    |
| 47  | ArcanistEmptyStatementXHPASTLinterRule                | Empty Block Statement                                             |
| 48  | ArcanistArraySeparatorXHPASTLinterRule                | Array Separator                                                   |
| 49  | ArcanistConstructorParenthesesXHPASTLinterRule        | Constructor Parentheses                                           |
| 50  | ArcanistDuplicateSwitchCaseXHPASTLinterRule           | Duplicate Case Statements                                         |
| 51  | ArcanistBlacklistedFunctionXHPASTLinterRule           | Use of Blacklisted Function                                       |
| 52  | ArcanistImplicitVisibilityXHPASTLinterRule            | Implicit Method Visibility                                        |
| 53  | ArcanistCallTimePassByReferenceXHPASTLinterRule       | Call-Time Pass-By-Reference                                       |
| 54  | ArcanistFormattedStringXHPASTLinterRule               | Formatted String                                                  |
| 55  | ArcanistUnnecessaryFinalModifierXHPASTLinterRule      | Unnecessary Final Modifier                                        |
| 56  | ArcanistUnnecessarySemicolonXHPASTLinterRule          | Unnecessary Semicolon                                             |
| 57  | ArcanistSelfMemberReferenceXHPASTLinterRule           | Self Member Reference                                             |
| 58  | ArcanistLogicalOperatorsXHPASTLinterRule              | Logical Operators                                                 |
| 59  | ArcanistInnerFunctionXHPASTLinterRule                 | Inner Functions                                                   |
| 60  | ArcanistDefaultParametersXHPASTLinterRule             | Default Parameters                                                |
| 61  | ArcanistLowercaseFunctionsXHPASTLinterRule            | Lowercase Functions                                               |
| 62  | ArcanistClassNameLiteralXHPASTLinterRule              | Class Name Literal                                                |
| 63  | ArcanistUselessOverridingMethodXHPASTLinterRule       | Useless Overriding Method                                         |
| 64  | ArcanistNoParentScopeXHPASTLinterRule                 | No Parent Scope                                                   |
| 65  | ArcanistAliasFunctionXHPASTLinterRule                 | Alias Functions                                                   |
| 66  | ArcanistCastSpacingXHPASTLinterRule                   | Cast Spacing                                                      |
| 67  | ArcanistToStringExceptionXHPASTLinterRule             | Throwing Exception in `__toString` Method                         |
| 68  | ArcanistLambdaFuncFunctionXHPASTLinterRule            | `__lambda_func` Function                                          |
| 69  | ArcanistInstanceOfOperatorXHPASTLinterRule            | `instanceof` Operator                                             |
| 70  | ArcanistInvalidDefaultParameterXHPASTLinterRule       | Invalid Default Parameter                                         |
| 71  | ArcanistModifierOrderingXHPASTLinterRule              | Modifier Ordering                                                 |
| 72  | ArcanistInvalidModifiersXHPASTLinterRule              | Invalid Modifiers                                                 |
| 73  | ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule  | Space After Unary Prefix Operator                                 |
| 74  | ArcanistObjectOperatorSpacingXHPASTLinterRule         | Object Operator Spacing                                           |
| 75  | ArcanistUnaryPostfixExpressionSpacingXHPASTLinterRule | Space Before Unary Postfix Operator                               |
| 76  | ArcanistArrayValueXHPASTLinterRule                    | Array Element                                                     |
| 77  | ArcanistListAssignmentXHPASTLinterRule                | List Assignment                                                   |
| 78  | ArcanistInlineHTMLXHPASTLinterRule                    | Inline HTML                                                       |
| 79  | ArcanistGlobalVariableXHPASTLinterRule                | Global Variables                                                  |
| 80  | ArcanistParseStrUseXHPASTLinterRule                   | Questionable Use of `parse_str`                                   |
| 81  | ArcanistNewlineAfterOpenTagXHPASTLinterRule           | Newline After PHP Open Tag                                        |
| 82  | ArcanistEmptyFileXHPASTLinterRule                     | Empty File                                                        |
| 83  | ArcanistParentMemberReferenceXHPASTLinterRule         | Parent Member Reference                                           |
| 84  | ArcanistArrayCombineXHPASTLinterRule                  | `array_combine()` Unreliable                                      |
| 85  | ArcanistDeprecationXHPASTLinterRule                   | Use of Deprecated Function                                        |
| 86  | ArcanistUnsafeDynamicStringXHPASTLinterRule           | Unsafe Usage of Dynamic String                                    |
| 87  | ArcanistRaggedClassTreeEdgeXHPASTLinterRule           | Class Not `abstract` Or `final`                                   |
| 88  | ArcanistClassExtendsObjectXHPASTLinterRule            | Class Not Extending `Phobject`                                    |
| 90  | ArcanistNestedNamespacesXHPASTLinterRule              | Nested `namespace` Statements                                     |
| 91  | ArcanistThisReassignmentXHPASTLinterRule              | `$this` Reassignment                                              |
| 92  | ArcanistUnexpectedReturnValueXHPASTLinterRule         | Unexpected `return` Value                                         |
| 97  | ArcanistUseStatementNamespacePrefixXHPASTLinterRule   | `use` Statement Namespace Prefix                                  |
| 99  | ArcanistUnnecessarySymbolAliasXHPASTLinterRule        | Unnecessary Symbol Alias                                          |
| 107 | ArcanistAbstractPrivateMethodXHPASTLinterRule         | `abstract` Method Cannot Be Declared `private`                    |
| 108 | ArcanistAbstractMethodBodyXHPASTLinterRule            | `abstract` Method Cannot Contain Body                             |
| 113 | ArcanistClassMustBeDeclaredAbstractXHPASTLinterRule   | `class` Containing `abstract` Methods Must Be Declared `abstract` |
+=====+=======================================================+===================================================================+
```

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14563
2015-12-02 06:27:33 +11:00
Joshua Spence
87306cfe14 Add a linter rule for variable reference spacing
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
2015-12-01 07:20:03 +11:00
Joshua Spence
578f540a92 Add a linter rule for *val functions
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
2015-11-30 07:37:07 +11:00
epriestley
4f1141d0c5 Improve error handling in arc install-certificate
Summary: Fixes T9858. Reasonable typos and misunderstandings currently produce very confusing error messages.

Test Plan:
```
$ arc install certificate
Usage Exception: Server URI "certificate" must include a protocol and domain. It should be in the form "https://phabricator.example.com/".
```

  - Also used a good URI.
  - Also used no URI.

Reviewers: joshuaspence, chad

Reviewed By: chad

Maniphest Tasks: T9858

Differential Revision: https://secure.phabricator.com/D14577
2015-11-27 09:05:50 -08:00
Joshua Spence
eeaa176cfc Add a linter rule to ensure that namespace is the first statement in a file
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
2015-11-26 07:34:27 +11:00
Joshua Spence
600dcf83dd Fix linter rule ID
Two XHPAST linter rules currently have the same ID.
2015-11-26 07:31:10 +11:00
Joshua Spence
b323ad4d64 Add a linter rule for abstract methods within an interface
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
2015-11-26 07:23:22 +11:00
Joshua Spence
8183a45804 Add a linter rule for interface method bodies
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
2015-11-26 07:12:00 +11:00
Joshua Spence
28f5b0ddc6 Fix a compatibility issue with ArcanistUnsafeDynamicStringXHPASTLinterRule
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
2015-11-26 06:18:19 +11:00
Joshua Spence
9e78d15fc0 Improve the "unexpected return value" linter rule
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
2015-11-24 10:01:35 +11:00
Joshua Spence
72d9013d29 Add a linter rule for abstract methods containing a body
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
2015-11-24 08:19:35 +11:00
Joshua Spence
37571d8839 Add a linter rule to determine whether a class should be marked as abstract
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
2015-11-24 08:18:38 +11:00
Joshua Spence
20e99acf0a Add a linter rule for abstract private methods
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
2015-11-24 08:18:07 +11:00
Joshua Spence
6f908f633b Add a linter rule for unnecessary symbol aliases
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
2015-11-24 08:17:25 +11:00
Joshua Spence
3b41e62f87 Use Remarkup in linter messages
Summary: Use Remarkup (specifically, backticks) within linter messages. Depends on D14485.

Test Plan: Eyeball it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14489
2015-11-24 06:43:09 +11:00
Joshua Spence
ae210fda9f Add a linter rule for use statement namespace prefixes
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
2015-11-24 06:42:39 +11:00
epriestley
c71fe67ccb Address feedback from D14530
Summary: See D14530.

Test Plan: Careful scrutiny.

Reviewers: chad, alexmv

Reviewed By: alexmv

Differential Revision: https://secure.phabricator.com/D14554
2015-11-23 10:32:13 -08:00
Alex Vandiver
e730ececbc Examine upstream path instead of assuming "origin"
Summary:
Instead of blindly assuming that "origin" is the repository that
arcanist should communicate with, use the remote that is configured
for the branch in git.

Test Plan:
Used `arc which` with a branch with no upstream, an
origin/master upstream, and an upstream/master upstream -- the last of
which is being used to create and land this diff.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: joshuaspence, Korvin

Differential Revision: https://secure.phabricator.com/D14530
2015-11-23 10:29:20 -08:00
epriestley
b32149495b Don't give Mercurial empty string as a remote name
Summary:
Fixes T9807. We currently run commands like this in some cases:

  hg push -r master ''

From T9807, it seems that older Mercurial treated `''` in the same way it would treat no argument, while newer Mercurial does not.

Passing `''` is unusual and not intended.

Test Plan:
From T9807, @cspeckmim confirmed that running this command without the `''` works, and @jgelgens tested the patch itself.

I didn't actually run this code myself, since I don't have Mercurial 3.6.1 installed and the fix seems straightfoward.

Reviewers: chad

Reviewed By: chad

Subscribers: cspeckmim

Maniphest Tasks: T9807

Differential Revision: https://secure.phabricator.com/D14531
2015-11-21 09:54:05 -08:00
Alex Vandiver
a77a77817a Try switching to the branch of the same name, instead of detached head
Summary:
Landing from a branch that directly tracks origin/master places one in
a detached HEAD state.  Instead, examine if there is a local branch of
the name that we landed onto, that also tracks the upstream; if so,
switch to that.

Test Plan:
Made a branch via `git checkout -b testing origin/master`
and tried to `arc land`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T9723

Differential Revision: https://secure.phabricator.com/D14420
2015-11-20 10:58:37 -08:00
Joshua Spence
2eada58960 Fix cppcheck linter test case
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
2015-11-20 10:27:25 +11:00
Joshua Spence
187e10e781 Fix parsing of ruby version for ArcanistRubyLinter
Summary:
This fails to extract the version correctly locally.

```
> ruby --version
ruby 1.8.7 (2014-01-28 patchlevel 376) [x86_64-linux]
```

Test Plan: `arc unit`

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14525
2015-11-20 10:27:21 +11:00
Joshua Spence
581829a99e Write a linter rule for $this reassignment
Summary: Re-assigning `$this` is an invalid PHP construct, see https://3v4l.org/TauX9.

Test Plan: Added unit tests.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14514
2015-11-20 06:50:27 +11:00