Improve error message for type mismatch in Navigator.pop and maybePop.#186571
Improve error message for type mismatch in Navigator.pop and maybePop.#186571puneetkukreja98 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a debug-time type check for Navigator results by adding the _debugCheckCanConsumeResult method to the Route class and integrating it into pop and maybePop. This change aims to provide clearer error messages when a route is popped with a mismatched result type. Feedback suggests using _lastRouteEntryWhereOrNull instead of lastWhere in the pop method to prevent a potential StateError if the navigation history is empty and to ensure consistency with the maybePop implementation.
| assert(() { | ||
| final _RouteEntry entry = _history.lastWhere(_RouteEntry.isPresentPredicate); | ||
| return entry.route._debugCheckCanConsumeResult(result, methodName: 'pop'); | ||
| }()); |
There was a problem hiding this comment.
This assertion block performs a redundant search of the navigation history using _history.lastWhere, which will be repeated in the production code immediately following this block. Additionally, calling lastWhere on an empty history will throw a StateError with a generic 'No element' message. Using the existing _lastRouteEntryWhereOrNull helper would be more robust and consistent with the implementation in maybePop.
assert(() {
final _RouteEntry? entry = _lastRouteEntryWhereOrNull(_RouteEntry.isPresentPredicate);
return entry?.route._debugCheckCanConsumeResult(result, methodName: 'pop') ?? true;
}());There was a problem hiding this comment.
I think I agree with Gemini here.
Updated the error handling in Navigator.pop and maybePop to provide a detailed FlutterError when a result type doesn't match the Route's expected type. This replaces the generic and cryptic TypeError with a clear message including the expected type, actual type, and route details
Added a test case that reproduces the scenario from flutter#16987. Verifies that both pop and maybePop throw the new descriptive FlutterError with the correct diagnostic information.
b68e861 to
762181d
Compare
justinmc
left a comment
There was a problem hiding this comment.
LGTM minus a comment from Gemini. Thanks for making Flutter's error messages better!
| assert(() { | ||
| final _RouteEntry entry = _history.lastWhere(_RouteEntry.isPresentPredicate); | ||
| return entry.route._debugCheckCanConsumeResult(result, methodName: 'pop'); | ||
| }()); |
There was a problem hiding this comment.
I think I agree with Gemini here.
| /// Asserts that the given result is of a type that can be consumed by this route. | ||
| /// | ||
| /// This is used by [Navigator] to provide a clear error message when route is popped with mismatched result type. | ||
| bool _debugCheckCanConsumeResult(dynamic result, {required String methodName}) { |
There was a problem hiding this comment.
Good call putting all of this in a _debug* method.
| /// Asserts that the given result is of a type that can be consumed by this route. | ||
| /// | ||
| /// This is used by [Navigator] to provide a clear error message when route is popped with mismatched result type. | ||
| bool _debugCheckCanConsumeResult(dynamic result, {required String methodName}) { |
There was a problem hiding this comment.
| bool _debugCheckCanConsumeResult(dynamic result, {required String methodName}) { | |
| bool _debugCheckCanConsumeResult(Object? result, {required String methodName}) { |
| 'or when a generic type is explicitly provided to a route creation method ' | ||
| '(such as showDialog<T>()) but the popped value does not match this type.', | ||
| ), | ||
| DiagnosticsProperty<Route<dynamic>>('The route was', this), |
There was a problem hiding this comment.
| DiagnosticsProperty<Route<dynamic>>('The route was', this), | |
| DiagnosticsProperty<Route<Object?>>('The route was', this), |
|
|
||
| testWidgets( | ||
| 'Navigator.pop and maybePop throw Flutter error when popped with mismatched type via showDialog', | ||
| experimentalLeakTesting: LeakTesting.settings.withIgnoredAll(), |
There was a problem hiding this comment.
Why does this need a leak tracking override?
| Object? maybePopException; | ||
|
|
||
| await tester.pumpWidget( | ||
| MaterialApp( |
There was a problem hiding this comment.
Don't use Material impoorts in test/widgets
You can use TestWidgetsApp, TestButton and modal routes directly for this test I think.
| await tester.tap(find.text('NO')); | ||
| expect(popException, isFlutterError); | ||
| final popError = popException! as FlutterError; | ||
| expect( |
There was a problem hiding this comment.
Should this be split into a test for pop and maybePop separately?
Context
Currently, if a developer opens a strongly-typed route (such as
showDialog<Null>) but accidentally pops it with an incompatible value (likeNavigator.pop(context, "CANCEL")). Because the generic types do not match, the Dart runtime instantly throws a cryptic nativeTypeError(e.g.,type 'String' is not a subtype of type 'Null') without any context about which route or widget caused it.Changes
_debugCheckCanConsumeResulthelper toRoute<T>that proactively validatesresult is T?.Navigator.pop,Navigator.maybePop, before they modify the stack or lock the Navigator.FlutterErrorthat pinpoints the exact expected type, the provided type, and the specificRouteobject involved.navigator_test.dartto verify this behavior forpopandmaybePopusing a real-worldshowDialogscenario.Fixes #16987
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.