Refactor core uses of FlutterError. #30983
Conversation
jacob314
force-pushed
the
structured_error_stripped
branch
from
1791396 to
2b717a3
Compare
|
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
force-pushed
the
structured_error_stripped
branch
2 times, most recently
from
ff14f3f to
3439a0a
Compare
pq
left a comment
pq
left a comment
There was a problem hiding this comment.
Couple of nits but looking great. 👍
There was a problem hiding this comment.
I think this line needs updating?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
goderbauer
added
the
framework
jacob314
force-pushed
the
structured_error_stripped
branch
5 times, most recently
from
4bb7820 to
0cfbf7a
Compare
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
Looked at the stuff in foundation. Seems mostly fine with some nits.
There was a problem hiding this comment.
I don't see this _ErrorDiagnostic constructor defined anywhere? Is this supposed to be a placeholder for ErrorSummary, ErrorDetails or ErrorHint?
There was a problem hiding this comment.
Might be worthwhile to point that out or use a concrete example.
There was a problem hiding this comment.
I went ahead and added a default constructor for this class.
There was a problem hiding this comment.
Should this be an abstract class? Or is it intended to be instantiated directly?
There was a problem hiding this comment.
I've marked it as abstract to clarify that.
There was a problem hiding this comment.
Why's this not a limitation for the other Error* classes?
There was a problem hiding this comment.
The limitation should apply to all these classes. Updated the documentation in all 3 similar classes to reflect that.
There was a problem hiding this comment.
not 100% sure: Should there be some kind of punctuation before otherwise?
There was a problem hiding this comment.
nit: missing blank line after "See also:"
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
yes that is a bad name.
some alternate ideas:
- whitespaceNoChildren <-- that is what it is
- errorProperty <-- that is what it is for.
There was a problem hiding this comment.
Fixed this. I've renamed this to errorProperty as that is what it is.
There was a problem hiding this comment.
It's unclear for me what this does based on the description.
There was a problem hiding this comment.
Updated the comment and moved this next to the related afterName property.
There was a problem hiding this comment.
Where are the two spaces coming from? The example below shows everything at the same indentation.
There was a problem hiding this comment.
There is no indentation. Clearly I started with the comment from the whitespace style and failed to update it.
There was a problem hiding this comment.
The comment was wrong. Fixed it. All the example diagrams were carefully constructed but some comments were out of sync with the content.
There was a problem hiding this comment.
Can this be broken up in more than one runaway sentence to make it clearer what this does?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The example below has two lines?
There was a problem hiding this comment.
Done. Updated the description and renamed to
errorProperty
There was a problem hiding this comment.
Updated the description to match the example.
jacob314
force-pushed
the
structured_error_stripped
branch
from
304560a to
a5fd4c6
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm putting together a list of burndown errors to go after after this CL lands. I've added this to the list.
There was a problem hiding this comment.
the context is just telling you where the error occurred, not what the error is (that's error).
There was a problem hiding this comment.
correct. Removed this one from the burndown list.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What would be the ErrorSummary for this error, since it's not specified?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe we should spell out 'NaN' to make the message more beginner friendly.
There was a problem hiding this comment.
Added that to the list of items to cleanup with a followup CL.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Switched to a description. Added to my list of things that could be nice to have a hint.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Downgraded this one as well.
There was a problem hiding this comment.
also noted that it would be nice to dedupe the text.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
updated the doc link. done.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Should this line be the error's summary?
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
I'd consider this part as an ErrorDescription.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed. Made it a description.
There was a problem hiding this comment.
The hint starts from this line. I'd consider the three lines above it a description of the error.
There was a problem hiding this comment.
I'd consider this a description, since it's unclear what the user should try to resolve the error.
There was a problem hiding this comment.
@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.
|
Looks good to me. Thanks! |
jacob314
force-pushed
the
structured_error_stripped
branch
from
ff43c2d to
ceed498
Compare
|
All comments have been addressed. Please take another look. |
There was a problem hiding this comment.
this makes it visible to all dartdocs everywhere. Since the relevant class is private anyway, might be worth making it // docs instead of /// docs.
There was a problem hiding this comment.
Done. Made the problem comment a // comment as it isn't for public docs anyway.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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") : ""}');
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
it's what you have as the description below, "Asynchronous call to guarded function leaked."
There was a problem hiding this comment.
Agreed. That is the summary. Done.
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
LGTM with some minor nits.
There was a problem hiding this comment.
uber-nit: first paragraph should be one-sentence summary.
There was a problem hiding this comment.
is it on purpose that the following is just a regular // comment and not a dart docs /// comment? Why?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you add one line describing why I should look there?
There was a problem hiding this comment.
Added the same explaination to all these cases indicating how the two are related
[DiagnosticsTreeStyle.foo], uses this style for ASCII art display.
|
Uber-nit: Can you give this PR a proper description (see the template we have for PRs now) to give context for anybody looking at this again in the future? |
jacob314
force-pushed
the
structured_error_stripped
branch
from
94e9f71 to
0437f3a
Compare
Change signature of InformationCollector to make it work well with sync*
jacob314
force-pushed
the
structured_error_stripped
branch
from
0437f3a to
1ba17fc
Compare
| /// | ||
| /// This method wraps a sequence of text where only some spans of text can be | ||
| /// used as wrap boundaries. | ||
| static Iterable<String> _wordWrapLine(String message, List<int> wrapRanges, int width, { int startOffset = 0, int otherLineOffset = 0}) sync* { |
There was a problem hiding this comment.
@jacob314 any idea why you copied this from print.dart instead of using the one in print.dart? is it ok if we merge them?
6 participants
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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?