search

flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
165.24k stars 27.26k forks source link

Support structured error messages #27327

Closed jacob314 closed 5 years ago

jacob314 commented 5 years ago

Currently Flutter error messages are at the core large blobs of text with limited structure which has been often slows users down fixing errors as it is hard to distinguish the signal from the noise and so users get lost in long non interactive stack traces and distinguishing what parts of the error is the core problem that occurred, background information, or hints on how to fix the problem.

We would like to add structure to error messages making the errors practical to format beautifully on the client. The goal is to have enough structure that users can quickly scan the message to see hints for how to solve the problem, the core error message, links to interactive ui to fix the ui, and interactive links to open objects related to the error in the Inspector. Similarly, errors will start with the minimum useful information to show expanded with ui affordances to expand out to see additional information such as extra stack frames and additional parent widgets.

Basic examples of what we could do with added structure:

Note in this example, the widget names are clickable navigating the user to the matching location in the widget tree. Lists of ancestors and stack frames stack out truncated and only expand when a user clicks. Portions of the error that are hints are shown with a different background color and additional padding. The core error message is in red so it is harder to miss. image Example for an error on the rendering layer. In this case the RenderObject is similarly clickable to navigate over to the inspector to view. image

Example of a ui mock of a more advanced error message that really lets the user understand a layout issue without switching contexts. Note the links to interactive UI elements to apply fixes, graphical visualization of the overflow failure that occurred, and header titles calling out core elements of the error. All of these ui features are speculative and may or may not prove out with user studies but they are features are are promising enough that we would like to iterate on them without churning through refactoring code in package:flutter for every UI iteration.

screen shot 2019-01-30 at 5 34 32 pm

Prototype CL showing what it would take to add structure to all flutter errors at the cost of more structured syntax writing error messages than writing unstructured blob of text. https://github.com/flutter/flutter/pull/27202 If nothing else this CL provides a good overview of what blocks of content exist in the existing error messages. Some observations: there are common tasks like embedding a structured object for a RenderObject that are called 20 times across the code base. Currently the same boilerplate code to emit strings describing a widget and its immediate children is repeated for each of these cases. If you look at FlutterErrorBuilder, WidgetErrorBuilder, and RenderErrorBuilder you will see the full list of all commonly occurring patterns of structured data that is included in errors (StackTrace, RenderObject, Widget, ancestor chain of widget) and error parts (e.g. hint, error, constraint).

The core concern about this prototype CL is that the builder syntax used is not productive enough for users to use and will reduce the number or quality of errors. A counter point is that it took about 2 days to refactor all errors in package:flutter to use the format. For someone who wants to write an error message particularly in the simpler case an alternate approach is to add named arguments provided on the default FlutterError constructor but unfortunately once you exceed the capabilities of that constructor you will have to start using builder syntax. Builder syntax is needed to handle multiple messages of the same type such as multiple hints. Looking at the facts on the ground there are multiple flutter errors with multiple separate hints.

Benefits are it is hard for users to do the wrong thing and there are builder classes that clarify what helper methods should be used depending on the context the error is being thrown in (base, Render, or Widget). The structure of the error messages is the same DiagnosticsNode format used by the WidgetInspector making it reasonable to include errors directly in the widget tree as properties of a widget indicating where the problem occurred.

Risks:

The builder syntax may be unwieldy and demotivate framework authors and users from writing good structured errors.

Alternate design:

Markdown lite for Flutter errors. Add magic characters in the error templates that indicate error parts (e.g. hint, error, contract).

Risks:

Markdown like templates may be fragile and error prone as users will get extremely limited compile time checking that they are using the scheme in contrast to the other design where you will get an immediate compile time error if you attempt to pass the wrong type of object to a builder method. Similarly, opening and closing blocks in this case will be done with some level of tags embedded as magic unicode characters so mismatched tags will cause the usual problems. This can be mitigated with careful test coverage although the current code base shows that there is limited coverage of errors.

Breaking change:

This change will modify the toString for all objects implementing Diagnosticable. As Diagnosticable objects have great toString functions, there tend to be a lot of tests that use them to easily write golden tests. Currently modifying the toString for Diagnosticable breaks 19 tests in package:flutter and would break third party tests as well. Modifying the equalsIgnoreHashCodes matcher to ignore the temporary object ids embedded in the toString as well would help somewhat but still leads to 16 tests failing. This is because not all tests using the equalsIgnoringHashCodes matcher as it wouldn't make sense for them to as the golden output did not contain hash codes and because adding the magic unicode character impacts line breaking for multiple diagnostics output so even though equalsIgnoreHashCodes is used. Fixing the line breaking helper used reduces the number of test failures in package:flutter to 13. The failures would be easy enough to fix by switching to a matcher that ignores the magic characters but users would need to know such a matcher exists and use it. Otherwise the tests will become mysteriously flaking as the object ids in the matchers would change unpredictably.

This breaks anyone who linebreaks debug output and doesn't know about the magic characters. Lines will be broken incorrectly resulting in shorter lines than otherwise. The common debugWrap helper can be updated to reflect the magic characters to mitigate this risk. With debugWrap modified to ignore magic characters the # of failing tests in package:flutter is reduced to People with golden tests should expect to have to rebaseline them periodically so perhaps this isn't that big a deal.

Alternatives to minimize the breaking:

Make the magic unicode characters be off by default in tests. This will mean that debuggers depending on them will be broken for tests and tests testing debug output are testing the wrong thing but would minimize the breakage.

Example failing test:

    expect(description, <String>[
      'backgroundColor: Color(0xff123456)',
      'elevation: 8.0',
      'titleTextStyle: TextStyle(inherit: true, color: Color(0xffffffff))',
      'contentTextStyle: TextStyle(inherit: true, color: Color(0xff000000))',
    ]);

now the toString for each TextStyle contains at least one marker character.

Fixing debugWrap to account for the magic characters still leaves 13 failures.

Unsolved challenges:

Object lifecycles using this scheme are fragile as Dart string templating is quite limited so we have no efficient way of distinguishing a toString call part of a template that will be used for an error from a time someone is just calling toString arbitrarily on an object. This suggests the only feasible approach to avoid memory leaks is to have only a ring buffer of object references. This can work alright as long as care is taken to quickly deserialize the object references out of the format when capturing them for display at a later point either in a logging view or in an interactive view on the device or in the inspector such as a FlutterError widget.

jacob314 commented 5 years ago

@goderbauer @Hixie @pq @InMatrix @devoncarew

jacob314 commented 5 years ago

CL with strawman approach adding magic unicode markers in the string to indicate the styles instead of Dart code. https://github.com/flutter/flutter/compare/master...Hixie:pretty_errors

pq commented 5 years ago

To make it apples to apples, maybe we could look at a few side-by-side examples?

To start, how would this one look in a "Markdown lite" style?

throw FlutterError.from(FlutterErrorBuilder()
    ..addError('AnimationController.dispose() called more than once.')
    ..addHint('A given $runtimeType cannot be disposed more than once.\n')
    ..addProperty('The following $runtimeType object was disposed multiple times', this)
);
jacob314 commented 5 years ago

