Switch transformers over to source_span

Switch various transformers and libraries they use over to source_span.

This would release html5lib 0.12.0 and code_transformers 0.2.0.

BUG=19930
R=blois@google.com, sigmund@google.com

Committed: https://code.google.com/p/dart/source/detail?r=38754

[Jennifer Messerly, 2014-07-29 20:28:40]

Siggi do you mind taking a look?

[Siggi Cherem (dart-lang), 2014-07-29 20:56:32]

https://codereview.chromium.org/421503004/diff/1/pkg/polymer/pubspec.yaml
File pkg/polymer/pubspec.yaml (right):

https://codereview.chromium.org/421503004/diff/1/pkg/polymer/pubspec.yaml#new...
pkg/polymer/pubspec.yaml:15: html5lib: '>=0.11.0 <0.13.0'
There are a couple changes we need in polymer too, otherwise this might break in
checked mode. Here are the cases I've found:

pkg/polymer/lib/src/build/:
 - linter.dart: line 133, 385
 I think removing the types there should fix it here.

 - import_inliner.dart: 425
 Same here, but this one is tricky because this file creates old-style spans and
will pass them here, so `_newUrl` would need to accept both.

 - script_compactor.dart: 505
 We can delete the whole function, it seems like dead code.

I believe running the transformer tests (pkg/polymer/test/build/) will help
catch any other issues.

https://codereview.chromium.org/421503004/diff/1/pkg/third_party/html5lib/lib...
File pkg/third_party/html5lib/lib/src/inputstream.dart (right):

https://codereview.chromium.org/421503004/diff/1/pkg/third_party/html5lib/lib...
pkg/third_party/html5lib/lib/src/inputstream.dart:138: fileInfo = new
SourceFile.decoded(_chars, url: sourceUrl);
could we do this maybe only when generateSpans is true?

  fileInfo = generateSpans ? ... : null;

that would match how we ignored the text entirely before if generateSpans was
false. It seems that all uses of fileInfo are guarded by that check as well.
Tests should just pass, if it's not easy to fix that, then let's add a TODO
about it here.

[Siggi Cherem (dart-lang), 2014-07-29 20:57:33]

On 2014/07/29 20:28:40, John Messerly wrote:
> Siggi do you mind taking a look?

no problem. Just send a couple comments but otherwise looks good.

[nweiz, 2014-07-29 21:12:51]

Code review changes

[nweiz, 2014-07-29 21:13:17]

https://codereview.chromium.org/421503004/diff/1/pkg/polymer/pubspec.yaml
File pkg/polymer/pubspec.yaml (right):

https://codereview.chromium.org/421503004/diff/1/pkg/polymer/pubspec.yaml#new...
pkg/polymer/pubspec.yaml:15: html5lib: '>=0.11.0 <0.13.0'
On 2014/07/29 20:56:32, Siggi Cherem (dart-lang) wrote:
> There are a couple changes we need in polymer too, otherwise this might break
in
> checked mode. Here are the cases I've found:
> 
> pkg/polymer/lib/src/build/:
>  - linter.dart: line 133, 385
>  I think removing the types there should fix it here.
> 
>  - import_inliner.dart: 425
>  Same here, but this one is tricky because this file creates old-style spans
and
> will pass them here, so `_newUrl` would need to accept both.
> 
>  - script_compactor.dart: 505
>  We can delete the whole function, it seems like dead code.
> 
> I believe running the transformer tests (pkg/polymer/test/build/) will help
> catch any other issues.

I hadn't realized that these spans were transitively passed to barback. In that
case, I'll wait on this CL until barback support for source_span has landed.
Once it has, I'll switch the transformers entirely over to source_span as well.

https://codereview.chromium.org/421503004/diff/1/pkg/third_party/html5lib/lib...
File pkg/third_party/html5lib/lib/src/inputstream.dart (right):

https://codereview.chromium.org/421503004/diff/1/pkg/third_party/html5lib/lib...
pkg/third_party/html5lib/lib/src/inputstream.dart:138: fileInfo = new
SourceFile.decoded(_chars, url: sourceUrl);
On 2014/07/29 20:56:32, Siggi Cherem (dart-lang) wrote:
> could we do this maybe only when generateSpans is true?
> 
>   fileInfo = generateSpans ? ... : null;
> 
> that would match how we ignored the text entirely before if generateSpans was
> false. It seems that all uses of fileInfo are guarded by that check as well.
> Tests should just pass, if it's not easy to fix that, then let's add a TODO
> about it here.

Done.

[Siggi Cherem (dart-lang), 2014-07-29 21:21:08]

https://codereview.chromium.org/421503004/diff/1/pkg/polymer/pubspec.yaml
File pkg/polymer/pubspec.yaml (right):

