Add more structure to errors. #34684

jacob314 · 2019-06-18T23:36:44Z

Description

This CL refactors more FlutterError messages to take advantage of structured error functionality.
This is a follow up to #30983

Related Issues

Replace this paragraph with a list of issues related to this PR from our [issue database]. Indicate, which of these issues are resolved or fixed by this PR.

Tests

I added tests for the error messages in multiple packages as existing test coverage was quite spotty.

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 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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read [Handling breaking changes]). https://groups.google.com/forum/#!topic/flutter-announce/SMLYDtl_3nk
No, this is not a breaking change.

pq

Exciting to see this all come together! 🔥

pq · 2019-06-19T15:01:44Z

packages/flutter/lib/src/rendering/object.dart

-          'Tried to paint a RenderObject reentrantly.\n'
-          'The following RenderObject was already being painted when it was '
-          'painted again:\n'
-          '  ${toStringShallow(joiner: "\n    ")}\n'


Nice to see these going away! 👍

pq · 2019-06-19T15:05:36Z

packages/flutter/lib/src/widgets/framework.dart

+          throw FlutterError.fromParts(<DiagnosticsNode>[
+            ErrorSummary('A GlobalKey was used multiple times inside one widget\'s child list.'),
+            DiagnosticsProperty<GlobalKey>('The offending GlobalKey was', key),
+            parent.describeElement('The parent of the widgets with that key was'),


I guess we don't need to qualify describeElement with parent here?

This code looks consistent with the previous error message which was describing the parent. If the parent was removed the error would be describing a describing the widget with the key not its parent.

'The parent of the widgets with that key was:\n $parent\n'

became

parent.describeElement('The parent of the widgets with that key was'),

pq · 2019-06-19T15:08:01Z

packages/flutter/test/widgets/framework_test.dart

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+import 'package:flutter/material.dart';


Consider sorting these imports?

Removing that import. Auto-import of unimported libraries gets a bit confused in the flutter sdk.

Checks for unused imports also appear to be confused likely by re-exported methods that are used. For example, both material and widget re-export some methods from foundation that are indeed used. Fixing that issue would be a bit subtle.

goderbauer · 2019-06-19T16:06:43Z

packages/flutter/lib/src/rendering/object.dart

+            'example, a RenderSliver cannot be the child of a RenderBox because '
+            'a RenderSliver does not understand the RenderBox layout protocol.',
+          ),
+          ErrorDescription(''),


These ErrorDescription('') look kinda odd...

Should there maybe be a configuration option somewhere to mark paragraphs that should be surrounded by blank lines?

Or a specific Error DiagnosticsNode class that just represents a blank line?

I agree those look bad.
I'm happy to add an DiagnosticSpacer or ErrorSpacer class that is sugar for this.
Let me know if that sounds reasonable.

One other option would be to add a trailing '\n' to the previous ErrorDescription but that makes the intent less obvious to someone reading the message at it would be reasonable to assume that all ErrorDescription objects should just have a trailing \n.

I like the syntactic sugar idea.

Done. Added ErrorSpacer, used it everywhere we were creating empty DiagnosticsNode objects as spacer or adhoc adding multiple line breaks at the end of sections in error messages.
Fixed a couple cases where we had accidentally extra unintended whitespace in the process and added tests.

jacob314 · 2019-06-19T23:03:35Z

Ready for another look. I'm not sure if the existing spacing between blocks in the Semantics error message case was intentional or accidental. Left in line breaks between entries in the list in case they were considered useful. Please comment in semantics_test.dart if they should be removed.

goderbauer

LGTM

goderbauer · 2019-06-19T23:04:10Z

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

@@ -175,6 +175,19 @@ class ErrorHint extends _ErrorDiagnostic {
  ErrorHint._fromParts(List<Object> messageParts) : super._fromParts(messageParts, level:DiagnosticLevel.hint);
 }

+/// An [ErrorSpacer] creates an empty [DiagnosticsNode], that can be used tune


nit: there seems to be a word missing? "can be used to tune"?

goderbauer · 2019-06-19T23:04:35Z

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

+/// An [ErrorSpacer] creates an empty [DiagnosticsNode], that can be used tune
+/// the spacing between other [DiagnosticsNode] objects.
+class ErrorSpacer extends DiagnosticsProperty<void> {
+  /// Creates a empty space to insert into a list of [DiagnosticNode] objects


a -> an empty space?

Breaking change to extremely rarely used ParentDataWidget.debugDescribeInvalidAncestorChain api changing the return type of the method from String to DiagnosticsNode.

* Add structured errors in Animations, TabView, ChangeNotifier * Add structured error on MaterialPageRoute, BoxBorder, DecorationImagePainter, TextSpan * Add structured errors in Debug * Fix test errors * Add structured errors in Scaffold and Stepper * Add structured errors in part of Rendering Layer * Fix failing test due to FloatingPoint precision * Fix failing tests due to precision error and not using final * Fix failing test due to floating precision error with RegEx instead * Add structured error in CustomLayout and increase test coverage * Add structured error & its test in ListBody * Add structured error in ProxyBox and increase test coverage * Add structured error message in Viewport * Fix styles and add more assertions on ErrorHint and DiagnosticProperty * Add structured error in scheduler/binding and scheduler/ticker Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Add structured error in AssetBundle and TextInput Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Add structured errors in several widgets #1 Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Remove unused import Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Add assertions on hint messages Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Fix catch spacing Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Add structured error in several widgets part 2 and increase code coverage Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Add structured error in flutter_test/widget_tester * Fix floating precision accuracy by using RegExp Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Remove todo to add tests in Scaffold showBottomSheet Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Fix reviews by indenting lines and fixing the assertion orders Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Fix failing tests due to renaming class Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Try skipping the NetworkBundleTest Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Remove leading space in material/debug error hint Signed-off-by: Albertus Angga Raharja <albertusangga@google.com>

…#42640) * Add structured errors in Animations, TabView, ChangeNotifier * Add structured error on MaterialPageRoute, BoxBorder, DecorationImagePainter, TextSpan * Add structured errors in Debug * Fix test errors * Add structured errors in Scaffold and Stepper * Add structured errors in part of Rendering Layer * Fix failing test due to FloatingPoint precision * Fix failing tests due to precision error and not using final * Fix failing test due to floating precision error with RegEx instead * Add structured error in CustomLayout and increase test coverage * Add structured error & its test in ListBody * Add structured error in ProxyBox and increase test coverage * Add structured error message in Viewport * Fix styles and add more assertions on ErrorHint and DiagnosticProperty * Add structured error in scheduler/binding and scheduler/ticker Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Add structured error in AssetBundle and TextInput Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Add structured errors in several widgets #1 Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Remove unused import Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Add assertions on hint messages Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Fix catch spacing Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Add structured error in several widgets part 2 and increase code coverage Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Add structured error in flutter_test/widget_tester * Fix floating precision accuracy by using RegExp Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Remove todo to add tests in Scaffold showBottomSheet Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Fix reviews by indenting lines and fixing the assertion orders Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Fix failing tests due to renaming class Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Try skipping the NetworkBundleTest Signed-off-by: Albertus Angga Raharja <albertusangga@google.com> * Remove leading space in material/debug error hint Signed-off-by: Albertus Angga Raharja <albertusangga@google.com>

googlebot added the cla: yes label Jun 18, 2019

jacob314 requested review from goderbauer and pq June 18, 2019 23:36

pq approved these changes Jun 19, 2019

View reviewed changes

goderbauer reviewed Jun 19, 2019

View reviewed changes

goderbauer added the framework flutter/packages/flutter repository. See also f: labels. label Jun 19, 2019

goderbauer approved these changes Jun 19, 2019

View reviewed changes

jacob314 added 2 commits June 20, 2019 09:20

Add more structure to errors.

e751e31

Code review comment fixes.

c613e28

jacob314 force-pushed the structured_error_step_2 branch from a2148cf to c613e28 Compare June 20, 2019 16:28

jacob314 mentioned this pull request Jun 20, 2019

ParentDataWidget.debugDescribeInvalidAncestorChain returns a String instead of a DiagnosticsNode #34682

Closed

jacob314 merged commit 38f8490 into flutter:master Jun 20, 2019

jacob314 deleted the structured_error_step_2 branch June 20, 2019 17:56

This was referenced Oct 13, 2019

Properly throw FlutterError when route builder returns null on CupertinoPageRoute #42602

Merged

Add more structure to errors (continuation of #34684) #42640

Merged

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

Add more structure to errors. #34684

Add more structure to errors. #34684

Uh oh!

Conversation

jacob314 commented Jun 18, 2019

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

pq 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!

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!

jacob314 commented Jun 19, 2019

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!

Uh oh!