Here's a diff with side by side examples. I think with the pretty_errors style it would be really important to enforce that the first line always has to be the red error problem line as adding it specifically definitely seems cumbersome. https://github.com/flutter/flutter/commit/81062a53542f61d3e67f8b09152d8334e66cab80

Builder style is on the left, magic unicode characters is on the right: Here's an example of some complex error building cases:

screen shot 2019-01-30 at 7 44 04 pm

Here are some simple cases:

screen shot 2019-01-30 at 7 45 22 pm

Medium complexity case:

screen shot 2019-01-30 at 7 49 18 pm
jonahwilliams commented 5 years ago

I find it easier to write (in general) If I have some structure to guide me. Speaking for myself, the API provided by the structured error messages is helpful, especially when combined with a corpus of existing error messages.

Regarding the API on the right, I think it would be difficult to make sure the characters are inserted correctly. This would presumably require more manually checking.

pq commented 5 years ago

Regarding the API on the right, I think it would be difficult to make sure the characters are inserted correctly. This would presumably require more manually checking.

Or help from customized analysis. With either approach we could certainly add linting to nudge authors towards well-formed errors. Having said that, the explicit structure case would be far easier...

pq commented 5 years ago

@Hixie, in response to Jacob's original PR you wrote:

I think we should attempt to find a solution that has a much lighter API. My fear is that engineers (that is to say, we) in general already find writing error messages to be highly aversive, and anything we do to raise the barrier to writing error messages would only make things worse. Ideally making objects in a string inspectable would require zero overhead from the perspective of framework developers. For example, one could imagine changing Object.toString or Diagnosticable.toString to encode information in the output (which normally gets stripped out but which IDEs and maybe the flutter tool can detect).

I don't know if the embedded tag idea is a straw-man proposal but I'm curious why you think something like it would be less of a barrier to authoring good errors (especially in the more complex cases). Like @jonahwilliams says above, to my eyes having to navigate an implicit structure is more burdensome (and has the added disadvantage of being much harder to accommodate with tooling). In contrast, a builder pattern is a pretty common approach in cases like this and I would expect it to be pretty familiar to authors generally, is less prone to simple errors and is easier to tool.

Could you maybe elaborate on your misgivings?

@InMatrix: any thoughts from the UX perspective?

devoncarew commented 5 years ago

I'm very excited about how we'll be able to consume this info from tools; it should improve both presentation and navigation, and give us more options going forward.

I'm less invested in how the errors are constructed; I'm sure the framework authors will have good insight here.

In terms of the UX of using a builder class (FlutterErrorBuilder) - if we feel that it adds a slight barrier to writing errors, I wonder if we can't identify a set of errors that could be formed w/ a slightly simpler API? Perhaps FlutterError could have a constructor with a set of named params. This may not work for creating a more sophisticated error - people could use the full FlutterErrorBuilder API there - but could work for simpler cases, and be slightly less work for the author when building the error.

jacob314 commented 5 years ago

Some edge cases with multiple hints and hints with multiple paragraphs. These cases are useful to test how complex the markdown style syntax needs to be to avoid exposing api cliffs. https://github.com/flutter/flutter/commit/62d86ac39d3fc9e09e41177cd8a91bf2e7a0735a This case requires ending and closing tags. I'm not sure how discoverable this would be and likely most times users would forget the tags to open and close the block to ensure the hint goes over multiple paragraphs. image

This case is fine because even though the hint is multiple paragraphs, we can automatically link them due to the mark down syntax implied by the indent and/or line ending in colon. image

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-exposing-api-clif

jacob314 commented 5 years ago

@devoncarew In the strawman CL the FlutterError class does have an API with named parameters for the common cases which could be used a lot more than it is with light wordsmithing of the flutter errors. Currently not all errors currently meet the expected order for error blocks but it would be reasonable to wordsmith the errors to match that consistent style. If we went this route, we would want to add a lint if you call specific methods with named parameters in the wrong order as otherwise there would be significant bugs to error text not being in the error implied by viewing the definition code. In general I think such an api would be a fine alternative for common cases and the lint would be useful for other APIs where parameter order is important to a user understanding the API.

jacob314 commented 5 years ago

I find it easier to write (in general) If I have some structure to guide me. Speaking for myself, the API provided by the structured error messages is helpful, especially when combined with a corpus of existing error messages.

Regarding the API on the right, I think it would be difficult to make sure the characters are inserted correctly. This would presumably require more manually checking.

As graphical displays of the errors IDEs become more advanced and diverge more from the straight text output I think having the structure guide users in the right direction is a big win. With the the magic characters option I think we would want to provide a way to preview the renders of the structured output in the IDE to visually check that the error looks generally as expected when writing code. However this is a bit hard for complex cases like some of the ones shown where the errors are build up incrementally over multiple lines or where calling helper methods to perform common tasks like building a beautiful display of a render object where a single helper will result in 10+ lines of error output with significant structure.

Hixie commented 5 years ago

Comparing the two proposals above to each other isn't useful. What we need to do is evaluate them relative to what they are both replacing. What we do today is just build a string. It's straight-forward and intuitive. What we need is a solution that is as straight-forward and intuitive. It might not be either of the solutions described above.

The interesting part of the marker strawman isn't the bits where we annotate hints or errors, it's the part where any Diagnosticable object that's embedded in a string gets automatically marked up. In other words, the difference is between:

assert(foo.isBar, 'The following object is not bar: $foo\nThis method unbars foos.');

...and:

assert(() {
  if (!foo.isBar) {
    throw new FlutterError(FlutterErrorBuilder()
      ..addSomething('The following object is not bar:', something: foo);
      ..addSomethingElse('This method unbars foos.');
    );
  }
}());

Maybe I'm an exception but I know I won't be able to remember what "Something", "something", and "SomethingElse" are. Having to figure it out each time would motivate me to write fewer, and less helpful, error messages, in the same way that having the diagnostics API has motivated me to write fewer useful diagnostics than when it was just a simple matter of writing a toString method.

I think it would be preferable to entirely ditch the styling of hints vs errors and only have the ability to inspect objects in error messages, than it would be to complicate our error-reporting code to the extent that is implied by the FlutterErrorBuilder API. It is more important that we give our users useful information than that we give our users color. We have done studies and the main thing that helped was giving our users whitespace; giving color in some cases actually made things worse.

@jonahwilliams What has your experience been with adding the diagnostics (as opposed to adding the toStrings) on objects since we added Diagnosticable? I suspect the experience with FlutterErrorBuilder would be similar.

jacob314 commented 5 years ago

Assert takes an object not a string as the second pattern so the example given can be significantly simplified. Unless I'm missing something we could support the following pretty easily to cut down the verbosity.

assert(
   foo.isBar,
    FlutterErrorBuilder()
      ..addSomething('The following object is not bar:', something: foo)
      ..addSomethingElse('This method unbars foos.')
);

In general there are ways the builder syntax can be tightened up if reducing verbosity is a priority. For example, with minor tweaks we could let you write:

   FlutterError error = new FlutterErrorBuilder()
      .addSomething('The following object is not bar:', something: foo)
      .addSomethingElse('This method unbars foos.')
      .build();

