Skip to content

Improve error message for type mismatch in Navigator.pop and maybePop. #186571

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.

Sign up for GitHub

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

Open
puneetkukreja98 wants to merge 2 commits into
base: master
Choose a base branch
from
+126 −1
Open

Improve error message for type mismatch in Navigator.pop and maybePop. #186571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef714c4
feat: Improve Navigator pop type mismatch error messages
puneetkukreja98 May 14, 2026
762181d
test: Add test for Navigator pop type mismatches
puneetkukreja98 May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion packages/flutter/lib/src/widgets/navigator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,28 @@ abstract class Route<T> extends _RoutePlaceholder {
return _navigator?._firstRouteEntryWhereOrNull(_RouteEntry.isRoutePredicate(this))?.isPresent ??
false;
}

/// 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}) {
Copy link
Copy Markdown
Contributor

@justinmc justinmc May 15, 2026

Choose a reason for hiding this comment

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

Good call putting all of this in a _debug* method.

Copy link
Copy Markdown
Contributor

@navaronbracke navaronbracke May 15, 2026

Choose a reason for hiding this comment

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

Suggested change
bool _debugCheckCanConsumeResult(dynamic result, {required String methodName}) {
bool _debugCheckCanConsumeResult(Object? result, {required String methodName}) {

if (result is! T?) {
throw FlutterError.fromParts(<DiagnosticsNode>[
ErrorSummary(
'A request was made to pop a route with a result of type ${result.runtimeType}, but the route expected a value of type $T.',
),
ErrorDescription(
'This usually happens when the type provided to Navigator.$methodName() '
'is not a subtype of the type expected by the Route (e.g. DialogRoute<Null>), '
'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),
Copy link
Copy Markdown
Contributor

@navaronbracke navaronbracke May 15, 2026

Choose a reason for hiding this comment

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

Suggested change
DiagnosticsProperty<Route<dynamic>>('The route was', this),
DiagnosticsProperty<Route<Object?>>('The route was', this),

DiagnosticsProperty<Object?>('The provided result was', result),
]);
}
return true;
}
}

/// Data that might be useful in constructing a [Route].
Expand Down Expand Up @@ -5565,7 +5587,7 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
return false;
}
assert(lastEntry.route._isInstalledIn(this));

assert(lastEntry.route._debugCheckCanConsumeResult(result, methodName: 'maybePop'));
// TODO(justinmc): When the deprecated willPop method is removed, delete
// this code and use only popDisposition, below.
if (await lastEntry.route.willPop() == RoutePopDisposition.doNotPop) {
Expand Down Expand Up @@ -5621,6 +5643,10 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin, Res
@optionalTypeArgs
void pop<T extends Object?>([T? result]) {
assert(!_debugLocked);
assert(() {
final _RouteEntry entry = _history.lastWhere(_RouteEntry.isPresentPredicate);
return entry.route._debugCheckCanConsumeResult(result, methodName: 'pop');
}());
Comment on lines +5646 to +5649
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 15, 2026

Choose a reason for hiding this comment

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

medium

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;
    }());

Copy link
Copy Markdown
Contributor

@justinmc justinmc May 15, 2026

Choose a reason for hiding this comment

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

I think I agree with Gemini here.

assert(() {
_debugLocked = true;
return true;
Expand Down
99 changes: 99 additions & 0 deletions packages/flutter/test/widgets/navigator_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6342,6 +6342,105 @@ void main() {
},
variant: TargetPlatformVariant.only(TargetPlatform.iOS),
);

testWidgets(
'Navigator.pop and maybePop throw Flutter error when popped with mismatched type via showDialog',
experimentalLeakTesting: LeakTesting.settings.withIgnoredAll(),
Copy link
Copy Markdown
Contributor

@navaronbracke navaronbracke May 15, 2026

Choose a reason for hiding this comment

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

Why does this need a leak tracking override?

(WidgetTester tester) async {
Object? popException;
Object? maybePopException;

await tester.pumpWidget(
MaterialApp(
Copy link
Copy Markdown
Contributor

@navaronbracke navaronbracke May 15, 2026

Choose a reason for hiding this comment

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

Don't use Material impoorts in test/widgets

You can use TestWidgetsApp, TestButton and modal routes directly for this test I think.

home: Scaffold(
body: Builder(
builder: (BuildContext context) {
return ElevatedButton(
onPressed: () {
showDialog<bool>(
context: context,
builder: (BuildContext context) {
return AlertDialog(
actions: <Widget>[
TextButton(
onPressed: () {
try {
Navigator.pop(context, 'NO');
} catch (e) {
popException = e;
}
},
child: const Text('NO'),
),
TextButton(
onPressed: () {
Navigator.maybePop(context, 'YES').catchError((Object e) {
maybePopException = e;
return false;
});
},
child: const Text('YES'),
),
],
);
},
);
},
child: const Text('Open Dialog'),
);
},
),
),
),
);
await tester.tap(find.text('Open Dialog'));
await tester.pumpAndSettle();
await tester.tap(find.text('NO'));
expect(popException, isFlutterError);
final popError = popException! as FlutterError;
expect(
Copy link
Copy Markdown
Contributor

@navaronbracke navaronbracke May 15, 2026

Choose a reason for hiding this comment

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

Should this be split into a test for pop and maybePop separately?

popError.toStringDeep(),
equalsIgnoringHashCodes(
'FlutterError\n'
' A request was made to pop a route with a result of type String,\n'
' but the route expected a value of type bool.\n'
' This usually happens when the type provided to Navigator.pop() is\n'
' not a subtype of the type expected by the Route (e.g.\n'
' DialogRoute<Null>), or when a generic type is explicitly provided\n'
' to a route creation method (such as showDialog<T>()) but the\n'
' popped value does not match this type.\n'
' The route was: DialogRoute<bool>(RouteSettings(none, null),\n'
' animation: AnimationController#00000(⏭ 1.000; paused; for\n'
' DialogRoute<bool>))\n'
' The provided result was: NO\n'
'',
),
);

await tester.tap(find.text('YES'));
await tester.pumpAndSettle();
expect(maybePopException, isFlutterError);
final maybePopError = maybePopException! as FlutterError;
expect(
maybePopError.toStringDeep(),
equalsIgnoringHashCodes(
'FlutterError\n'
' A request was made to pop a route with a result of type String,\n'
' but the route expected a value of type bool.\n'
' This usually happens when the type provided to\n'
' Navigator.maybePop() is not a subtype of the type expected by the\n'
' Route (e.g. DialogRoute<Null>), or when a generic type is\n'
' explicitly provided to a route creation method (such as\n'
' showDialog<T>()) but the popped value does not match this type.\n'
' The route was: DialogRoute<bool>(RouteSettings(none, null),\n'
' animation: AnimationController#00000(⏭ 1.000; paused; for\n'
' DialogRoute<bool>))\n'
' The provided result was: YES\n'
'',
),
);
},
);
}

typedef AnnouncementCallBack = void Function(Route<dynamic>?);
Expand Down
Loading