Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor core uses of FlutterError. #30983

Merged
merged 5 commits into from May 1, 2019

Conversation

@jacob314
Copy link
Contributor

@jacob314 jacob314 commented Apr 12, 2019

Description

Make FlutterError objects more structured so they can be displayed better in debugging tools such as Dart DevTools.

Related Issues

#27327

Tests

Most tests pass as is as the text output does not change as the refactor does not break existing text based error displays.

New test cases are added in flutter_test to verify the structure is maintained and new tests are added to diagnostics_test.dart to verify that the additional text formatting logic works as expected.
Other test cases are updated for cases where the output changed in way that is minor and either completely neutral or positive.

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?

@jacob314 jacob314 force-pushed the structured_error_stripped branch from 1791396 to 2b717a3 Apr 12, 2019
@jacob314 jacob314 requested review from goderbauer and pq Apr 12, 2019
@jacob314
Copy link
Contributor Author

@jacob314 jacob314 commented Apr 12, 2019

This CL has only the core changes for the FlutterError refactor. Follow up CLs apply the refactor to more places. The diff is still large but take heart that a lot of the additions are just the text rendering logic and golden style tests in diagnosticable.dart.

Two follow up CLs will apply the refactor to more places.

@jacob314 jacob314 force-pushed the structured_error_stripped branch 2 times, most recently from ff14f3f to 3439a0a Apr 13, 2019
pq
pq approved these changes Apr 14, 2019
Copy link
Contributor

@pq pq left a comment

Couple of nits but looking great. 👍

/// include hash codes.
///
/// A [FlutterError] must start with an [ErrorSummary] and may not contain
/// multiple summaries.
Copy link
Contributor

@pq pq Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line needs updating?

Copy link
Contributor Author

@jacob314 jacob314 Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry can you clarify what about this line should be updated? It is still technically correct that every FlutterError starts with an ErrorSummary although sometimes the ErrorSummary is inferred parsing the text of the message.

Copy link
Contributor

@pq pq Apr 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wasn't clear. I just wondered if you wanted to remove the reference to the ErrorSummary type in favor of a string since that's what folks will be passing in. A not really so no worries either way!