The strawman makes the builder methods void so chaining has to be done with .. but builders that chain with . are also reasonable and it would require minimal extra work to make that happen.

jacob314 commented 5 years ago

Bringing up the cases of raw asserts is interesting. I'll admit all my experimentation has been targeted as cases where there was already a FlutterError object created as those seems like cases more complex error messages. For example, there appear to be only two asserts in the framework with more than one line error and a $ Looking at all the asserts in the existing code base with a $ there are only a couple would clearly benefit from additional structure in the text output but maybe I'm just looking in the wrong places so I would appreciate links to existing asserts that should be logging Diagnosticable objects but aren't.

For example, here are the are about 62 existing asserts with at least one $. Almost all of them are single line.

Here are the couple that are multiple lines.

 assert(
      delegate._route == null,
      'The ${delegate.runtimeType} instance is currently used by another active '
      'search. Please close that search by calling close() on the SearchDelegate '
      'before openening another search with the same delegate instance.',
    );
  assert(
        trackedAnnotation != null,
        'Unable to find annotation $annotation in tracked annotations. '
        'Check that attachAnnotation has been called for all annotated layers.');

Neither of these asserts have objects that are Diagnosticable so I'm not sure we have a way to making the objects referenced by those asserts structured in GUI tools without adding more verbosity to the error message beyond string interpolation or making significantly more classes Diagnosticable. If the conclusion is we have to make 5X more classes in Flutter Diagnosticable that is a fine conclusion. Snapshot of single line asserts. A couple of them have render objects but none are using the best practice patterns used elsewhere in the code for displaying a render object and its properties.

List of all asserts with a $ in them currently 

lib/src/animation/tween_sequence.dart:    assert(false, 'TweenSequence.evaluate() could not find a interval for $t');
lib/src/gestures/arena.dart:    assert(_debugLogDiagnostic(pointer, 'Adding: $member'));
lib/src/gestures/arena.dart:      assert(_debugLogDiagnostic(pointer, 'Winner: ${state.members.first}'));
lib/src/gestures/arena.dart:    assert(_debugLogDiagnostic(pointer, '${ disposition == GestureDisposition.accepted ? "Accepting" : "Rejecting" }: $member'));
lib/src/gestures/arena.dart:        assert(_debugLogDiagnostic(pointer, 'Self-declared winner: $member'));
lib/src/gestures/arena.dart:      assert(_debugLogDiagnostic(pointer, 'Eager winner: ${state.eagerWinner}'));
lib/src/gestures/arena.dart:    assert(_debugLogDiagnostic(pointer, 'Default winner: ${state.members.first}'));
lib/src/gestures/mouse_tracking.dart:    assert(trackedAnnotation != null, "Tried to detach an annotation that wasn't attached: $annotation");
lib/src/painting/box_border.dart:    assert(textDirection != null, 'The textDirection argument to $runtimeType.getInnerPath must not be null.');
lib/src/painting/box_border.dart:    assert(textDirection != null, 'The textDirection argument to $runtimeType.getOuterPath must not be null.');
lib/src/rendering/box.dart:    assert(hasSize, 'RenderBox was not laid out: ${toString()}');
lib/src/rendering/flex.dart:          assert(textDirection != null, 'Horizontal $runtimeType with multiple children has a null textDirection, so the layout order is undefined.');
lib/src/rendering/flex.dart:          assert(verticalDirection != null, 'Vertical $runtimeType with multiple children has a null verticalDirection, so the layout order is undefined.');
lib/src/rendering/flex.dart:          assert(textDirection != null, 'Horizontal $runtimeType with $mainAxisAlignment has a null textDirection, so the alignment cannot be resolved.');
lib/src/rendering/flex.dart:          assert(verticalDirection != null, 'Vertical $runtimeType with $mainAxisAlignment has a null verticalDirection, so the alignment cannot be resolved.');
lib/src/rendering/flex.dart:          assert(verticalDirection != null, 'Horizontal $runtimeType with $crossAxisAlignment has a null verticalDirection, so the alignment cannot be resolved.');
lib/src/rendering/flex.dart:          assert(textDirection != null, 'Vertical $runtimeType with $crossAxisAlignment has a null textDirection, so the alignment cannot be resolved.');
lib/src/rendering/object.dart:    assert(!_needsLayout, 'Updated layout information required for $this to calculate semantics.');
lib/src/rendering/viewport.dart:      assert(child.parent != null, '$target must be a descendant of $this');
lib/src/rendering/wrap.dart:          assert(textDirection != null, 'Horizontal $runtimeType with multiple children has a null textDirection, so the layout order is undefined.');
lib/src/rendering/wrap.dart:          assert(verticalDirection != null, 'Vertical $runtimeType with multiple children has a null verticalDirection, so the layout order is undefined.');
lib/src/rendering/wrap.dart:          assert(textDirection != null, 'Horizontal $runtimeType with alignment $alignment has a null textDirection, so the alignment cannot be resolved.');
lib/src/rendering/wrap.dart:          assert(verticalDirection != null, 'Vertical $runtimeType with alignment $alignment has a null verticalDirection, so the alignment cannot be resolved.');
lib/src/rendering/wrap.dart:          assert(verticalDirection != null, 'Horizontal $runtimeType with runAlignment $runAlignment has a null verticalDirection, so the alignment cannot be resolved.');
lib/src/rendering/wrap.dart:          assert(textDirection != null, 'Vertical $runtimeType with runAlignment $runAlignment has a null textDirection, so the alignment cannot be resolved.');
lib/src/rendering/wrap.dart:          assert(verticalDirection != null, 'Horizontal $runtimeType with crossAxisAlignment $crossAxisAlignment has a null verticalDirection, so the alignment cannot be resolved.');
lib/src/rendering/wrap.dart:          assert(textDirection != null, 'Vertical $runtimeType with crossAxisAlignment $crossAxisAlignment has a null textDirection, so the alignment cannot be resolved.');
lib/src/semantics/semantics.dart:       assert(label == '' || textDirection != null, 'A SemanticsData object with label "$label" had a null textDirection.'),
lib/src/semantics/semantics.dart:       assert(value == '' || textDirection != null, 'A SemanticsData object with value "$value" had a null textDirection.'),
lib/src/semantics/semantics.dart:       assert(hint == '' || textDirection != null, 'A SemanticsData object with hint "$hint" had a null textDirection.'),
lib/src/semantics/semantics.dart:       assert(decreasedValue == '' || textDirection != null, 'A SemanticsData object with decreasedValue "$decreasedValue" had a null textDirection.'),
lib/src/semantics/semantics.dart:       assert(increasedValue == '' || textDirection != null, 'A SemanticsData object with increasedValue "$increasedValue" had a null textDirection.'),
lib/src/semantics/semantics.dart:        assert(!child.isInvisible, 'Child $child is invisible and should not be added as a child of $this.');
lib/src/services/platform_views.dart:    assert(_state != _AndroidViewState.disposed, 'trying to size a disposed Android View. View id: $id');
lib/src/services/platform_views.dart:    assert(_state != _AndroidViewState.disposed,'trying to set a layout direction for a disposed UIView. View id: $id');
lib/src/services/platform_views.dart:    assert(!_debugDisposed, 'trying to set a layout direction for a disposed Android View. View id: $id');
lib/src/services/raw_keyboard_android.dart:    assert(false, 'Not handling $key type properly.');
lib/src/services/raw_keyboard_fuschia.dart:    assert(false, 'Not handling $key type properly.');
lib/src/widgets/editable_text.dart:    assert(result != null, '$runtimeType created without a textDirection and with no ambient Directionality.');
lib/src/widgets/gesture_detector.dart:    assert(type == T, 'GestureRecognizerFactory of type $T was used where type $type was specified.');
lib/src/widgets/gesture_detector.dart:      assert(_recognizers[type].runtimeType == type, 'GestureRecognizerFactory of type $type created a GestureRecognizer of type ${_recognizers[type].runtimeType}. The GestureRecognizerFactory must be specialized with the type of the class that it returns from its constructor method.');
lib/src/widgets/routes.dart:    assert(!_transitionCompleter.isCompleted, 'Cannot reuse a $runtimeType after disposing it.');
lib/src/widgets/routes.dart:    assert(!_transitionCompleter.isCompleted, 'Cannot reuse a $runtimeType after disposing it.');
lib/src/widgets/routes.dart:    assert(!_transitionCompleter.isCompleted, 'Cannot install a $runtimeType after disposing it.');
lib/src/widgets/routes.dart:    assert(_controller != null, '$runtimeType.createAnimationController() returned null.');
lib/src/widgets/routes.dart:    assert(_animation != null, '$runtimeType.createAnimation() returned null.');
lib/src/widgets/routes.dart:    assert(_controller != null, '$runtimeType.didPush called before calling install() or after calling dispose().');
lib/src/widgets/routes.dart:    assert(!_transitionCompleter.isCompleted, 'Cannot reuse a $runtimeType after disposing it.');
lib/src/widgets/routes.dart:    assert(_controller != null, '$runtimeType.didReplace called before calling install() or after calling dispose().');
lib/src/widgets/routes.dart:    assert(!_transitionCompleter.isCompleted, 'Cannot reuse a $runtimeType after disposing it.');
lib/src/widgets/routes.dart:    assert(_controller != null, '$runtimeType.didPop called before calling install() or after calling dispose().');
lib/src/widgets/routes.dart:    assert(!_transitionCompleter.isCompleted, 'Cannot reuse a $runtimeType after disposing it.');
lib/src/widgets/routes.dart:    assert(_controller != null, '$runtimeType.didPopNext called before calling install() or after calling dispose().');
lib/src/widgets/routes.dart:    assert(!_transitionCompleter.isCompleted, 'Cannot reuse a $runtimeType after disposing it.');
lib/src/widgets/routes.dart:    assert(_controller != null, '$runtimeType.didChangeNext called before calling install() or after calling dispose().');
lib/src/widgets/routes.dart:    assert(!_transitionCompleter.isCompleted, 'Cannot reuse a $runtimeType after disposing it.');
lib/src/widgets/routes.dart:    assert(!_transitionCompleter.isCompleted, 'Cannot dispose a $runtimeType twice.');
test/animation/curves_test.dart:      assert(deltaY.abs() < delta * maximumSlope, '${curve.toString()} discontinuous at $x');
test/cupertino/nav_bar_transition_test.dart:  assert(false, 'Unexpected nav bar type ${navBar.runtimeType}');
Hixie commented 5 years ago

