Provide source map info for simple async methods.

Provide source map info for simple async methods.

R=sigmund@google.com

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

[Johnni Winther, 2016-11-17 13:45:34]

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

[Siggi Cherem (dart-lang), 2016-11-17 15:20:54]

lgtm

https://codereview.chromium.org/2510073002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/stacktrace_test.dart (right):

https://codereview.chromium.org/2510073002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:133: // This call is no
longer on the stack when the error is thrown.
if you are relying on this, consider adding one extra async step just in case,
maybe a `await new Future(() {}))`.

The reason is that I believe Florian is experimenting with making async methods
return a sync future and not introduce an extra microtask. In that case, the
call to test2() will still be in the stack.

[Johnni Winther, 2016-11-18 08:22:08]

https://codereview.chromium.org/2510073002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/stacktrace_test.dart (right):

https://codereview.chromium.org/2510073002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:133: // This call is no
longer on the stack when the error is thrown.
On 2016/11/17 15:20:54, Siggi Cherem (dart-lang) wrote:
> if you are relying on this, consider adding one extra async step just in case,
> maybe a `await new Future(() {}))`.
> 
> The reason is that I believe Florian is experimenting with making async
methods
> return a sync future and not introduce an extra microtask. In that case, the
> call to test2() will still be in the stack.

I'd rather have the test fail if this changes. Having the call test2 on the
stack would highly improve the quality of the stack trace.

[Johnni Winther, 2016-11-18 08:28:48]

Description was changed from

==========
Provide source map info for simple async methods.
==========

to

==========
Provide source map info for simple async methods.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d0e057903e3784747563f5dd6b8eefb53c25146b
==========

[Johnni Winther, 2016-11-18 08:28:49]

Committed patchset #1 (id:1) manually as
d0e057903e3784747563f5dd6b8eefb53c25146b (presubmit successful).

[Siggi Cherem (dart-lang), 2016-11-18 15:31:39]

https://codereview.chromium.org/2510073002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/stacktrace_test.dart (right):

https://codereview.chromium.org/2510073002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:133: // This call is no
longer on the stack when the error is thrown.
On 2016/11/18 08:22:08, Johnni Winther wrote:
> On 2016/11/17 15:20:54, Siggi Cherem (dart-lang) wrote:
> > if you are relying on this, consider adding one extra async step just in
case,
> > maybe a `await new Future(() {}))`.
> > 
> > The reason is that I believe Florian is experimenting with making async
> methods
> > return a sync future and not introduce an extra microtask. In that case, the
> > call to test2() will still be in the stack.
> 
> I'd rather have the test fail if this changes. Having the call test2 on the
> stack would highly improve the quality of the stack trace.

At that point we would be only testing exceptions that happen before we unwind
the stack, so it would be equivalent to a stack trace where test1/test2 are not
marked async, right?

Do you also want to test that the stack traces are good if the exception is
thrown after the stack has been unwound?

[Johnni Winther, 2016-11-21 08:44:20]

https://codereview.chromium.org/2510073002/diff/1/tests/compiler/dart2js/sour...
File tests/compiler/dart2js/sourcemaps/stacktrace_test.dart (right):

https://codereview.chromium.org/2510073002/diff/1/tests/compiler/dart2js/sour...
tests/compiler/dart2js/sourcemaps/stacktrace_test.dart:133: // This call is no
longer on the stack when the error is thrown.
On 2016/11/18 15:31:38, Siggi Cherem (dart-lang) wrote:
> On 2016/11/18 08:22:08, Johnni Winther wrote:
> > On 2016/11/17 15:20:54, Siggi Cherem (dart-lang) wrote:
> > > if you are relying on this, consider adding one extra async step just in
> case,
> > > maybe a `await new Future(() {}))`.
> > > 
> > > The reason is that I believe Florian is experimenting with making async
> > methods
> > > return a sync future and not introduce an extra microtask. In that case,
the
> > > call to test2() will still be in the stack.
> > 
> > I'd rather have the test fail if this changes. Having the call test2 on the
> > stack would highly improve the quality of the stack trace.
> 
> At that point we would be only testing exceptions that happen before we unwind
> the stack, so it would be equivalent to a stack trace where test1/test2 are
not
> marked async, right?

Yes. I realize that the test wouldn't break from some a change since we only
test existence of the annotated positions.
> 
> Do you also want to test that the stack traces are good if the exception is
> thrown after the stack has been unwound?

Not in itself, only boundary cases. I'll update the test to test for positions
expected not to be present.