ESLint autofix is wrong

[Timmmm]

This took me a while to figure out but the ESLintLinter uses the column from ESLint to tell Arc what to fix:

        $message->setChar(idx($offense, 'column'));

However this is the position of the error. It can actually be different to the position of the suggestion. For example with this code:

import { addTab, addIframeTab } from "./tabs.js";

ESLint will output:

      {
        "ruleId": "sort-imports",
        "severity": 2,
        "message": "Member 'addIframeTab' of the import declaration should be sorted alphabetically.",
        "line": 1,
        "column": 18,
        "nodeType": "ImportSpecifier",
        "messageId": "sortMembersAlphabetically",
        "endLine": 1,
        "endColumn": 30,
        "fix": {
          "range": [
            9,
            29
          ],
          "text": "addIframeTab, addTab"
        }
      },

It should use the 9 offset to calculate the column, but it actually uses 18, resulting in this silly patch:

  Auto-Fix  (ESLINT) sort-imports
    Member 'addIframeTab' of the import declaration should be sorted
    alphabetically.

    >>> -      1 import { addTab, addTab, addIframeom "./tabs.js";
        +        import { addTab, addIframeTab, addom "./tabs.js";

Unfortunately it doesn't look trivial to fix. My suggestion would be to apply all the fixes from the JSON in ESLintLinter and then just give the whole file as a single fix in one message.

I did want to just use the --fix flag but unfortunately it can add or remove lines and then the positions of the other errors are wrong (and Arc freaks out if they are beyond the end of the fixed file).

[Timmmm]

I looked into this a bit and unfortunately it is not easy to fix:

1. I don't think you can apply all of the fixes automatically. Even if you sort them and do everything properly there are still some whose ranges can overlap. I'm not exactly sure what ESLint does but as far as I can tell the only way to actually get a properly auto-fixed file is to have ESLint do it via --fix or --fix-dry-run.
2. Unfortunately if you use either of those options, ESLint changes the line/column numbers of the errors/warnings to be those of the fixed file.

Therefore the only way I found to do this correctly is to run ESLint twice - once to report errors/warnings and once to only do the autofix. It's a bit horrible but eh.

[Timmmm]

I got it working like this, but you have to add it to .arclint twice - once with fix_mode true and once with it false. That kind of sucks but eh.

<?php
/**
 * Copyright 2016 Pinterest, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

/**
 * Lints JavaScript and JSX files using ESLint
 */
final class ESLintLinter extends NodeExternalLinter {
  const ESLINT_WARNING = '1';
  const ESLINT_ERROR = '2';

  private $flags = array();
  private $fixMode = false;

  public function getInfoName() {
    return 'ESLint';
  }

  public function getInfoURI() {
    return 'https://eslint.org/';
  }

  public function getInfoDescription() {
    return pht('The pluggable linting utility for JavaScript and JSX');
  }

  public function getLinterName() {
    return 'ESLINT';
  }

  public function getLinterConfigurationName() {
    return 'eslint';
  }

  public function getNodeBinary() {
    return 'eslint';
  }

  public function getVersion() {
    list($err, $stdout, $stderr) = exec_manual('%C -v', $this->getExecutableCommand());

    $matches = array();
    if (preg_match('/^v(\d\.\d\.\d)$/', $stdout, $matches)) {
      return $matches[1];
    } else {
      return false;
    }
  }

  protected function getMandatoryFlags() {
    return array(
      '--format=json',
      '--no-color',
    );
  }

  protected function getDefaultFlags() {
    if ($this->cwd) {
      $this->flags[] = '--resolve-plugins-relative-to';
      $this->flags[] = $this->cwd;
    }
    return $this->flags;
  }

  public function getLinterConfigurationOptions() {
    $options = array(
      'eslint.fix_mode' => array(
        'type' => 'optional bool',
        'help' => pht('If run in this mode, instead of reporting errors/warnings it will apply auto-fixes.'),
      ),
    );
    return $options + parent::getLinterConfigurationOptions();
  }

  public function setLinterConfigurationValue($key, $value) {
    switch ($key) {
      case 'eslint.fix_mode':
        if ($value) {
          $this->fixMode = true;
          $this->flags[] = '--fix-dry-run';
        }
        return;
    }
    return parent::setLinterConfigurationValue($key, $value);
  }

  protected function canCustomizeLintSeverities() {
    return false;
  }

  protected function parseLinterOutput($path, $err, $stdout, $stderr) {
    // Gate on $stderr b/c $err (exit code) is expected.
    if ($stderr) {
      return false;
    }

    $json = json_decode($stdout, true);
    $messages = array();

    foreach ($json as $file) {
      foreach ($file['messages'] as $offense) {
        // Skip file ignored warning: if a file is ignored by .eslintingore
        // but linted explicitly (by arcanist), a warning will be reported,
        // containing only: `{fatal:false,severity:1,message:...}`.
        if (strpos($offense['message'], "File ignored ") === 0) {
          continue;
        }

        /**
         * Example ESLint message:
         * {
         *     "ruleId": "prettier/prettier",
         *     "severity": 2,
         *     "message": "Replace `(flow.component·&&·flow.component.archived)` \
         *         with `flow.component·&&·flow.component.archived`",
         *     "line": 61,
         *     "column": 10,
         *     "nodeType": null,
         *     "messageId": "replace",
         *     "endLine": 61,
         *     "endColumn": 53,
         *     "fix": {
         *         "range": [
         *             1462,
         *             1505
         *         ],
         *         "text": "flow.component && flow.component.archived"
         *     }
         * },
         */

        if (!$this->fixMode) {
          $message = new ArcanistLintMessage();
          $message->setPath($file['filePath']);
          $message->setName(nonempty(idx($offense, 'ruleId'), 'unknown'));
          $message->setDescription(idx($offense, 'message'));
          $message->setLine(idx($offense, 'line'));
          $message->setChar(idx($offense, 'column'));
          $message->setCode($this->getLinterName());
          $message->setSeverity($this->mapSeverity(idx($offense, 'severity', '0')));

          $messages[] = $message;
        }
      }

      // `output` is present if --fix or --fix-dry-run was passed, *and*
      // some fixes were required.
      if ($this->fixMode && isset($file['output'])) {
        $originalText = $this->getData($path);

        // This requires --fix or --fix-dry-run.
        $fixedText = $file['output'];

        // Make a new auto-fix message for the whole file.
        $message = new ArcanistLintMessage();
        $message->setPath($file['filePath']);
        $message->setName('AUTOFIXES');
        $message->setDescription('Autofixes');
        $message->setLine(1);
        $message->setChar(1);
        $message->setCode($this->getLinterName());
        $message->setOriginalText($originalText);
        $message->setReplacementText($fixedText);
        $message->setSeverity(ArcanistLintSeverity::SEVERITY_AUTOFIX);

        $messages[] = $message;
      }

      // Arc freaks out the exit code is not 0 and we didn't produce any
      // messages, which can happen here (e.g. if there's nothing to fix but there
      // were unfixable errors). In that case make a dummy message which is
      // SEVERITY_DISABLED so it isn't shown to the user.
      if ($err && count($messages) === 0) {
        // Make a new auto-fix message for the whole file.
        $message = new ArcanistLintMessage();
        $message->setPath($file['filePath']);
        $message->setName('IGNORE_THIS');
        $message->setDescription('Ignore this message. It is just here to work around an Arc bug.');
        $message->setLine(1);
        $message->setChar(1);
        $message->setCode($this->getLinterName());
        $message->setSeverity(ArcanistLintSeverity::SEVERITY_DISABLED);

        $messages[] = $message;
      }
    }

    return $messages;
  }

  private function mapSeverity($eslintSeverity) {
    switch($eslintSeverity) {
      case '0':
      case '1':
        return ArcanistLintSeverity::SEVERITY_WARNING;
      case '2':
      default:
        return ArcanistLintSeverity::SEVERITY_ERROR;
    }
  }
}