It is my dear hope that the current number of asserts and exceptions with strings pales in comparison to the future number of asserts and exceptions with strings. What matters isn't the current messages, but the future ones.

It's currently too hard to create error messages (we know this, because otherwise we'd have many more). That's why I'm so eager to make sure that whatever solution we have here is super simple. Ideally, even simple than what we have now, though I can't for the life of me imagine how we could do that.

The problem isn't verbosity, it's what you need to know in order to write a useful error message. Today you need to know:

This, it turns out, is already a really high bar. I am very skeptical of adding to that list.

(I'm also a bit dubious of trying to fit error messages into a mold. Gradle does this, and the result is pretty much uniformly terrible -- every error message has way too much text and often ends up being so confusing that it would have been simple if it just said "Error" and left it at that.)

Maybe we should take a step back, though. We're arguing over solutions here, but I feel that I don't yet fully understand the problem space.

What is our goal? The original comment says:

The goal is to have enough structure that users can quickly scan the message to see hints for how to solve the problem, the core error message, links to interactive ui to fix the ui, and interactive links to open objects related to the error in the Inspector. Similarly, errors will start with the minimum useful information to show expanded with ui affordances to expand out to see additional information such as extra stack frames and additional parent widgets.

What our studies have shown is that at the core of it, merely adding blank lines between paragraphs is the best improvement we have tested. (It scored better than adding colour in some cases, and about the same in others. cc @InMatrix) Do we really need that much structure to allow users to scan the message? Aren't paragraph breaks all we actually need? (We have those already.) (These aren't rhetorical questions. It would be good to see concrete comparisons of error messages as they are today, as they would be with some trivial post-processing to create clear paragraph separation, and as they would be with elaborate styling as proposed earlier.)

We already have enough structure to detect the stack traces (we already detect them to avoid line-wrapping them), so we could collapse those if we wanted to.

This leaves the interactive links part. I agree that it would be nice to be able to jump from an error message to the tree view to see the context. It would also be good to be able to do that from random print statements when doing print debugging, which is common. A solution that works both for print and for exceptions (which also end up going through print), and maybe for other things like error messages in FlutterError, seems like it would be more valuable than one that requires special code in exception throwing logic. Can we find a more general solution to this?

Is there anything else we want to do?

InMatrix commented 5 years ago

Disclaimer: I co-designed the named-parameter + builder API, and I'm a co-author of the study mentioned in this thread. My primary goal in writing this comment is not advocating for the named-parameter + builder API per se but trying to help us avoid an impasse.

I'd like to address three questions in this comment. Feel free to jump to their respective headings. Sorry about the number of words I have to use in this comment.

Will the proposed API make writing error messages more onerous than it is today?

What we need to do is evaluate them relative to what they are both replacing. What we do today is just build a string.

I don't think what we do today is affected by either proposal. My impression is that nothing is getting replaced by any of the API designs being considered in this thread. Whatever API we may end up with should not seek to remove the developer's ability to create error messages from simple strings. The goal of designing this API is to make it possible to add structure and metadata to an error which either already has a detailed but poorly presented message or an error which does require substantially more information to be actionable.

Let me unpack this goal a little bit further:

Are blank lines between paragraphs all we need to make our error messages usable?

What our studies have shown is that at the core of it, merely adding blank lines between paragraphs is the best improvement we have tested.

