First shot at source maps generation in dart2js.

First shot at source maps generation in dart2js.

This allows to generate a source map with begin and end positions of all functions.

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

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

[podivilov, 2012-06-19 16:37:54]

The way source map generation is plugged into compiler seems wrong, but I'm not
sure how it should be done. What do you think?

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/compiler....
File lib/compiler/compiler.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/compiler....
lib/compiler/compiler.dart:36: class CompiledScript {
This is ugly, not sure how to return sourceMap from compile. class Compilation
suggested by Bob may help here.

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/compiler....
lib/compiler/compiler.dart:57: SourceFile compiledFile = new SourceFile(null,
code);
We could pass SourceMapListener interface/handler to compile and remove source
map building details from this path. WDYT?

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
lib/compiler/implementation/emitter.dart:1016: void
addSourceMapping(FunctionElement element, int emittedBeginPosition, int
emittedEndPosition) {
This should probably be a method of SourceMapListener interface.

[ahe, 2012-06-19 17:00:41]

Neat! I would like to tweak to how the source map is communicated to
dart2js.dart.

Also, could you please ensure that you have no lines longer than 80 characters?

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/compiler....
File lib/compiler/compiler.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/compiler....
lib/compiler/compiler.dart:36: class CompiledScript {
On 2012/06/19 16:37:54, podivilov wrote:
> This is ugly, not sure how to return sourceMap from compile. class Compilation
> suggested by Bob may help here.

Yes. Johnni will handle that.

In the meantime, how about using a custom Diagnostic (new API I just added)?

I would really prefer if we avoid modifying this file.

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/compiler....
lib/compiler/compiler.dart:58: String sourceMap = new SourceMapBuilder().build(
This code does not belong here. I think the source map should be created by
emitter.assembleProgram(). I would communicate the result back to dart2js.dart
by adding something like this to the end of assembleProgram:

compiler.reportDiagnostic(null, new SourceMapBuilder().build(sourceMappings),
new api.Diagnostic(-1, 'source map'));

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart2js.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart2js.dart:131: new OptionHandler('--no-colors',
(_) => colors.enabled = false),
I think you accidentally copied a line here.

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart2js.dart:183: void handler(Uri uri, int begin,
int end, String message, bool fatal) {
Earlier today, I created changed "fatal" to kind.

How about we hack in sources maps using this approach:

if (kind.name == 'source map') {
  // store source map.
  return;
}

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
lib/compiler/implementation/emitter.dart:1016: void
addSourceMapping(FunctionElement element, int emittedBeginPosition, int
emittedEndPosition) {
Long lines.

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
File lib/compiler/implementation/util/source_map_builder.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
lib/compiler/implementation/util/source_map_builder.dart:17:
SourceMappingEntry(this.sourceFile, this.sourceOffset, this.targetOffset,
[this.sourceName]);
Lines should be limited to 80 columns.

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
lib/compiler/implementation/util/source_map_builder.dart:66: return
JSON.stringify(sourceMap);
Seriously?

[podivilov, 2012-06-20 09:37:14]

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/compiler....
File lib/compiler/compiler.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/compiler....
lib/compiler/compiler.dart:36: class CompiledScript {
On 2012/06/19 17:00:41, ahe wrote:
> On 2012/06/19 16:37:54, podivilov wrote:
> > This is ugly, not sure how to return sourceMap from compile. class
Compilation
> > suggested by Bob may help here.
> 
> Yes. Johnni will handle that.
> 
> In the meantime, how about using a custom Diagnostic (new API I just added)?
> 
> I would really prefer if we avoid modifying this file.

Done.

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/compiler....
lib/compiler/compiler.dart:58: String sourceMap = new SourceMapBuilder().build(
On 2012/06/19 17:00:41, ahe wrote:
> This code does not belong here. I think the source map should be created by
> emitter.assembleProgram(). I would communicate the result back to dart2js.dart
> by adding something like this to the end of assembleProgram:
> 
> compiler.reportDiagnostic(null, new SourceMapBuilder().build(sourceMappings),
> new api.Diagnostic(-1, 'source map'));

Done.

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
File lib/compiler/implementation/dart2js.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart2js.dart:131: new OptionHandler('--no-colors',
(_) => colors.enabled = false),
On 2012/06/19 17:00:41, ahe wrote:
> I think you accidentally copied a line here.

Done.

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
lib/compiler/implementation/dart2js.dart:183: void handler(Uri uri, int begin,
int end, String message, bool fatal) {
On 2012/06/19 17:00:41, ahe wrote:
> Earlier today, I created changed "fatal" to kind.
> 
> How about we hack in sources maps using this approach:
> 
> if (kind.name == 'source map') {
>   // store source map.
>   return;
> }

Done.

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
lib/compiler/implementation/emitter.dart:1016: void
addSourceMapping(FunctionElement element, int emittedBeginPosition, int
emittedEndPosition) {
On 2012/06/19 17:00:41, ahe wrote:
> Long lines.

Done.

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
File lib/compiler/implementation/util/source_map_builder.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/1/lib/compiler/implement...
lib/compiler/implementation/util/source_map_builder.dart:17:
SourceMappingEntry(this.sourceFile, this.sourceOffset, this.targetOffset,
[this.sourceName]);
On 2012/06/19 17:00:41, ahe wrote:
> Lines should be limited to 80 columns.

Done.

[kasperl, 2012-06-20 11:35:35]

DBC: It is very exciting to see this take form. Source maps FTW!

[ahe, 2012-06-20 11:56:59]

LGTM provided you remove the option.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/apiimpl.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/apiimpl.dart:34: generateSourceMap: options.some((e)
=> e.startsWith(
As discussed, we don't want to make it a command line option. So this should
just be true.

I think it is nice to keep the bool in the implementation, so it can be turned
off for debugging.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:64: JavaScriptBackend(Compiler
compiler, [bool generateSourceMap = false])
Prefer no optional arguments to a method (or constructor in this case) that is
basically used once.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:196: generateSourceMap = false])
I think the default should be true.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/dart2js.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/dart2js.dart:130: new
OptionHandler('--source-map-out=.+', setSourceMapOutput),
I just talked to the bigwigs. We want to turn on source maps always. So we don't
need an option.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/dart2js.dart:246: uri, libraryRoot, packageRoot,
provider, handler, options).value;
This change seem unnecessary.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:1052: compiler.reportDiagnostic(
Add TODO to explain this.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/leg.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/leg.dart:19:
#import('util/source_map_builder.dart');
This doesn't feel like something that belongs in util. I think this is highly JS
specific.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/util/source_map_builder.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/util/source_map_builder.dart:70: return
JSON.stringify(sourceMap);
I'm really not convinced that JSON.stringify is the best possible option here.
Could you please explain why you cannot stream this?

[podivilov, 2012-06-20 14:15:15]

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/apiimpl.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/apiimpl.dart:34: generateSourceMap: options.some((e)
=> e.startsWith(
On 2012/06/20 11:56:59, ahe wrote:
> As discussed, we don't want to make it a command line option. So this should
> just be true.
> 
> I think it is nice to keep the bool in the implementation, so it can be turned
> off for debugging.

Done.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:64: JavaScriptBackend(Compiler
compiler, [bool generateSourceMap = false])
On 2012/06/20 11:56:59, ahe wrote:
> Prefer no optional arguments to a method (or constructor in this case) that is
> basically used once.

Done.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:196: generateSourceMap = false])
On 2012/06/20 11:56:59, ahe wrote:
> I think the default should be true.

Done.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/dart2js.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/dart2js.dart:130: new
OptionHandler('--source-map-out=.+', setSourceMapOutput),
On 2012/06/20 11:56:59, ahe wrote:
> I just talked to the bigwigs. We want to turn on source maps always. So we
don't
> need an option.

Done.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/dart2js.dart:246: uri, libraryRoot, packageRoot,
provider, handler, options).value;
On 2012/06/20 11:56:59, ahe wrote:
> This change seem unnecessary.

Done.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:1052: compiler.reportDiagnostic(
On 2012/06/20 11:56:59, ahe wrote:
> Add TODO to explain this.

Done.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/leg.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/leg.dart:19:
#import('util/source_map_builder.dart');
On 2012/06/20 11:56:59, ahe wrote:
> This doesn't feel like something that belongs in util. I think this is highly
JS
> specific.

Moved to implementation/. Please let me know if there's a better place. I think
we could use it for dart2dart as well, at least theoretically.

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/util/source_map_builder.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/util/source_map_builder.dart:70: return
JSON.stringify(sourceMap);
On 2012/06/20 11:56:59, ahe wrote:
> I'm really not convinced that JSON.stringify is the best possible option here.
> Could you please explain why you cannot stream this?

OK, but I think it makes sense to use JSON.stringify for lists (they are not
large) to make code simpler.

[ahe, 2012-06-20 17:08:16]

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/util/source_map_builder.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/util/source_map_builder.dart:70: return
JSON.stringify(sourceMap);
On 2012/06/20 14:15:15, podivilov wrote:
> On 2012/06/20 11:56:59, ahe wrote:
> > I'm really not convinced that JSON.stringify is the best possible option
here.
> > Could you please explain why you cannot stream this?
> 
> OK, but I think it makes sense to use JSON.stringify for lists (they are not
> large) to make code simpler.

How many copies are you going to make of the output?

[ahe, 2012-06-20 17:08:47]

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/leg.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/leg.dart:19:
#import('util/source_map_builder.dart');
On 2012/06/20 14:15:15, podivilov wrote:
> On 2012/06/20 11:56:59, ahe wrote:
> > This doesn't feel like something that belongs in util. I think this is
highly
> JS
> > specific.
> 
> Moved to implementation/. Please let me know if there's a better place. I
think
> we could use it for dart2dart as well, at least theoretically.

SGTM.

[podivilov, 2012-06-21 07:58:49]

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
File lib/compiler/implementation/util/source_map_builder.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/6002/lib/compiler/implem...
lib/compiler/implementation/util/source_map_builder.dart:70: return
JSON.stringify(sourceMap);
On 2012/06/20 17:08:16, ahe wrote:
> On 2012/06/20 14:15:15, podivilov wrote:
> > On 2012/06/20 11:56:59, ahe wrote:
> > > I'm really not convinced that JSON.stringify is the best possible option
> here.
> > > Could you please explain why you cannot stream this?
> > 
> > OK, but I think it makes sense to use JSON.stringify for lists (they are not
> > large) to make code simpler.
> 
> How many copies are you going to make of the output?

Sorry, not sure what do you mean. I believe there's no unneeded copying in the
latest version
https://chromiumcodereview.appspot.com/10579019/diff/2002/lib/compiler/implem....

[ahe, 2012-06-21 08:12:13]

https://chromiumcodereview.appspot.com/10579019/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/source_map_builder.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/2002/lib/compiler/implem...
lib/compiler/implementation/source_map_builder.dart:31: Map<String, int>
sourceURLMap;
Forgot to mention this earlier: we try to use camelCase even for acronyms. So it
should be "sourceUrlMap".

https://chromiumcodereview.appspot.com/10579019/diff/2002/lib/compiler/implem...
lib/compiler/implementation/source_map_builder.dart:71: return
buffer.toString();
Let's say sourceURLList contains the string "http://example.com/". How many
times is that string copied because you use JSON.stringify and String
interpolation above?

[podivilov, 2012-06-21 08:56:25]

On 2012/06/21 08:12:13, ahe wrote:
>
https://chromiumcodereview.appspot.com/10579019/diff/2002/lib/compiler/implem...
> File lib/compiler/implementation/source_map_builder.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10579019/diff/2002/lib/compiler/implem...
> lib/compiler/implementation/source_map_builder.dart:31: Map<String, int>
> sourceURLMap;
> Forgot to mention this earlier: we try to use camelCase even for acronyms. So
it
> should be "sourceUrlMap".
> 
>
https://chromiumcodereview.appspot.com/10579019/diff/2002/lib/compiler/implem...
> lib/compiler/implementation/source_map_builder.dart:71: return
> buffer.toString();
> Let's say sourceURLList contains the string "http://example.com/". How many
> times is that string copied because you use JSON.stringify and String
> interpolation above?

Yes, there will be two extra copies for list's data, but 99% of the map is the
"mappings" field which is not copied anymore. 

I think there's a problem with current dart:json API that was designed a while
ago when string+ destiny wasn't yet determined. Let me update JSON library and
fix this code afterwards.

[podivilov, 2012-06-21 09:50:42]

Thanks for review! Landing...

https://chromiumcodereview.appspot.com/10579019/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/source_map_builder.dart (right):

https://chromiumcodereview.appspot.com/10579019/diff/2002/lib/compiler/implem...
lib/compiler/implementation/source_map_builder.dart:31: Map<String, int>
sourceURLMap;
On 2012/06/21 08:12:13, ahe wrote:
> Forgot to mention this earlier: we try to use camelCase even for acronyms. So
it
> should be "sourceUrlMap".

Done.

https://chromiumcodereview.appspot.com/10579019/diff/2002/lib/compiler/implem...
lib/compiler/implementation/source_map_builder.dart:71: return
buffer.toString();
On 2012/06/21 08:12:13, ahe wrote:
> Let's say sourceURLList contains the string "http://example.com/". How many
> times is that string copied because you use JSON.stringify and String
> interpolation above?

Added TODO to migrate to fast version when corresponding API is available.