@@ -1563,7 +1628,7 @@ mixin WidgetInspectorService {

List<Object> _getProperties(String diagnosticsNodeId, String groupName) {
final DiagnosticsNode node = toObject(diagnosticsNodeId);
return _nodesToJson(node == null ? const <DiagnosticsNode>[] : node.getProperties(), _SerializeConfig(groupName: groupName));
return _nodesToJson(node == null ? const <DiagnosticsNode>[] : node.getProperties(), _SerializeConfig(groupName: groupName), parent: null);
Copy link
Contributor

@pq pq Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should parent be node?

Copy link
Contributor Author

@jacob314 jacob314 Apr 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jacob314 jacob314 force-pushed the structured_error_stripped branch 5 times, most recently from 4bb7820 to 0cfbf7a Apr 17, 2019
Copy link
Member

@goderbauer goderbauer left a comment

Looked at the stuff in foundation. Seems mostly fine with some nits.

///
/// {@tool sample}
/// ```dart
/// _ErrorDiagnostic('Element $element must be $color')
Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this _ErrorDiagnostic constructor defined anywhere? Is this supposed to be a placeholder for ErrorSummary, ErrorDetails or ErrorHint?

Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worthwhile to point that out or use a concrete example.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and added a default constructor for this class.

/// and other callbacks that collect information describing an error.
typedef InformationCollector = Iterable<DiagnosticsNode> Function();

class _ErrorDiagnostic extends DiagnosticsProperty<List<Object>> {
Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an abstract class? Or is it intended to be instantiated directly?

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've marked it as abstract to clarify that.

/// information, etc.
/// * [FlutterError], which is the most common place to use an [ErrorHint].
class ErrorHint extends _ErrorDiagnostic {
/// A lint enforces that this constructor can only be called with a string
Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why's this not a limitation for the other Error* classes?

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The limitation should apply to all these classes. Updated the documentation in all 3 similar classes to reflect that.


/// Returns a short (one line) description of the problem that was detected.
///
/// If the exception contains an [ErrorSummary] that summary is used otherwise
Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not 100% sure: Should there be some kind of punctuation before otherwise?

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a comma

/// Render the tree on a single line without showing children.
///
/// See also:
/// * [FlutterErrorDetails], which uses this style to display the header
Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing blank line after "See also:"

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

/// describing the context where an error occurred.
headerLine,

/// Render the tree on a single line with the name and value on separate
Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems oxymoronic. How can it be a single line if name and value are presented on separate lines, which makes it at least two lines?

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that is a bad name.
some alternate ideas:

  • whitespaceNoChildren <-- that is what it is
  • errorProperty <-- that is what it is for.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this. I've renamed this to errorProperty as that is what it is.

@@ -178,6 +235,13 @@ class TextTreeConfiguration {
/// Prefix to add to the first line to display a child with this style.
final String prefixLineOne;

/// Similar to prefixLineOne but applies even when this is the root node in
Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear for me what this does based on the description.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment and moved this next to the related afterName property.

@@ -495,6 +638,41 @@ final TextTreeConfiguration whitespaceTextConfiguration = TextTreeConfiguration(
isBlankLineBetweenPropertiesAndChildren: false,
);

/// Whitespace only configuration where children are consistently indented
Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are the two spaces coming from? The example below shows everything at the same indentation.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no indentation. Clearly I started with the comment from the whitespace style and failed to update it.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment was wrong. Fixed it. All the example diagrams were carefully constructed but some comments were out of sync with the content.

prefixOtherLinesRootNode: '',
);

/// Render a node as a single line omitting children styling the node like it is
Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be broken up in more than one runaway sentence to make it clearer what this does?

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've instead eliminated this style as the cases it was helping with are now handled by ErrorDescription instead which allows concatenating together strings and values to look like a header.

propertyPrefixIfChildren: '',
propertyPrefixNoChildren: '',
linkCharacter: '',
prefixOtherLinesRootNode: '',
);
/// Render a node as a single line omitting children.
Copy link
Member

@goderbauer goderbauer Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example below has two lines?

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Updated the description and renamed to
errorProperty

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the description to match the example.

@jacob314 jacob314 force-pushed the structured_error_stripped branch from 304560a to a5fd4c6 Apr 18, 2019
@@ -16,7 +16,7 @@ void main() {
handleUncaughtError:(Zone zone, ZoneDelegate delegate, Zone parent, Object error, StackTrace stackTrace) {
FlutterError.reportError(FlutterErrorDetails(
exception: error,
context: 'In the Zone handleUncaughtError handler',
context: ErrorDescription('In the Zone handleUncaughtError handler'),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this message is saying, but since it's the same as it was before. Let's land it for now and revise later.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm putting together a list of burndown errors to go after after this CL lands. I've added this to the list.

Copy link
Member

@Hixie Hixie Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the context is just telling you where the error occurred, not what the error is (that's error).

Copy link
Contributor Author

@jacob314 jacob314 Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct. Removed this one from the burndown list.

/// An [ErrorHint] provides specific, non-obvious advice that may be applicable.
///
/// If your message provides obvious advice that is always applicable it is an
/// [ErrorDescription] not a hint.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the advice is obvious from the summary/description of the error, it probably should not be included in the message? I feel this line is a bit extranous.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like having the advice duplicated here as a reminder to only use hints for things that meet the fairly narrow charter of what a hint is.

'The $runtimeType sending notification was',
this,
style: DiagnosticsTreeStyle.errorProperty,
);
Copy link

@InMatrix InMatrix Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the ErrorSummary for this error, since it's not specified?

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception is the piece that is expected to contain an ErrorSummary.
If the exception is a FlutterError we have assertions if it doesn't have an ErrorSummary.
We could be smart and if the exception is just a String, treat that String as an ErrorSummary.

@@ -524,27 +527,27 @@ class BoxConstraints extends Constraints {
} else {
whichFields = affectedFieldsList.single;
}
throwError('BoxConstraints has ${affectedFieldsList.length == 1 ? 'a NaN value' : 'NaN values' } in $whichFields.');
throwError(ErrorSummary('BoxConstraints has ${affectedFieldsList.length == 1 ? 'a NaN value' : 'NaN values' } in $whichFields.'));
Copy link

@InMatrix InMatrix Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should spell out 'NaN' to make the message more beginner friendly.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added that to the list of items to cleanup with a followup CL.

if (owner != null && owner.debugDoingLayout)
hint = 'Only the object itself can set its size. It is a contract violation for other objects to set it.';
information.add(ErrorHint('Only the object itself can set its size. It is a contract violation for other objects to set it.'));
Copy link

@InMatrix InMatrix Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one looks more like a description. It backs up the previous statement. A hint should suggest a specific way to get out of the error state.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I was on the fence about that one. I only had it as a "hint" as the variable name was called "hint" in the previous code.
While auditing this myself I set this as a ErrorDescription and then moved it back to a hint.

ErrorHint(
'This probably means that it is a render object that tries to be '
'as big as possible, but it was put inside another render object '
'that allows its children to pick their own size.'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an ErrorDescription, too. To add a hint, what advice could we give someone who runs into this error? Would something like "try giving a finite size to the render object [link to documentation]" be useful as a hint here?

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to a description. Added to my list of things that could be nice to have a hint.

'$runtimeType object was given an infinite size during layout.\n'
final List<DiagnosticsNode> errorParts = <DiagnosticsNode>[];
errorParts.add(ErrorSummary('$runtimeType object was given an infinite size during layout.'));
errorParts.add(ErrorHint(
'This probably means that it is a render object that tries to be '
'as big as possible, but it was put inside another render object '
Copy link

@InMatrix InMatrix Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this message is the same as the one in L1732. I'm not sure if this is a problem or not. Please see my comment there about whether this qualifies as a hint.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Downgraded this one as well.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also noted that it would be nice to dedupe the text.

errorParts.addAll(information);
errorParts.add(DiagnosticsProperty<BoxConstraints>('The constraints that applied to the $runtimeType were', constraints, style: DiagnosticsTreeStyle.errorProperty));
errorParts.add(DiagnosticsProperty<Size>('The exact size it was given was', _size, style: DiagnosticsTreeStyle.errorProperty));
errorParts.add(ErrorHint('See https://flutter.io/layout/ for more information.'));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very clearly a hint, though probably not specific enough. The new canonical URL for this doc is https://flutter.dev/docs/development/ui/layout/box-constraints.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the doc link. done.

'your fault. Contact support: https://github.com/flutter/flutter/issues/new?template=BUG.md'
);
throw FlutterError.fromParts(<DiagnosticsNode>[
ErrorSummary('$runtimeType does not meet its constraints.'),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A side question: will the user be able to locate this particular widget in their code, especially when there are many instances of this type?

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They will be as soon as the kernel transformer for ErrorSummary is enabled, click on the type name in the UI will lead you to the RenderObject matching the type in the inspector.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

@@ -232,18 +239,22 @@ mixin DebugOverflowIndicatorMixin on RenderObject {
overflows[overflows.length - 1] = 'and ${overflows[overflows.length - 1]}';
overflowText = overflows.join(', ');
}
// TODO(jacobr): add the overflows in pixels as structured data so they can
// be visualized in debugging tools.
FlutterError.reportError(
FlutterErrorDetailsForRendering(
exception: 'A $runtimeType overflowed by $overflowText.',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this line be the error's summary?

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. We shouldn't have exceptions that are strings. @pq we should probably add a lint to catch this case although it is low priority because the good news is this is the only place in the framework where that mistake was made.
Screen Shot 2019-04-18 at 4 17 38 PM
Fixed this to construct a proper FlutterError object with a summary. The reason for this bug is the Dart type system lacks a single base class for all exceptions.

'The edge of the $runtimeType that is '
'overflowing has been marked in the rendering with a yellow and black '
'striped pattern. This is usually caused by the contents being too big '
'for the $runtimeType.'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider this part as an ErrorDescription.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

'is content that cannot be seen. If the content is legitimately bigger '
'than the available space, consider clipping it with a ClipRect widget '
'before putting it in the $runtimeType, or using a scrollable '
'container, like a ListView.'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this part is a hint.

..add(ErrorHint(
'If none of the above helps enough to fix this problem, please don\'t hesitate to file a bug:\n'
' https://github.com/flutter/flutter/issues/new?template=BUG.md'
)));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This last part is neither a hint nor a description, following our definitions. I suggest we treat this as a description for now to avoid drawing undeserving attention to it. We should probably track how often this error reporting link is collected, maybe we can set up a URL redirect via flutter.dev.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Made it a description.

'The edge of the $runtimeType that is overflowing has been marked '
'in the rendering with a yellow and black striped pattern. This is '
'usually caused by the contents being too big for the $runtimeType. '
'Consider applying a flex factor (e.g. using an Expanded widget) to '

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hint starts from this line. I'd consider the three lines above it a description of the error.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (owner != null && owner.debugDoingLayout)
hint = 'Only the object itself can set its geometry. It is a contract violation for other objects to set it.';
hint = ErrorHint('Only the object itself can set its geometry. It is a contract violation for other objects to set it.');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider this a description, since it's unclear what the user should try to resolve the error.

Copy link
Contributor Author

@jacob314 jacob314 Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hixie it looks like you wrote the line that originally gave this the variable name hint.
Do you think it should be called a hint with the current definition of what a hint is?

To make it clearer what action to take we could write something a bit redundant like.
Look for where the geometry is being set outside outside the object refactor the logic to set the geometry from inside the object.
For now set this as an ErrorDescription as
hint = ErrorDescription(
is a great breadcrumb to later go back and cleanup ErrorDescription objects.

if (collidingGuarder.className == null && collidingGuarder.methodName == 'expect') {
message.writeln(
information.add(ErrorHint(
'If you are confident that all test APIs are being called using "await", and '
'this expect() call is not being called at the top level but is itself being '
'called from some sort of callback registered before the ${originalGuarder.methodName} '
'method was called, then consider using expectSync() instead.'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is a hint.

@InMatrix
Copy link

@InMatrix InMatrix commented Apr 19, 2019

Looks good to me. Thanks!

@jacob314 jacob314 force-pushed the structured_error_stripped branch from ff43c2d to ceed498 Apr 19, 2019
@jacob314
Copy link
Contributor Author

@jacob314 jacob314 commented Apr 19, 2019

All comments have been addressed. Please take another look.
Thanks!

// Examples can assume:
// dynamic element;
// dynamic color;
// dynamic _ErrorDiagnostic;
Copy link
Member

@Hixie Hixie Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes it visible to all dartdocs everywhere. Since the relevant class is private anyway, might be worth making it // docs instead of /// docs.

Copy link
Contributor Author

@jacob314 jacob314 Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Made the problem comment a // comment as it isn't for public docs anyway.

@@ -64,7 +234,7 @@ class FlutterErrorDetails {
/// following the word "thrown", as in "thrown while obtaining the image from
/// the network" (for the context "while obtaining the image from the
/// network").
final String context;
final DiagnosticsNode context;
Copy link
Member

@Hixie Hixie Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one surprises me. The context was intended to just be a string that gets interpolated into another; does it still work well if it's a DiagnosticsNode?

Copy link
Contributor Author

@jacob314 jacob314 Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. DiagnosticsNode in general and ErrorDescription in particular works great with String interpolation. As long as you wrap each call to string interpolation with an ErrorDescription you will maintain all the nested object references so that they are still clickable in the inspector regardless of how much nested interpolation you did.
Basically you can think of an ErrorDescription as just a String with object references captured for purposes of String interpolation.

Examples of use cases this supports:

          context: ErrorDescription('during a service extension callback for "$method"'),
          context: ErrorDescription('while notifying listeners for $runtimeType'),

Advanced case in debugFillProperties for FlutterErrorDetails where we use this

    final DiagnosticsNode verb = ErrorDescription('thrown${ context != null ? ErrorDescription(" $context") : ""}');

Copy link
Contributor Author

@jacob314 jacob314 Apr 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hixie does this address your concerns about context being a DiagnosticsNode? Put more concisely: this allows the same interpolation use case and if you use ErrorDescription in every level of interpolation you don't lose the object references.

Copy link
Member

@Hixie Hixie Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sounds good.

while (_scopeStack.isNotEmpty) {
closedScope = _scopeStack.removeLast();
if (closedScope == scope)
break;
// TODO(jacobr): what is the ErrorSummary for this error?
Copy link
Member

@Hixie Hixie Apr 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's what you have as the description below, "Asynchronous call to guarded function leaked."

Copy link
Contributor Author

@jacob314 jacob314 Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. That is the summary. Done.

Copy link
Member

@goderbauer goderbauer left a comment

LGTM with some minor nits.

@@ -573,7 +573,7 @@ abstract class Constraints {
/// The `informationCollector` argument takes an optional callback which is
/// called when an exception is to be thrown. The collected information is
/// then included in the message after the error line.
///
//
Copy link
Member

@goderbauer goderbauer Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: make this /// again?

Copy link
Contributor Author

@jacob314 jacob314 May 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

List<DiagnosticsNode> describeMissingAncestor({ @required Type expectedAncestorType });

/// Adds a description of the ownership chain from a specific [Element]
/// to the error report. It's useful for debugging the source of an element.
Copy link
Member

@goderbauer goderbauer Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uber-nit: first paragraph should be one-sentence summary.

Copy link
Contributor Author

@jacob314 jacob314 May 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

/// In debug builds, a kernel transformer rewrites calls to the default
/// constructors for [ErrorSummary], [ErrorDetails], and [ErrorHint] to use
/// this constructor.
//
Copy link
Member

@goderbauer goderbauer Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it on purpose that the following is just a regular // comment and not a dart docs /// comment? Why?

Copy link
Contributor Author

@jacob314 jacob314 May 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is we don't want this in the official docs and are worried about someone copying and pasting it so it would show up in the officel docs. This should not be in the public docs because it references privates. There is a discussion of this issue with Ian.

/// ════════════════════════════════════════════════════════════════
/// ```
///
/// /// See also:
Copy link
Member

@goderbauer goderbauer Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove the extra ///?

Copy link
Contributor Author

@jacob314 jacob314 May 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.