As a co-author of the study, I'd like to make a few clarifications. First, the "Spaces" variant, which the above comment refers to, included more than "blank lines between paragraphs." It also added section headings such as "Explanation" and "Potential Fix." The blank lines were added to separate meaningful sections in an error report rather than paragraphs, which of course should be separated to follow basic English writing styles.

In deed, both the white space between sections and the section headings helped study participants read the message more efficiently. As these two comments from participants suggest:

Though the "spaces" variant did score best in most tests, highlighting key information in the "colors" variant and collapsing widget constructor, ancestor tree, and stacktrace in the "ellipses" variant also substantially outperformed the original presentation of the message. Moreover, users found them useful for different reasons.

While the "spaces" variant enabled users to skim a long error message, the "colors" variant helped them notice the most important information first, as this participant commented:

"The color and bolding makes the information easier to parse. The light gray font indicates to me that I can probably ignore that information (if this is accurate, then definitely a pro). The explanation (in both) is a pro because I'm newer to Flutter, but if I was a more-experienced engineer, I would think the verbose error messages were a con."

The "ellipses" variant was effective for simply cutting down the visual noise, allowing progressive disclosure of details. For example, this participant explained its advantage as follows:

"B (the 'ellipses' variant) is to the point while A (the original) has too much unhelpful debug details. I lost my train of thought before getting to what matter in A. While B I was able to read quickly to the issue."

To sum up, "adding blank lines between paragraphs" to Flutter error messages is certainly a good starting point and low hangout fruit, but it's important to remember that it is the bare minimum we could do and it falls short from the original design of the "spaces" variant tested well in our study. There are good reasons to believe that combining design elements from all three variants we tested could further enhance the efficiency of reading an error message, especially when it comes to a complex error.

In addition, we really just scratched the surface of making errors easier to understand and more actionable. While I have no problem with doing things in stages, I hope we won't stop at the bare minimum and implement an API that could preclude further experimentation.

So, which API should we implement?

Either is fine by me. Doing nothing is not. Though the two proposals come with different usability trade-offs, none has critical shortcomings in my opinion. Let's consider those trade-offs in two scenarios.

Scenario 1: The developer wants to improve the presentation of a complex error message authored by someone else.

Scenario 2: The developer is creating a new error, and he wants to make use of the pretty error presentation from Flutter's dev tools. He's aware of the FlutterError class, but he hasn't used it to create structured errors before.

In conclusion, I think the cost of inaction is much greater than choosing either design. If we have a process to move this debate forward, let's use that process. If not, let's flip a coin (seriously).

Hixie commented 5 years ago

We definitely shouldn't flip a coin. We should work together to find something we're all actually happy with.

jacob314 commented 5 years ago

I'm working on an option 3 that I hope we can all actually be happy with which I'll send out more details on later today. I've verifying it hangs together now but the concept is to use named arguments to cover >=90% hopefully >= 95 of the error messages with a complex error message with a solution that doesn't leave API cliffs and provides some promising ways to move forward for improving the 5000 asserts that even have a short text error message and shouldn't have one.

goderbauer commented 5 years ago

