Extract out a source_span package from source_maps.

pkg/source_span/lib/src/source_span.dart

Index: pkg/source_span/lib/src/source_span.dart
diff --git a/pkg/source_span/lib/src/source_span.dart b/pkg/source_span/lib/src/source_span.dart
new file mode 100644
index 0000000000000000000000000000000000000000..ba7f7aba469d7b9d3665d461bc6f0c6717d3f1eb
--- /dev/null
+++ b/pkg/source_span/lib/src/source_span.dart
@@ -0,0 +1,76 @@
+// 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.source_span;
+
+import 'source_location.dart';
+import 'source_span_mixin.dart';
+
+/// A class that describes a segment of source text.
+abstract class SourceSpan implements Comparable<SourceSpan> {
+  /// The start location of this span.
+  final SourceLocation start;
+
+  /// The end location of this span, exclusive.
+  final SourceLocation end;
+
+  /// The source text for this span.
+  final String text;
+
+  /// The URL of the source (typically a file) of this span.
+  ///
+  /// This may be null, indicating that the source URL is unknown or
+  /// unavailable.
+  final Uri sourceUrl;
+
+  /// The length of this span, in characters.
+  final int length;
+
+  /// Creates a new span from [start] to [end] (exclusive) containing [text].
+  ///
+  /// [start] and [end] must have the same source URL and [start] must come
+  /// before [end]. [text] must have a number of characters equal to the
+  /// distance between [start] and [end].
+  factory SourceSpan(SourceLocation start, SourceLocation end, String text) =>
+      new _SourceSpan(start, end, text);
+
+  /// Creates a new span that's the union of [this] and [other].
+  ///
+  /// The two spans must have the same source URL and may not be disjoint.

[Siggi Cherem (dart-lang), 2014/07/16 21:26:10]

I don't think we should make disjointness an error.

This is not used much, but were we use it so far we assume it is ok to give
disjoint spans. There is an example usage in html5lib, where we give the first
and last of a list of spans to compute the range where all of them occur.

We could also add a separate method that checks for this if you like, for
example 'append' or 'expand' might be a reasonable name for that? 

[nweiz, 2014/07/17 20:22:08]

I made it forbid disjoint spans because I really wanted it to work with any sort
of span, and in order to compute the text of the union of a non-FileSpan, the
full text being spanned has to be available.

This restriction doesn't necessarily apply to FileSpans, though, so I'll add an
expand method for those in particular.

[Siggi Cherem (dart-lang), 2014/07/17 22:24:39]

Ah, I had not realized until now that you made `text` mandatory. It used to be
optional. I actually think we still support making it optional.

If text is optional - then we can make it `null` for disjoint unions. At that
point instead of `expand`, we could add an optional argument here:

  SourceSpan union(SourceSpan other, {allowDisjoint: false})

So:
- if they are disjoint & allowDisjoint == false, we get the current behavior
- if they are disjoint and `allowDisjoint: true`, then `text` may be null if it
can't be computed (e.g. it wont be null for FileSpan)

[nweiz, 2014/07/17 23:18:15]

Do we have a use case for spans without text available? My intuition says that
if a span is being created, the creator knows the text it's associated with.

The reason I'm pushing back on this so hard is that I'm very sympathetic to
people writing code to process arbitrary spans. Every additional variation on
valid formats for the fields a span exposes makes their lives harder, since they
have to gracefully handle all those variations. So if we can restrict the set of
valid spans without hurting span generators—which I think we can here—I think
it's worth doing so.

[Siggi Cherem (dart-lang), 2014/07/18 00:11:54]

I understand.

The only use-case I've seen is in the source_maps package itself. The parser
produces spans when reading data out of the source map file. When a token is an
identifier, it returns a span with text, when it is not, it returns a span with
no text. In the latter case, however, it seems that we are returning a
single-point span, so we can actually legitimately set the text to be empty.

We can always add it later I guess - and I think if we were to add it, we could
do it as a non-breaking change.

For now - I'd like to either include allowDisjoint for FileSpan or expose
'expand' so we can easily migrate html5lib to use the new SourceSpans in the
near future.

As for names for that method, what are your thoughts on which should be which?
- `expand` and `append` sound stricter to me, as if they are a better fit for
the non-disjoint model.
- `union` could work for both, but when I think of Span as ranges, I think of
`union` as allowing disjointness.
- I'm trying to think of other names that sound more permissive, but I haven't
come up with something that convinces me (`enclose`, `cover`)?

[nweiz, 2014/07/18 00:30:24]

I like "expand"—I've added it to FileSpan, which I think is sufficient to cover
html5lib's use case.

[Siggi Cherem (dart-lang), 2014/07/18 00:45:24]

Ah - thanks, I somehow missed it. After the file rename there were no diffs to
guide me :(

+  /// [text] is computed by combining [this.text] and [other.text].
+  SourceSpan union(SourceSpan other);
+
+  /// Compares two spans.
+  ///
+  /// [other] must have the same source URL as [this]. This orders spans by
+  /// [start] then [length].
+  int compareTo(SourceSpan other);
+
+  /// Formats [message] in a human-friendly way associated with this span.
+  ///
+  /// [color] may either be a [String], a [bool], or `null`. If it's a string,
+  /// it indicates an ANSII terminal color escape that should be used to
+  /// highlight the span's text. If it's `true`, it indicates that the text
+  /// should be highlighted using the default color. If it's `false` or `null`,
+  /// it indicates that the text shouldn't be highlighted.

[Siggi Cherem (dart-lang), 2014/07/16 21:26:09]

that's a lot of semantics in one argument. I don't know if that is a bit
confusing for people. If not, let's keep it. Otherwise, one option is to bring
back 2 variables (useColors, color), but make it implicit that if color is set,
useColors is implied to be true.

So you mostly would end up using color or useColors but not both. Thoughts?

[nweiz, 2014/07/17 20:22:08]

I agree that this is packing "color" a little full, but I think it reads really
well in either use:

    print(span.message("oh no", color: true));
    print(span.message("oh no", color: colors.RED));

I also like that it makes it easier to implement methods that forward arguments
to this (e.g. SpanException.toString) and that it uses the short name "color"
for the most common use-case of enabling colorization without choosing a color.
I think users will understand pretty well what this parameter does once they use
it once or twice.

[Siggi Cherem (dart-lang), 2014/07/17 22:24:39]

ok. sounds good

+  String message(String message, {color});
+}
+
+class _SourceSpan extends SourceSpanMixin {
+  final SourceLocation start;
+  final SourceLocation end;
+  final String text;
+
+  _SourceSpan(this.start, this.end, this.text) {

[Siggi Cherem (dart-lang), 2014/07/16 21:26:10]

why not make this class public? alternatively, make the checks in the
constructor an extra method in the mixin, so others can use that too. For
instance,

_SourceSpan(this.start, this.end, this.text) {
  checkValidInput();
}

[nweiz, 2014/07/17 20:22:08]

I tried to put this stuff in SourceSpan, but it can't include SourceSpanMixin
and also be implemented by SourceSpanMixin. I can make this public as
SourceSpanBase, though.

[Siggi Cherem (dart-lang), 2014/07/17 22:24:39]

Your idea sounds good, it seems to match what we do for List/ListBase/ListMixin.
The alternative would be to declare all the abstract members in the mixin
instead (remove the implements from there). Them people can directly extend from
SourceSpan.

I'm ok with either approach. Up to you.

[nweiz, 2014/07/17 23:18:15]

I like this idea a lot, but it means that someone writing just "extends
SourceSpanMixin" or "with SourceSpanMixin" won't get a class that inherits from
SourceSpan, which I think is too confusing.

+    if (end.sourceUrl != start.sourceUrl) {
+      throw new ArgumentError("Source URLs \"${start.sourceUrl}\" and "
+          " \"${end.sourceUrl}\" don't match.");
+    } else if (end.offset < start.offset) {
+      throw new ArgumentError('End $end must come after start $start.');
+    } else if (text.length != start.distance(end)) {
+      throw new ArgumentError('Text "$text" must be ${start.distance(end)} '
+          'characters long.');
+    }
+  }
+}