From b2c65acd5ac6129a1eacdd3834a3cfcdb8d915e5 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Tue, 4 Apr 2023 12:25:27 -1000 Subject: [PATCH] [flutter_tools] Remove --no-sound-null-safety #4 (#124039) Re-land of https://github.com/flutter/flutter/pull/123297 without any of the commits at the end attempting to fix customer-testing. Fixes https://github.com/flutter/flutter/issues/118810 --- dev/bots/test.dart | 51 ++++-------- .../test/foundation/isolates_test.dart | 7 +- .../test_private/bin/test_private.dart | 2 +- packages/flutter_tools/lib/src/globals.dart | 5 ++ .../lib/src/runner/flutter_command.dart | 71 +++++++--------- .../commands.shard/hermetic/run_test.dart | 46 ++++++++++ .../test/general.shard/args_test.dart | 5 +- .../runner/flutter_command_test.dart | 21 ----- .../flutter_build_null_unsafe_test.dart | 83 ------------------- 9 files changed, 104 insertions(+), 187 deletions(-) delete mode 100644 packages/flutter_tools/test/integration.shard/flutter_build_null_unsafe_test.dart diff --git a/dev/bots/test.dart b/dev/bots/test.dart index f188ae7e81..68dae76a2d 100644 --- a/dev/bots/test.dart +++ b/dev/bots/test.dart @@ -573,9 +573,7 @@ Future _runExampleProjectBuildTests(Directory exampleDirectory, [File? mai // Only verify caching with flutter gallery. final bool verifyCaching = exampleDirectory.path.contains('flutter_gallery'); final String examplePath = path.relative(exampleDirectory.path, from: Directory.current.path); - final bool hasNullSafety = File(path.join(examplePath, 'null_safety')).existsSync(); final List additionalArgs = [ - if (hasNullSafety) '--no-sound-null-safety', if (mainFile != null) path.relative(mainFile.path, from: exampleDirectory.absolute.path), ]; if (Directory(path.join(examplePath, 'android')).existsSync()) { @@ -771,8 +769,6 @@ Future _runAddToAppLifeCycleTests() async { } Future _runFrameworkTests() async { - final List soundNullSafetyOptions = ['--null-assertions', '--sound-null-safety']; - final List mixedModeNullSafetyOptions = ['--null-assertions', '--no-sound-null-safety']; final List trackWidgetCreationAlternatives = ['--track-widget-creation', '--no-track-widget-creation']; Future runWidgets() async { @@ -780,7 +776,7 @@ Future _runFrameworkTests() async { for (final String trackWidgetCreationOption in trackWidgetCreationAlternatives) { await _runFlutterTest( path.join(flutterRoot, 'packages', 'flutter'), - options: [trackWidgetCreationOption, ...soundNullSafetyOptions], + options: [trackWidgetCreationOption], tests: [ path.join('test', 'widgets') + path.separator ], ); } @@ -795,13 +791,13 @@ Future _runFrameworkTests() async { // Run release mode tests (see packages/flutter/test_release/README.md) await _runFlutterTest( path.join(flutterRoot, 'packages', 'flutter'), - options: ['--dart-define=dart.vm.product=true', ...soundNullSafetyOptions], + options: ['--dart-define=dart.vm.product=true'], tests: ['test_release${path.separator}'], ); // Run profile mode tests (see packages/flutter/test_profile/README.md) await _runFlutterTest( path.join(flutterRoot, 'packages', 'flutter'), - options: ['--dart-define=dart.vm.product=false', '--dart-define=dart.vm.profile=true', ...soundNullSafetyOptions], + options: ['--dart-define=dart.vm.product=false', '--dart-define=dart.vm.profile=true'], tests: ['test_profile${path.separator}'], ); } @@ -817,7 +813,7 @@ Future _runFrameworkTests() async { for (final String trackWidgetCreationOption in trackWidgetCreationAlternatives) { await _runFlutterTest( path.join(flutterRoot, 'packages', 'flutter'), - options: [trackWidgetCreationOption, ...soundNullSafetyOptions], + options: [trackWidgetCreationOption], tests: tests, ); } @@ -837,9 +833,9 @@ Future _runFrameworkTests() async { workingDirectory: path.join(flutterRoot, 'examples', 'api'), ); } - await _runFlutterTest(path.join(flutterRoot, 'examples', 'api'), options: soundNullSafetyOptions); - await _runFlutterTest(path.join(flutterRoot, 'examples', 'hello_world'), options: soundNullSafetyOptions); - await _runFlutterTest(path.join(flutterRoot, 'examples', 'layers'), options: soundNullSafetyOptions); + await _runFlutterTest(path.join(flutterRoot, 'examples', 'api')); + await _runFlutterTest(path.join(flutterRoot, 'examples', 'hello_world')); + await _runFlutterTest(path.join(flutterRoot, 'examples', 'layers')); } Future runTracingTests() async { @@ -945,7 +941,6 @@ Future _runFrameworkTests() async { Future runPrivateTests() async { final List args = [ - '--sound-null-safety', 'run', 'bin/test_private.dart', ]; @@ -989,17 +984,17 @@ Future _runFrameworkTests() async { await _runFlutterTest(path.join(flutterRoot, 'dev', 'tools', 'gen_defaults')); await _runFlutterTest(path.join(flutterRoot, 'dev', 'tools', 'gen_keycodes')); await _runFlutterTest(path.join(flutterRoot, 'dev', 'benchmarks', 'test_apps', 'stocks')); - await _runFlutterTest(path.join(flutterRoot, 'packages', 'flutter_driver'), tests: [path.join('test', 'src', 'real_tests')], options: soundNullSafetyOptions); + await _runFlutterTest(path.join(flutterRoot, 'packages', 'flutter_driver'), tests: [path.join('test', 'src', 'real_tests')]); await _runFlutterTest(path.join(flutterRoot, 'packages', 'integration_test'), options: [ '--enable-vmservice', // Web-specific tests depend on Chromium, so they run as part of the web_long_running_tests shard. '--exclude-tags=web', ]); - await _runFlutterTest(path.join(flutterRoot, 'packages', 'flutter_goldens'), options: soundNullSafetyOptions); - await _runFlutterTest(path.join(flutterRoot, 'packages', 'flutter_localizations'), options: soundNullSafetyOptions); - await _runFlutterTest(path.join(flutterRoot, 'packages', 'flutter_test'), options: soundNullSafetyOptions); - await _runFlutterTest(path.join(flutterRoot, 'packages', 'fuchsia_remote_debug_protocol'), options: soundNullSafetyOptions); - await _runFlutterTest(path.join(flutterRoot, 'dev', 'integration_tests', 'non_nullable'), options: mixedModeNullSafetyOptions); + await _runFlutterTest(path.join(flutterRoot, 'packages', 'flutter_goldens')); + await _runFlutterTest(path.join(flutterRoot, 'packages', 'flutter_localizations')); + await _runFlutterTest(path.join(flutterRoot, 'packages', 'flutter_test')); + await _runFlutterTest(path.join(flutterRoot, 'packages', 'fuchsia_remote_debug_protocol')); + await _runFlutterTest(path.join(flutterRoot, 'dev', 'integration_tests', 'non_nullable')); const String httpClientWarning = 'Warning: At least one test in this suite creates an HttpClient. When\n' 'running a test suite that uses TestWidgetsFlutterBinding, all HTTP\n' @@ -1234,7 +1229,7 @@ Future _runWebLongRunningTests() async { '--dart-define=TEST_FLUTTER_ENGINE_VERSION=$engineVersion', ]), () => _runWebDebugTest('test/test.dart'), - () => _runWebDebugTest('lib/null_safe_main.dart', enableNullSafety: true), + () => _runWebDebugTest('lib/null_safe_main.dart'), () => _runWebDebugTest('lib/web_define_loading.dart', additionalArguments: [ '--dart-define=test.valueA=Example,A', @@ -1247,12 +1242,8 @@ Future _runWebLongRunningTests() async { '--dart-define=test.valueB=Value', ] ), - () => _runWebDebugTest('lib/sound_mode.dart', additionalArguments: [ - '--sound-null-safety', - ]), - () => _runWebReleaseTest('lib/sound_mode.dart', additionalArguments: [ - '--sound-null-safety', - ]), + () => _runWebDebugTest('lib/sound_mode.dart'), + () => _runWebReleaseTest('lib/sound_mode.dart'), () => _runFlutterWebTest( 'html', path.join(flutterRoot, 'packages', 'integration_test'), @@ -1311,7 +1302,6 @@ Future _runFlutterDriverWebTest({ if (driver != null) '--driver=$driver', '--target=$target', '--browser-name=chrome', - '--no-sound-null-safety', '-d', 'web-server', '--$buildMode', @@ -1353,7 +1343,6 @@ Future _runWebTreeshakeTest() async { 'build', 'web', '--target=$target', - '--no-sound-null-safety', '--profile', ], workingDirectory: testAppDirectory, @@ -1579,7 +1568,6 @@ Future _runGalleryE2eWebTest(String buildMode, { bool canvasKit = false }) '--driver=test_driver/transitions_perf_e2e_test.dart', '--target=test_driver/transitions_perf_e2e.dart', '--browser-name=chrome', - '--no-sound-null-safety', '-d', 'web-server', '--$buildMode', @@ -1686,7 +1674,6 @@ Future _runWebReleaseTest(String target, { /// /// Instead, we use `flutter run --debug` and sniff out the standard output. Future _runWebDebugTest(String target, { - bool enableNullSafety = false, List additionalArguments = const[], }) async { final String testAppDirectory = path.join(flutterRoot, 'dev', 'integration_tests', 'web'); @@ -1700,11 +1687,6 @@ Future _runWebDebugTest(String target, { [ 'run', '--debug', - if (enableNullSafety) - ...[ - '--no-sound-null-safety', - '--null-assertions', - ], '-d', 'chrome', '--web-run-headless', @@ -1747,7 +1729,6 @@ Future _runFlutterWebTest(String webRenderer, String workingDirectory, Lis '--platform=chrome', '--web-renderer=$webRenderer', '--dart-define=DART_HHH_BOT=$_runningInDartHHHBot', - '--sound-null-safety', ...flutterTestArgs, ...tests, ], diff --git a/packages/flutter/test/foundation/isolates_test.dart b/packages/flutter/test/foundation/isolates_test.dart index 912e9baa66..571d9a1e6b 100644 --- a/packages/flutter/test/foundation/isolates_test.dart +++ b/packages/flutter/test/foundation/isolates_test.dart @@ -80,8 +80,7 @@ Future test5CallCompute(int value) { return compute(test5, value); } -Future expectFileSuccessfullyCompletes(String filename, - [bool unsound = false]) async { +Future expectFileSuccessfullyCompletes(String filename) async { // Run a Dart script that calls compute(). // The Dart process will terminate only if the script exits cleanly with // all isolate ports closed. @@ -93,12 +92,10 @@ Future expectFileSuccessfullyCompletes(String filename, final String packageRoot = fs.path.dirname(fs.path.fromUri(platform.script)); final String scriptPath = fs.path.join(packageRoot, 'test', 'foundation', filename); - final String nullSafetyArg = - unsound ? '--no-sound-null-safety' : '--sound-null-safety'; // Enable asserts to also catch potentially invalid assertions. final ProcessResult result = await Process.run( - dartPath, [nullSafetyArg, 'run', '--enable-asserts', scriptPath]); + dartPath, ['run', '--enable-asserts', scriptPath]); expect(result.exitCode, 0); } diff --git a/packages/flutter/test_private/bin/test_private.dart b/packages/flutter/test_private/bin/test_private.dart index 7005e34926..25b89f3c38 100644 --- a/packages/flutter/test_private/bin/test_private.dart +++ b/packages/flutter/test_private/bin/test_private.dart @@ -225,7 +225,7 @@ class TestCase { for (final File test in tests) { final String testPath = path.join(path.dirname(test.path), 'lib', path.basenameWithoutExtension(test.path)); final ProcessRunnerResult result = await runner.runProcess( - [flutter, 'test', '--enable-experiment=non-nullable', '--no-sound-null-safety', '--null-assertions', testPath], + [flutter, 'test', testPath], failOk: true, ); if (result.exitCode != 0) { diff --git a/packages/flutter_tools/lib/src/globals.dart b/packages/flutter_tools/lib/src/globals.dart index 596c5a6c41..68904a25ee 100644 --- a/packages/flutter_tools/lib/src/globals.dart +++ b/packages/flutter_tools/lib/src/globals.dart @@ -43,6 +43,7 @@ import 'pre_run_validator.dart'; import 'project.dart'; import 'reporting/crash_reporting.dart'; import 'reporting/reporting.dart'; +import 'runner/flutter_command.dart'; import 'runner/local_engine.dart'; import 'version.dart'; @@ -285,3 +286,7 @@ const String kDefaultFrameworkChannel = 'master'; // Used to build RegExp instances which can detect the VM service message. final RegExp kVMServiceMessageRegExp = RegExp(r'The Dart VM service is listening on ((http|//)[a-zA-Z0-9:/=_\-\.\[\]]+)'); + +// The official tool no longer allows non-null safe builds. This can be +// overridden in other clients. +NonNullSafeBuilds get nonNullSafeBuilds => context.get() ?? NonNullSafeBuilds.notAllowed; diff --git a/packages/flutter_tools/lib/src/runner/flutter_command.dart b/packages/flutter_tools/lib/src/runner/flutter_command.dart index 95eefe2dfe..ea33e6b176 100644 --- a/packages/flutter_tools/lib/src/runner/flutter_command.dart +++ b/packages/flutter_tools/lib/src/runner/flutter_command.dart @@ -21,7 +21,6 @@ import '../bundle.dart' as bundle; import '../cache.dart'; import '../convert.dart'; import '../dart/generate_synthetic_packages.dart'; -import '../dart/language_version.dart'; import '../dart/package_map.dart'; import '../dart/pub.dart'; import '../device.dart'; @@ -822,20 +821,13 @@ abstract class FlutterCommand extends Command { void addNullSafetyModeOptions({ required bool hide }) { argParser.addFlag(FlutterOptions.kNullSafety, - help: - 'Whether to override the inferred null safety mode. This allows null-safe ' - 'libraries to depend on un-migrated (non-null safe) libraries. By default, ' - 'Flutter mobile & desktop applications will attempt to run at the null safety ' - 'level of their entrypoint library (usually lib/main.dart). Flutter web ' - 'applications will default to sound null-safety, unless specifically configured.', + help: 'This flag is deprecated as only null-safe code is supported.', defaultsTo: true, - hide: hide, + hide: true, ); argParser.addFlag(FlutterOptions.kNullAssertions, - help: - 'Perform additional null assertions on the boundaries of migrated and ' - 'un-migrated code. This setting is not currently supported on desktop ' - 'devices.' + help: 'This flag is deprecated as only null-safe code is supported.', + hide: true, ); } @@ -1148,39 +1140,17 @@ abstract class FlutterCommand extends Command { NullSafetyMode nullSafetyMode = NullSafetyMode.sound; if (argParser.options.containsKey(FlutterOptions.kNullSafety)) { - // Explicitly check for `true` and `false` so that `null` results in not - // passing a flag. Examine the entrypoint file to determine if it - // is opted in or out. final bool wasNullSafetyFlagParsed = argResults?.wasParsed(FlutterOptions.kNullSafety) ?? false; - if (!wasNullSafetyFlagParsed && (argParser.options.containsKey('target') || forcedTargetFile != null)) { - final File entrypointFile = forcedTargetFile ?? globals.fs.file(targetFile); - final LanguageVersion languageVersion = determineLanguageVersion( - entrypointFile, - packageConfig.packageOf(entrypointFile.absolute.uri), - Cache.flutterRoot!, - ); - // Extra frontend options are only provided if explicitly - // requested. - if ((languageVersion.major > nullSafeVersion.major) || - (languageVersion.major == nullSafeVersion.major && languageVersion.minor >= nullSafeVersion.minor)) { + // Extra frontend options are only provided if explicitly + // requested. + if (wasNullSafetyFlagParsed) { + if (boolArg(FlutterOptions.kNullSafety)) { nullSafetyMode = NullSafetyMode.sound; + extraFrontEndOptions.add('--sound-null-safety'); } else { - throwToolExit( - 'This application does not support sound null-safety (its language version is $languageVersion).\n' - 'To build this application, you must provide the CLI flag --no-sound-null-safety. Dart 3 will only ' - 'support sound null safety, see https://dart.dev/null-safety.', - ); + nullSafetyMode = NullSafetyMode.unsound; + extraFrontEndOptions.add('--no-sound-null-safety'); } - } else if (!wasNullSafetyFlagParsed) { - // This mode is only used for commands which do not build a single target like - // 'flutter test'. - nullSafetyMode = NullSafetyMode.autodetect; - } else if (boolArg(FlutterOptions.kNullSafety)) { - nullSafetyMode = NullSafetyMode.sound; - extraFrontEndOptions.add('--sound-null-safety'); - } else { - nullSafetyMode = NullSafetyMode.unsound; - extraFrontEndOptions.add('--no-sound-null-safety'); } } @@ -1487,6 +1457,16 @@ abstract class FlutterCommand extends Command { /// rather than calling [runCommand] directly. @mustCallSuper Future verifyThenRunCommand(String? commandPath) async { + if (argParser.options.containsKey(FlutterOptions.kNullSafety) && + argResults![FlutterOptions.kNullSafety] == false && + globals.nonNullSafeBuilds == NonNullSafeBuilds.notAllowed) { + throwToolExit(''' +Could not find an option named "no-${FlutterOptions.kNullSafety}". + +Run 'flutter -h' (or 'flutter -h') for available flutter commands and options. +'''); + } + globals.preRunValidator.validate(); if (refreshWirelessDevices) { @@ -1754,3 +1734,12 @@ DevelopmentArtifact? artifactFromTargetPlatform(TargetPlatform targetPlatform) { /// Returns true if s is either null, empty or is solely made of whitespace characters (as defined by String.trim). bool _isBlank(String s) => s.trim().isEmpty; + +/// Whether the tool should allow non-null safe builds. +/// +/// The Dart SDK no longer supports non-null safe builds, so this value in the +/// tool's context should always be [NonNullSafeBuilds.notAllowed]. +enum NonNullSafeBuilds { + allowed, + notAllowed, +} diff --git a/packages/flutter_tools/test/commands.shard/hermetic/run_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/run_test.dart index c3d7524ffb..631341ad4a 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/run_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/run_test.dart @@ -67,6 +67,52 @@ void main() { Logger: () => BufferLogger.test(), }); + testUsingContext('does not support --no-sound-null-safety by default', () async { + fileSystem.file('lib/main.dart').createSync(recursive: true); + fileSystem.file('pubspec.yaml').createSync(); + fileSystem.file('.packages').createSync(); + + final TestRunCommandThatOnlyValidates command = TestRunCommandThatOnlyValidates(); + await expectLater( + () => createTestCommandRunner(command).run([ + 'run', + '--use-application-binary=app/bar/faz', + '--no-sound-null-safety', + ]), + throwsA(isException.having( + (Exception exception) => exception.toString(), + 'toString', + contains('Could not find an option named "no-sound-null-safety"'), + )), + ); + }, overrides: { + FileSystem: () => fileSystem, + ProcessManager: () => FakeProcessManager.any(), + Logger: () => BufferLogger.test(), + }); + + testUsingContext('supports --no-sound-null-safety with an overridden NonNullSafeBuilds', () async { + fileSystem.file('lib/main.dart').createSync(recursive: true); + fileSystem.file('pubspec.yaml').createSync(); + fileSystem.file('.packages').createSync(); + + final FakeDevice device = FakeDevice(isLocalEmulator: true, platformType: PlatformType.android); + + testDeviceManager.devices = [device]; + final TestRunCommandThatOnlyValidates command = TestRunCommandThatOnlyValidates(); + await createTestCommandRunner(command).run(const [ + 'run', + '--use-application-binary=app/bar/faz', + '--no-sound-null-safety', + ]); + }, overrides: { + DeviceManager: () => testDeviceManager, + FileSystem: () => fileSystem, + Logger: () => BufferLogger.test(), + NonNullSafeBuilds: () => NonNullSafeBuilds.allowed, + ProcessManager: () => FakeProcessManager.any(), + }); + testUsingContext('does not support "--use-application-binary" and "--fast-start"', () async { fileSystem.file('lib/main.dart').createSync(recursive: true); fileSystem.file('pubspec.yaml').createSync(); diff --git a/packages/flutter_tools/test/general.shard/args_test.dart b/packages/flutter_tools/test/general.shard/args_test.dart index 0dc1b58b87..6fe315e4b9 100644 --- a/packages/flutter_tools/test/general.shard/args_test.dart +++ b/packages/flutter_tools/test/general.shard/args_test.dart @@ -176,7 +176,10 @@ void verifyOptions(String? command, Iterable