[tool] Wire dependency injection into runner and executable with required non-nullable contexts - #190922
[tool] Wire dependency injection into runner and executable with required non-nullable contexts#190922bkonyi wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors dependency injection in flutter_tools by introducing AndroidContext, AppleContext, ToolContext, and ToolDependencies to manage and bootstrap tool dependencies instead of relying on global variables. Feedback on these changes highlights opportunities to improve testability and efficiency, such as using the shared finalShutdownHooks for the file system, utilizing finalSystemClock.now() for the terminal, preferring injected analytics over global analytics, avoiding a potential null pointer crash with DevtoolsLauncher.instance, and optimizing path traversal in _gatherProjectPaths by using fs.path.basename.
| delegate: LocalFileSystem( | ||
| LocalSignals.instance, | ||
| Signals.defaultExitSignals, | ||
| shutdownHooks ?? ShutdownHooks(), | ||
| ), |
There was a problem hiding this comment.
In ToolDependencies.bootstrap, finalShutdownHooks is defined on line 136 as shutdownHooks ?? ShutdownHooks(). However, on line 178, a new instance is created via shutdownHooks ?? ShutdownHooks() and passed to LocalFileSystem. Because shutdownHooks is typically null during normal execution, this creates a completely separate, orphaned ShutdownHooks instance for the file system. Any shutdown hooks registered by the file system (such as deleting temporary directories/files) will be registered on this orphaned instance and will never be executed when the main finalShutdownHooks are triggered. We should pass finalShutdownHooks instead.
| delegate: LocalFileSystem( | |
| LocalSignals.instance, | |
| Signals.defaultExitSignals, | |
| shutdownHooks ?? ShutdownHooks(), | |
| ), | |
| delegate: LocalFileSystem( | |
| LocalSignals.instance, | |
| Signals.defaultExitSignals, | |
| finalShutdownHooks, | |
| ), |
References
- Avoid duplicating state: Keep only one source of truth. (link)
| AnsiTerminal( | ||
| stdio: finalStdio, | ||
| platform: finalPlatform, | ||
| now: DateTime.now(), | ||
| shutdownHooks: finalShutdownHooks, | ||
| ); |
There was a problem hiding this comment.
AnsiTerminal is initialized with DateTime.now(), which bypasses the mockable finalSystemClock defined on line 134. This makes it impossible to mock or control the time for the terminal in unit tests. We should use finalSystemClock.now() instead.
| AnsiTerminal( | |
| stdio: finalStdio, | |
| platform: finalPlatform, | |
| now: DateTime.now(), | |
| shutdownHooks: finalShutdownHooks, | |
| ); | |
| AnsiTerminal( | |
| stdio: finalStdio, | |
| platform: finalPlatform, | |
| now: finalSystemClock.now(), | |
| shutdownHooks: finalShutdownHooks, | |
| ); |
| if ((topLevelResults[FlutterGlobalOptions.kSuppressAnalyticsFlag] as bool?) ?? false) { | ||
| globals.analytics.suppressTelemetry(); | ||
| } |
There was a problem hiding this comment.
To align with the dependency injection refactoring, prefer using the injected toolDependencies?.analytics over the global globals.analytics.
| if ((topLevelResults[FlutterGlobalOptions.kSuppressAnalyticsFlag] as bool?) ?? false) { | |
| globals.analytics.suppressTelemetry(); | |
| } | |
| if ((topLevelResults[FlutterGlobalOptions.kSuppressAnalyticsFlag] as bool?) ?? false) { | |
| (toolDependencies?.analytics ?? globals.analytics).suppressTelemetry(); | |
| } |
| if ((topLevelResults[FlutterGlobalOptions.kVersionFlag] as bool?) ?? false) { | ||
| globals.analytics.send( | ||
| Event.flutterCommandResult( | ||
| commandPath: 'version', | ||
| result: 'success', | ||
| commandHasTerminal: globals.stdio.hasTerminal, | ||
| ), | ||
| ); | ||
| final FlutterVersion version = globals.flutterVersion.fetchTagsAndGetVersion( | ||
| clock: globals.systemClock, | ||
| try { | ||
| globals.analytics.send( | ||
| Event.flutterCommandResult( | ||
| commandPath: 'version', | ||
| result: 'success', | ||
| commandHasTerminal: _toolContext.stdio.hasTerminal, | ||
| ), | ||
| ); | ||
| } on UnsupportedError catch (_) { | ||
| // Context not available in unit tests without context. | ||
| } |
There was a problem hiding this comment.
Prefer using the injected toolDependencies?.analytics over the global globals.analytics to support proper dependency injection.
| if ((topLevelResults[FlutterGlobalOptions.kVersionFlag] as bool?) ?? false) { | |
| globals.analytics.send( | |
| Event.flutterCommandResult( | |
| commandPath: 'version', | |
| result: 'success', | |
| commandHasTerminal: globals.stdio.hasTerminal, | |
| ), | |
| ); | |
| final FlutterVersion version = globals.flutterVersion.fetchTagsAndGetVersion( | |
| clock: globals.systemClock, | |
| try { | |
| globals.analytics.send( | |
| Event.flutterCommandResult( | |
| commandPath: 'version', | |
| result: 'success', | |
| commandHasTerminal: _toolContext.stdio.hasTerminal, | |
| ), | |
| ); | |
| } on UnsupportedError catch (_) { | |
| // Context not available in unit tests without context. | |
| } | |
| if ((topLevelResults[FlutterGlobalOptions.kVersionFlag] as bool?) ?? false) { | |
| try { | |
| (toolDependencies?.analytics ?? globals.analytics).send( | |
| Event.flutterCommandResult( | |
| commandPath: "version", | |
| result: "success", | |
| commandHasTerminal: _toolContext.stdio.hasTerminal, | |
| ), | |
| ); | |
| } on UnsupportedError catch (_) { | |
| // Context not available in unit tests without context. | |
| } |
| if (shouldPrintDtdUri) { | ||
| DevtoolsLauncher.instance!.printDtdUri = shouldPrintDtdUri; | ||
| } |
There was a problem hiding this comment.
Using the null-assertion operator ! on DevtoolsLauncher.instance can cause a runtime crash if it is null, even when shouldPrintDtdUri is true. Since DevtoolsLauncher.instance is often null in unit tests, we should use the null-safe operator ?. instead.
| if (shouldPrintDtdUri) { | |
| DevtoolsLauncher.instance!.printDtdUri = shouldPrintDtdUri; | |
| } | |
| if (shouldPrintDtdUri) { | |
| DevtoolsLauncher.instance?.printDtdUri = shouldPrintDtdUri; | |
| } |
| if (entity is Directory && !fs.path.split(entity.path).contains('.dart_tool')) { | ||
| return _gatherProjectPaths(fs, entity.path); | ||
| } |
There was a problem hiding this comment.
In _gatherProjectPaths, checking !fs.path.split(entity.path).contains('.dart_tool') splits the entire path string into a list of segments and searches through it on every single directory traversal. Since we are doing a recursive pre-order traversal and skip .dart_tool at the first level we encounter it, we only need to check if the current directory's name (the basename) is .dart_tool. Using fs.path.basename(entity.path) != '.dart_tool' is much more efficient and avoids unnecessary string and list allocations.
| if (entity is Directory && !fs.path.split(entity.path).contains('.dart_tool')) { | |
| return _gatherProjectPaths(fs, entity.path); | |
| } | |
| if (entity is Directory && fs.path.basename(entity.path) != ".dart_tool") { | |
| return _gatherProjectPaths(fs, entity.path); | |
| } |
References
- Suggest simplification and refactoring: Assess whether the code can be made simpler or refactored to enhance readability and maintainability. (link)
cf41bc2 to
cb1f566
Compare
cb1f566 to
5533bb8
Compare
flutter#190724) ## Summary Part 1 of the modular dependency injection migration. Defines the foundational modular dependency injection containers and bootstrapper for `flutter_tools`: 1. **`ToolContext`**: Layer 1 OS wrappers (`HostEnvironment`) and Layer 2 SDK state (`ToolConfiguration`). 2. **Nullable Sub-contexts**: `AndroidContext` and `AppleContext` for clean platform isolation. 3. **`ToolDependencies`**: Topologically instantiates and manages the dependency graph at startup with explicit overrides and lazy closures. 4. **Hermetic Test Doubles**: Includes `dependency_injection_test.dart` and fake context doubles. Followed by Part 2 (flutter#190922) for runner wiring. Part of flutter#47161
Summary
Part 2 of the modular dependency injection migration (stacked on #190724).
Wires explicit dependency injection into the Flutter tool entrypoints:
ToolDependencies.bootstrapintorunner.dartto initialize dependencies at startup.FlutterCommandRunnerto accept non-nullabletoolContext,androidContext,appleContext, andtoolDependencies.executable.dartto forwardToolDependenciesintogenerateCommands.test_flutter_command_runner.dartand hermetic test doubles.Part of #47161