Drop special support for point spans at the end of files (#31)

Drop special support for point spans at the end of files This broke point spans at the beginning of lines elsewhere in the file. Unfortunately, there's no way to disambiguate these currently. SourceSpanWithContext doesn't have any information about where the span appears in the context, so for point spans at column 0, it could be at the beginning of the line or after the trailing newline. We should eventually add something like SourceSpanWithContext.indexInContext, but that'll require a breaking release, since downstream users implement FileSpan as an interface.
dart-lang · Jan 29, 2019 · 1a97871 · 1a97871
1 parent 69fdf30
commit 1a97871
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 64 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,8 @@
+# 1.5.4
+
+* `FileSpan.highlight()` now properly highlights point spans at the beginning of
+  lines.
+
 # 1.5.3
 
 * Fix an edge case where `FileSpan.highlight()` would put the highlight

diff --git a/lib/src/file.dart b/lib/src/file.dart
@@ -304,9 +304,9 @@ class _FileSpan extends SourceSpanMixin implements FileSpan {
 
       if (length == 0) {
         // ...unless this is a point span, in which case we want to include the
-        // next line (or the last line if this is the end of the file).
+        // next line (or the empty string if this is the end of the file).
         return endLine == file.lines - 1
-            ? file.getText(file.getOffset(endLine - 1))
+            ? ""
             : file.getText(
                 file.getOffset(endLine), file.getOffset(endLine + 1));
       }

diff --git a/lib/src/highlighter.dart b/lib/src/highlighter.dart
@@ -136,33 +136,18 @@ class Highlighter {
   /// than at the beginning of the next line.
   static SourceSpanWithContext _normalizeEndOfLine(SourceSpanWithContext span) {
     if (span.end.column != 0) return span;
+    if (span.end.line == span.start.line) return span;
 
-    if (span.length == 0) {
-      if (span.end.offset == 0) return span;
+    var text = span.text.substring(0, span.text.length - 1);
 
-      // If [span] is a point span with an empty context, there's no useful
-      // adjustment we can do.
-      if (span.context.isEmpty) return span;
-
-      var location = new SourceLocation(span.end.offset - 1,
-          sourceUrl: span.sourceUrl,
-          line: span.end.line - 1,
-          column: _lastLineLength(span.context));
-      return new SourceSpanWithContext(location, location, "", span.context);
-    } else {
-      if (span.end.line == span.start.line) return span;
-
-      var text = span.text.substring(0, span.text.length - 1);
-
-      return new SourceSpanWithContext(
-          span.start,
-          new SourceLocation(span.end.offset - 1,
-              sourceUrl: span.sourceUrl,
-              line: span.end.line - 1,
-              column: _lastLineLength(text)),
-          text,
-          span.context);
-    }
+    return new SourceSpanWithContext(
+        span.start,
+        new SourceLocation(span.end.offset - 1,
+            sourceUrl: span.sourceUrl,
+            line: span.end.line - 1,
+            column: _lastLineLength(text)),
+        text,
+        span.context);
   }
 
   /// Returns the length of the last line in [text], whether or not it ends in a

diff --git a/pubspec.yaml b/pubspec.yaml
@@ -1,5 +1,5 @@
 name: source_span
-version: 1.5.3
+version: 1.5.4
 
 description: A library for identifying source spans and locations.
 author: Dart Team <[email protected]>

diff --git a/test/file_test.dart b/test/file_test.dart
@@ -276,21 +276,21 @@ zip zap zop""", url: "bar.dart").span(10, 11);
         expect(span.context, equals('whiz bang boom\n'));
       });
 
-      group("contains the last line for a point span at the end of a file", () {
-        test("without a newline", () {
+      group("for a point span at the end of a file", () {
+        test("without a newline, contains the last line", () {
           var span = file.span(file.length, file.length);
           expect(span.context, equals('zip zap zop'));
         });
 
-        test("with a newline", () {
+        test("with a newline, contains an empty line", () {
           file = new SourceFile.fromString("""
 foo bar baz
 whiz bang boom
 zip zap zop
 """, url: "foo.dart");
 
           var span = file.span(file.length, file.length);
-          expect(span.context, equals('zip zap zop\n'));
+          expect(span.context, isEmpty);
         });
       });
     });

diff --git a/test/highlight_test.dart b/test/highlight_test.dart
@@ -45,77 +45,84 @@ zip zap zop
   '"""));
   });
 
