Fixes #254 - Better stacktrace support

Fixes #254 - Better stacktrace support

R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/59a85c5495c2c8fffe913d604d64582a7202ead7

[vsm, 2015-07-13 13:50:44]

vsm@google.com changed reviewers:
+ jmesserly@google.com

[Jennifer Messerly, 2015-07-13 16:39:17]

https://codereview.chromium.org/1233553005/diff/1/lib/runtime/_operations.js
File lib/runtime/_operations.js (right):

https://codereview.chromium.org/1233553005/diff/1/lib/runtime/_operations.js#...
lib/runtime/_operations.js:278: exports.throw_ = throw_;
btw, this should be `throw` ... all names work for properties

https://codereview.chromium.org/1233553005/diff/1/lib/runtime/_operations.js#...
lib/runtime/_operations.js:280: function rethrow(obj) {
if this doesn't do anything, why have the helper?

https://codereview.chromium.org/1233553005/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1233553005/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2325: return js.statement('dart.throw_(#);',
expr);
commented on the bug, but I have some concerns over never generating `throw`.

Brainstorming how we can get back to it:

* by default, capture stack in expression/error ctor.
* if Error/Exception is immediately thrown, use `throw` statement (or dart.throw
helper for expressions--the old version that just throws)
* otherwise, if we don't know where `e` comes from, fall back to
`dart.captureStackTrace(e)` followed by throw.

And we should probably split stack trace helper APIs in to their own module at
some point.

I think this would fix most of the changed lines in the SDK code, since `throw
new ...` is the common pattern.

[Jennifer Messerly, 2015-07-13 16:39:49]

link to bug discussion: https://github.com/dart-lang/dev_compiler/issues/254

[vsm, 2015-07-14 13:54:48]

https://codereview.chromium.org/1233553005/diff/1/lib/runtime/_operations.js
File lib/runtime/_operations.js (right):

https://codereview.chromium.org/1233553005/diff/1/lib/runtime/_operations.js#...
lib/runtime/_operations.js:278: exports.throw_ = throw_;
On 2015/07/13 16:39:17, John Messerly wrote:
> btw, this should be `throw` ... all names work for properties

Done.

https://codereview.chromium.org/1233553005/diff/1/lib/runtime/_operations.js#...
lib/runtime/_operations.js:280: function rethrow(obj) {
On 2015/07/13 16:39:17, John Messerly wrote:
> if this doesn't do anything, why have the helper?

Removed

https://codereview.chromium.org/1233553005/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1233553005/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2325: return js.statement('dart.throw_(#);',
expr);
On 2015/07/13 16:39:17, John Messerly wrote:
> commented on the bug, but I have some concerns over never generating `throw`.
> 
> Brainstorming how we can get back to it:
> 
> * by default, capture stack in expression/error ctor.
> * if Error/Exception is immediately thrown, use `throw` statement (or
dart.throw
> helper for expressions--the old version that just throws)
> * otherwise, if we don't know where `e` comes from, fall back to
> `dart.captureStackTrace(e)` followed by throw.
> 
> And we should probably split stack trace helper APIs in to their own module at
> some point.
> 
> I think this would fix most of the changed lines in the SDK code, since `throw
> new ...` is the common pattern.
> 

Done in the case of Error - Exception is only an interface.

[vsm, 2015-07-14 17:04:26]

Actually, I'd like to undo most of this patch and file as an enhancement to do
this properly.  For now, the patch is a step backward at runtime as it pollutes
the stack trace we end up with.  We'll need a way to prune / clean up the stack
traces we generate.  

I'll let you know when this is ready.

[vsm, 2015-07-14 17:20:17]

On 2015/07/14 17:04:26, vsm wrote:
> Actually, I'd like to undo most of this patch and file as an enhancement to do
> this properly.  For now, the patch is a step backward at runtime as it
pollutes
> the stack trace we end up with.  We'll need a way to prune / clean up the
stack
> traces we generate.  
> 
> I'll let you know when this is ready.

PTAL

