Create and test source mapping for invocations.

Create and test source mapping for invocations.

BUG=
R=floitsch@google.com

Committed: https://github.com/dart-lang/sdk/commit/048a4651fe9d6d6ef6a80562b8ec55b2072d9aa3

[Johnni Winther, 2015-06-24 14:17:54]

johnniwinther@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2015-06-29 08:55:52]

LGTM.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
File pkg/compiler/lib/src/io/position_information.dart (right):

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:141: null,
why does this not have a start-position?

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:147: SourceInformation
buildBegin(Node node) {
add documentation.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:279: if (codePosition == null)
return;
that sounds wrong: first an assert, but then a test with a simple 'return'?
shouldn't you at least throw ?

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:315: getCodePosition(node),
CodePositionKind.START,
Add comments when "start" and when "close" is used.
for example, why is the fun using closing and not start?

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:376: if (LOG) {
I would remove this and the const bool.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:412: // Don't know....
Given that there is an assert, we should throw an internal error.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:430: node,  startPosition,
endPosition, closingPosition);
extra space.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
File pkg/compiler/lib/src/io/source_information.dart (right):

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/source_information.dart:67: /// Generate
[SourceInformation] for a implicit return in [element].
an implicit

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
File pkg/compiler/lib/src/io/start_end_information.dart (right):

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/start_end_information.dart:65: // computed based on the
element by provided from statements and expressions.
-by-

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/start_end_information.dart:82: // TODO(podivilov): find
the right sourceFile here and remove offset
I think we can safely reassign this TODO.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/start_end_information.dart:108: StringBuffer sb = new
StringBuffer();
create a helper function?
Only seems to differ in the length of the uri.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/start_end_information.dart:136: js.Node root;
comments what "root" is.
unless there are subclasses or accesses from the outside, it is only used as
boolean (in which case a boolean would be better).

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js/js_debug.dart (right):

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/js_debug.dart:28: visitNodeWithChildren(Node node,
String type) {
add return type

https://codereview.chromium.org/1196433002/diff/40001/tests/compiler/dart2js/...
File tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart (right):

https://codereview.chromium.org/1196433002/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart:115: USE_NEW_SOURCE_INFO
= true;
If we can assign to it, it's not a constant and shouldn't be in all-capitals.

[Johnni Winther, 2015-06-29 12:36:30]

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
File pkg/compiler/lib/src/io/position_information.dart (right):

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:141: null,
On 2015/06/29 08:55:51, floitsch wrote:
> why does this not have a start-position?

The start of a function (the signature) is not a steppable point.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:147: SourceInformation
buildBegin(Node node) {
On 2015/06/29 08:55:51, floitsch wrote:
> add documentation.

Done.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:279: if (codePosition == null)
return;
On 2015/06/29 08:55:51, floitsch wrote:
> that sounds wrong: first an assert, but then a test with a simple 'return'?
> shouldn't you at least throw ?

It _should_ be the case that we have points for the information but I don't want
compilation to fail because of it. In time, when the system is more mature, we
should be able to use the strong invariant.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:376: if (LOG) {
On 2015/06/29 08:55:52, floitsch wrote:
> I would remove this and the const bool.

Done.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:412: // Don't know....
On 2015/06/29 08:55:51, floitsch wrote:
> Given that there is an assert, we should throw an internal error.

I don't want compilation to fail because of this (yet). When the system is more
mature it should.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/position_information.dart:430: node,  startPosition,
endPosition, closingPosition);
On 2015/06/29 08:55:52, floitsch wrote:
> extra space.

Done.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
File pkg/compiler/lib/src/io/source_information.dart (right):

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/source_information.dart:67: /// Generate
[SourceInformation] for a implicit return in [element].
On 2015/06/29 08:55:52, floitsch wrote:
> an implicit

Done.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
File pkg/compiler/lib/src/io/start_end_information.dart (right):

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/start_end_information.dart:65: // computed based on the
element by provided from statements and expressions.
On 2015/06/29 08:55:52, floitsch wrote:
> -by-

TODO was stale. Replaced with the appropriate.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/start_end_information.dart:82: // TODO(podivilov): find
the right sourceFile here and remove offset
On 2015/06/29 08:55:52, floitsch wrote:
> I think we can safely reassign this TODO.

Done.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/start_end_information.dart:108: StringBuffer sb = new
StringBuffer();
On 2015/06/29 08:55:52, floitsch wrote:
> create a helper function?
> Only seems to differ in the length of the uri.

Done.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/io...
pkg/compiler/lib/src/io/start_end_information.dart:136: js.Node root;
On 2015/06/29 08:55:52, floitsch wrote:
> comments what "root" is.
> unless there are subclasses or accesses from the outside, it is only used as
> boolean (in which case a boolean would be better).

Done.

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js/js_debug.dart (right):

https://codereview.chromium.org/1196433002/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/js_debug.dart:28: visitNodeWithChildren(Node node,
String type) {
On 2015/06/29 08:55:52, floitsch wrote:
> add return type

Done.

https://codereview.chromium.org/1196433002/diff/40001/tests/compiler/dart2js/...
File tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart (right):

https://codereview.chromium.org/1196433002/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/sourcemaps/sourcemap_helper.dart:115: USE_NEW_SOURCE_INFO
= true;
On 2015/06/29 08:55:52, floitsch wrote:
> If we can assign to it, it's not a constant and shouldn't be in all-capitals.

Done.

[Johnni Winther, 2015-06-29 15:09:48]

PTAL @ the last patch

[floitsch, 2015-06-29 16:30:44]

LGTM.

https://codereview.chromium.org/1196433002/diff/100001/pkg/compiler/lib/src/i...
File pkg/compiler/lib/src/io/position_information.dart (right):

https://codereview.chromium.org/1196433002/diff/100001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/io/position_information.dart:264: /// Associates
[sourceInformation] with [node] the code positions of
Sentence doesn't make sense to me.

Maybe make it several sentences?

https://codereview.chromium.org/1196433002/diff/100001/pkg/js_ast/test/printe...
File pkg/js_ast/test/printer_callback_test.dart (right):

https://codereview.chromium.org/1196433002/diff/100001/pkg/js_ast/test/printe...
pkg/js_ast/test/printer_callback_test.dart:169: }, const ['foo']),
maybe give it different names, to make it clearer, that there is a substitution?

https://codereview.chromium.org/1196433002/diff/100001/pkg/js_ast/test/printe...
pkg/js_ast/test/printer_callback_test.dart:194:
print('$code:${node.runtimeType}');
debug print.

[Johnni Winther, 2015-07-02 08:38:09]

https://codereview.chromium.org/1196433002/diff/100001/pkg/compiler/lib/src/i...
File pkg/compiler/lib/src/io/position_information.dart (right):

https://codereview.chromium.org/1196433002/diff/100001/pkg/compiler/lib/src/i...
pkg/compiler/lib/src/io/position_information.dart:264: /// Associates
[sourceInformation] with [node] the code positions of
On 2015/06/29 16:30:44, floitsch wrote:
> Sentence doesn't make sense to me.
> 
> Maybe make it several sentences?

Done.

https://codereview.chromium.org/1196433002/diff/100001/pkg/js_ast/test/printe...
File pkg/js_ast/test/printer_callback_test.dart (right):

https://codereview.chromium.org/1196433002/diff/100001/pkg/js_ast/test/printe...
pkg/js_ast/test/printer_callback_test.dart:194:
print('$code:${node.runtimeType}');
On 2015/06/29 16:30:44, floitsch wrote:
> debug print.

Removed.

[Johnni Winther, 2015-07-02 08:38:58]

Committed patchset #8 (id:140001) manually as
048a4651fe9d6d6ef6a80562b8ec55b2072d9aa3 (presubmit successful).

[Johnni Winther, 2015-07-02 12:43:05]

https://codereview.chromium.org/1196433002/diff/100001/pkg/js_ast/test/printe...
File pkg/js_ast/test/printer_callback_test.dart (right):

https://codereview.chromium.org/1196433002/diff/100001/pkg/js_ast/test/printe...
pkg/js_ast/test/printer_callback_test.dart:169: }, const ['foo']),
On 2015/06/29 16:30:44, floitsch wrote:
> maybe give it different names, to make it clearer, that there is a
substitution?

Done.