Make _focusDebug not interpolate in debug mode (#119680)

* Make _focusDebug not interpolate in debug mode

* Add test

* Revert undesired change

* Fix test to fail before too

* Remove accidental skips

* Switch to using a generating closure for arguments.

* Remove a word

Author: gspencergoog

packages/flutter/lib/src/widgets/focus_manager.dart

@@ -22,16 +22,38 @@ import 'framework.dart';
 /// be logged.
 bool debugFocusChanges = false;
 
-bool _focusDebug(String message, [Iterable<String>? details]) {
-  if (debugFocusChanges) {
-    debugPrint('FOCUS: $message');
-    if (details != null && details.isNotEmpty) {
-      for (final String detail in details) {
-        debugPrint('    $detail');
-      }
+// When using _focusDebug, always call it like so:
+//
+// assert(_focusDebug(() => 'Blah $foo'));
+//
+// It needs to be inside the assert in order to be removed in release mode, and
+// it needs to use a closure to generate the string in order to avoid string
+// interpolation when debugFocusChanges is false.
+//
+// It will throw a StateError if you try to call it when the app is in release
+// mode.
+bool _focusDebug(
+  String Function() messageFunc, [
+  Iterable<Object> Function()? detailsFunc,
+]) {
+  if (kReleaseMode) {
+    throw StateError(
+      '_focusDebug was called in Release mode. It should always be wrapped in '
+      'an assert. Always call _focusDebug like so:\n'
+      r"  assert(_focusDebug(() => 'Blah $foo'));"
+    );
+  }
+  if (!debugFocusChanges) {
+    return true;
+  }
+  debugPrint('FOCUS: ${messageFunc()}');
+  final Iterable<Object> details = detailsFunc?.call() ?? const <Object>[];
+  if (details.isNotEmpty) {
+    for (final Object detail in details) {
+      debugPrint('    $detail');
     }
   }
-  // Return true so that it can be easily used inside of an assert.
+  // Return true so that it can be used inside of an assert.
   return true;
 }
 
@@ -118,10 +140,10 @@ class _Autofocus {
                            && scope.focusedChild == null
                            && autofocusNode.ancestors.contains(scope);
     if (shouldApply) {
-      assert(_focusDebug('Applying autofocus: $autofocusNode'));
+      assert(_focusDebug(() => 'Applying autofocus: $autofocusNode'));
       autofocusNode._doRequestFocus(findFirstFocus: true);
     } else {
-      assert(_focusDebug('Autofocus request discarded for node: $autofocusNode.'));
+      assert(_focusDebug(() => 'Autofocus request discarded for node: $autofocusNode.'));
     }
   }
 }
@@ -169,7 +191,7 @@ class FocusAttachment {
   ///
   /// Calling [FocusNode.dispose] will also automatically detach the node.
   void detach() {
-    assert(_focusDebug('Detaching node:', <String>[_node.toString(), 'With enclosing scope ${_node.enclosingScope}']));
+    assert(_focusDebug(() => 'Detaching node:', () => <Object>[_node, 'With enclosing scope ${_node.enclosingScope}']));
     if (isAttached) {
       if (_node.hasPrimaryFocus || (_node._manager != null && _node._manager!._markedForFocus == _node)) {
         _node.unfocus(disposition: UnfocusDisposition.previouslyFocusedChild);
@@ -877,7 +899,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
         scope._doRequestFocus(findFirstFocus: true);
         break;
     }
-    assert(_focusDebug('Unfocused node:', <String>['primary focus was $this', 'next focus will be ${_manager?._markedForFocus}']));
+    assert(_focusDebug(() => 'Unfocused node:', () => <Object>['primary focus was $this', 'next focus will be ${_manager?._markedForFocus}']));
   }
 
   /// Removes the keyboard token from this focus node if it has one.
@@ -1063,7 +1085,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
   // Note that this is overridden in FocusScopeNode.
   void _doRequestFocus({required bool findFirstFocus}) {
     if (!canRequestFocus) {
-      assert(_focusDebug('Node NOT requesting focus because canRequestFocus is false: $this'));
+      assert(_focusDebug(() => 'Node NOT requesting focus because canRequestFocus is false: $this'));
       return;
     }
     // If the node isn't part of the tree, then we just defer the focus request
@@ -1078,7 +1100,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
       return;
     }
     _hasKeyboardToken = true;
-    assert(_focusDebug('Node requesting focus: $this'));
+    assert(_focusDebug(() => 'Node requesting focus: $this'));
     _markNextFocus(this);
   }
 
@@ -1109,7 +1131,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
     FocusNode scopeFocus = this;
     for (final FocusScopeNode ancestor in ancestors.whereType<FocusScopeNode>()) {
       assert(scopeFocus != ancestor, 'Somehow made a loop by setting focusedChild to its scope.');
-      assert(_focusDebug('Setting $scopeFocus as focused child for scope:', <String>[ancestor.toString()]));
+      assert(_focusDebug(() => 'Setting $scopeFocus as focused child for scope:', () => <Object>[ancestor]));
       // Remove it anywhere in the focused child history.
       ancestor._focusedChildren.remove(scopeFocus);
       // Add it to the end of the list, which is also the top of the queue: The
@@ -1276,7 +1298,7 @@ class FocusScopeNode extends FocusNode {
   /// tree, the given scope must be a descendant of this scope.
   void setFirstFocus(FocusScopeNode scope) {
     assert(scope != this, 'Unexpected self-reference in setFirstFocus.');
-    assert(_focusDebug('Setting scope as first focus in $this to node:', <String>[scope.toString()]));
+    assert(_focusDebug(() => 'Setting scope as first focus in $this to node:', () => <Object>[scope]));
     if (scope._parent == null) {
       _reparent(scope);
     }
@@ -1306,7 +1328,7 @@ class FocusScopeNode extends FocusNode {
     }
 
     assert(_manager != null);
-    assert(_focusDebug('Autofocus scheduled for $node: scope $this'));
+    assert(_focusDebug(() => 'Autofocus scheduled for $node: scope $this'));
     _manager?._pendingAutofocuses.add(_Autofocus(scope: this, autofocusNode: node));
     _manager?._markNeedsUpdate();
   }
@@ -1542,7 +1564,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
   void _markDetached(FocusNode node) {
     // The node has been removed from the tree, so it no longer needs to be
     // notified of changes.
-    assert(_focusDebug('Node was detached: $node'));
+    assert(_focusDebug(() => 'Node was detached: $node'));
     if (_primaryFocus == node) {
       _primaryFocus = null;
     }
@@ -1551,7 +1573,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
 
   void _markPropertiesChanged(FocusNode node) {
     _markNeedsUpdate();
-    assert(_focusDebug('Properties changed for node $node.'));
+    assert(_focusDebug(() => 'Properties changed for node $node.'));
     _dirtyNodes.add(node);
   }
 
@@ -1575,7 +1597,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
   // Request that an update be scheduled, optionally requesting focus for the
   // given newFocus node.
   void _markNeedsUpdate() {
-    assert(_focusDebug('Scheduling update, current focus is $_primaryFocus, next focus will be $_markedForFocus'));
+    assert(_focusDebug(() => 'Scheduling update, current focus is $_primaryFocus, next focus will be $_markedForFocus'));
     if (_haveScheduledUpdate) {
       return;
     }
@@ -1597,7 +1619,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
       // then revert to the root scope.
       _markedForFocus = rootScope;
     }
-    assert(_focusDebug('Refreshing focus state. Next focus will be $_markedForFocus'));
+    assert(_focusDebug(() => 'Refreshing focus state. Next focus will be $_markedForFocus'));
     // A node has requested to be the next focus, and isn't already the primary
     // focus.
     if (_markedForFocus != null && _markedForFocus != _primaryFocus) {
@@ -1613,7 +1635,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
     }
     assert(_markedForFocus == null);
     if (previousFocus != _primaryFocus) {
-      assert(_focusDebug('Updating focus from $previousFocus to $_primaryFocus'));
+      assert(_focusDebug(() => 'Updating focus from $previousFocus to $_primaryFocus'));
       if (previousFocus != null) {
         _dirtyNodes.add(previousFocus);
       }
@@ -1624,7 +1646,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
     for (final FocusNode node in _dirtyNodes) {
       node._notify();
     }
-    assert(_focusDebug('Notified ${_dirtyNodes.length} dirty nodes:', _dirtyNodes.toList().map<String>((FocusNode node) => node.toString())));
+    assert(_focusDebug(() => 'Notified ${_dirtyNodes.length} dirty nodes:', () => _dirtyNodes));
     _dirtyNodes.clear();
     if (previousFocus != _primaryFocus) {
       notifyListeners();
@@ -1766,9 +1788,9 @@ class _HighlightModeManager {
     _lastInteractionWasTouch = false;
     updateMode();
 
-    assert(_focusDebug('Received key event $message'));
+    assert(_focusDebug(() => 'Received key event $message'));
     if (FocusManager.instance.primaryFocus == null) {
-      assert(_focusDebug('No primary focus for key event, ignored: $message'));
+      assert(_focusDebug(() => 'No primary focus for key event, ignored: $message'));
       return false;
     }
 
@@ -1794,11 +1816,11 @@ class _HighlightModeManager {
         case KeyEventResult.ignored:
           continue;
         case KeyEventResult.handled:
-          assert(_focusDebug('Node $node handled key event $message.'));
+          assert(_focusDebug(() => 'Node $node handled key event $message.'));
           handled = true;
           break;
         case KeyEventResult.skipRemainingHandlers:
-          assert(_focusDebug('Node $node stopped key event propagation: $message.'));
+          assert(_focusDebug(() => 'Node $node stopped key event propagation: $message.'));
           handled = false;
           break;
       }
@@ -1808,7 +1830,7 @@ class _HighlightModeManager {
       break;
     }
     if (!handled) {
-      assert(_focusDebug('Key event not handled by anyone: $message.'));
+      assert(_focusDebug(() => 'Key event not handled by anyone: $message.'));
     }
     return handled;
   }

packages/flutter/test/widgets/focus_manager_test.dart

@@ -1700,4 +1700,61 @@ void main() {
     expect(messagesStr, contains('FOCUS: Notified 2 dirty nodes'));
     expect(messagesStr, contains(RegExp(r'FOCUS: Scheduling update, current focus is null, next focus will be FocusScopeNode#.*parent1')));
   });
+
+  testWidgets("doesn't call toString on a focus node when debugFocusChanges is false", (WidgetTester tester) async {
+    final bool oldDebugFocusChanges = debugFocusChanges;
+    final DebugPrintCallback oldDebugPrint = debugPrint;
+    final StringBuffer messages = StringBuffer();
+    debugPrint = (String? message, {int? wrapWidth}) {
+      messages.writeln(message ?? '');
+    };
+    Future<void> testDebugFocusChanges() async {
+      final BuildContext context = await setupWidget(tester);
+      final FocusScopeNode parent1 = FocusScopeNode(debugLabel: 'parent1');
+      final FocusAttachment parent1Attachment = parent1.attach(context);
+      final FocusNode child1 = debugFocusChanges ? FocusNode(debugLabel: 'child1') : _LoggingTestFocusNode(debugLabel: 'child1');
+      final FocusAttachment child1Attachment = child1.attach(context);
+      parent1Attachment.reparent(parent: tester.binding.focusManager.rootScope);
+      child1Attachment.reparent(parent: parent1);
+
+      child1.requestFocus();
+      await tester.pump();
+      child1.dispose();
+      parent1.dispose();
+      await tester.pump();
+    }
+    try {
+      debugFocusChanges = false;
+      await testDebugFocusChanges();
+      expect(messages, isEmpty);
+      expect(tester.takeException(), isNull);
+      debugFocusChanges = true;
+      await testDebugFocusChanges();
+      expect(messages.toString(), contains('FOCUS: Notified 3 dirty nodes:'));
+      expect(tester.takeException(), isNull);
+    } finally {
+      debugFocusChanges = oldDebugFocusChanges;
+      debugPrint = oldDebugPrint;
+    }
+  });
+}
+
+class _LoggingTestFocusNode extends FocusNode {
+  _LoggingTestFocusNode({super.debugLabel});
+
+  @override
+  String toString({
+    DiagnosticLevel minLevel = DiagnosticLevel.debug,
+  }) {
+    throw StateError("Shouldn't call toString here");
+  }
+
+  @override
+  String toStringDeep({
+    String prefixLineOne = '',
+    String? prefixOtherLines,
+    DiagnosticLevel minLevel = DiagnosticLevel.debug,
+  }) {
+    throw StateError("Shouldn't call toStringDeep here");
+  }
 }