I'll update the bug for future enhancements.  Main issues with using plain
throw:
- Limited to cases where we're explicitly extending (transitively) Error
- Doesn't work for Exception (an interface) without changing and whitelisting a
bunch of classes
- We need to prune the constructor frames (and there are often many) from the
trace

[vsm, 2015-07-14 17:50:12]

On 2015/07/14 17:20:17, vsm wrote:
> On 2015/07/14 17:04:26, vsm wrote:
> > Actually, I'd like to undo most of this patch and file as an enhancement to
do
> > this properly.  For now, the patch is a step backward at runtime as it
> pollutes
> > the stack trace we end up with.  We'll need a way to prune / clean up the
> stack
> > traces we generate.  
> > 
> > I'll let you know when this is ready.
> 
> PTAL
> 
> I'll update the bug for future enhancements.  Main issues with using plain
> throw:
> - Limited to cases where we're explicitly extending (transitively) Error
> - Doesn't work for Exception (an interface) without changing and whitelisting
a
> bunch of classes
> - We need to prune the constructor frames (and there are often many) from the
> trace

Also added a couple related changes to improve what we print out in dev tools.

[Jennifer Messerly, 2015-07-14 18:14:20]

On 2015/07/14 17:20:17, vsm wrote:
> On 2015/07/14 17:04:26, vsm wrote:
> > Actually, I'd like to undo most of this patch and file as an enhancement to
do
> > this properly.  For now, the patch is a step backward at runtime as it
> pollutes
> > the stack trace we end up with.  We'll need a way to prune / clean up the
> stack
> > traces we generate.  
> > 
> > I'll let you know when this is ready.
> 
> PTAL
> 
> I'll update the bug for future enhancements.  Main issues with using plain
> throw:
> - Limited to cases where we're explicitly extending (transitively) Error
> - Doesn't work for Exception (an interface) without changing and whitelisting
a
> bunch of classes
> - We need to prune the constructor frames (and there are often many) from the
> trace

we need to prune the helper call too, right?

[vsm, 2015-07-14 18:16:22]

On 2015/07/14 18:14:20, John Messerly wrote:
> On 2015/07/14 17:20:17, vsm wrote:
> > On 2015/07/14 17:04:26, vsm wrote:
> > > Actually, I'd like to undo most of this patch and file as an enhancement
to
> do
> > > this properly.  For now, the patch is a step backward at runtime as it
> > pollutes
> > > the stack trace we end up with.  We'll need a way to prune / clean up the
> > stack
> > > traces we generate.  
> > > 
> > > I'll let you know when this is ready.
> > 
> > PTAL
> > 
> > I'll update the bug for future enhancements.  Main issues with using plain
> > throw:
> > - Limited to cases where we're explicitly extending (transitively) Error
> > - Doesn't work for Exception (an interface) without changing and
whitelisting
> a
> > bunch of classes
> > - We need to prune the constructor frames (and there are often many) from
the
> > trace
> 
> we need to prune the helper call too, right?

Yes, that too.  For now, that's just one as opposed to several though.

[Jennifer Messerly, 2015-07-14 18:30:33]

okay. I guess this "lgtm" for check in purposes and to unblock you, though I
think we still need to do follow ups to get back in a good state here. We're
diverging a bit from our goals of reusing/interoperating with the JS ecosystem
(including tools). The fact that we have a special throw breaks assumptions,
which I think "stackPrint" captures well.

Normally we'd address this through type system restrictions, which seem like
they might work here. If we're throwing normal JS errors they'll have a stack
trace.

I wonder if some of this is just improvements in Chrome debug tools, which would
benefit everyone. Also, I guess you couldn't turn on "break when exception is
thrown"? Is that because Angular uses exceptions for their normal operation? If
so, they're just making it harder on their uses, that might suggest a fix to
Angular's framework code. As a rule, we certainly tried to avoid exceptions in
.NET (which also had filtering on exception type, and filtering of exceptions
thrown/caught inside the framework code...)

But anyway, I know none of this is consolation when you're feeling real
debugging pain right now with angular. So feel free to land.

https://codereview.chromium.org/1233553005/diff/80001/lib/runtime/_operations.js
File lib/runtime/_operations.js (right):