https://codereview.chromium.org/421503004/diff/1/pkg/polymer/pubspec.yaml#new...
pkg/polymer/pubspec.yaml:15: html5lib: '>=0.11.0 <0.13.0'
On 2014/07/29 21:13:17, nweiz wrote:
> On 2014/07/29 20:56:32, Siggi Cherem (dart-lang) wrote:
> > There are a couple changes we need in polymer too, otherwise this might
break
> in
> > checked mode. Here are the cases I've found:
> > 
> > pkg/polymer/lib/src/build/:
> >  - linter.dart: line 133, 385
> >  I think removing the types there should fix it here.
> > 
> >  - import_inliner.dart: 425
> >  Same here, but this one is tricky because this file creates old-style spans
> and
> > will pass them here, so `_newUrl` would need to accept both.
> > 
> >  - script_compactor.dart: 505
> >  We can delete the whole function, it seems like dead code.
> > 
> > I believe running the transformer tests (pkg/polymer/test/build/) will help
> > catch any other issues.
> 
> I hadn't realized that these spans were transitively passed to barback. In
that
> case, I'll wait on this CL until barback support for source_span has landed.
> Once it has, I'll switch the transformers entirely over to source_span as
well.

Ah - ok, I didn't know barback didn't have the support yet. Yes, most of the
spans in html5lib end up being passed to the barback loggers.

[nweiz, 2014-07-29 23:10:01]

Update barback deps.

[nweiz, 2014-07-29 23:12:22]

+blois for code_transformer changes

Barback 0.14.2 supports both span types now, so I've expanded this CL to cover
all the packages in the repo that define transformers that use spans.

[Siggi Cherem (dart-lang), 2014-07-29 23:41:37]

nice - seems that we are missing csslib.

I think your changes may work without it. In particular, line 221 in
pkg/third_party/html5lib/lib/src/query_selector.dart reads it, but it doesn't
pass the span around, so I think it's safe.

But if you want to convert all packages, we might as well fix it here too.

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/common.dart (left):

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/common.dart:19: import 'package:source_maps/span.dart'
show Span;
we probably need the equivalent import, otherwise the analyzer will complain
about the reference in the comment below. Alternatively, change the comment to
say source span in words

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:505: _getSpan(SourceFile file,
AstNode node) => file.span(node.offset, node.end);
let's delete this line

https://codereview.chromium.org/421503004/diff/60001/pkg/third_party/html5lib...
File pkg/third_party/html5lib/lib/parser.dart (right):

https://codereview.chromium.org/421503004/diff/60001/pkg/third_party/html5lib...
pkg/third_party/html5lib/lib/parser.dart:2181: span =
characterTokens[0].span.union(characterTokens.last.span);
just realized, this is where the spans might be disjoint. Use 'expand' ?

[nweiz, 2014-07-29 23:48:07]

Code review changes

[nweiz, 2014-07-29 23:48:39]

