Correct some lint console renderer issues with files missing trailing newlines

Summary:
See PHI162. This corrects a couple more bugs:

  - If the old file didn't end in a newline, we could end up printing two lines next to each other in the output.
  - If the patch targeted "Line 6, character 1" instead of "line 5, character 3" in a file "1\n2\n3\n4\n5\n", we would fail to figure out what that meant when computing an offset because the last line has 0 characters on it.

Test Plan: Added failing unit tests, made them pass. Also tested with some fake linters similar to the ones described in PHI162.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18716

src/lint/renderer/ArcanistConsoleLintRenderer.php

@@ -216,8 +216,11 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
     }
 
     for ($ii = $start; $ii < $start + $new_impact; $ii++) {
+      // If the patch was at the end of the file and ends with a newline, we
+      // won't have an actual entry in the array for the last line, even though
+      // we want to show it in the diff.
       $out[] = array(
-        'text' => $new_lines[$ii - 1],
+        'text' => idx($new_lines, $ii - 1, ''),
         'type' => '+',
         'chevron' => ($ii == $start),
       );
@@ -246,9 +249,17 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
         }
       }
 
+      // If the line doesn't actually end in a newline, add one so the layout
+      // doesn't mess up. This can happen when the last line of the old file
+      // didn't have a newline at the end.
+      $text = $spec['text'];
+      if (!preg_match('/\n\z/', $spec['text'])) {
+        $text .= "\n";
+      }
+
       $result[] = $this->renderLine(
         idx($spec, 'number'),
-        $spec['text'],
+        $text,
         $chevron,
         idx($spec, 'type'));
 
@@ -298,6 +309,15 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
       $offset += strlen($line);
     }
 
+    // If the last line ends in a newline, add a virtual offset for the final
+    // line with no characters on it. This allows lint messages to target the
+    // last line of the file at character 1.
+    if ($lines) {
+      if (preg_match('/\n\z/', $line)) {
+        $line_map[$number] = $offset;
+      }
+    }
+
     return $line_map;
   }
 

src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php

@@ -120,6 +120,28 @@ EOTEXT;
         'original' => "\n",
         'replacement' => '',
       ),
+
+      'eofnewline' => array(
+        'line' => 1,
+        'char' => 7,
+        'original' => '',
+        'replacement' => "\n",
+      ),
+
+      'eofmultilinechar' => array(
+        'line' => 5,
+        'char' => 3,
+        'original' => '',
+        'replacement' => "\nX\nY\n",
+      ),
+
+      'eofmultilineline' => array(
+        'line' => 6,
+        'char' => 1,
+        'original' => '',
+        'replacement' => "\nX\nY\n",
+      ),
+
     );
 
     $defaults = array(
@@ -132,8 +154,6 @@ EOTEXT;
 
     foreach ($map as $key => $test_case) {
       $data = $this->readTestData("{$key}.txt");
-      $data = preg_replace('/~+\s*$/m', '', $data);
-
       $expect = $this->readTestData("{$key}.expect");
 
       $test_case = $test_case + $defaults;
@@ -178,11 +198,6 @@ EOTEXT;
         throw $ex;
       }
 
-      // Trim "~" off the ends of lines. This allows the "expect" file to test
-      // for trailing whitespace without actually containing trailing
-      // whitespace.
-      $expect = preg_replace('/~+$/m', '', $expect);
-
       $this->assertEqual(
         $expect,
         $actual,
@@ -194,7 +209,19 @@ EOTEXT;
 
   private function readTestData($filename) {
     $path = dirname(__FILE__).'/data/'.$filename;
-    return Filesystem::readFile($path);
+    $data = Filesystem::readFile($path);
+
+    // If we find "~~~" at the end of the file, get rid of it and any whitespace
+    // afterwards. This allows specifying data files with trailing empty
+    // lines.
+    $data = preg_replace('/~~~\s*\z/', '', $data);
+
+    // Trim "~" off the ends of lines. This allows the "expect" file to test
+    // for trailing whitespace without actually containing trailing
+    // whitespace.
+    $data = preg_replace('/~$/m', '', $data);
+
+    return $data;
   }
 
 }

src/lint/renderer/__tests__/data/eofmultilinechar.expect

@@ -0,0 +1,12 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               3 3
+               4 4
+               5 5
+    >>> +        ~
+        +        X
+        +        Y

src/lint/renderer/__tests__/data/eofmultilinechar.txt

@@ -0,0 +1,5 @@
+1
+2
+3
+4
+5

src/lint/renderer/__tests__/data/eofmultilineline.expect

@@ -0,0 +1,12 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               3 3
+               4 4
+               5 5
+    >>> +        ~
+        +        X
+        +        Y

src/lint/renderer/__tests__/data/eofmultilineline.txt

@@ -0,0 +1,5 @@
+1
+2
+3
+4
+5

src/lint/renderer/__tests__/data/eofnewline.expect

@@ -0,0 +1,9 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+    >>> -      1 abcdef
+        +        abcdef
+        +        ~

src/lint/renderer/__tests__/data/eofnewline.txt

@@ -0,0 +1 @@
+abcdef~~~

src/lint/renderer/__tests__/data/extrawhitespace.txt

@@ -1,3 +1,3 @@
 Adrift upon the sea.
 
-~
+~~~