Document why some lints aren't enabled and fix some minor issues. (#91527)

analysis_options.yaml

@@ -27,13 +27,12 @@ analyzer:
     missing_required_param: warning
     # treat missing returns as a warning (not a hint)
     missing_return: warning
-    # allow having TODO/HACK comments in the code
+    # allow having TODO comments in the code
     todo: ignore
-    hack: ignore
     # allow self-reference to deprecated members (we do this because otherwise we have
     # to annotate every member in every test, assert, etc, when we deprecate something)
     deprecated_member_use_from_same_package: ignore
-    # TODO(https://github.com/flutter/flutter/issues/74381):
+    # TODO(ianh): https://github.com/flutter/flutter/issues/74381
     # Clean up existing unnecessary imports, and remove line to ignore.
     unnecessary_import: ignore
     # Turned off until null-safe rollout is complete.
@@ -92,12 +91,12 @@ linter:
     - avoid_unnecessary_containers
     - avoid_unused_constructor_parameters
     - avoid_void_async
-    # - avoid_web_libraries_in_flutter # not yet tested
+    # - avoid_web_libraries_in_flutter # we use web libraries in web-specific code, and our tests prevent us from using them elsewhere
     - await_only_futures
     - camel_case_extensions
     - camel_case_types
     - cancel_subscriptions
-    # - cascade_invocations # not yet tested
+    # - cascade_invocations # doesn't match the typical style of this repo
     - cast_nullable_to_non_nullable
     # - close_sinks # not reliable enough
     # - comment_references # blocked on https://github.com/dart-lang/linter/issues/1142
@@ -105,9 +104,9 @@ linter:
     - control_flow_in_finally
     # - curly_braces_in_flow_control_structures # not required by flutter style
     - deprecated_consistency
-    # - diagnostic_describe_all_properties # not yet tested
+    # - diagnostic_describe_all_properties # enabled only at the framework level (packages/flutter/lib)
     - directives_ordering
-    # - do_not_use_environment # we do this commonly
+    # - do_not_use_environment # there are appropriate times to use the environment, especially in our tests and build logic
     - empty_catches
     - empty_constructor_bodies
     - empty_statements
@@ -125,7 +124,7 @@ linter:
     - library_private_types_in_public_api
     # - lines_longer_than_80_chars # not required by flutter style
     - list_remove_unrelated_type
-    # - literal_only_boolean_expressions # too many false positives: https://github.com/dart-lang/sdk/issues/34181
+    # - literal_only_boolean_expressions # too many false positives: https://github.com/dart-lang/linter/issues/453
     - missing_whitespace_between_adjacent_strings
     - no_adjacent_strings_in_list
     # - no_default_cases # too many false positives

dev/devicelab/lib/framework/utils.dart

@@ -21,22 +21,14 @@ String cwd = Directory.current.path;
 
 /// The local engine to use for [flutter] and [evalFlutter], if any.
 String? get localEngine {
-  // Use two distinct `defaultValue`s to determine whether a 'localEngine'
+  const bool isDefined = bool.hasEnvironment('localEngine');
-  // declaration exists in the environment.
-  const bool isDefined =
-      String.fromEnvironment('localEngine', defaultValue: 'a') ==
-          String.fromEnvironment('localEngine', defaultValue: 'b');
   return isDefined ? const String.fromEnvironment('localEngine') : null;
 }
 
 /// The local engine source path to use if a local engine is used for [flutter]
 /// and [evalFlutter].
 String? get localEngineSrcPath {
-  // Use two distinct `defaultValue`s to determine whether a
+  const bool isDefined = bool.hasEnvironment('localEngineSrcPath');
-  // 'localEngineSrcPath' declaration exists in the environment.
-  const bool isDefined =
-      String.fromEnvironment('localEngineSrcPath', defaultValue: 'a') ==
-          String.fromEnvironment('localEngineSrcPath', defaultValue: 'b');
   return isDefined ? const String.fromEnvironment('localEngineSrcPath') : null;
 }
 

packages/flutter/lib/analysis_options.yaml

@@ -0,0 +1,5 @@
+include: ../analysis_options.yaml
+
+linter:
+  rules:
+    # - diagnostic_describe_all_properties # blocked on https://github.com/dart-lang/sdk/issues/47418

packages/flutter_driver/lib/src/driver/driver.dart