I have a separate CL out for csslib
(https://codereview.chromium.org/426053003/).

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/common.dart (left):

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/common.dart:19: import 'package:source_maps/span.dart'
show Span;
On 2014/07/29 23:41:37, Siggi Cherem (dart-lang) wrote:
> we probably need the equivalent import, otherwise the analyzer will complain
> about the reference in the comment below. Alternatively, change the comment to
> say source span in words

I don't think the analyzer looks at references in doc comments.

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:505: _getSpan(SourceFile file,
AstNode node) => file.span(node.offset, node.end);
On 2014/07/29 23:41:37, Siggi Cherem (dart-lang) wrote:
> let's delete this line

Done.

https://codereview.chromium.org/421503004/diff/60001/pkg/third_party/html5lib...
File pkg/third_party/html5lib/lib/parser.dart (right):

https://codereview.chromium.org/421503004/diff/60001/pkg/third_party/html5lib...
pkg/third_party/html5lib/lib/parser.dart:2181: span =
characterTokens[0].span.union(characterTokens.last.span);
On 2014/07/29 23:41:37, Siggi Cherem (dart-lang) wrote:
> just realized, this is where the spans might be disjoint. Use 'expand' ?

Done.

[Siggi Cherem (dart-lang), 2014-07-30 00:00:06]

lgtm

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/common.dart (left):

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/common.dart:19: import 'package:source_maps/span.dart'
show Span;
On 2014/07/29 23:48:39, nweiz wrote:
> On 2014/07/29 23:41:37, Siggi Cherem (dart-lang) wrote:
> > we probably need the equivalent import, otherwise the analyzer will complain
> > about the reference in the comment below. Alternatively, change the comment
to
> > say source span in words
> 
> I don't think the analyzer looks at references in doc comments.

mmm - I think I got confused because it will not complain about an unused
warning when you use the symbol in a comment. 

Regardless, I guess it still matters for dart docs. I'm ok fixing it either way
(add import, or rewrite the actual comment)

[nweiz, 2014-07-30 00:17:56]

Code review changes

[nweiz, 2014-07-30 00:24:29]

Code review changes

[nweiz, 2014-07-30 00:25:53]

https://codereview.chromium.org/421503004/diff/1/pkg/third_party/html5lib/lib...
File pkg/third_party/html5lib/lib/src/inputstream.dart (right):

https://codereview.chromium.org/421503004/diff/1/pkg/third_party/html5lib/lib...
pkg/third_party/html5lib/lib/src/inputstream.dart:138: fileInfo = new
SourceFile.decoded(_chars, url: sourceUrl);
On 2014/07/29 21:13:17, nweiz wrote:
> On 2014/07/29 20:56:32, Siggi Cherem (dart-lang) wrote:
> > could we do this maybe only when generateSpans is true?
> > 
> >   fileInfo = generateSpans ? ... : null;
> > 
> > that would match how we ignored the text entirely before if generateSpans
was
> > false. It seems that all uses of fileInfo are guarded by that check as well.
> > Tests should just pass, if it's not easy to fix that, then let's add a TODO
> > about it here.
> 
> Done.

It turns out this made a test fail, so I changed it back and added a TODO.

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/common.dart (left):

https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/common.dart:19: import 'package:source_maps/span.dart'
show Span;
On 2014/07/30 00:00:06, Siggi Cherem (dart-lang) wrote:
> On 2014/07/29 23:48:39, nweiz wrote:
> > On 2014/07/29 23:41:37, Siggi Cherem (dart-lang) wrote:
> > > we probably need the equivalent import, otherwise the analyzer will
complain
> > > about the reference in the comment below. Alternatively, change the
comment
> to
> > > say source span in words
> > 
> > I don't think the analyzer looks at references in doc comments.
> 
> mmm - I think I got confused because it will not complain about an unused
> warning when you use the symbol in a comment. 
> 
> Regardless, I guess it still matters for dart docs. I'm ok fixing it either
way
> (add import, or rewrite the actual comment)

Done.

[blois, 2014-07-30 21:15:53]

On 2014/07/30 00:25:53, nweiz wrote:
>
https://codereview.chromium.org/421503004/diff/1/pkg/third_party/html5lib/lib...
> File pkg/third_party/html5lib/lib/src/inputstream.dart (right):
> 
>
https://codereview.chromium.org/421503004/diff/1/pkg/third_party/html5lib/lib...
> pkg/third_party/html5lib/lib/src/inputstream.dart:138: fileInfo = new
> SourceFile.decoded(_chars, url: sourceUrl);
> On 2014/07/29 21:13:17, nweiz wrote:
> > On 2014/07/29 20:56:32, Siggi Cherem (dart-lang) wrote:
> > > could we do this maybe only when generateSpans is true?
> > > 
> > >   fileInfo = generateSpans ? ... : null;
> > > 
> > > that would match how we ignored the text entirely before if generateSpans
> was
> > > false. It seems that all uses of fileInfo are guarded by that check as
well.
> > > Tests should just pass, if it's not easy to fix that, then let's add a
TODO
> > > about it here.
> > 
> > Done.
> 
> It turns out this made a test fail, so I changed it back and added a TODO.
> 
>
https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
> File pkg/polymer/lib/src/build/common.dart (left):
> 
>
https://codereview.chromium.org/421503004/diff/60001/pkg/polymer/lib/src/buil...
> pkg/polymer/lib/src/build/common.dart:19: import
'package:source_maps/span.dart'
> show Span;
> On 2014/07/30 00:00:06, Siggi Cherem (dart-lang) wrote:
> > On 2014/07/29 23:48:39, nweiz wrote:
> > > On 2014/07/29 23:41:37, Siggi Cherem (dart-lang) wrote:
> > > > we probably need the equivalent import, otherwise the analyzer will
> complain
> > > > about the reference in the comment below. Alternatively, change the
> comment
> > to
> > > > say source span in words
> > > 
> > > I don't think the analyzer looks at references in doc comments.
> > 
> > mmm - I think I got confused because it will not complain about an unused
> > warning when you use the symbol in a comment. 
> > 
> > Regardless, I guess it still matters for dart docs. I'm ok fixing it either
> way
> > (add import, or rewrite the actual comment)
> 
> Done.

lgtm

[nweiz, 2014-07-30 21:36:54]

Committed patchset #7 manually as r38754 (presubmit successful).