Add StackTrace.current getter.

Add StackTrace.current getter.

R=floitsch@google.com, iposva@google.com, sra@google.com

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

[Lasse Reichstein Nielsen, 2015-11-16 12:28:49]

lrn@google.com changed reviewers:
+ floitsch@google.com, iposva@google.com, sra@google.com

[Lasse Reichstein Nielsen, 2015-11-16 12:28:49]

WDYT?

[floitsch, 2015-11-16 19:30:21]

LGTM, but wait for the others.

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:687: throw 0;
Which browsers still need this?

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dart
File sdk/lib/core/stacktrace.dart (right):

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dar...
sdk/lib/core/stacktrace.dart:40: * but the getter tries to get the stack without
throwing any object
This won't read nice in a formatted output. Just make it a separate sentence.

"The getter ..."

I would be more assertive: "doesn't" (and treat it as a bug for those that do".

[Ivan Posva, 2015-11-16 20:14:12]

iposva@google.com changed reviewers:
+ asiva@google.com

[Ivan Posva, 2015-11-16 20:14:13]

More in-depth review later...

-Ivan

https://codereview.chromium.org/1448003002/diff/1/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1448003002/diff/1/runtime/lib/core_patch.dart...
runtime/lib/core_patch.dart:78: /* patch */ static StackTrace get current native
"StackTrace_current";
This is likely not the right file for this functionality.

[Lasse Reichstein Nielsen, 2015-11-17 13:18:53]

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:687: throw 0;
IE11 as far as I can see. Don't know about Safari or MS Edge (can't check).

[kevmoo, 2015-11-17 18:50:46]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2015-11-17 18:50:47]

Please include an update to the README for this change

+Nweiz: currently the stack_trace package does a lot of throw/catch – this would
make debugging MUCH easier

[kevmoo, 2015-11-17 18:51:44]

DBC

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dart
File sdk/lib/core/stacktrace.dart (right):

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dar...
sdk/lib/core/stacktrace.dart:38: *     try { throw 0; } catch (_, stack) {
return stack; }
Please use
```dart
try { throw 0; } ...
```

syntax for code blocks

[sra1, 2015-11-17 19:11:38]

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:678: var error = JS("Object",
"new Error()");
Don't use "Object".
That means any subclass of Object.
"=Object" means a plain JavaScript object, which is better but not really true.

It is best to use an empty spec, which means dynamic, but not a type new to the
program (so not pulling in all of IndexDB etc). This is OK since all we do is
compare to null, other JS code and call wrapping functions.

Use single quotes throughout.

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:679: var stack = JS("Object",
"#.stack", error);
The type here should be 'String|Null'.
That will give better type inference for StackTrace.fromString(stack)

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:680: if (stack != null) return
new StackTrace.fromString(stack);
It would be a little safer for this test to be 'is String'

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:683: var stack = JS("Object",
"#.stack", error);
'String|Null'

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:689: return stackTrace;
This there a measurable performance benefit over simply having this code?
The above code is a but fussy, so I'd rather have the fussiness in one place
unless the benefit is noticeable.

[sra1, 2015-11-17 19:13:44]

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dart
File sdk/lib/core/stacktrace.dart (right):

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dar...
sdk/lib/core/stacktrace.dart:38: *     try { throw 0; } catch (_, stack) {
return stack; }
On 2015/11/17 18:51:44, kevmoo wrote:
> Please use
> ```dart
> try { throw 0; } ...
> ```
> 
> syntax for code blocks

Shouldn't that be the default for Dart documentation?

[kevmoo, 2015-11-17 23:02:04]

On 2015/11/17 19:13:44, sra1 wrote:
> https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dart
> File sdk/lib/core/stacktrace.dart (right):
> 
>
https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dar...
> sdk/lib/core/stacktrace.dart:38: *     try { throw 0; } catch (_, stack) {
> return stack; }
> On 2015/11/17 18:51:44, kevmoo wrote:
> > Please use
> > ```dart
> > try { throw 0; } ...
> > ```
> > 
> > syntax for code blocks
> 
> Shouldn't that be the default for Dart documentation?

It is – but it's really nice to encourage the convention

[floitsch, 2015-11-17 23:22:32]

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dart
File sdk/lib/core/stacktrace.dart (right):

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dar...
sdk/lib/core/stacktrace.dart:38: *     try { throw 0; } catch (_, stack) {
return stack; }
On 2015/11/17 at 19:13:44, sra1 wrote:
> On 2015/11/17 18:51:44, kevmoo wrote:
> > Please use
> > ```dart
> > try { throw 0; } ...
> > ```
> > 
> > syntax for code blocks
> 
> Shouldn't that be the default for Dart documentation?

There should be no need to specify "dart" as information string.
I also don't see the ``` as a big advantage over few lines of indented code.
Until we allow non-leading "*" it isn't much easier to write either.

In the following example "```" would be easier to edit.

/**
No leading "*"
```
non prefixed code
```
*/

[Lasse Reichstein Nielsen, 2015-11-18 10:11:39]

