StackTrace parser, fix assertion error message #48343

dnfield · 2020-01-07T20:53:07Z

Description

Adds a StackTrace parser (fixes #4021) and fixes up the way we print out "this is likely a bug in the framework" message (fixes #18412).

The main goal here is to change the way we print out requests to file a bug on assertion errors. It currently always prints that message because Dart has changed the way it throws assertions (there's an additional frame before the one we used to look for). However, even fixing that still isn't quite right. The new logic here will only print the message on an AssertionError that has at least two frames of framework code before any other user code - the idea being that we either need to add a new assert to the framework, or stop violating our own asserts in such a case.

Related Issues

#4021
#18412

Tests

I added the following tests:

Tests for stack parsing
Tests for checking our fault/not our fault on stack traces.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

We do not consider this a breaking change, because although it changes tests, it's only changing the shape of an error message, and so shouldn't nbe considered breaking (/cc @Hixie)

packages/flutter/lib/src/foundation/stack_frame.dart

dnfield · 2020-01-07T23:00:00Z

packages/flutter/test/foundation/error_reporting_test.dart

  final StackTrace sampleStack = await getSampleStack();

-  test('Error reporting - pretest', () async {
+  setUp(() async {


Because we need this before any individual test. When we eventually randomize our tests this will be important.

dnfield · 2020-01-07T23:00:19Z

packages/flutter/test/foundation/error_reporting_test.dart

    };
  });

+  tearDown(() async {


Ditto. Without these changes, none of these tests can be run independently.

Hixie · 2020-01-07T23:00:55Z

packages/flutter/lib/src/foundation/stack_frame.dart

+
+  @override
+  bool operator ==(Object other) {
+    return other is StackFrame &&


we should compare the runtimeType here; see style guide for a template

Done. We should update our style guide - @a14n changed the way this works for removing the avoid_as lint and this + the runtimeType check is actually the convention used now.

Please don't hesitate to update the style guide.

Hixie · 2020-01-07T23:01:10Z

packages/flutter/lib/src/foundation/stack_frame.dart

+  }
+
+  @override
+  String toString() => '$runtimeType{#$number, $packageScheme:$package/$packagePath:$line:$column, className: $className, method: $method}';


trivial nit: ( instead of { is our convention

Hixie · 2020-01-07T23:02:00Z

Hixie · 2020-01-07T23:02:53Z

If you want to overachieve even more, there's other code in the framework that does stack parsing, like defaultStackFilter.

Hixie · 2020-01-07T23:04:40Z

packages/flutter/lib/src/foundation/stack_frame.dart

+
+  @override
+  String toString() => '$runtimeType{#$number, $packageScheme:$package/$packagePath:$line:$column, className: $className, method: $method}';
+}


would be nice to have a way to print a frame in a manner that matches what the compiler does, so that you could take a List, filter it, then reprint it and it would come out in the same format as the original, just with some frames omitted.

also, check that this works on web and in AOT mode -- maybe the web stack has differently-shaped stack traces, and I know the AOT compiler has different details (e.g. omits line numbers in some cases?).

I believe the line number thing was fixed (saw issues for that), but I'll check.

We could also just stuff the whole original string into the object instead of reconstructing it.

AOT is missing columns. Will fix.

goderbauer

NICE!

goderbauer · 2020-01-08T00:05:39Z

packages/flutter/lib/src/foundation/stack_frame.dart

+/// final List<StackFrame> currentFrames = StackFrame.fromStackTrace(StackTrace.current);
+/// ```
+///
+/// To create a traversable parsed stack.


I believe all text that's outside the dart code block will render above the code in dart docs. So this may look awkward there. I would rephrase this to have the prose text all above the code.

goderbauer · 2020-01-08T00:07:19Z

packages/flutter/lib/src/foundation/stack_frame.dart

+
+  /// A stack frame representing an asynchronous suspension.
+  static const StackFrame asynchronousSuspension = StackFrame(
+    -1,


Should this also be a named parameter? (currently, I'd have to click through to StackFrame to learn what the -1 means.)

I had it as named but it felt awkward. I could make it named again.

you could also just use nulls, fwiw.

dnfield · 2020-01-08T00:19:25Z

I've updated this for defaultStackFilter.

…her test

fluttergithubbot · 2020-01-08T05:08:01Z

This pull request is not suitable for automatic merging in its current state.

Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

Hixie · 2020-01-08T05:26:51Z

...silly bot doesn't like my stamps. *pout*

dnfield · 2020-01-08T05:35:09Z

I may want to add one more thing to this before landing. I think we should keep the original source line - it would make resolving #4174 easier

Hixie · 2020-01-08T05:42:19Z

You can also just do that in a separate PR (e.g. as part of #4174 if you're interested in that one).

dnfield · 2020-01-08T06:11:02Z

Yeah I'll do that in a separate PR. I'm not sure I'll actually need the source line, but since the parsing is lossy it seems useful to have. Parsing stack traces shouldn't need to worry too much about an extra copy of a string anyway.

a14n · 2020-01-08T06:44:10Z

packages/flutter/lib/src/foundation/stack_frame.dart

@@ -0,0 +1,230 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.


All files in this repo are dated to 2014 to keep things consistent

a14n · 2020-01-08T06:48:26Z

packages/flutter/lib/src/foundation/stack_frame.dart

+        ? RegExp(r'^(package:.+) (\d+):(\d+)\s+(.+)$')
+        : RegExp(r'^(.+) (\d+):(\d+)\s+(.+)$');
+    final Match match = parser.firstMatch(line);
+    assert(match != null, 'Expecgted $line to match $parser.');


typo: Expected

Saw this right after landing :( will open a PR to fix, thanks for catching that

a14n · 2020-01-08T07:13:48Z

packages/flutter/lib/src/foundation/stack_frame.dart

+        column == other.column &&
+        className == other.className &&
+        method == other.method &&
+        source == other.source;


If I promoted a style it would be to prefer something like:

return other is StackFrame && other.number == number && other.package == package && other.line == line ...

(starting line with && other.)

It's the pattern mostly used inside operator== methods.

This is what the style guide actually now says - I just did it wrong here. I'll update it in the other PR.

a14n · 2020-01-08T07:24:00Z

packages/flutter/test/foundation/assertions_test.dart

+
+  test('Identifies user fault', () {
+    // User fault because they called `new Text(null)` from their own code.
+    final StackTrace stack = StackTrace.fromString('''#0      _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:42:39)


The first line could also be on a newline for readability (the new line followin ''' in multiline string is ignored)

a14n · 2020-01-08T07:24:17Z

packages/flutter/test/foundation/assertions_test.dart

+  test('Identifies our fault', () {
+    // Our fault because we should either have an assertion in `text_helper.dart`
+    // or we should make sure not to pass bad values into new Text.
+    final StackTrace stack = StackTrace.fromString('''#0      _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:42:39)


StackTrace parser, fix assertion error message

cf95e84

dnfield requested review from Hixie and goderbauer January 7, 2020 20:53

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 7, 2020

googlebot added the cla: yes label Jan 7, 2020

dnfield commented Jan 7, 2020

View reviewed changes

packages/flutter/lib/src/foundation/stack_frame.dart Show resolved Hide resolved

dnfield requested a review from gspencergoog January 7, 2020 20:54

dnfield added 2 commits January 7, 2020 13:09

Merge remote-tracking branch 'upstream/master' into stack_assert

45bafc0

Fix parsing for more complex cases, more test, fix test

d599599

dnfield commented Jan 7, 2020

View reviewed changes

..

c823db6

Hixie reviewed Jan 7, 2020

View reviewed changes

..

1a628e1

review

3f72c2c

Hixie reviewed Jan 7, 2020

View reviewed changes

web and aot

4f046a5

goderbauer reviewed Jan 8, 2020

View reviewed changes

dnfield added 2 commits January 7, 2020 16:14

..

4af4202

more review

e3ed681

dnfield added 3 commits January 7, 2020 16:48

fix my own tests

7211021

allow missing cols/lines, fix typo in test, fix invalid stack in anot…

51b0729

…her test

another web edge case

c11d7e1

dnfield added the waiting for tree to go green label Jan 8, 2020

fluttergithubbot removed the waiting for tree to go green label Jan 8, 2020

Hixie approved these changes Jan 8, 2020

View reviewed changes

add source

5ca28ef

a14n reviewed Jan 8, 2020

View reviewed changes

dnfield merged commit 505af78 into flutter:master Jan 8, 2020

dnfield deleted the stack_assert branch January 8, 2020 06:50

a14n reviewed Jan 8, 2020

View reviewed changes

bkonyi pushed a commit that referenced this pull request Jan 9, 2020

StackTrace parser, fix assertion error message (#48343)

933ae62

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021

		@@ -0,0 +1,230 @@
		// Copyright 2014 The Flutter Authors. All rights reserved.

StackTrace parser, fix assertion error message #48343

StackTrace parser, fix assertion error message #48343

Uh oh!

Conversation

dnfield commented Jan 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jan 7, 2020

Uh oh!

Hixie commented Jan 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Jan 8, 2020

Uh oh!

fluttergithubbot commented Jan 8, 2020

Uh oh!

Hixie commented Jan 8, 2020

Uh oh!

dnfield commented Jan 8, 2020

Uh oh!

Hixie commented Jan 8, 2020

Uh oh!

dnfield commented Jan 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

dnfield commented Jan 7, 2020 •

edited

Loading