Support external implementations of FileSpan.

lib/src/file.dart

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 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';
 import 'span_with_context.dart';
 import 'utils.dart';
 
 // Constants to determine end-of-lines.
 const int _LF = 10;
 const int _CR = 13;
 
@@ skipping 190 matching lines @@
       throw new ArgumentError('End $_end must come after start $_start.');
     } else if (_end > file.length) {
       throw new RangeError("End $_end must not be greater than the number "
           "of characters in the file, ${file.length}.");
     } else if (_start < 0) {
       throw new RangeError("Start may not be negative, was $_start.");
     }
   }
 
   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);
     return result == 0 ? _end.compareTo(otherFile._end) : result;
   }
 
   SourceSpan union(SourceSpan other) {
     if (other is! FileSpan) return super.union(other);
 
     var span = expand(other);
     var beginSpan = span._start == _start ? this : other;
     var endSpan = span._end == _end ? this : other;
 
     if (beginSpan._end < endSpan._start) {
       throw new ArgumentError("Spans $this and $other are disjoint.");
     }
 
     return span;
   }
 
   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;
+    }
+
+    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 :-/.

   }
 
-  int get hashCode => _start.hashCode + 5 * _end.hashCode +
-      7 * sourceUrl.hashCode;
-
   /// Returns a new span that covers both [this] and [other].
   ///
   /// Unlike [union], [other] may be disjoint from [this]. If it is, the text
   /// between the two will be covered by the returned span.
   FileSpan expand(FileSpan other) {
     if (sourceUrl != other.sourceUrl) {
       throw new ArgumentError("Source URLs \"${sourceUrl}\" and "
           " \"${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);
+    }
   }
 }