https://codereview.chromium.org/1233553005/diff/80001/lib/runtime/_operations...
lib/runtime/_operations.js:284: // This is a utility function: it is only
intended to be called from dev tools.
long line.

Maybe we need a module of debug utils? It doesn't seem like we want extra code
in our runtime.
Doesn't console.log(error) work pretty well for JS errors?

Side note, this really shows why doing Dart specific stuff is not good for
interop =\. It means the JS tools don't work out of the box for us anymore. I'd
love to get back in the business of having "throw" be "throw"

https://codereview.chromium.org/1233553005/diff/80001/lib/runtime/_operations...
lib/runtime/_operations.js:287: console.log(error.stack ? error.stack : 'No
stack trace for: ' + error);
is this different from _js_helper.getTraceFromException ?

I'm a bit confused by stackPrint being different from stackTrace. IMO,
console.log(stackTrace(error)); seems reasonable to ask folks to write.

https://codereview.chromium.org/1233553005/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1233553005/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2602: return js.call('dart.throw(#)',
[js.escapedString(comment)]);
not related to your CL but: maybe we should throw an actual error here?

[vsm, 2015-07-14 20:15:39]

On 2015/07/14 18:30:33, John Messerly wrote:
> okay. I guess this "lgtm" for check in purposes and to unblock you, though I
> think we still need to do follow ups to get back in a good state here. We're
> diverging a bit from our goals of reusing/interoperating with the JS ecosystem
> (including tools). The fact that we have a special throw breaks assumptions,
> which I think "stackPrint" captures well.
> 
> Normally we'd address this through type system restrictions, which seem like
> they might work here. If we're throwing normal JS errors they'll have a stack
> trace.
> 
> I wonder if some of this is just improvements in Chrome debug tools, which
would
> benefit everyone. Also, I guess you couldn't turn on "break when exception is
> thrown"? Is that because Angular uses exceptions for their normal operation?
If
> so, they're just making it harder on their uses, that might suggest a fix to
> Angular's framework code. As a rule, we certainly tried to avoid exceptions in
> .NET (which also had filtering on exception type, and filtering of exceptions
> thrown/caught inside the framework code...)
> 
> But anyway, I know none of this is consolation when you're feeling real
> debugging pain right now with angular. So feel free to land.
> 
>
https://codereview.chromium.org/1233553005/diff/80001/lib/runtime/_operations.js
> File lib/runtime/_operations.js (right):
> 
>
https://codereview.chromium.org/1233553005/diff/80001/lib/runtime/_operations...
> lib/runtime/_operations.js:284: // This is a utility function: it is only
> intended to be called from dev tools.
> long line.
> 
> Maybe we need a module of debug utils? It doesn't seem like we want extra code
> in our runtime.
> Doesn't console.log(error) work pretty well for JS errors?
> 
> Side note, this really shows why doing Dart specific stuff is not good for
> interop =\. It means the JS tools don't work out of the box for us anymore.
I'd
> love to get back in the business of having "throw" be "throw"
> 
>
https://codereview.chromium.org/1233553005/diff/80001/lib/runtime/_operations...
> lib/runtime/_operations.js:287: console.log(error.stack ? error.stack : 'No
> stack trace for: ' + error);
> is this different from _js_helper.getTraceFromException ?
> 
> I'm a bit confused by stackPrint being different from stackTrace. IMO,
> console.log(stackTrace(error)); seems reasonable to ask folks to write.
> 
>
https://codereview.chromium.org/1233553005/diff/80001/lib/src/codegen/js_code...
> File lib/src/codegen/js_codegen.dart (right):
> 
>
https://codereview.chromium.org/1233553005/diff/80001/lib/src/codegen/js_code...
> lib/src/codegen/js_codegen.dart:2602: return js.call('dart.throw(#)',
> [js.escapedString(comment)]);
> not related to your CL but: maybe we should throw an actual error here?

Thanks, landing.  Filed #255 and #256 to capture open threads.  But this really
helps me get going.

[vsm, 2015-07-14 20:33:09]

Committed patchset #6 (id:100001) manually as
59a85c5495c2c8fffe913d604d64582a7202ead7 (presubmit successful).