From 299d484903c05182f28bccd6126c0f7384c49635 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Thu, 14 Oct 2021 22:03:03 -0700 Subject: [PATCH] Enable more lints (#91642) --- analysis_options.yaml | 20 +++++++++--------- dev/analysis_options.yaml | 9 ++++++-- dev/bots/analysis_options.yaml | 5 +++++ examples/layers/services/isolate.dart | 2 +- .../lib/src/foundation/_bitfield_io.dart | 2 +- .../flutter/lib/src/foundation/binding.dart | 21 +++++++------------ .../lib/src/foundation/diagnostics.dart | 2 ++ .../flutter/lib/src/rendering/editable.dart | 1 + packages/flutter/lib/src/rendering/error.dart | 10 ++++++--- .../rendering/sliver_persistent_header.dart | 1 + .../flutter/lib/src/services/binding.dart | 2 +- .../lib/src/services/message_codecs.dart | 2 +- .../lib/src/services/platform_channel.dart | 4 ++-- .../flutter/lib/src/services/restoration.dart | 4 +++- packages/flutter/lib/src/widgets/actions.dart | 1 + .../lib/src/widgets/editable_text.dart | 1 + packages/flutter/lib/src/widgets/form.dart | 1 + .../flutter/lib/src/widgets/framework.dart | 17 ++++++++++----- .../flutter/lib/src/widgets/navigator.dart | 1 + .../lib/src/widgets/scroll_position.dart | 1 + .../flutter/lib/src/widgets/scrollbar.dart | 5 ++++- packages/flutter/lib/src/widgets/sliver.dart | 1 + .../lib/src/widgets/text_selection.dart | 8 ++++++- .../lib/src/widgets/widget_inspector.dart | 2 ++ packages/flutter/test/analysis_options.yaml | 6 +++++- .../test/services/message_codecs_vm_test.dart | 4 ++-- .../lib/src/driver/vmservice_driver.dart | 2 ++ .../lib/src/driver/web_driver.dart | 18 ++++++++++------ .../lib/skia_client.dart | 12 +++-------- packages/flutter_test/lib/src/binding.dart | 6 ++++-- .../flutter_test/test/analysis_options.yaml | 7 +++++++ packages/flutter_tools/analysis_options.yaml | 1 + packages/flutter_tools/lib/runner.dart | 10 ++++----- .../lib/src/base/async_guard.dart | 2 +- .../lib/src/base/error_handling_io.dart | 2 +- .../lib/src/base/task_queue.dart | 2 +- .../lib/src/navigation/utils.dart | 2 +- .../lib/src/dart/dart_vm.dart | 2 ++ .../lib/src/fuchsia_remote_connection.dart | 7 +++---- .../integration_test/lib/_callback_web.dart | 3 +-- .../integration_test/lib/_extension_web.dart | 10 +++------ .../lib/integration_test_driver_extended.dart | 6 ++++-- 42 files changed, 138 insertions(+), 87 deletions(-) create mode 100644 dev/bots/analysis_options.yaml create mode 100644 packages/flutter_test/test/analysis_options.yaml diff --git a/analysis_options.yaml b/analysis_options.yaml index 41fdde6cff..24e9fdbfbe 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -56,10 +56,10 @@ linter: - annotate_overrides # - avoid_annotating_with_dynamic # conflicts with always_specify_types - avoid_bool_literals_in_conditional_expressions - # - avoid_catches_without_on_clauses # we do this commonly - # - avoid_catching_errors # we do this commonly + # - avoid_catches_without_on_clauses # blocked on https://github.com/dart-lang/linter/issues/3023 + # - avoid_catching_errors # blocked on https://github.com/dart-lang/linter/issues/3023 - avoid_classes_with_only_static_members - # - avoid_double_and_int_checks # only useful when targeting JS runtime + - avoid_double_and_int_checks - avoid_dynamic_calls - avoid_empty_else - avoid_equals_and_hash_code_on_mutable_classes @@ -68,7 +68,7 @@ linter: - avoid_function_literals_in_foreach_calls - avoid_implementing_value_types - avoid_init_to_null - # - avoid_js_rounded_ints # only useful when targeting JS runtime + - avoid_js_rounded_ints # - avoid_multiple_declarations_per_line # seems to be a stylistic choice we don't subscribe to - avoid_null_checks_in_equality_operators # - avoid_positional_boolean_parameters # would have been nice to enable this but by now there's too many places that break it @@ -81,7 +81,7 @@ linter: # - avoid_returning_null # still violated by some pre-nnbd code that we haven't yet migrated - avoid_returning_null_for_future - avoid_returning_null_for_void - # - avoid_returning_this # there are plenty of valid reasons to return this + # - avoid_returning_this # there are enough valid reasons to return `this` that this lint ends up with too many false positives - avoid_setters_without_getters - avoid_shadowing_type_parameters - avoid_single_cascade_in_expression_statements @@ -201,7 +201,7 @@ linter: - tighten_type_of_initializing_formals # - type_annotate_public_apis # subset of always_specify_types - type_init_formals - # - unawaited_futures # too many false positives + # - unawaited_futures # too many false positives, especially with the way AnimationController works - unnecessary_await_in_return - unnecessary_brace_in_string_interps - unnecessary_const @@ -216,24 +216,24 @@ linter: - unnecessary_nullable_for_final_variable_declarations - unnecessary_overrides - unnecessary_parenthesis - # - unnecessary_raw_strings # not yet tested + # - unnecessary_raw_strings # what's "necessary" is a matter of opinion; consistency across strings can help readability more than this lint - unnecessary_statements - unnecessary_string_escapes - unnecessary_string_interpolations - unnecessary_this - unrelated_type_equality_checks - # - unsafe_html # not yet tested + - unsafe_html - use_build_context_synchronously - use_full_hex_values_for_flutter_colors - use_function_type_syntax_for_parameters - # - use_if_null_to_convert_nulls_to_bools # not yet tested + # - use_if_null_to_convert_nulls_to_bools # blocked on https://github.com/dart-lang/sdk/issues/47436 - use_is_even_rather_than_modulo - use_key_in_widget_constructors - use_late_for_private_fields_and_variables - use_named_constants - use_raw_strings - use_rethrow_when_possible - # - use_setters_to_change_properties # not yet tested + - use_setters_to_change_properties # - use_string_buffers # has false positives: https://github.com/dart-lang/sdk/issues/34182 - use_test_throws_matchers # - use_to_and_as_if_applicable # has false positives, so we prefer to catch this by code-review diff --git a/dev/analysis_options.yaml b/dev/analysis_options.yaml index 091804565d..1a940d0a68 100644 --- a/dev/analysis_options.yaml +++ b/dev/analysis_options.yaml @@ -2,5 +2,10 @@ include: ../analysis_options.yaml linter: rules: - avoid_print: false # We use prints as debugging tools here all the time. - only_throw_errors: false # Tests use a less... rigorous style. + avoid_print: false # We use prints as debugging tools here a lot. + + # Tests try to throw and catch things in exciting ways all the time, so + # we disable these lints for the tests. + only_throw_errors: false + avoid_catching_errors: false + avoid_catches_without_on_clauses: false diff --git a/dev/bots/analysis_options.yaml b/dev/bots/analysis_options.yaml new file mode 100644 index 0000000000..979daa509c --- /dev/null +++ b/dev/bots/analysis_options.yaml @@ -0,0 +1,5 @@ +include: ../analysis_options.yaml + +linter: + rules: + avoid_js_rounded_ints: false # CLI code doesn't need to worry about JS issues diff --git a/examples/layers/services/isolate.dart b/examples/layers/services/isolate.dart index c9d9112e38..77506b9445 100644 --- a/examples/layers/services/isolate.dart +++ b/examples/layers/services/isolate.dart @@ -46,7 +46,7 @@ class Calculator { final List result = decoder.convert(_data) as List; final int n = result.length; onResultListener('Decoded $n results'); - } catch (e, stack) { + } on FormatException catch (e, stack) { debugPrint('Invalid JSON file: $e'); debugPrint('$stack'); } diff --git a/packages/flutter/lib/src/foundation/_bitfield_io.dart b/packages/flutter/lib/src/foundation/_bitfield_io.dart index a4f3bac8bc..fac9a01712 100644 --- a/packages/flutter/lib/src/foundation/_bitfield_io.dart +++ b/packages/flutter/lib/src/foundation/_bitfield_io.dart @@ -5,7 +5,7 @@ import 'bitfield.dart' as bitfield; /// The dart:io implementation of [bitfield.kMaxUnsignedSMI]. -const int kMaxUnsignedSMI = 0x3FFFFFFFFFFFFFFF; +const int kMaxUnsignedSMI = 0x3FFFFFFFFFFFFFFF; // ignore: avoid_js_rounded_ints, (VM-only code) /// The dart:io implementation of [bitfield.Bitfield]. class BitField implements bitfield.BitField { diff --git a/packages/flutter/lib/src/foundation/binding.dart b/packages/flutter/lib/src/foundation/binding.dart index 7380fa03d3..d19f431b04 100644 --- a/packages/flutter/lib/src/foundation/binding.dart +++ b/packages/flutter/lib/src/foundation/binding.dart @@ -597,34 +597,27 @@ abstract class BindingBase { return Future.delayed(Duration.zero); }); - Object? caughtException; - StackTrace? caughtStack; late Map result; try { result = await callback(parameters); } catch (exception, stack) { - caughtException = exception; - caughtStack = stack; - } - if (caughtException == null) { - result['type'] = '_extensionType'; - result['method'] = method; - return developer.ServiceExtensionResponse.result(json.encode(result)); - } else { FlutterError.reportError(FlutterErrorDetails( - exception: caughtException, - stack: caughtStack, + exception: exception, + stack: stack, context: ErrorDescription('during a service extension callback for "$method"'), )); return developer.ServiceExtensionResponse.error( developer.ServiceExtensionResponse.extensionError, json.encode({ - 'exception': caughtException.toString(), - 'stack': caughtStack.toString(), + 'exception': exception.toString(), + 'stack': stack.toString(), 'method': method, }), ); } + result['type'] = '_extensionType'; + result['method'] = method; + return developer.ServiceExtensionResponse.result(json.encode(result)); }); } diff --git a/packages/flutter/lib/src/foundation/diagnostics.dart b/packages/flutter/lib/src/foundation/diagnostics.dart index 928e453bf4..0015b7351c 100644 --- a/packages/flutter/lib/src/foundation/diagnostics.dart +++ b/packages/flutter/lib/src/foundation/diagnostics.dart @@ -2832,6 +2832,8 @@ class DiagnosticsProperty extends DiagnosticsNode { try { _value = _computeValue!(); } catch (exception) { + // The error is reported to inspector; rethrowing would destroy the + // debugging experience. _exception = exception; _value = null; } diff --git a/packages/flutter/lib/src/rendering/editable.dart b/packages/flutter/lib/src/rendering/editable.dart index bf17a26b66..234c533905 100644 --- a/packages/flutter/lib/src/rendering/editable.dart +++ b/packages/flutter/lib/src/rendering/editable.dart @@ -1091,6 +1091,7 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin, /// The prompt rectangle will only be requested on non-web iOS applications. /// /// When set to null, the currently displayed prompt rectangle (if any) will be dismissed. + // ignore: use_setters_to_change_properties, (API predates enforcing the lint) void setPromptRectRange(TextRange? newRange) { _autocorrectHighlightPainter.highlightedRange = newRange; } diff --git a/packages/flutter/lib/src/rendering/error.dart b/packages/flutter/lib/src/rendering/error.dart index 199ca294ca..41fa359da6 100644 --- a/packages/flutter/lib/src/rendering/error.dart +++ b/packages/flutter/lib/src/rendering/error.dart @@ -49,7 +49,9 @@ class RenderErrorBox extends RenderBox { _paragraph = null; } } catch (error) { - // Intentionally left empty. + // If an error happens here we're in a terrible state, so we really should + // just forget about it and let the developer deal with the already-reported + // errors. It's unlikely that these errors are going to help with that. } } @@ -161,8 +163,10 @@ class RenderErrorBox extends RenderBox { } context.canvas.drawParagraph(_paragraph!, offset + Offset(left, top)); } - } catch (e) { - // Intentionally left empty. + } catch (error) { + // If an error happens here we're in a terrible state, so we really should + // just forget about it and let the developer deal with the already-reported + // errors. It's unlikely that these errors are going to help with that. } } } diff --git a/packages/flutter/lib/src/rendering/sliver_persistent_header.dart b/packages/flutter/lib/src/rendering/sliver_persistent_header.dart index 19f8085af8..c479c22add 100644 --- a/packages/flutter/lib/src/rendering/sliver_persistent_header.dart +++ b/packages/flutter/lib/src/rendering/sliver_persistent_header.dart @@ -640,6 +640,7 @@ abstract class RenderSliverFloatingPersistentHeader extends RenderSliverPersiste } /// Update the last known ScrollDirection when scrolling began. + // ignore: use_setters_to_change_properties, (API predates enforcing the lint) void updateScrollStartDirection(ScrollDirection direction) { _lastStartedScrollDirection = direction; } diff --git a/packages/flutter/lib/src/services/binding.dart b/packages/flutter/lib/src/services/binding.dart index ef612f285c..afc201bbda 100644 --- a/packages/flutter/lib/src/services/binding.dart +++ b/packages/flutter/lib/src/services/binding.dart @@ -319,6 +319,7 @@ mixin ServicesBinding on BindingBase, SchedulerBinding { /// /// * [SystemChrome.setEnabledSystemUIMode], which specifies the /// [SystemUiMode] to have visible when the application is running. + // ignore: use_setters_to_change_properties, (API predates enforcing the lint) void setSystemUiChangeCallback(SystemUiChangeCallback? callback) { _systemUiChangeCallback = callback; } @@ -387,7 +388,6 @@ class _DefaultBinaryMessenger extends BinaryMessenger { try { response = await handler(data); } catch (exception, stack) { - FlutterError.reportError(FlutterErrorDetails( exception: exception, stack: stack, diff --git a/packages/flutter/lib/src/services/message_codecs.dart b/packages/flutter/lib/src/services/message_codecs.dart index 41a8dabb72..a03053a62e 100644 --- a/packages/flutter/lib/src/services/message_codecs.dart +++ b/packages/flutter/lib/src/services/message_codecs.dart @@ -376,7 +376,7 @@ class StandardMessageCodec implements MessageCodec { // decoding because we use tags to detect the type of value. buffer.putUint8(_valueFloat64); buffer.putFloat64(value); - } else if (value is int) { + } else if (value is int) { // ignore: avoid_double_and_int_checks, JS code always goes through the `double` path above if (-0x7fffffff - 1 <= value && value <= 0x7fffffff) { buffer.putUint8(_valueInt32); buffer.putInt32(value); diff --git a/packages/flutter/lib/src/services/platform_channel.dart b/packages/flutter/lib/src/services/platform_channel.dart index 9176c31df3..a57deb694c 100644 --- a/packages/flutter/lib/src/services/platform_channel.dart +++ b/packages/flutter/lib/src/services/platform_channel.dart @@ -410,8 +410,8 @@ class MethodChannel { ); } on MissingPluginException { return null; - } catch (e) { - return codec.encodeErrorEnvelope(code: 'error', message: e.toString()); + } catch (error) { + return codec.encodeErrorEnvelope(code: 'error', message: error.toString()); } } diff --git a/packages/flutter/lib/src/services/restoration.dart b/packages/flutter/lib/src/services/restoration.dart index c8d4d88259..5ca2731bdd 100644 --- a/packages/flutter/lib/src/services/restoration.dart +++ b/packages/flutter/lib/src/services/restoration.dart @@ -1006,7 +1006,9 @@ bool debugIsSerializableForRestoration(Object? object) { try { const StandardMessageCodec().encodeMessage(object); result = true; - } catch (_) { + } catch (error) { + // This is only used in asserts, so reporting the exception isn't + // particularly useful, since the assert itself will likely fail. result = false; } return true; diff --git a/packages/flutter/lib/src/widgets/actions.dart b/packages/flutter/lib/src/widgets/actions.dart index 8a09437af5..ed517d7f7b 100644 --- a/packages/flutter/lib/src/widgets/actions.dart +++ b/packages/flutter/lib/src/widgets/actions.dart @@ -160,6 +160,7 @@ abstract class Action with Diagnosticable { final ObserverList _listeners = ObserverList(); Action? _currentCallingAction; + // ignore: use_setters_to_change_properties, (code predates enabling of this lint) void _updateCallingAction(Action? value) { _currentCallingAction = value; } diff --git a/packages/flutter/lib/src/widgets/editable_text.dart b/packages/flutter/lib/src/widgets/editable_text.dart index 8b45816f0b..550d44dca3 100644 --- a/packages/flutter/lib/src/widgets/editable_text.dart +++ b/packages/flutter/lib/src/widgets/editable_text.dart @@ -2342,6 +2342,7 @@ class EditableTextState extends State with AutomaticKeepAliveClien } Rect? _currentCaretRect; + // ignore: use_setters_to_change_properties, (this is used as a callback, can't be a setter) void _handleCaretChanged(Rect caretRect) { _currentCaretRect = caretRect; } diff --git a/packages/flutter/lib/src/widgets/form.dart b/packages/flutter/lib/src/widgets/form.dart index dd4d87a97c..cf8a0315ab 100644 --- a/packages/flutter/lib/src/widgets/form.dart +++ b/packages/flutter/lib/src/widgets/form.dart @@ -421,6 +421,7 @@ class FormFieldState extends State> with RestorationMixin { /// the value should be set by a call to [didChange], which ensures that /// `setState` is called. @protected + // ignore: use_setters_to_change_properties, (API predates enforcing the lint) void setValue(T? value) { _value = value; } diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index df2e9875ff..ed0424f23c 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -4453,8 +4453,10 @@ class ErrorWidget extends LeafRenderObjectWidget { static String _stringify(Object? exception) { try { return exception.toString(); - } catch (e) { - // intentionally left empty. + } catch (error) { + // If we get here, it means things have really gone off the rails, and we're better + // off just returning a simple string and letting the developer find out what the + // root cause of all their problems are by looking at the console logs. } return 'Error'; } @@ -5426,6 +5428,9 @@ abstract class RenderObjectElement extends Element { if (badAncestors.isNotEmpty) { badAncestors.insert(0, result); try { + // We explicitly throw here (even though we immediately redirect the + // exception elsewhere) so that debuggers will notice it when they + // have "break on exception" enabled. throw FlutterError.fromParts([ ErrorSummary('Incorrect use of ParentDataWidget.'), ErrorDescription('The following ParentDataWidgets are providing parent data to the same RenderObject:'), @@ -5763,9 +5768,10 @@ abstract class RenderObjectElement extends Element { ]); } } on FlutterError catch (e) { - // Catching the exception directly to avoid activating the ErrorWidget. - // Since the tree is in a broken state, adding the ErrorWidget would - // cause more exceptions. + // We catch the exception directly to avoid activating the ErrorWidget, + // while still allowing debuggers to break on exception. Since the tree + // is in a broken state, adding the ErrorWidget would likely cause more + // exceptions, which is not good for the debugging experience. _debugReportException(ErrorSummary('while applying parent data.'), e, e.stackTrace); } return true; @@ -6036,6 +6042,7 @@ abstract class RootRenderObjectElement extends RenderObjectElement { /// to [runApp]. The binding is responsible for driving the build pipeline by /// calling the build owner's [BuildOwner.buildScope] method. See /// [WidgetsBinding.drawFrame]. + // ignore: use_setters_to_change_properties, (API predates enforcing the lint) void assignOwner(BuildOwner owner) { _owner = owner; } diff --git a/packages/flutter/lib/src/widgets/navigator.dart b/packages/flutter/lib/src/widgets/navigator.dart index d608197a4d..f149ea10ff 100644 --- a/packages/flutter/lib/src/widgets/navigator.dart +++ b/packages/flutter/lib/src/widgets/navigator.dart @@ -175,6 +175,7 @@ abstract class Route { } } + // ignore: use_setters_to_change_properties, (setters can't be private) void _updateRestorationId(String? restorationId) { _restorationScopeId.value = restorationId; } diff --git a/packages/flutter/lib/src/widgets/scroll_position.dart b/packages/flutter/lib/src/widgets/scroll_position.dart index 52f3aeb09a..470c9dbfaf 100644 --- a/packages/flutter/lib/src/widgets/scroll_position.dart +++ b/packages/flutter/lib/src/widgets/scroll_position.dart @@ -320,6 +320,7 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { /// middle of layout and applying the new position immediately. /// * [animateTo], which is like [jumpTo] but animating to the /// destination offset. + // ignore: use_setters_to_change_properties, (API is intended to discourage setting value) void correctPixels(double value) { _pixels = value; } diff --git a/packages/flutter/lib/src/widgets/scrollbar.dart b/packages/flutter/lib/src/widgets/scrollbar.dart index 2297caafbd..0b1588663b 100644 --- a/packages/flutter/lib/src/widgets/scrollbar.dart +++ b/packages/flutter/lib/src/widgets/scrollbar.dart @@ -1277,7 +1277,10 @@ class RawScrollbarState extends State with TickerProv assert (() { try { scrollController!.position; - } catch (_) { + } catch (error) { + if (scrollController == null || scrollController.positions.length <= 1) { + rethrow; + } throw FlutterError.fromParts([ ErrorSummary( 'The $controllerForError is currently attached to more than one ' diff --git a/packages/flutter/lib/src/widgets/sliver.dart b/packages/flutter/lib/src/widgets/sliver.dart index 5e9d74ae44..132991b013 100644 --- a/packages/flutter/lib/src/widgets/sliver.dart +++ b/packages/flutter/lib/src/widgets/sliver.dart @@ -204,6 +204,7 @@ abstract class SliverChildDelegate { if (children != null) description.add('estimated child count: $children'); } catch (e) { + // The exception is forwarded to widget inspector. description.add('estimated child count: EXCEPTION (${e.runtimeType})'); } } diff --git a/packages/flutter/lib/src/widgets/text_selection.dart b/packages/flutter/lib/src/widgets/text_selection.dart index e1d91304a4..6b30c3a212 100644 --- a/packages/flutter/lib/src/widgets/text_selection.dart +++ b/packages/flutter/lib/src/widgets/text_selection.dart @@ -1587,7 +1587,13 @@ class ClipboardStatusNotifier extends ValueNotifier with Widget final bool hasStrings; try { hasStrings = await Clipboard.hasStrings(); - } catch (stacktrace) { + } catch (exception, stack) { + FlutterError.reportError(FlutterErrorDetails( + exception: exception, + stack: stack, + library: 'widget library', + context: ErrorDescription('while checking if the clipboard has strings'), + )); // In the case of an error from the Clipboard API, set the value to // unknown so that it will try to update again later. if (_disposed || value == ClipboardStatus.unknown) { diff --git a/packages/flutter/lib/src/widgets/widget_inspector.dart b/packages/flutter/lib/src/widgets/widget_inspector.dart index 6e308282c3..3ca2436537 100644 --- a/packages/flutter/lib/src/widgets/widget_inspector.dart +++ b/packages/flutter/lib/src/widgets/widget_inspector.dart @@ -1962,6 +1962,8 @@ mixin WidgetInspectorService { FlutterErrorDetails( exception: exception, stack: stack, + library: 'widget inspector library', + context: ErrorDescription('while tracking widget repaints'), ), ); } diff --git a/packages/flutter/test/analysis_options.yaml b/packages/flutter/test/analysis_options.yaml index 5b7a383f6c..594830c78b 100644 --- a/packages/flutter/test/analysis_options.yaml +++ b/packages/flutter/test/analysis_options.yaml @@ -2,4 +2,8 @@ include: ../analysis_options.yaml linter: rules: - only_throw_errors: false # We are more flexible for tests. + # Tests try to throw and catch things in exciting ways all the time, so + # we disable these lints for the tests. + only_throw_errors: false + avoid_catches_without_on_clauses: false + avoid_catching_errors: false diff --git a/packages/flutter/test/services/message_codecs_vm_test.dart b/packages/flutter/test/services/message_codecs_vm_test.dart index 93ef9c6314..8bb069f7b8 100644 --- a/packages/flutter/test/services/message_codecs_vm_test.dart +++ b/packages/flutter/test/services/message_codecs_vm_test.dart @@ -18,7 +18,7 @@ void main() { checkEncodeDecode(json, -9223372036854775807); }); test('should encode and decode list with a big number', () { - final List message = [-7000000000000000007]; + final List message = [-7000000000000000007]; // ignore: avoid_js_rounded_ints, since we check for round-tripping, the actual value doesn't matter! checkEncodeDecode(json, message); }); }); @@ -72,7 +72,7 @@ void main() { }); test('should encode and decode a list containing big numbers', () { final List message = [ - -7000000000000000007, + -7000000000000000007, // ignore: avoid_js_rounded_ints, browsers are skipped below Int64List.fromList([-0x7fffffffffffffff - 1, 0, 0x7fffffffffffffff]), ]; checkEncodeDecode(standard, message); diff --git a/packages/flutter_driver/lib/src/driver/vmservice_driver.dart b/packages/flutter_driver/lib/src/driver/vmservice_driver.dart index bcaa5bde34..db061c5b8a 100644 --- a/packages/flutter_driver/lib/src/driver/vmservice_driver.dart +++ b/packages/flutter_driver/lib/src/driver/vmservice_driver.dart @@ -564,6 +564,8 @@ Future _waitAndConnect(String url, Map? headers) await service.getVersion(); return service; } catch (e) { + // We should not be catching all errors arbitrarily here, this might hide real errors. + // TODO(ianh): Determine which exceptions to catch here. await socket?.close(); if (attempts > 5) { _log('It is taking an unusually long time to connect to the VM...'); diff --git a/packages/flutter_driver/lib/src/driver/web_driver.dart b/packages/flutter_driver/lib/src/driver/web_driver.dart index e62ec2071b..4e7852bd02 100644 --- a/packages/flutter_driver/lib/src/driver/web_driver.dart +++ b/packages/flutter_driver/lib/src/driver/web_driver.dart @@ -115,9 +115,10 @@ class WebFlutterDriver extends FlutterDriver { response = data != null ? (json.decode(data as String) as Map?)! : {}; _logCommunication('<<< $response'); } catch (error, stackTrace) { - throw DriverError("Failed to respond to $command due to remote error\n : \$flutterDriver('${jsonEncode(serialized)}')", - error, - stackTrace + throw DriverError( + "Failed to respond to $command due to remote error\n : \$flutterDriver('${jsonEncode(serialized)}')", + error, + stackTrace ); } if (response['isError'] == true) @@ -261,8 +262,10 @@ class FlutterWebConnection { dynamic result; try { await _driver.execute(script, []); - } catch (_) { - // In case there is an exception, do nothing + } catch (error) { + // We should not just arbitrarily throw all exceptions on the ground. + // This is probably hiding real errors. + // TODO(ianh): Determine what exceptions are expected here and handle those specifically. } try { @@ -271,7 +274,10 @@ class FlutterWebConnection { matcher: isNotNull, timeout: duration ?? const Duration(days: 30), ); - } catch (_) { + } catch (error) { + // We should not just arbitrarily throw all exceptions on the ground. + // This is probably hiding real errors. + // TODO(ianh): Determine what exceptions are expected here and handle those specifically. // Returns null if exception thrown. return null; } finally { diff --git a/packages/flutter_goldens_client/lib/skia_client.dart b/packages/flutter_goldens_client/lib/skia_client.dart index 22fdc175e8..fbe71a0c59 100644 --- a/packages/flutter_goldens_client/lib/skia_client.dart +++ b/packages/flutter_goldens_client/lib/skia_client.dart @@ -329,15 +329,9 @@ class SkiaGoldClient { final Uri requestForImage = Uri.parse( 'https://flutter-gold.skia.org/img/images/$imageHash.png', ); - - try { - final io.HttpClientRequest request = await httpClient.getUrl(requestForImage); - final io.HttpClientResponse response = await request.close(); - await response.forEach((List bytes) => imageBytes.addAll(bytes)); - - } catch(e) { - rethrow; - } + final io.HttpClientRequest request = await httpClient.getUrl(requestForImage); + final io.HttpClientResponse response = await request.close(); + await response.forEach((List bytes) => imageBytes.addAll(bytes)); }, SkiaGoldHttpOverrides(), ); diff --git a/packages/flutter_test/lib/src/binding.dart b/packages/flutter_test/lib/src/binding.dart index 34220c11f4..1a8cf7927c 100644 --- a/packages/flutter_test/lib/src/binding.dart +++ b/packages/flutter_test/lib/src/binding.dart @@ -743,8 +743,10 @@ abstract class TestWidgetsFlutterBinding extends BindingBase DiagnosticsNode treeDump; try { treeDump = renderViewElement?.toDiagnosticsNode() ?? DiagnosticsNode.message(''); - // TODO(jacobr): this is a hack to make sure the tree can safely be fully dumped. - // Potentially everything is good enough without this case. + // We try to stringify the tree dump here (though we immediately discard the result) because + // we want to make sure that if it can't be serialised, we replace it with a message that + // says the tree could not be serialised. Otherwise, the real exception might get obscured + // by side-effects of the underlying issues causing the tree dumping code to flail. treeDump.toStringDeep(); } catch (exception) { treeDump = DiagnosticsNode.message('', level: DiagnosticLevel.error); diff --git a/packages/flutter_test/test/analysis_options.yaml b/packages/flutter_test/test/analysis_options.yaml new file mode 100644 index 0000000000..a2754263be --- /dev/null +++ b/packages/flutter_test/test/analysis_options.yaml @@ -0,0 +1,7 @@ +include: ../../analysis_options.yaml + +linter: + rules: + # Tests try to catch errors all the time, so we don't worry about these lints for tests: + avoid_catches_without_on_clauses: false + avoid_catching_errors: false diff --git a/packages/flutter_tools/analysis_options.yaml b/packages/flutter_tools/analysis_options.yaml index 2a89b53168..1fc02c6491 100644 --- a/packages/flutter_tools/analysis_options.yaml +++ b/packages/flutter_tools/analysis_options.yaml @@ -3,6 +3,7 @@ include: ../analysis_options.yaml linter: rules: avoid_catches_without_on_clauses: true + avoid_catching_errors: false # TODO(ianh): we should refactor the tool codebase to avoid relying on this so much curly_braces_in_flow_control_structures: true library_private_types_in_public_api: false # Tool does not have any public API. no_runtimeType_toString: false # We use runtimeType for debugging in the tool. diff --git a/packages/flutter_tools/lib/runner.dart b/packages/flutter_tools/lib/runner.dart index 965f2f7d63..f5435aca6e 100644 --- a/packages/flutter_tools/lib/runner.dart +++ b/packages/flutter_tools/lib/runner.dart @@ -70,12 +70,11 @@ Future run( // We already hit some error, so don't return success. The error path // (which should be in progress) is responsible for calling _exit(). return 1; - // This catches all exceptions to send to crash logging, etc. - } catch (error, stackTrace) { // ignore: avoid_catches_without_on_clauses + } catch (error, stackTrace) { // ignore: avoid_catches_without_on_clauses + // This catches all exceptions to send to crash logging, etc. firstError = error; firstStackTrace = stackTrace; - return _handleToolError( - error, stackTrace, verbose, args, reportCrashes, getVersion); + return _handleToolError(error, stackTrace, verbose, args, reportCrashes, getVersion); } }, onError: (Object error, StackTrace stackTrace) async { // ignore: deprecated_member_use // If sending a crash report throws an error into the zone, we don't want @@ -164,7 +163,8 @@ Future _handleToolError( } catch (error) { // ignore: avoid_catches_without_on_clauses globals.stdio.stderrWrite( 'Unable to generate crash report due to secondary error: $error\n' - '${globals.userMessages.flutterToolBugInstructions}\n'); + '${globals.userMessages.flutterToolBugInstructions}\n', + ); // Any exception thrown here (including one thrown by `_exit()`) will // get caught by our zone's `onError` handler. In order to avoid an // infinite error loop, we throw an error that is recognized above diff --git a/packages/flutter_tools/lib/src/base/async_guard.dart b/packages/flutter_tools/lib/src/base/async_guard.dart index 1e75a2b713..f35b99b6da 100644 --- a/packages/flutter_tools/lib/src/base/async_guard.dart +++ b/packages/flutter_tools/lib/src/base/async_guard.dart @@ -114,7 +114,7 @@ Future asyncGuard( } // This catches all exceptions so that they can be propagated to the // caller-supplied error handling or the completer. - } catch (e, s) { // ignore: avoid_catches_without_on_clauses + } catch (e, s) { // ignore: avoid_catches_without_on_clauses, forwards to Future handleError(e, s); } }, onError: (Object e, StackTrace s) { // ignore: deprecated_member_use diff --git a/packages/flutter_tools/lib/src/base/error_handling_io.dart b/packages/flutter_tools/lib/src/base/error_handling_io.dart index 4675a889e6..331246ee9c 100644 --- a/packages/flutter_tools/lib/src/base/error_handling_io.dart +++ b/packages/flutter_tools/lib/src/base/error_handling_io.dart @@ -348,7 +348,7 @@ class ErrorHandlingFile sink.writeFromSync(buffer, 0, chunkLength); bytes += chunkLength; } - } catch (err) { // ignore: avoid_catches_without_on_clauses + } catch (err) { // ignore: avoid_catches_without_on_clauses, rethrows ErrorHandlingFileSystem.deleteIfExists(resultFile, recursive: true); rethrow; } finally { diff --git a/packages/flutter_tools/lib/src/base/task_queue.dart b/packages/flutter_tools/lib/src/base/task_queue.dart index 0190f3c1ee..a83baff2ab 100644 --- a/packages/flutter_tools/lib/src/base/task_queue.dart +++ b/packages/flutter_tools/lib/src/base/task_queue.dart @@ -89,7 +89,7 @@ class _TaskQueueItem { Future run() async { try { _completer.complete(await _closure()); - } catch (e) { // ignore: avoid_catches_without_on_clauses + } catch (e) { // ignore: avoid_catches_without_on_clauses, forwards to Future _completer.completeError(e); } finally { onComplete?.call(); diff --git a/packages/flutter_web_plugins/lib/src/navigation/utils.dart b/packages/flutter_web_plugins/lib/src/navigation/utils.dart index 7cf7081b37..5cb18924db 100644 --- a/packages/flutter_web_plugins/lib/src/navigation/utils.dart +++ b/packages/flutter_web_plugins/lib/src/navigation/utils.dart @@ -11,7 +11,7 @@ final AnchorElement _urlParsingNode = AnchorElement(); /// Example: for the url `http://example.com/foo`, the extracted pathname will /// be `/foo`. String extractPathname(String url) { - _urlParsingNode.href = url; + _urlParsingNode.href = url; // ignore: unsafe_html, node is never exposed to the user final String pathname = _urlParsingNode.pathname ?? ''; return (pathname.isEmpty || pathname[0] == '/') ? pathname : '/$pathname'; } diff --git a/packages/fuchsia_remote_debug_protocol/lib/src/dart/dart_vm.dart b/packages/fuchsia_remote_debug_protocol/lib/src/dart/dart_vm.dart index 29000213ac..fe421ad818 100644 --- a/packages/fuchsia_remote_debug_protocol/lib/src/dart/dart_vm.dart +++ b/packages/fuchsia_remote_debug_protocol/lib/src/dart/dart_vm.dart @@ -54,6 +54,8 @@ Future _waitAndConnect( await service.getVersion(); return service; } catch (e) { + // We should not be catching all errors arbitrarily here, this might hide real errors. + // TODO(ianh): Determine which exceptions to catch here. await socket.close(); if (attempts > 5) { _log.warning('It is taking an unusually long time to connect to the VM...'); diff --git a/packages/fuchsia_remote_debug_protocol/lib/src/fuchsia_remote_connection.dart b/packages/fuchsia_remote_debug_protocol/lib/src/fuchsia_remote_connection.dart index a40e4727b5..cff0d833a7 100644 --- a/packages/fuchsia_remote_debug_protocol/lib/src/fuchsia_remote_connection.dart +++ b/packages/fuchsia_remote_debug_protocol/lib/src/fuchsia_remote_connection.dart @@ -679,14 +679,13 @@ class _SshPortForwarder implements PortForwarder { /// If successful returns a valid [ServerSocket] (which must be disconnected /// later). static Future _createLocalSocket() async { - ServerSocket s; try { - s = await ServerSocket.bind(_ipv4Loopback, 0); + return await ServerSocket.bind(_ipv4Loopback, 0); } catch (e) { - // Failures are signaled by a return value of 0 from this function. + // We should not be catching all errors arbitrarily here, this might hide real errors. + // TODO(ianh): Determine which exceptions to catch here. _log.warning('_createLocalSocket failed: $e'); return null; } - return s; } } diff --git a/packages/integration_test/lib/_callback_web.dart b/packages/integration_test/lib/_callback_web.dart index ce5658ea71..fa9a0bd8c9 100644 --- a/packages/integration_test/lib/_callback_web.dart +++ b/packages/integration_test/lib/_callback_web.dart @@ -65,8 +65,7 @@ class WebCallbackManager implements CallbackManager { 'driver side'); } } catch (exception) { - throw Exception('Web Driver Command failed: ${command.type} with ' - 'exception $exception'); + throw Exception('Web Driver Command failed: ${command.type} with exception $exception'); } finally { // Reset the completer. _driverCommandComplete = Completer(); diff --git a/packages/integration_test/lib/_extension_web.dart b/packages/integration_test/lib/_extension_web.dart index b02c907cc8..4fb077a14a 100644 --- a/packages/integration_test/lib/_extension_web.dart +++ b/packages/integration_test/lib/_extension_web.dart @@ -16,10 +16,8 @@ import 'dart:js_util' as js_util; /// See also: /// /// * `_extension_io.dart`, which has the dart:io implementation -void registerWebServiceExtension( - Future> Function(Map) callback) { - js_util.setProperty(html.window, r'$flutterDriver', - allowInterop((dynamic message) async { +void registerWebServiceExtension(Future> Function(Map) callback) { + js_util.setProperty(html.window, r'$flutterDriver', allowInterop((dynamic message) async { try { final Map messageJson = jsonDecode(message as String) as Map; final Map params = messageJson.cast(); @@ -27,9 +25,7 @@ void registerWebServiceExtension( context[r'$flutterDriverResult'] = json.encode(result); } catch (error, stackTrace) { // Encode the error in the same format the FlutterDriver extension uses. - // - // See: - // * packages\flutter_driver\lib\src\extension\extension.dart + // See //packages/flutter_driver/lib/src/extension/extension.dart context[r'$flutterDriverResult'] = json.encode({ 'isError': true, 'response': '$error\n$stackTrace', diff --git a/packages/integration_test/lib/integration_test_driver_extended.dart b/packages/integration_test/lib/integration_test_driver_extended.dart index 3ed6473479..6ded1b03c8 100644 --- a/packages/integration_test/lib/integration_test_driver_extended.dart +++ b/packages/integration_test/lib/integration_test_driver_extended.dart @@ -106,8 +106,10 @@ Future integrationDriver( try { ok = await onScreenshot(screenshotName, screenshotBytes.cast()); } catch (exception) { - throw StateError('Screenshot failure:\n' - 'onScreenshot("$screenshotName", ) threw an exception: $exception'); + throw StateError( + 'Screenshot failure:\n' + 'onScreenshot("$screenshotName", ) threw an exception: $exception', + ); } if (!ok) { failures.add(screenshotName);