@@ -556,53 +556,55 @@ abstract class FlutterDriver {
   ///
   /// The image will be returned as a PNG.
   ///
-  ///  HACK: There will be a 2-second artificial delay before screenshotting,
+  /// **Warning:** This is not reliable.
-  ///        the delay here is to deal with a race between the driver script and
-  ///        the raster thread (formerly known as the GPU thread). The issue is
-  ///        that driver API synchronizes with the framework based on transient
-  ///        callbacks, which are out of sync with the raster thread.
-  ///        Here's the timeline of events in ASCII art:
   ///
-  ///        -------------------------------------------------------------------
+  /// There is a two-second artificial delay before screenshotting. The delay
-  ///        Without this delay:
+  /// here is to deal with a race between the driver script and the raster
-  ///        -------------------------------------------------------------------
+  /// thread (formerly known as the GPU thread). The issue is that the driver
-  ///        UI    : <-- build -->
+  /// API synchronizes with the framework based on transient callbacks, which
-  ///        Raster:               <-- rasterize -->
+  /// are out of sync with the raster thread.
-  ///        Gap   :              | random |
-  ///        Driver:                        <-- screenshot -->
   ///
-  ///        In the diagram above, the gap is the time between the last driver
+  /// Here's the timeline of events in ASCII art:
-  ///        action taken, such as a `tap()`, and the subsequent call to
-  ///        `screenshot()`. The gap is random because it is determined by the
-  ///        unpredictable network communication between the driver process and
-  ///        the application. If this gap is too short, which it typically will
-  ///        be, the screenshot is taken before the raster thread is done
-  ///        rasterizing the frame, so the screenshot of the previous frame is
-  ///        taken, which is wrong.
   ///
-  ///        -------------------------------------------------------------------
+  ///     ---------------------------------------------------------------
-  ///        With this delay, if we're lucky:
+  ///     Without this delay:
-  ///        -------------------------------------------------------------------
+  ///     ---------------------------------------------------------------
-  ///        UI    : <-- build -->
+  ///     UI    : <-- build -->
-  ///        Raster:               <-- rasterize -->
+  ///     Raster:               <-- rasterize -->
-  ///        Gap   :              |    2 seconds or more   |
+  ///     Gap   :              | random |
-  ///        Driver:                                        <-- screenshot -->
+  ///     Driver:                        <-- screenshot -->
   ///
-  ///        The two-second gap should be long enough for the raster thread to
+  /// In the diagram above, the gap is the time between the last driver action
-  ///        finish rasterizing the frame, but not longer than necessary to keep
+  /// taken, such as a `tap()`, and the subsequent call to `screenshot()`. The
-  ///        driver tests as fast a possible.
+  /// gap is random because it is determined by the unpredictable communication
+  /// channel between the driver process and the application. If this gap is too
+  /// short, which it typically will be, the screenshot is taken before the
+  /// raster thread is done rasterizing the frame, so the screenshot of the
+  /// previous frame is taken, which is not what is intended.
   ///
-  ///        -------------------------------------------------------------------
+  ///     ---------------------------------------------------------------
-  ///        With this delay, if we're not lucky:
+  ///     With this delay, if we're lucky:
-  ///        -------------------------------------------------------------------
+  ///     ---------------------------------------------------------------
-  ///        UI    : <-- build -->
+  ///     UI    : <-- build -->
-  ///        Raster:               <-- rasterize randomly slow today -->
+  ///     Raster:               <-- rasterize -->
-  ///        Gap   :              |    2 seconds or more   |
+  ///     Gap   :              |    2 seconds or more   |
-  ///        Driver:                                        <-- screenshot -->
+  ///     Driver:                                        <-- screenshot -->
   ///
-  ///        In practice, sometimes the device gets really busy for a while and
+  /// The two-second gap should be long enough for the raster thread to finish
-  ///        even two seconds isn't enough, which means that this is still racy
+  /// rasterizing the frame, but not longer than necessary to keep driver tests
-  ///        and a source of flakes.
+  /// as fast a possible.
+  ///
+  ///     ---------------------------------------------------------------
+  ///     With this delay, if we're not lucky:
+  ///     ---------------------------------------------------------------
+  ///     UI    : <-- build -->
+  ///     Raster:               <-- rasterize randomly slow today -->
+  ///     Gap   :              |    2 seconds or more   |
+  ///     Driver:                                        <-- screenshot -->
+  ///
+  /// In practice, sometimes the device gets really busy for a while and even
+  /// two seconds isn't enough, which means that this is still racy and a source
+  /// of flakes.
   Future<List<int>> screenshot() async {
     throw UnimplementedError();
   }

packages/flutter_tools/lib/src/commands/packages.dart

@@ -305,10 +305,11 @@ class PackagesInteractiveGetCommand extends FlutterCommand {
     List<String> rest = argResults.rest;
     final bool isHelp = rest.contains('-h') || rest.contains('--help');
     String target;
-    if (rest.length == 1 && (rest[0].contains('/') || rest[0].contains(r'\'))) {
+    if (rest.length == 1 && (rest.single.contains('/') || rest.single.contains(r'\'))) {
-      // HACK: Supporting flutter specific behavior where you can pass a
+      // For historical reasons, if there is one argument to the command and it contains
-      //       folder to the command.
+      // a multiple-component path (i.e. contains a slash) then we use that to determine
-      target = findProjectRoot(globals.fs, rest[0]);
+      // to which project we're applying the command.
+      target = findProjectRoot(globals.fs, rest.single);
       rest = <String>[];
     } else {
       target = findProjectRoot(globals.fs);