From f824b76e51b298441bced1fd4d56a68fe1cf5eb1 Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Sat, 18 Jan 2025 18:08:16 -0800 Subject: [PATCH 1/4] Use recompile-restart instruction when hot restarting on the web recompile has been split into recompile and recompile-restart in the frontend server so that DDC can distinguish between hot reload recompiles and hot restart recompiles, and therefore emit rejection errors only on hot reload. --- packages/flutter_tools/lib/src/compile.dart | 14 +++- .../lib/src/isolated/devfs_web.dart | 1 + .../test/general.shard/devfs_test.dart | 1 + .../resident_runner_helpers.dart | 1 + .../resident_web_runner_test.dart | 1 + .../test/test_compiler_test.dart | 1 + .../web/devfs_web_ddc_modules_test.dart | 1 + .../general.shard/web/devfs_web_test.dart | 1 + .../web.shard/hot_reload_web_errors_test.dart | 64 +++++++++++++++++++ 9 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart diff --git a/packages/flutter_tools/lib/src/compile.dart b/packages/flutter_tools/lib/src/compile.dart index 0a5ae557d8484..65b1b60ad2c2b 100644 --- a/packages/flutter_tools/lib/src/compile.dart +++ b/packages/flutter_tools/lib/src/compile.dart @@ -410,6 +410,7 @@ class _RecompileRequest extends _CompilationRequest { this.suppressErrors, { this.additionalSourceUri, this.nativeAssetsYamlUri, + required this.recompileRestart, }); Uri mainUri; @@ -419,6 +420,7 @@ class _RecompileRequest extends _CompilationRequest { bool suppressErrors; final Uri? additionalSourceUri; final Uri? nativeAssetsYamlUri; + final bool recompileRestart; @override Future _run(DefaultResidentCompiler compiler) async => compiler._recompile(this); @@ -533,6 +535,9 @@ abstract class ResidentCompiler { /// If [checkDartPluginRegistry] is true, it is the caller's responsibility /// to ensure that the generated registrant file has been updated such that /// it is wrapping [mainUri]. + /// + /// If [recompileRestart] is true, uses the `recompile-restart` instruction + /// instead. Future recompile( Uri mainUri, List? invalidatedFiles, { @@ -544,6 +549,7 @@ abstract class ResidentCompiler { bool checkDartPluginRegistry = false, File? dartPluginRegistrant, Uri? nativeAssetsYaml, + bool recompileRestart = false, }); Future compileExpression( @@ -695,6 +701,7 @@ class DefaultResidentCompiler implements ResidentCompiler { String? projectRootPath, FileSystem? fs, Uri? nativeAssetsYaml, + bool recompileRestart = false, }) async { if (!_controller.hasListener) { _controller.stream.listen(_handleCompilationRequest); @@ -717,6 +724,7 @@ class DefaultResidentCompiler implements ResidentCompiler { suppressErrors, additionalSourceUri: additionalSourceUri, nativeAssetsYamlUri: nativeAssetsYaml, + recompileRestart: recompileRestart, ), ); return completer.future; @@ -759,7 +767,11 @@ class DefaultResidentCompiler implements ResidentCompiler { server.stdin.writeln('native-assets $nativeAssets'); _logger.printTrace('<- native-assets $nativeAssets'); } - server.stdin.writeln('recompile $mainUri $inputKey'); + if (request.recompileRestart) { + server.stdin.writeln('recompile-restart $mainUri $inputKey'); + } else { + server.stdin.writeln('recompile $mainUri $inputKey'); + } _logger.printTrace('<- recompile $mainUri $inputKey'); final List? invalidatedFiles = request.invalidatedFiles; if (invalidatedFiles != null) { diff --git a/packages/flutter_tools/lib/src/isolated/devfs_web.dart b/packages/flutter_tools/lib/src/isolated/devfs_web.dart index 4423a59d07998..6b294093fc94d 100644 --- a/packages/flutter_tools/lib/src/isolated/devfs_web.dart +++ b/packages/flutter_tools/lib/src/isolated/devfs_web.dart @@ -1138,6 +1138,7 @@ class WebDevFS implements DevFS { projectRootPath: projectRootPath, fs: globals.fs, dartPluginRegistrant: dartPluginRegistrant, + recompileRestart: fullRestart, ); if (compilerOutput == null || compilerOutput.errorCount > 0) { return UpdateFSReport(); diff --git a/packages/flutter_tools/test/general.shard/devfs_test.dart b/packages/flutter_tools/test/general.shard/devfs_test.dart index ee1cab23a4502..b99e49d1ad800 100644 --- a/packages/flutter_tools/test/general.shard/devfs_test.dart +++ b/packages/flutter_tools/test/general.shard/devfs_test.dart @@ -966,6 +966,7 @@ class FakeResidentCompiler extends Fake implements ResidentCompiler { bool checkDartPluginRegistry = false, File? dartPluginRegistrant, Uri? nativeAssetsYaml, + bool recompileRestart = false, }) { return onRecompile?.call(mainUri, invalidatedFiles) ?? Future.value(const CompilerOutput('', 1, [])); diff --git a/packages/flutter_tools/test/general.shard/resident_runner_helpers.dart b/packages/flutter_tools/test/general.shard/resident_runner_helpers.dart index e8c47c86ab15c..5774df5f730a4 100644 --- a/packages/flutter_tools/test/general.shard/resident_runner_helpers.dart +++ b/packages/flutter_tools/test/general.shard/resident_runner_helpers.dart @@ -321,6 +321,7 @@ class FakeResidentCompiler extends Fake implements ResidentCompiler { bool checkDartPluginRegistry = false, File? dartPluginRegistrant, Uri? nativeAssetsYaml, + bool recompileRestart = false, }) async { recompileCalled = true; receivedNativeAssetsYaml = nativeAssetsYaml; diff --git a/packages/flutter_tools/test/general.shard/resident_web_runner_test.dart b/packages/flutter_tools/test/general.shard/resident_web_runner_test.dart index 9cc275e4aab0b..52c78683a52bb 100644 --- a/packages/flutter_tools/test/general.shard/resident_web_runner_test.dart +++ b/packages/flutter_tools/test/general.shard/resident_web_runner_test.dart @@ -1604,6 +1604,7 @@ class FakeResidentCompiler extends Fake implements ResidentCompiler { bool checkDartPluginRegistry = false, File? dartPluginRegistrant, Uri? nativeAssetsYaml, + bool recompileRestart = false, }) async { return const CompilerOutput('foo.dill', 0, []); } diff --git a/packages/flutter_tools/test/general.shard/test/test_compiler_test.dart b/packages/flutter_tools/test/general.shard/test/test_compiler_test.dart index b5e4ae4460ea3..04201bca63642 100644 --- a/packages/flutter_tools/test/general.shard/test/test_compiler_test.dart +++ b/packages/flutter_tools/test/general.shard/test/test_compiler_test.dart @@ -311,6 +311,7 @@ class FakeResidentCompiler extends Fake implements ResidentCompiler { bool checkDartPluginRegistry = false, File? dartPluginRegistrant, Uri? nativeAssetsYaml, + bool recompileRestart = false, }) async { if (compilerOutput != null) { fileSystem!.file(compilerOutput!.outputFilename).createSync(recursive: true); diff --git a/packages/flutter_tools/test/general.shard/web/devfs_web_ddc_modules_test.dart b/packages/flutter_tools/test/general.shard/web/devfs_web_ddc_modules_test.dart index e8dc55adb5895..13f2b864b5ee4 100644 --- a/packages/flutter_tools/test/general.shard/web/devfs_web_ddc_modules_test.dart +++ b/packages/flutter_tools/test/general.shard/web/devfs_web_ddc_modules_test.dart @@ -1386,6 +1386,7 @@ class FakeResidentCompiler extends Fake implements ResidentCompiler { bool checkDartPluginRegistry = false, File? dartPluginRegistrant, Uri? nativeAssetsYaml, + bool recompileRestart = false, }) async { return output; } diff --git a/packages/flutter_tools/test/general.shard/web/devfs_web_test.dart b/packages/flutter_tools/test/general.shard/web/devfs_web_test.dart index c780e6b63b0da..db3395ccf37aa 100644 --- a/packages/flutter_tools/test/general.shard/web/devfs_web_test.dart +++ b/packages/flutter_tools/test/general.shard/web/devfs_web_test.dart @@ -1703,6 +1703,7 @@ class FakeResidentCompiler extends Fake implements ResidentCompiler { bool checkDartPluginRegistry = false, File? dartPluginRegistrant, Uri? nativeAssetsYaml, + bool recompileRestart = false, }) async { return output; } diff --git a/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart b/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart new file mode 100644 index 0000000000000..d3cac707563da --- /dev/null +++ b/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart @@ -0,0 +1,64 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +@Tags(['flutter-test-driver']) +library; + +import 'package:file/file.dart'; + +import '../integration.shard/test_data/hot_reload_const_project.dart'; +import '../integration.shard/test_driver.dart'; +import '../integration.shard/test_utils.dart'; +import '../src/common.dart'; + +void main() { + late Directory tempDir; + final HotReloadConstProject project = HotReloadConstProject(); + late FlutterRunTestDriver flutter; + + final List additionalCommandArgs = [ + '--extra-front-end-options=--dartdevc-canary,--dartdevc-module-format=ddc', + ]; + + setUp(() async { + tempDir = createResolvedTempDirectorySync('hot_reload_test.'); + await project.setUpIn(tempDir); + flutter = FlutterRunTestDriver(tempDir); + }); + + tearDown(() async { + await flutter.stop(); + tryToDelete(tempDir); + }); + + testWithoutContext( + 'hot reload displays an error message when removing a field from a const class', + () async { + await flutter.run(chrome: true, additionalCommandArgs: additionalCommandArgs); + project.removeFieldFromConstClass(); + + expect( + flutter.hotReload(), + throwsA( + isA().having( + (Exception e) => e.toString(), + 'message', + // TODO(srujzs): Change this to "Try performing a hot restart instead." once we emit + // that string in the delta inspector. Then unify this test with + // hot_reload_errors_test.dart. + contains('Const class cannot become non-const'), + ), + ), + ); + }, + ); + + testWithoutContext('hot restart succeeds when removing a field from a const class', () async { + // We should have used `recompile-restart` to avoid the errors the DDC delta + // inspector emits. + await flutter.run(chrome: true, additionalCommandArgs: additionalCommandArgs); + project.removeFieldFromConstClass(); + await flutter.hotRestart(); + }); +} From 935286bcc0267dd1d228fe2e794e988e425b004c Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Mon, 3 Feb 2025 11:02:39 -0800 Subject: [PATCH 2/4] Clarify dart doc for parameter --- packages/flutter_tools/lib/src/compile.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/flutter_tools/lib/src/compile.dart b/packages/flutter_tools/lib/src/compile.dart index 65b1b60ad2c2b..dbf60e73a6e4d 100644 --- a/packages/flutter_tools/lib/src/compile.dart +++ b/packages/flutter_tools/lib/src/compile.dart @@ -537,7 +537,7 @@ abstract class ResidentCompiler { /// it is wrapping [mainUri]. /// /// If [recompileRestart] is true, uses the `recompile-restart` instruction - /// instead. + /// intended for a hot restart instead. Future recompile( Uri mainUri, List? invalidatedFiles, { From 2b9a0ca2f579002331f08a5c446ddfcbd8aca32a Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Mon, 3 Feb 2025 13:15:35 -0800 Subject: [PATCH 3/4] Update error message in test --- .../test/web.shard/hot_reload_web_errors_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart b/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart index d3cac707563da..070c47bd761f7 100644 --- a/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart +++ b/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart @@ -47,7 +47,7 @@ void main() { // TODO(srujzs): Change this to "Try performing a hot restart instead." once we emit // that string in the delta inspector. Then unify this test with // hot_reload_errors_test.dart. - contains('Const class cannot become non-const'), + contains('Const class cannot remove fields'), ), ), ); From a6210a9c4e0187de6ff99c886b40a4e2bbced6e9 Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Mon, 3 Feb 2025 13:24:35 -0800 Subject: [PATCH 4/4] Unify implementation of hot reload errors test --- .../hot_reload_errors_test.dart | 40 +----------- .../test_data/hot_reload_errors_common.dart | 60 ++++++++++++++++++ .../web.shard/hot_reload_web_errors_test.dart | 63 ++++--------------- 3 files changed, 74 insertions(+), 89 deletions(-) create mode 100644 packages/flutter_tools/test/integration.shard/test_data/hot_reload_errors_common.dart diff --git a/packages/flutter_tools/test/integration.shard/hot_reload_errors_test.dart b/packages/flutter_tools/test/integration.shard/hot_reload_errors_test.dart index 830b2444a1a99..5ed446b87c1fa 100644 --- a/packages/flutter_tools/test/integration.shard/hot_reload_errors_test.dart +++ b/packages/flutter_tools/test/integration.shard/hot_reload_errors_test.dart @@ -5,45 +5,9 @@ @Tags(['flutter-test-driver']) library; -import 'package:file/file.dart'; - import '../src/common.dart'; -import 'test_data/hot_reload_const_project.dart'; -import 'test_driver.dart'; -import 'test_utils.dart'; +import 'test_data/hot_reload_errors_common.dart'; void main() { - late Directory tempDir; - final HotReloadConstProject project = HotReloadConstProject(); - late FlutterRunTestDriver flutter; - - setUp(() async { - tempDir = createResolvedTempDirectorySync('hot_reload_test.'); - await project.setUpIn(tempDir); - flutter = FlutterRunTestDriver(tempDir); - }); - - tearDown(() async { - await flutter.stop(); - tryToDelete(tempDir); - }); - - testWithoutContext( - 'hot reload displays a formatted error message when removing a field from a const class', - () async { - await flutter.run(); - project.removeFieldFromConstClass(); - - expect( - flutter.hotReload(), - throwsA( - isA().having( - (Exception e) => e.toString(), - 'message', - contains('Try performing a hot restart instead.'), - ), - ), - ); - }, - ); + testAll(); } diff --git a/packages/flutter_tools/test/integration.shard/test_data/hot_reload_errors_common.dart b/packages/flutter_tools/test/integration.shard/test_data/hot_reload_errors_common.dart new file mode 100644 index 0000000000000..e1a313f188279 --- /dev/null +++ b/packages/flutter_tools/test/integration.shard/test_data/hot_reload_errors_common.dart @@ -0,0 +1,60 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:file/file.dart'; + +import '../../src/common.dart'; +import '../test_driver.dart'; +import '../test_utils.dart'; +import 'hot_reload_const_project.dart'; + +void testAll({ + bool chrome = false, + List additionalCommandArgs = const [], + String constClassFieldRemovalErrorMessage = 'Try performing a hot restart instead.', + Object? skip = false, +}) { + group('chrome: $chrome' + '${additionalCommandArgs.isEmpty ? '' : ' with args: $additionalCommandArgs'}', () { + late Directory tempDir; + final HotReloadConstProject project = HotReloadConstProject(); + late FlutterRunTestDriver flutter; + + setUp(() async { + tempDir = createResolvedTempDirectorySync('hot_reload_test.'); + await project.setUpIn(tempDir); + flutter = FlutterRunTestDriver(tempDir); + }); + + tearDown(() async { + await flutter.stop(); + tryToDelete(tempDir); + }); + + testWithoutContext( + 'hot reload displays a formatted error message when removing a field from a const class', + () async { + await flutter.run(); + project.removeFieldFromConstClass(); + + expect( + flutter.hotReload(), + throwsA( + isA().having( + (Exception e) => e.toString(), + 'message', + contains(constClassFieldRemovalErrorMessage), + ), + ), + ); + }, + ); + + testWithoutContext('hot restart succeeds when removing a field from a const class', () async { + await flutter.run(chrome: true, additionalCommandArgs: additionalCommandArgs); + project.removeFieldFromConstClass(); + await flutter.hotRestart(); + }); + }); +} diff --git a/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart b/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart index 070c47bd761f7..41f7b1f93788e 100644 --- a/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart +++ b/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart @@ -5,60 +5,21 @@ @Tags(['flutter-test-driver']) library; -import 'package:file/file.dart'; +import 'dart:io'; -import '../integration.shard/test_data/hot_reload_const_project.dart'; -import '../integration.shard/test_driver.dart'; -import '../integration.shard/test_utils.dart'; +import '../integration.shard/test_data/hot_reload_errors_common.dart'; import '../src/common.dart'; void main() { - late Directory tempDir; - final HotReloadConstProject project = HotReloadConstProject(); - late FlutterRunTestDriver flutter; - - final List additionalCommandArgs = [ - '--extra-front-end-options=--dartdevc-canary,--dartdevc-module-format=ddc', - ]; - - setUp(() async { - tempDir = createResolvedTempDirectorySync('hot_reload_test.'); - await project.setUpIn(tempDir); - flutter = FlutterRunTestDriver(tempDir); - }); - - tearDown(() async { - await flutter.stop(); - tryToDelete(tempDir); - }); - - testWithoutContext( - 'hot reload displays an error message when removing a field from a const class', - () async { - await flutter.run(chrome: true, additionalCommandArgs: additionalCommandArgs); - project.removeFieldFromConstClass(); - - expect( - flutter.hotReload(), - throwsA( - isA().having( - (Exception e) => e.toString(), - 'message', - // TODO(srujzs): Change this to "Try performing a hot restart instead." once we emit - // that string in the delta inspector. Then unify this test with - // hot_reload_errors_test.dart. - contains('Const class cannot remove fields'), - ), - ), - ); - }, + testAll( + chrome: true, + additionalCommandArgs: [ + '--extra-front-end-options=--dartdevc-canary,--dartdevc-module-format=ddc', + ], + // TODO(srujzs): Remove this custom message once we have the delta inspector emitting the same + // string as the VM. + constClassFieldRemovalErrorMessage: 'Const class cannot remove fields', + // https://github.com/flutter/flutter/issues/162567 + skip: Platform.isWindows, ); - - testWithoutContext('hot restart succeeds when removing a field from a const class', () async { - // We should have used `recompile-restart` to avoid the errors the DDC delta - // inspector emits. - await flutter.run(chrome: true, additionalCommandArgs: additionalCommandArgs); - project.removeFieldFromConstClass(); - await flutter.hotRestart(); - }); }