When it comes to reading code I find the Builder pattern pretty nice. It makes it immediately clear to me as the reader what part of the error the author considered as e.g. the solution to a problem. The Builder pattern is familiar to most readers, so they know what to expect. When reading the code with the markdown tags, those tags felt like distracting noise. Forgetting the discussion here, it was not immediately clear to me what their purpose is (I feel like in an IDE I'd have to click through to read the documentation on why somebody included ${FlutterError.hint} in an exception string), which results in an annoying context switch. The Builder pattern did not urge me to switch context away from the actual exception in the code.

Again, the above is about reading code with the two patterns in use. Now when it comes to writing exceptions both patterns scare me a little. This may be my english-as-a-second-language talking - but I have no idea what the difference of a "claim" or a "ground" in an exception is supposed to be (to just name two). With both APIs I feel like I need to consult a dictionary to work out the semantics difference to provide good error messages (or look at lots of other examples to extract the differences from them). I can totally see how instead of doing that developers (not excluding me) get lazy and fall back to providing less then optimal error messages. I am not sure how to overcome this problem.

Having said that, the ultimate goal to make exceptions more accessible and actionable is of course a great cause!

jacob314 commented 5 years ago

I've created a third option. I've verified that this option means the builder syntax is not needed for about 90% of all cases. Additionally this option enforces more structure than even the builder syntax so I've caught a few cases where error messages were irregular in undesirable ways just by applying it to the code base. The core concept is that all the important core classes like Widget, State, RenderObject, and Element provide a method called describeError and sometimes describeChildError which takes a list of named parameters describing the error and indicating what attributes of the Element or RenderObject are relevant to describe the error. Prototype CL illustrating the transformation this allows. https://github.com/flutter/flutter/commit/a81b262be3468612c030ebd8486fe609c4436be1 I think the results are clearly better than either of the other two options. Internally this third option builds on some of the better ideas from the other two options. hints and fixes can contain urls without requiring additional parameters with the markdown like conventions implicit in the existing errors guiding how IDEs choose to render those messages. Each of the describeError messages internally is a trivial call to a a FlutterErrorBuilder, RenderErrorBuilder, and WidgetErrorBuilder as that provides an exceedingly terse way to write the build methods. There are a few dynamic cases around some of the layout errors that are built up dynamically where describeError isn't used and the builder syntax is used directly however these were cases where the builder syntax was actually an improvement of the previous adhoc string concat and a definite improvement over the magic tags approach.

On a related note: Lets start with 4 categories of levels for the initial version as we can always add more later as needed. error, hint, fix, and summary/description. I don't have an opinion about the names and defer to whatever is most familiar to the team.

Note that in the flutter code base there appears to have been a misunderstanding that an assert can take an arbitrary type as a second argument. I cleaned up a few errors to take advantage of that little known dart feature and the output looks cleaner as well as being more efficient due to one less closure creation and evaluation per assert.

This solution provides a clear path to what to do next to make the 5000 bare asserts in package:flutter better. For those cases we should have a kernel transformer that rewrites

assert(foo == bar);

as

assert(foo == bar, describeError('assertion error: foo != bar'));

anytime this has a type with a describeError method. Now you get a high quality structured baseline description for every error. The describeError for a RenderObject will tell you the properties of that render object and provide a link to navigate to it in the inspector. Same for RenderObject and BuildContext. We could even get cleverer and prefer to call describeError on the buildContext if one is in scope instead of on this. For build methods, calling describeError on the buildContext is probably what you wanted.

A few screenshots from the CL calling out the typical usage.

screen shot 2019-02-04 at 7 56 47 pm screen shot 2019-02-04 at 7 56 35 pm
InMatrix commented 5 years ago

Option 3 looks really nice to me.

Lets start with 4 categories of levels for the initial version as we can always add more later as needed. error, hint, fix, and summary/description. I don't have an opinion about the names and defer to whatever is most familiar to the team.

I agree. I'd recommend the following names. I also described rationale for identifying each category.

Category "summary:"

Category "description:"

Categories "hint" and "fix:"

Hixie commented 5 years ago

Looking at https://github.com/flutter/flutter/compare/master...jacob314:structured_error_ac I agree that this is definitely an improvement. I think I would push for even further simplification, but I need to study the patch in more detail to give a more informed opinion.

jacob314 commented 5 years ago

Note that master...jacob314:structured_error_ac only shows a fraction of the errors refactored with the simpler helper methods. I expect at least 90% of the errors listed can be refactored to use the simpler syntax while keeping the number of named parameters from getting out of control.

A possible refinement I'd like your opinion on is separating the part of describeError that indicates how the context should be displayed from the part describing what error occurred because all errors regardless of the context they occurred on have the same needs for describing the error but the code to describe the context is completely custom to the context the error occurred. A prototype for doing this is adding a simple ErrorMessage class that is used by all defineError methods. The goal is it separates out the part of the errorDescription that is independent of the context the error occurred from the part that is describing the context the error occurred. This would reduce the duplicated logic between the various describeError messages with the downside of making the calls to describeError a little more verbose. The benefit is it is a clear separation of which part of the call to describeError is the part describing the error that occurred and which part is explaining the context the error occurred in. A possible refinement is to in the future rewrite cases a raw ErrorMessage is thrown to inject the call to describeError on the context that appears most appropriate based on types in scope. This is analogous to the case described previously of rewriting asserts with zero arguments to inject an call to describeError passing in the expression being asserted.

Example:


Example usage:
Old:
   context.describeError(
      'No MediaQuery widget found.',
      description: '${context.widget.runtimeType} widgets require a MediaQuery widget ancestor.',
      contextName: 'The specific widget that could not find a MediaQuery ancestor was',
      describeOwnershipChain: true,
      hint:
        'Typically, the MediaQuery widget is introduced by the MaterialApp or '
        'WidgetsApp widget at the top of your application widget tree.',
    ),

New:
   context.describeError(
      ErrorMessage(
        'No MediaQuery widget found.',
        description: '${context.widget.runtimeType} widgets require a MediaQuery widget ancestor.',
        hint:
          'Typically, the MediaQuery widget is introduced by the MaterialApp or '
          'WidgetsApp widget at the top of your application widget tree.',
      ),
      contextName: 'The specific widget that could not find a MediaQuery ancestor was',
      describeOwnershipChain: true,
    ),

class ErrorDescription {
  final String summary;
  final String description; 
  final Object descriptionObject;
  final List<String> hints;
  // Provide one or more convenient constructors for defining ErrorDescription objects.
}

class BuildContext {
  FlutterError describeError(
    ErrorDescription errorDescription, {
    // These 4 parameters are the configuration of how this BuildContext should be
    // converted into a useful description given this metadata about what parts of
    // the BuildContext are critical to the user.
    @required String contextName,  
    String renderObjectName,
    bool describeOwnershipChain = false,
    Type expectedAncestorType,
  });
Hixie commented 5 years ago

It's hard for me to evaluate this because I'm lacking context. Let me try to understand your branch and get back to you.

Hixie commented 5 years ago

Ok, I've studied this PR in detail now.

I think we should scale back what we are doing here. For example, I think we should avoid having "fix" until we have an actual good fix (none of the cases I found in the PR are what I would consider any more than hints). I think we should combine "hint" and "description", and as noted above, we should drop contract and violation.

I think it's unfortunate that this PR doesn't make it possible to click on values that are embedded inside strings, e.g. the enum value in violation: 'Instead, this element is in the $_debugLifecycleState state.' or the variables in:

            ..addViolation('Curves must map 0.0 to near zero and 1.0 to near one but '
            '${activeCurve.runtimeType} mapped $t to $transformedValue, which '
            'is near $roundedTransformedValue.')

It works around this for some values by having dedicated methods that assign a single label to a single value, as in:

            ..addDebugProperty('Active curve', activeCurve)

I think that we shouldn't do a workaround here. We should find a solution to expose values that are interpolated in general, and then that solution would apply to all the work we're doing here with errors. (It would also fix a large class of other problems, like inspecting things that are passed to print, or inspecting things that are interpolated into strings, etc. I can imagine various ways to do this using VM support in debug builds.) I think it'd be fine to address that problem in a separate PR; it would help keep the scope of this PR down.

I think we could do a lot to make the API simpler over all. For example, removing the "describeError" methods would actually simplify matters. They are intended as convenience methods, but they have a very large API (11 arguments in one case), and they mostly just call straight through to underlying APIs that aren't much worse. Similarly, we could remove features like addHints (just call addHint several times).

If we remove the convenience methods, remove all the types of strings except "hint", and remove the features that merely add objects in various forms (e.g. addProperty, addIterable, etc). Then we're actually left with an API that's much simpler. FlutterErrorBuilder for example ends up having only two methods, addHint and addError. At that point, we hardly even need a builder. We could just put it all on FlutterError. For example, we could have it take an array of objects:

  FlutterError(<FlutterErrorParts>[
    ErrorMessage('Invalid curve endpoint at $t.'),
    ErrorHint('Curves must map 0.0 to near zero and 1.0 to near one but '
              '${activeCurve.runtimeType} mapped $t to $transformedValue, which '
              'is near $roundedTransformedValue.'
    ),
  ]);

We could make this integrate more tightly with the diagnostics logic, which presumably would help with the transferring of data to the IDE, by making FlutterErrorParts actually be DiagnosticsNodes:

  FlutterError(<DiagnosticsNode>[
    ErrorMessage('Invalid curve endpoint at $t.'),
    ErrorHint('Curves must map 0.0 to near zero and 1.0 to near one but '
              '${activeCurve.runtimeType} mapped $t to $transformedValue, which '
              'is near $roundedTransformedValue.'
    ),
  ]);

This even gives us a free way to include inspectable objects into the error description before we do the real fix as mentioned above: 

  FlutterError(<DiagnosticsNode>[
    ErrorMessage('Invalid curve endpoint at $t.'),
    ErrorHint('Curves must map 0.0 to near zero and 1.0 to near one but '
              '${activeCurve.runtimeType} mapped $t to $transformedValue, which '
              'is near $roundedTransformedValue.'
    ),
    DiagnosticsProperty<Curve>('Active curve', activeCurve),
  ]);

"Free" in the sense that there's no additional API surface needed to support this, so it doesn't leave us with vestigial API surface when we eventually fix the problem for real at the VM level.

jacob314 commented 5 years ago

Merging hint and fix is reasonable. Having just summary, description, and hint to start works. We can revisit fix once we have some true fixes in the future. If Dart string interpolation was a more powerful feature with features to capture a templated string as something other than just a String I would fully support extracting object references like this but the feature is crippled and so we would have to make breaking changes to toString to make things work and then only for a fraction of objects. For example, Curve isn't currently Diagnosticable although it is on my list to change that. A reasonable middle ground would be to use a kernel transformer to extract object references from string interpolation expressions used for very specific APIs. However, any case where this is useful, the real answer is generally that we should also be calling out the properties with nice tool usable names because most of the visualizations of objects like Curve don't really work that well unless they can be on their own line.

Your example illustrates exactly why in this case breaking out the activeCurve value as a separate property with a good name "Active Curve" vs "activeCurve" is useful. I know how to write a great visualizer for Curve objects that displays what the curve is but making a visualization that will look good in the middle of a line of text is much harder. You could of course only make the value show up on hover of text but sadly few users will take advantage of that compared to actually showing the value by default.

For example (ignore the fact that "Active Curve" is not aligned well:

Curves must map 0.0 to near zero and 1.0 to near one but Curves.easeIn mapped X to Y, which is near Z. Active Curve: image

is great but

Curves must map 0.0 to near zero and 1.0 to near one but image mapped to X to Y, which is near Z.

Is a poor user experience.

For inspectable objects, the interactive displays of them tend to

Dart's string interpolation is not robust enough to be a good fit for automatically embedding

Hixie commented 5 years ago

How about:

Curves must map [0.0]() to near zero and [1.0]() to near one but [Curves.easeIn]() ([shown below]()) mapped [0.0]() to [0.6](), which is near [1.0]().

Curves.easeIn:

image

Each of the links except "shown below" is a link to the debugger to show the variable that was substituted in. "shown below" scrolls to the visualization.

Hixie commented 5 years ago

(In any case, the last part of my last comment supports including the curve as well, with no new APIs.)

InMatrix commented 5 years ago

FlutterErrorBuilder for example ends up having only two methods, addHint and addError.

Dropping "description" will really limit the IDE's ability to direct the user's attention to the most important piece of information in those error messages with substantially more detail than the invalid curve error mentioned above. In addition, we also need to consider the situation where the user knows how to solve the error as soon as they recognize it. We don't want to make the user read more than necessary in this case.

That said, losing the distinction between an error summary and more detail about the error can potentially be remedied by enforcing a writing convention. For example, if we require error authors to include a headline in the beginning of each error and add a blank line below it, then this is less a problem. The IDE can even infer the headline from this convention and style it differently. But it puts burden on code reviewers to remember this guideline and ask people to do it.

Another related factor is that we currently plan to use some part of the error summary as an error's ID for analytics purpose, assuming that error summary is more stable than other details. We might need to revisit that plan if we can't keep the distinction.

Hixie commented 5 years ago

@InMatrix In my proposal we keep the summary and the hint. I can't tell from your comment if you are saying we need those two categories, or if we need three categories. Can you elaborate?

InMatrix commented 5 years ago

I think we need three categories. In addition, I suggest revising their names to hopefully make the distinctions clearer. These three categories could be called summary, detail (previously called description), and resolution (previously hint). Among these three categories, a summary is always required for an error message, while the other two are optional.

Let's consider the possible effects of dropping the detail category in the following example:

missing material error - parts I divided the above message into five parts. Here is how I'd categorize them:

If we drop the category detail, what category would we assign Part 1, 3 and 5 to?

One possibility is to merge them with summary, but then we'll have a giant summary, which defeats its purpose. What's worse is that the actual summary (Part 2) will be buried in the middle of the message with little visual cues to make it stand out.

Alternatively, we could merge Part 1, 3 and 5 with resolution. But the content is hardly suggesting how to get out of this error state. The user is likely to stop reading before they reach the actual resolution (Part 4).

jacob314 commented 5 years ago

How about:

Curves must map 0.0 to near zero and 1.0 to near one but Curves.easeIn (shown below) mapped 0.0 to 0.6, which is near 1.0.

Curves.easeIn:

image

Each of the links except "shown below" is a link to the debugger to show the variable that was substituted in. "shown below" scrolls to the visualization.

That seems like a reasonable option for objects with a a good image representation and a short toString although I'm not sure it as clear as calling out explicitly what the Active curve looks like. However, there is benefit to supporting both and we can add a kernel transformer that will in the future make this case work, although not as well as actually describing what the properties are. I will file a separate bug describing the kernel transformer and associated lint that will let us do that in a robust way as long as we restrict it to just string interpolation templates.

We'd need to be careful that we only display the output like that for cases where the debugger view of the object is significantly different from the toString. For example:

'${context.widget.runtimeType} widgets require a Table widget ancestor.'
TableRowInkWell widgets require a Table widget ancestor.
TableRowInkWell: <TableRowInkWell Type object debugger ref>

would provide a bad user experience.

goderbauer commented 5 years ago

@InMatrix Those three suggested categories sound good to me and seem easy to distinguish.

However, in your example I'd be curious how to apply them. Would we want to mark part 2 both as summary and resolution?

InMatrix commented 5 years ago

@goderbauer Part 2 is the summary of the error. In some cases, the summary implies a resolution, and in those cases it might be Okay that the author doesn't include a resolution explicitly. In the missing material error example, Part 2 might be implying a resolution to more experienced developers who already know different kinds of Material widgets and the meaning of "ancestors" in Flutter. However, less experienced developers might well need the more explicit guidance in Part 4.

PS: I typed "Resolution: Part 2" in my previous comment by mistake. I've corrected it to "Resolution: Part 4."

Hixie commented 5 years ago

@InMatrix For your 5 parts: Part 1 is just saying what was going on when the exception was throw. Part 2 is a brief description of the error. Parts 3 and 4 are an explanation of what the error is, relevant background, etc. Part 5 is detail that might help track down the exact cause.

I understand how to distinguish those four concepts. What I don't understand is how to distinguish parts 3 and 4.

@jacob314 I agree that it's reasonable "for objects with a a good image representation" and that it might not be good in general... but that's the whole point. The IDE has special code for curves, it sees that a curve was mentioned, so it can handle it specially. For a Color it might just inline a color swatch instead, something that would be basically impossible if we inlined the string for the color and then said "Active color" at the end. IMHO therefore, this is an argument more in favour of what I was proposing. As you say, if we make the code decide when to show these messages or not, it means the code now has to know what the IDE does, which isn't a good design in general, but is especially bad since different IDEs may have different behaviours. We just can't know if, for example, TableRowInkWell has a special handling by the IDE.

I don't think "Active curve" is the right label, by the way. We've no idea if this curve is "active". It just happens to be the curve that threw the error.

jacob314 commented 5 years ago

Lets address the details of what cases can and cannot be well expressed with ad-hoc string interpolation vs name-value pairs in a separate issue. I think we are generally on the same page that ad-hoc string interpolation is a great thing to support with a kernel transformer. I was just trying to set practical expectations of how fragile some of the edge cases for it will be and when we will need to encourage users to use the more typical property-value display more common for debugging tools enumerating names and values. I will file a separate bug detailing how the kernel transformer + lint approach would be integrated and lets discuss the various cases there in more detail. I agree that color, icons and a few other cases are clear wins for inline display.

Doubling back to the proposal of

  FlutterError(<DiagnosticsNode>[
    ErrorMessage('Invalid curve endpoint at $t.'),
    ErrorHint('Curves must map 0.0 to near zero and 1.0 to near one but '
              '${activeCurve.runtimeType} mapped $t to $transformedValue, which '
              'is near $roundedTransformedValue.'
    ),
    DiagnosticsProperty<Curve>('Active curve', activeCurve),
  ]);

that would be a breaking change as FlutterError currently takes a String as a parameter. Are you ok with that breaking change or would it need to be a separate named constructor? Making the api just take a list of DiagnosticsNode objects is fine with me and I'm happy to just run with implementing it that way. The new list comprehension features landing do make writing APIs with that structure more convenient.

One question for you, how would you like to see that we avoid having users add a Widget to the error description when an Element is available? Adding the element is typically preferable as there is a 1-1 mapping of an element to a location in the inspector tree. On the other hand, it is less reliable to go from a Widget to a location in the Element tree as the widget may exist multiple times in the tree. The describeError api is intended to guide users down that path. Otherwise a developer that is used to only thinking in terms of the string output would assume a Widget is an equally fine object to include in the output.

InMatrix commented 5 years ago

Parts 3 and 4 are an explanation of what the error is, relevant background, etc. I understand how to distinguish those four concepts. What I don't understand is how to distinguish parts 3 and 4.

Part 3 is explaining what's going on in the program, while Part 4 is talking about the programmer and their potential actions. In addition, two features make Part 4 distinctly different from Part 3. First, it's led by a goal statement: "To introduce a Material Widget..." Second, it talks from the second-person point of view, which is marked by the use of second-person pronoun: "you can either directly include one..."

@Hixie Please let me know if you still have concerns about the distinction. The users in our study took advantage of the visual separation between hint (titled "Potential Fix" in the study) and the rest of the error. I understand it's a separate concern that error authors might not see the distinction. It's Okay that some authors don't initially take advantage of the "hint" category. We or other contributors could easily call out the "hint" part later, as long as there is a way to do so via the API.

jacob314 commented 5 years ago

Looking at the data, the FlutterError class constructor is barely used outside of Package flutter so I actually think we could go with the breaking change of making the one FlutterError constructor take a list of DiagnosticsNode objects without causing much pain at all. I think having FlutterError take a list of DiagnosticsNode objects leaves us with a core API that we will never have to change and can meet all our use cases going forward as DiagnosticsNode objects can support arbitrary structure. I will push on https://github.com/flutter/flutter/issues/27673 to support nice inline display of objects without having to resort to the more cumbersome DiagnosticsProperty syntax unless a truly custom display of a property is really desired.

I'll start working on a branch summarizing just the changes we have consensus on to make sure we are on the same page.

jacob314 commented 5 years ago

I've filed https://github.com/flutter/flutter/issues/27695 to track the related issue of barely used FlutterErrorDetails subclasses that could be eliminated or at least deprecated as part of simplifying to a standard way to expose metadata.

jacob314 commented 5 years ago

Here's a branch summarizing the changes we have consensus on and changes i'd like to get consensus on next so I can start the real refactor avoiding doing throwaway work. https://github.com/flutter/flutter/pull/27791/files

I've added comments to the CL calling out parts that are areas I'd like feedback on to reach consensus.

Hixie commented 5 years ago

Are you ok with that breaking change or would it need to be a separate named constructor?

It's probably ok... Hard to tell how often people use it. Please make sure to go through the breaking change process: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes

(In particular, part of that process is asking the community for feedback on whether they think this will break them.)

how would you like to see that we avoid having users add a Widget to the error description when an Element is available?

We should just put that in the style guide and make sure reviewers are aware of it. Maybe we can add a lint or something.

Part 3 is explaining what's going on in the program, while Part 4 is talking about the programmer and their potential actions.

The following two statements seem identical to me, but one sounds like it is "explaining what's going on in the program" while the other seems to be "talking about the programmer and their potential actions":

The foo constructor requires a bar. When calling the foo constructor, provide a bar.

These statements are in my mind identical in meaning. It seems very strange to me that depending on the mood of the person writing the error message, we might sometimes have something be described as a "detail" and sometimes as a "resolution".

I understand it's a separate concern that error authors might not see the distinction. It's Okay that some authors don't initially take advantage of the "hint" category. We or other contributors could easily call out the "hint" part later, as long as there is a way to do so via the API.

As the person who's probably written most of the error messages we're talking about, not to mention the person who's reviewed the PRs for most of the rest, I care very much that I should understand how to write error messages. I don't understand the distinction described here. As in, I wouldn't be able either write or review code that was required to distinguish between "description" and "hint".

InMatrix commented 5 years ago

The foo constructor requires a bar. When calling the foo constructor, provide a bar.

These two statements are not really identical to me. While the second version tells me what I need to do to get out of the error state, it doesn't tell me the definitive cause of the error. If I were to complete the second version with a cause, I could come up with more than one plausible result:

That said, I do agree that for simple error messages (e.g., the example you gave), a resolution could be implied by the error description, and there is no need to call out a "hint". It's optional, but it's useful when the error message is long and detailed guidance is given to the user to resolve the error. Here is an example from @jacob314's earlier prototype: 

    throw FlutterError.from(FlutterErrorBuilder()
      ..addError('Scaffold.of() called with a context that does not contain a Scaffold.')
      ..addDescription(
        'No Scaffold ancestor could be found starting from the context that was passed to Scaffold.of(). '
      'This usually happens when the context provided is from the same StatefulWidget as that '
      'whose build function actually creates the Scaffold widget being sought.')
      ..addHint(
        'There are several ways to avoid this problem. The simplest is to use a Builder to get a '
        'context that is "under" the Scaffold. For an example of this, please see the '
        'documentation for Scaffold.of()',
        url: 'https://docs.flutter.io/flutter/material/Scaffold/of.html'
      )
      ..addHint('A more efficient solution is to split your build function into several widgets. This '
      'introduces a new context from which you can obtain the Scaffold. In this solution, '
      'you would have an outer widget that creates the Scaffold populated by instances of '
      'your new inner widgets, and then in these inner widgets you would use Scaffold.of().\n'
      'A less elegant but more expedient solution is assign a GlobalKey to the Scaffold, '
      'then use the key.currentState property to obtain the ScaffoldState rather than '
      'using the Scaffold.of() function.')
      ..addSeparator()
      ..addProperty('The context used was', context)

Isn't the distinction between detail and hint clearer in this example? Styling Hint differently can motivate the user to read through the message, instead of scaring them away by showing a large blob of text with unclear value at first sight.

Hixie commented 5 years ago

I agree that in this case there are hints and that they are distinct from the error description.

How about this for a definition:

error message: A short (one line) description of the problem that was detected. Error messages from the same source location should have little variance, so that they can be recognized as related. For example, they shouldn't include hash codes.

description: An explanation of the problem and its cause, any information that may help track down the problem, background information, etc.

hint (optional): Specific, non-obvious advice that may be applicable.

Information that is always true (e.g. "this argument is required" or "you must include this argument") goes in the description. Information that may not be useful in all situations ("Consider using the Foo widget...") goes in a "hint" section.

InMatrix commented 5 years ago

That definition sounds good to me!

jacob314 commented 5 years ago

SGTM. That definition provides a nice crisp separation between what is a hint and what is a description. I'll go ahead and apply it to the existing error messages.

jacob314 commented 5 years ago

Landed the implementation of structured errors in package:flutter.

github-actions[bot] commented 3 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.