https://codereview.chromium.org/1448003002/diff/1/runtime/lib/stacktrace.cc
File runtime/lib/stacktrace.cc (right):

https://codereview.chromium.org/1448003002/diff/1/runtime/lib/stacktrace.cc#n...
runtime/lib/stacktrace.cc:64: }
The VM even has the option of dropping the first stack frame, so
`StackTrace.current` itself won't show up in the captured trace. I think we
should do that. It's not as easy in JS, but that shouldn't prevent us from doing
the right thing here.

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:678: var error = JS("Object",
"new Error()");
Done

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:679: var stack = JS("Object",
"#.stack", error);
Done.

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:680: if (stack != null) return
new StackTrace.fromString(stack);
Done

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:683: var stack = JS("Object",
"#.stack", error);
Done.

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:683: var stack = JS("Object",
"#.stack", error);
Done.

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:689: return stackTrace;
It's not about performance, but about not interfering with "break on throw"
debugging if it can be avoided.

It's basically a response to https://github.com/dart-lang/sdk/issues/24922

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dart
File sdk/lib/core/stacktrace.dart (right):

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dar...
sdk/lib/core/stacktrace.dart:38: *     try { throw 0; } catch (_, stack) {
return stack; }
I fail to see the improvement of

* ```dart
* try ...
* ```

over

*
*     try ...;
*

The latter is *easier* to read as source, and it should generate the same
result.
For larger examples, you may want to avoid the four characters of indentation,
but for a small example like this, it's not really worth it.

https://codereview.chromium.org/1448003002/diff/1/sdk/lib/core/stacktrace.dar...
sdk/lib/core/stacktrace.dart:40: * but the getter tries to get the stack without
throwing any object
Done.

[sra1, 2015-11-18 18:54:51]

lgtm

https://chromiumcodereview.appspot.com/1448003002/diff/20001/sdk/lib/_interna...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://chromiumcodereview.appspot.com/1448003002/diff/20001/sdk/lib/_interna...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:677: @patch static StackTrace
get current {
It is really unlikely that this would be inlined, but add @NoInline() anyway,
since inlining will change the deepest frame.

You might have to import NoInline()

https://chromiumcodereview.appspot.com/1448003002/diff/20001/sdk/lib/_interna...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:684: if (stack != null) return
new StackTrace.fromString(stack);
is String

[Lasse Reichstein Nielsen, 2015-11-20 08:41:35]

https://codereview.chromium.org/1448003002/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1448003002/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:677: @patch static StackTrace
get current {
Actually, always inlining would be *awesome*. I think the returned stack trace
would be better if it doesn't contain the `StackTrace.current` line. We can do
that on the VM, and I think I will.

Adding NoInline for now, but do consider whether it would be better with the
opposite.

https://codereview.chromium.org/1448003002/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/core_patch.dart:684: if (stack != null) return
new StackTrace.fromString(stack);
Done.

[Lasse Reichstein Nielsen, 2015-11-20 09:36:54]

@asiva PTAL

[Ivan Posva, 2015-11-24 00:53:58]

VM code LGTM

-Ivan

https://codereview.chromium.org/1448003002/diff/40001/runtime/lib/stacktrace.cc
File runtime/lib/stacktrace.cc (right):

https://codereview.chromium.org/1448003002/diff/40001/runtime/lib/stacktrace....
runtime/lib/stacktrace.cc:57: const GrowableObjectArray& code_list =
This whole code needs to be de-deduplicated:
https://github.com/dart-lang/sdk/issues/25029

https://codereview.chromium.org/1448003002/diff/40001/tests/corelib/stacktrac...
File tests/corelib/stacktrace_current_test.dart (right):

https://codereview.chromium.org/1448003002/diff/40001/tests/corelib/stacktrac...
tests/corelib/stacktrace_current_test.dart:12: // Primitiive way to get stack
trace,.
-i

[Lasse Reichstein Nielsen, 2015-11-24 06:52:56]

https://codereview.chromium.org/1448003002/diff/40001/runtime/lib/stacktrace.cc
File runtime/lib/stacktrace.cc (right):

https://codereview.chromium.org/1448003002/diff/40001/runtime/lib/stacktrace....
runtime/lib/stacktrace.cc:57: const GrowableObjectArray& code_list =
I'll start by moving it to a static function in this file, so at least the code
isn't duplicated here. If there are other occurrences, you can unify it with
those.

https://codereview.chromium.org/1448003002/diff/40001/tests/corelib/stacktrac...
File tests/corelib/stacktrace_current_test.dart (right):

https://codereview.chromium.org/1448003002/diff/40001/tests/corelib/stacktrac...
tests/corelib/stacktrace_current_test.dart:12: // Primitiive way to get stack
trace,.
Good catch. Fixed.

[Lasse Reichstein Nielsen, 2015-11-24 07:26:30]

Committed patchset #4 (id:60001) manually as
027b8dca39a753a95b01a9845d4fc5f7cd08be1a (presubmit successful).