Page MenuHomePhabricator

ArcanistTextLinter: Detect trailing tabs
Open, Needs TriagePublic


The ArcanistTextLinter currently detects all tab literals and warns about them. Our project uses tabs for indentation (and spaces for alignment), so we need to disable the LINT_TAB_LITERAL rule.
However, this has an inconvenient side effect: the LINT_TRAILING_WHITESPACE rule only checks for trailing spaces, not tabs, so disabling the tab check also prevents Arcanist from detecting trailing tabs.

Reproduction steps:
With the following .arclint, a trailing tab in a file will be ignored. They shouldn't be, since the trailing whitespace rule is not disabled.

  "linters": {
    "text": {
      "type": "text",
      "severity": {
        "2": "disabled"

Version information:

$ arc version
arcanist ada63de9722e4b60ec9f0900a757b5b3873fc2f7 (29 Apr 2017)
libphutil d02cc05931b02c684d4c729510090591ca45f951 (29 Apr 2017)

Revisions and Commits

Event Timeline

If you want to test it, I'll accept this change or some substantially similar change upstream:

diff --git a/src/lint/linter/ArcanistTextLinter.php b/src/lint/linter/ArcanistTextLinter.php
index 2f81b039..57cbfb0b 100644
--- a/src/lint/linter/ArcanistTextLinter.php
+++ b/src/lint/linter/ArcanistTextLinter.php
@@ -245,7 +245,7 @@ final class ArcanistTextLinter extends ArcanistLinter {
     $matches = null;
     $preg = preg_match_all(
-      '/ +$/m',
+      '/\s+$/m',

(Ideally, accompanied by new test coverage.)

That seems to decide that "\n\n" is "trailing whitespace", which doesn't really make sense to me, but [ \t] might work better than \s.

The patch above fixes the issue, but as you mention, double newlines are also considered whitespace, which is pretty bad and removes every empty line.
Additionally, \r is also covered by \s, so the patch above creates a new issue that is very similar to mine: the rule LINT_TRAILING_WHITESPACE fixes line endings even when the rule LINT_DOS_NEWLINE is disabled.

Using [ \t] fixes the problem... except with DOS line endings. 👿 So the regex should have \r?$ if I'm not mistaken, with the patch proposal only fixing the [ \t] part of the string.

Hi! I'd love to submit diff + better test cases for this case. I have 3 additional test cases and better character class for whitespace.

@epriestley - I have "almost" ready test suite, hovewer I can't work out line ending test with CR/LF endings. - D17814