Preserve dart stack traces

[alexeagle]

For angular/angular#1407

[mhevery]

ideally we should only get the stack if we need it, as it is expensive.

[yjbanov]

Dart, like JS, has Error, which has a getter stackTrace. We could, instead of catch(e, s), do this:

} catch(Error e) {
  print(e.stackTrace);
}

All Dart's own standard errors are subclasses of Error already, and so is our BaseException. So it should work just fine. This would extract the stacktrace on-demand and therefore not slow down the code.

[alexeagle]

How 'ideally' is that? It's challenging to look ahead in that way.

On Fri, May 8, 2015 at 2:55 PM Miško Hevery notifications@github.com
wrote:

In test/statement_test.ts
#153 (comment):

@@ -30,9 +30,9 @@ describe('statements', () => {
});
it('translates try/catch', () => {
expectTranslate('try {} catch(e) {} finally {}')

•    .to.equal(' try { } catch ( e ) { } finally { }');
  

•    .to.equal(' try { } catch ( e , e_stack ) { } finally { }');
  

ideally we should only get the stack if we need it, as it is expensive.

—
Reply to this email directly or view it on GitHub
https://github.com/angular/ts2dart/pull/153/files#r29980110.

[alexeagle]

Yegor, if I understand correctly you're saying no change is needed in
ts2dart because we should already be catching Error in the original angular
code? What change needs to be made in that case?

On Fri, May 8, 2015 at 3:03 PM Yegor notifications@github.com wrote:

In test/statement_test.ts
#153 (comment):

@@ -30,9 +30,9 @@ describe('statements', () => {
});
it('translates try/catch', () => {
expectTranslate('try {} catch(e) {} finally {}')

•    .to.equal(' try { } catch ( e ) { } finally { }');
  

•    .to.equal(' try { } catch ( e , e_stack ) { } finally { }');
  

Dart, like JS, has Error, which has a getter stackTrace. We could,
instead of catch(e, s), do this:

} catch(Error e) {
print(e.stackTrace);
}

All Dart's own standard errors are subclasses of Error already, and so is
our BaseException. So it should work just fine.

—
Reply to this email directly or view it on GitHub
https://github.com/angular/ts2dart/pull/153/files#r29980592.

[mprobst]

@mhevery if catching the stack is that expensive, surely the VM optimizes for the case where the stack isn't actually used subsequently, and we can just generate stupid code that always captures the stack?

[alexeagle]

Or dart2js could detect the unused variable.

On Sun, May 10, 2015, 8:43 AM Martin Probst notifications@github.com
wrote:

In test/statement_test.ts
#153 (comment):

@@ -30,9 +30,9 @@ describe('statements', () => {
});
it('translates try/catch', () => {
expectTranslate('try {} catch(e) {} finally {}')

•    .to.equal(' try { } catch ( e ) { } finally { }');
  

•    .to.equal(' try { } catch ( e , e_stack ) { } finally { }');
  

@mhevery https://github.com/mhevery if catching the stack is that
expensive, surely the VM optimizes for the case where the stack isn't
actually used subsequently, and we can just generate stupid code that
always captures the stack?

—
Reply to this email directly or view it on GitHub
https://github.com/angular/ts2dart/pull/153/files#r30003226.

[yjbanov]

To be precise, what I'm proposing long-term is that we use the common syntax in both Dart and JS and access stack via a facade (because the field names are different; it's err.stack vs err.stackTrace). Example:

// facade/error.dart
class ErrorWrapper {
  static stackTrace(Error err) => err.stackTrace;
}

// facade/error.ts
export class ErrorWrapper {
  static stackTrace(err:Error) {
    return err.stack;
  }
}

Then in all places in Angular 2 code we would:

try {
  ...
} catch(err) {
  var st = ErrorWrapper.stackTrace(err);
}

Works in both languages.

[alexeagle]

I would be concerned that it's hard to enforce using the facade since
coding the TS idiom would only break error reporting.
Why not code in idiomatic TypeScript and transpile from your second example
to the first? We just need to understand the types (will have soon)

On Sun, May 10, 2015, 12:44 PM Yegor notifications@github.com wrote:

In test/statement_test.ts
#153 (comment):

@@ -30,9 +30,9 @@ describe('statements', () => {
});
it('translates try/catch', () => {
expectTranslate('try {} catch(e) {} finally {}')

•    .to.equal(' try { } catch ( e ) { } finally { }');
  

•    .to.equal(' try { } catch ( e , e_stack ) { } finally { }');
  

To be precise, what I'm proposing long-term is that we use the common
syntax in both Dart and JS and access stack via a facade (because the field
names are different; it's err.stack vs err.stackTrace). Example:

// facade/error.dartclass ErrorWrapper {
static stackTrace(Error err) => err.stackTrace;
}

// facade/error.tsexport class ErrorWrapper {
static stackTrace(err:Error) {
return err.stack;
}
}

Then in all places in Angular 2 code we would:

try {
...
} catch(err) {
var st = ErrorWrapper.stackTrace(err);
}

Works in both languages.

—
Reply to this email directly or view it on GitHub
https://github.com/angular/ts2dart/pull/153/files#r30005268.

[yjbanov]

@mhevery, I believe it is only expensive when an error actually happens. During normal operation, errors should be very rare, so it should be ok to ask for stack trace for every error. @floitschG, could you please confirm this assumption?

[yjbanov]

/cc #152

[floitschG]

Dart2js works with JS Errors. That is, if you throw an error from within Dart, the error is wrapped into a JS error. For one this preserves the stacktrace, and it also makes it uniform with implicit errors.
When catching an error, dart2js builds the stacktrace local by instantiating the StackTrace class with the error as argument. During a toString() of this instance the .stack property of the JS error is extracted and returned.
This means that adding the stacktrace variable to the catch handler is cheap (an allocation of an instance with 2 fields).

Furthermore dart2js currently always extracts the stacktrace anyway. It should be easy to fix that in dart2js, but in the end it is not that expensive...
(The reason it always happens is that the extraction of the stacktrace is seen as having a side-effect, and is therefore not eliminated even though the stacktrace local is never used).

[yjbanov]

@floitschG, so it sounds to me that there is no difference at all if the code is not actually throwing anything. During normal operation, the application is not throwing many errors. We only catch errors for debugging.

[floitschG]

If the code is not throwing then it won't matter if you catch the stacktrace or not.
Even if you throw, dart2js has to include the stacktrace because it can't know if some error-handler will need it. For that it would need to dynamically look for the next error-handler (which the Dart VM can do).
So in short: adding the stacktrace parameter won't cost you.

[yjbanov]

Excellent! Thanks, Florian. Then this PR is good to go.

[alexeagle]

merged.