Fix bug handling cyclic diagnostics. #31960
Conversation
| @@ -2486,7 +2486,7 @@ class DiagnosticsProperty<T> extends DiagnosticsNode { | |||
| json['exception'] = exception.toString(); | |||
| json['propertyType'] = propertyType.toString(); | |||
| json['defaultLevel'] = describeEnum(_defaultLevel); | |||
| if (T is Diagnosticable || T is DiagnosticsNode) | |||
| if (value is Diagnosticable || value is DiagnosticsNode) | |||
There was a problem hiding this comment.
Using the type of the value is more useful in this case as you can't expand a null value anyway. My test was failing until I fixed this somewhat minor bug.
| @@ -1487,17 +1487,32 @@ mixin WidgetInspectorService { | |||
| } | |||
|
|
|||
| if (config.includeProperties) { | |||
| List<DiagnosticsNode> properties = node.getProperties(); | |||
| if (properties.isEmpty && value is Diagnosticable) { | |||
There was a problem hiding this comment.
this was the problem case. The fix could have been just to remove these 4 lines which I added in error. They are not safe as cycles between property values and their properties could lead to a stack overflow.
Inspecting this code I also noticed the previous code could sometimes override the json['properties'] field if a DiagnosticsProperty subclass returned properties as well has having a value that was Diagnosticable so I cleaned that case up.
| @@ -1515,7 +1583,7 @@ class TestWidgetInspectorService extends Object with WidgetInspectorService { | |||
| _CreationLocation location = knownLocations[id]; | |||
| expect(location.file, equals(file)); | |||
| // ClockText widget. | |||
| expect(location.line, equals(50)); | |||
| expect(location.line, equals(51)); | |||
There was a problem hiding this comment.
I had to update these line #s due to the added import at the top of the file.
Description
Cases where there was a cycle involving diagnostics referenced in a property and their properties could cause some InspectorService methods to stack overflow.
Related Issues
This pull request fixes
#31959
Tests
I added the following tests:
I added a test that reproduces the cyclic diagnostic without having to depend on the specific Diagnosticables that currently have cyclic dependencies and then make the test pass by fixing the bug.
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?