Support external implementations of FileSpan.

lib/src/file.dart

Index: lib/src/file.dart
diff --git a/lib/src/file.dart b/lib/src/file.dart
index 4b4e026cbd4013ea33da63dc93446f07499cf963..2c7a92b2a6199915c4bfc0af8d5b00eb30c664e4 100644
--- a/lib/src/file.dart
+++ b/lib/src/file.dart
@@ -7,9 +7,6 @@ library source_span.file;
 import 'dart:math' as math;
 import 'dart:typed_data';
 
-import 'package:path/path.dart' as p;
-
-import 'colors.dart' as colors;
 import 'location.dart';
 import 'span.dart';
 import 'span_mixin.dart';
@@ -220,7 +217,8 @@ class FileSpan extends SourceSpanMixin implements SourceSpanWithContext {
   }
 
   int compareTo(SourceSpan other) {
-    if (other is! FileSpan) return super.compareTo(other);
+    // Check runtimeType to be resilient to external FileSpan implementations.
+    if (other.runtimeType != FileSpan) return super.compareTo(other);

[Siggi Cherem (dart-lang), 2015/08/18 18:01:59]

Could we instead move some of the shared code into a mixin where it is safe to
do an `is` check?

We rarely use this code in client apps, but `.runtimeType` makes code more
expensive for dart2js.

[nweiz, 2015/08/18 20:28:51]

I don't think a mixin would work, but I've split out an implementation class.

 
     FileSpan otherFile = other;
     var result = _start.compareTo(otherFile._start);
@@ -242,13 +240,14 @@ class FileSpan extends SourceSpanMixin implements SourceSpanWithContext {
   }
 
   bool operator ==(other) {
+    // Check runtimeType to be resilient to external FileSpan implementations.
     if (other is! FileSpan) return super == other;
-    return _start == other._start && _end == other._end &&
-        sourceUrl == other.sourceUrl;
-  }
+    if (other.runtimeType != FileSpan) {
+      return super == other && sourceUrl == other.sourceUrl;
+    }
 
-  int get hashCode => _start.hashCode + 5 * _end.hashCode +
-      7 * sourceUrl.hashCode;
+    return _start == other._start && _end == other._end;

[Siggi Cherem (dart-lang), 2015/08/18 18:01:59]

mmm - I thought we would keep the check on sourceUrl here. Should we add it
back? Should we also add sourceUrl to SourceSpanMixin.== and
SourceSpanMixin.hashCode (fixing the inconsistency by adding that there
instead)?

[nweiz, 2015/08/18 20:28:51]

Yeah, good catch. Just a cut/paste error.
I'm not sure why I didn't have sourceUrl always factor into equality, but
changing it now would break backwards compatibility :-/.

+  }
 
   /// Returns a new span that covers both [this] and [other].
   ///
@@ -260,8 +259,15 @@ class FileSpan extends SourceSpanMixin implements SourceSpanWithContext {
           " \"${other.sourceUrl}\" don't match.");
     }
 
-    var start = math.min(this._start, other._start);
-    var end = math.max(this._end, other._end);
-    return new FileSpan._(file, start, end);
+    // Check runtimeType to be resilient to external FileSpan implementations.
+    if (other.runtimeType == FileSpan) {
+      var start = math.min(this._start, other._start);

[Siggi Cherem (dart-lang), 2015/08/18 18:01:59]

I wonder if we should have these optimizations or switch to just use
other.start.offset here directly (assuming we change the start & end getters to
memoize the FileLocations results, so we allocate them just once).

[nweiz, 2015/08/18 20:28:51]

In my experiments, there's a lot of benefit that comes from avoiding the
creation of FileLocations until the last minute. It's pretty common to create
and even expand spans during the course of parsing, and adding two extra
allocations for each expand is a real cost. I want to keep the non-error-case
cost of using this library as low as possible to encourage parsers to expose the
information.

+      var end = math.max(this._end, other._end);
+      return new FileSpan._(file, start, end);
+    } else {
+      var start = math.min(this._start, other.start.offset);
+      var end = math.max(this._end, other.end.offset);
+      return new FileSpan._(file, start, end);
+    }
   }
 }