-  test("works for a point span", () {
-    expect(file.location(4).pointSpan().highlight(), equals("""
+  group("highlights a point span", () {
+    test("in the middle of a line", () {
+      expect(file.location(4).pointSpan().highlight(), equals("""
   ,
 1 | foo bar baz
   |     ^
   '"""));
-  });
+    });
 
-  test("works for a point span at the beginning of the file", () {
-    expect(file.location(0).pointSpan().highlight(), equals("""
+    test("at the beginning of the file", () {
+      expect(file.location(0).pointSpan().highlight(), equals("""
   ,
 1 | foo bar baz
   | ^
   '"""));
-  });
+    });
+
+    test("at the beginning of a line", () {
+      expect(file.location(12).pointSpan().highlight(), equals("""
+  ,
+2 | whiz bang boom
+  | ^
+  '"""));
+    });
 
-  test("works for a point span at the end of a line", () {
-    expect(file.location(11).pointSpan().highlight(), equals("""
+    test("at the end of a line", () {
+      expect(file.location(11).pointSpan().highlight(), equals("""
   ,
 1 | foo bar baz
   |            ^
   '"""));
-  });
+    });
 
-  test("works for a point span at the end of the file", () {
-    expect(file.location(38).pointSpan().highlight(), equals("""
+    test("at the end of the file", () {
+      expect(file.location(38).pointSpan().highlight(), equals("""
   ,
 3 | zip zap zop
   |            ^
   '"""));
-  });
+    });
 
-  test("works for a point span after the end of the file", () {
-    expect(file.location(39).pointSpan().highlight(), equals("""
+    test("after the end of the file", () {
+      expect(file.location(39).pointSpan().highlight(), equals("""
   ,
-3 | zip zap zop
-  |            ^
+4 | 
+  | ^
   '"""));
-  });
+    });
 
-  test("works for a point span at the end of the file with no trailing newline",
-      () {
-    file = new SourceFile.fromString("zip zap zop");
-    expect(file.location(10).pointSpan().highlight(), equals("""
+    test("at the end of the file with no trailing newline", () {
+      file = new SourceFile.fromString("zip zap zop");
+      expect(file.location(10).pointSpan().highlight(), equals("""
   ,
 1 | zip zap zop
   |           ^
   '"""));
-  });
+    });
 
-  test(
-      "works for a point span after the end of the file with no trailing newline",
-      () {
-    file = new SourceFile.fromString("zip zap zop");
-    expect(file.location(11).pointSpan().highlight(), equals("""
+    test("after the end of the file with no trailing newline", () {
+      file = new SourceFile.fromString("zip zap zop");
+      expect(file.location(11).pointSpan().highlight(), equals("""
   ,
 1 | zip zap zop
   |            ^
   '"""));
-  });
+    });
 
-  test("works for a point span in an empty file", () {
-    expect(new SourceFile.fromString("").location(0).pointSpan().highlight(),
-        equals("""
+    test("in an empty file", () {
+      expect(new SourceFile.fromString("").location(0).pointSpan().highlight(),
+          equals("""
   ,
 1 | 
   | ^
   '"""));
+    });
   });
 
-  test("works for a single-line file without a newline", () {
+  test("highlights a single-line file without a newline", () {
     expect(
         new SourceFile.fromString("foo bar").span(0, 7).highlight(), equals("""
   ,