From ea1170426c9187534f736fe7fab82f17c3b82a2f Mon Sep 17 00:00:00 2001 From: Derek Xu Date: Tue, 2 Apr 2024 22:07:47 +0000 Subject: [PATCH 1/8] [CLI] Improve documentation of `dart run --resident` and `dart compilation-server` TEST=CI Issue: https://github.com/dart-lang/sdk/issues/54245 Change-Id: I1806e1ef62e17d626e1bc37a10f67e6e8b402a0a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/358741 Reviewed-by: Ben Konyi --- .../lib/src/commands/compilation_server.dart | 17 +++++++---------- pkg/dartdev/lib/src/commands/run.dart | 13 +++++++++---- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/pkg/dartdev/lib/src/commands/compilation_server.dart b/pkg/dartdev/lib/src/commands/compilation_server.dart index 931edaabd4f4..a01b683270e9 100644 --- a/pkg/dartdev/lib/src/commands/compilation_server.dart +++ b/pkg/dartdev/lib/src/commands/compilation_server.dart @@ -16,15 +16,14 @@ import '../utils.dart'; class CompilationServerCommand extends DartdevCommand { static const commandName = 'compilation-server'; - static const commandDescription = 'Control the resident frontend compiler.'; + static const commandDescription = 'Control resident frontend compilers.'; static const residentServerInfoFileFlag = 'resident-server-info-file'; static const residentServerInfoFileFlagDescription = - 'Specify the file that the Dart CLI uses to communicate with the ' - 'resident frontend compiler. Passing this flag results in having one ' - 'unique resident frontend compiler per file. This is needed when ' - 'writing unit tests that utilize resident mode in order to maintain ' - 'isolation.'; + 'The path to an info file that the Dart CLI will use to communicate with ' + 'a resident frontend compiler. Each unique info file is associated with ' + 'a unique resident frontend compiler. If this flag is ommitted, the ' + 'default info file will be used.'; CompilationServerCommand({bool verbose = false}) : super( @@ -41,7 +40,7 @@ class CompilationServerCommand extends DartdevCommand { class CompilationServerStartCommand extends DartdevCommand { static const commandName = 'start'; - static const commandDescription = 'Starts the resident frontend compiler.'; + static const commandDescription = 'Start a resident frontend compiler.'; CompilationServerStartCommand({bool verbose = false}) : super( @@ -82,9 +81,7 @@ class CompilationServerShutdownCommand extends DartdevCommand { static const commandName = 'shutdown'; static const commandDescription = ''' -Shut down the resident frontend compiler. - -Frontend compilers stay resident in memory when using 'dart run --resident'. This command will shutdown any remaining frontend compiler processes. +Shut down a resident frontend compiler. Note that this command name and usage could change as we evolve the resident frontend compiler behavior.'''; diff --git a/pkg/dartdev/lib/src/commands/run.dart b/pkg/dartdev/lib/src/commands/run.dart index 4e748c93aa5a..1906e75219ba 100644 --- a/pkg/dartdev/lib/src/commands/run.dart +++ b/pkg/dartdev/lib/src/commands/run.dart @@ -52,12 +52,17 @@ class RunCommand extends DartdevCommand { ) { argParser ..addFlag( - 'resident', + residentOption, abbr: 'r', negatable: false, - help: - 'Enable faster startup times with the resident frontend compiler.\n' - "See 'dart ${CompilationServerCommand.commandName} -h' for more information.", + help: 'Enable faster startup times by using a resident frontend ' + 'compiler for compilation.\n' + 'If --resident-server-info-file is provided in conjunction with ' + 'this flag, the specified info file will be used, otherwise the ' + 'default info file will be used. If there is not already a ' + 'compiler associated with the selected info file, one will be ' + "started. Refer to 'dart ${CompilationServerCommand.commandName} " + "start -h' for more information about info files.", hide: !verbose, ) ..addOption( From 1ea64da84bf45d26183f178e81a0fe8598b2a738 Mon Sep 17 00:00:00 2001 From: Derek Xu Date: Tue, 2 Apr 2024 22:07:47 +0000 Subject: [PATCH 2/8] [CLI] Replace --resident-server-info-file option with --resident-compiler-info-file option For backwards compatibility reasons, the arg parser will still understand --resident-server-info-file, but it will not print a help message for it. TEST=pkg/dartdev/test/commands/compilation_server_test.dart and pkg/dartdev/test/commands/compilation_server_test.dart Issue: https://github.com/dart-lang/sdk/issues/54245 Change-Id: I59a1a7c495194ff3c48de7fb65255de5d9f150f4 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/359660 Reviewed-by: Ben Konyi --- .../lib/src/commands/compilation_server.dart | 73 +++++++++++++------ pkg/dartdev/lib/src/commands/run.dart | 34 +++++---- .../lib/src/resident_frontend_constants.dart | 2 +- .../commands/compilation_server_test.dart | 51 ++++++++++--- pkg/dartdev/test/commands/run_test.dart | 71 ++++++++++++------ 5 files changed, 162 insertions(+), 69 deletions(-) diff --git a/pkg/dartdev/lib/src/commands/compilation_server.dart b/pkg/dartdev/lib/src/commands/compilation_server.dart index a01b683270e9..56aae7a0025d 100644 --- a/pkg/dartdev/lib/src/commands/compilation_server.dart +++ b/pkg/dartdev/lib/src/commands/compilation_server.dart @@ -18,8 +18,9 @@ class CompilationServerCommand extends DartdevCommand { static const commandDescription = 'Control resident frontend compilers.'; - static const residentServerInfoFileFlag = 'resident-server-info-file'; - static const residentServerInfoFileFlagDescription = + static const legacyResidentServerInfoFileFlag = 'resident-server-info-file'; + static const residentCompilerInfoFileFlag = residentCompilerInfoFileOption; + static const residentCompilerInfoFileFlagDescription = 'The path to an info file that the Dart CLI will use to communicate with ' 'a resident frontend compiler. Each unique info file is associated with ' 'a unique resident frontend compiler. If this flag is ommitted, the ' @@ -49,22 +50,31 @@ class CompilationServerStartCommand extends DartdevCommand { false, hidden: !verbose, ) { - argParser.addOption( - CompilationServerCommand.residentServerInfoFileFlag, - help: CompilationServerCommand.residentServerInfoFileFlagDescription, - ); + argParser + ..addOption( + CompilationServerCommand.residentCompilerInfoFileFlag, + help: CompilationServerCommand.residentCompilerInfoFileFlagDescription, + ) + ..addOption( + CompilationServerCommand.legacyResidentServerInfoFileFlag, + // This option is only available for backwards compatibility, and should + // never be shown in the help message. + hide: true, + ); } @override FutureOr run() async { final args = argResults!; - final hasServerInfoOption = args.wasParsed(serverInfoOption); - final residentServerInfoFile = hasServerInfoOption - ? File(maybeUriToFilename(args.option(serverInfoOption)!)) + final String? infoFileArg = + args[CompilationServerCommand.residentCompilerInfoFileFlag] ?? + args[CompilationServerCommand.legacyResidentServerInfoFileFlag]; + final residentCompilerInfoFile = infoFileArg != null + ? File(maybeUriToFilename(infoFileArg)) : defaultResidentServerInfoFile; try { - await ensureCompilationServerIsRunning(residentServerInfoFile!); + await ensureCompilationServerIsRunning(residentCompilerInfoFile!); } catch (e) { // We already print the error in `ensureCompilationServerIsRunning` when we // throw a state error. @@ -87,10 +97,17 @@ Note that this command name and usage could change as we evolve the resident fro CompilationServerShutdownCommand({bool verbose = false}) : super(commandName, commandDescription, false, hidden: !verbose) { - argParser.addOption( - CompilationServerCommand.residentServerInfoFileFlag, - help: CompilationServerCommand.residentServerInfoFileFlagDescription, - ); + argParser + ..addOption( + CompilationServerCommand.residentCompilerInfoFileFlag, + help: CompilationServerCommand.residentCompilerInfoFileFlagDescription, + ) + ..addOption( + CompilationServerCommand.legacyResidentServerInfoFileFlag, + // This option is only available for backwards compatibility, and should + // never be shown in the help message. + hide: true, + ); } // This argument parser is here solely to ensure that VM specific flags are @@ -104,21 +121,33 @@ Note that this command name and usage could change as we evolve the resident fro @override FutureOr run() async { final args = argResults!; - final serverInfoFile = args.wasParsed(serverInfoOption) - ? File(maybeUriToFilename(args.option(serverInfoOption)!)) - : defaultResidentServerInfoFile; + final File? residentCompilerInfoFile; + if (args.wasParsed(CompilationServerCommand.residentCompilerInfoFileFlag)) { + residentCompilerInfoFile = File(maybeUriToFilename( + args[CompilationServerCommand.residentCompilerInfoFileFlag], + )); + } else if (args.wasParsed( + CompilationServerCommand.legacyResidentServerInfoFileFlag, + )) { + residentCompilerInfoFile = File(maybeUriToFilename( + args[CompilationServerCommand.legacyResidentServerInfoFileFlag], + )); + } else { + residentCompilerInfoFile = defaultResidentServerInfoFile; + } - if (serverInfoFile == null || !serverInfoFile.existsSync()) { - log.stdout('No server instance running.'); + if (residentCompilerInfoFile == null || + !residentCompilerInfoFile.existsSync()) { + log.stdout('No resident frontend compiler instance running.'); return 0; } - final serverInfo = await serverInfoFile.readAsString(); + final serverInfo = await residentCompilerInfoFile.readAsString(); final serverResponse = await sendAndReceiveResponse( residentServerShutdownCommand, - serverInfoFile, + residentCompilerInfoFile, ); - cleanupResidentServerInfo(serverInfoFile); + cleanupResidentServerInfo(residentCompilerInfoFile); if (serverResponse.containsKey(responseErrorString)) { log.stderr(serverResponse[responseErrorString]); return DartdevCommand.errorExitCode; diff --git a/pkg/dartdev/lib/src/commands/run.dart b/pkg/dartdev/lib/src/commands/run.dart index 1906e75219ba..ea096b17aaaf 100644 --- a/pkg/dartdev/lib/src/commands/run.dart +++ b/pkg/dartdev/lib/src/commands/run.dart @@ -57,7 +57,7 @@ class RunCommand extends DartdevCommand { negatable: false, help: 'Enable faster startup times by using a resident frontend ' 'compiler for compilation.\n' - 'If --resident-server-info-file is provided in conjunction with ' + 'If --resident-compiler-info-file is provided in conjunction with ' 'this flag, the specified info file will be used, otherwise the ' 'default info file will be used. If there is not already a ' 'compiler associated with the selected info file, one will be ' @@ -66,9 +66,15 @@ class RunCommand extends DartdevCommand { hide: !verbose, ) ..addOption( - CompilationServerCommand.residentServerInfoFileFlag, + CompilationServerCommand.residentCompilerInfoFileFlag, hide: !verbose, - help: CompilationServerCommand.residentServerInfoFileFlagDescription, + help: CompilationServerCommand.residentCompilerInfoFileFlagDescription, + ) + ..addOption( + CompilationServerCommand.legacyResidentServerInfoFileFlag, + // This option is only available for backwards compatibility, and should + // never be shown in the help message. + hide: true, ); // NOTE: When updating this list of flags, be sure to add any VM flags to // the list of flags in Options::ProcessVMDebuggingOptions in @@ -317,9 +323,11 @@ class RunCommand extends DartdevCommand { nativeAssets = assets?.toFilePath(); } - final hasServerInfoOption = args.wasParsed(serverInfoOption); + final String? residentCompilerInfoFileArg = + args[CompilationServerCommand.residentCompilerInfoFileFlag] ?? + args[CompilationServerCommand.legacyResidentServerInfoFileFlag]; final useResidentServer = - args.wasParsed(residentOption) || hasServerInfoOption; + args.wasParsed(residentOption) || residentCompilerInfoFileArg != null; DartExecutableWithPackageConfig executable; final hasExperiments = args.enabledExperiments.isNotEmpty; try { @@ -333,21 +341,21 @@ class RunCommand extends DartdevCommand { return errorExitCode; } - final residentServerInfoFile = hasServerInfoOption - ? File(maybeUriToFilename(args.option(serverInfoOption)!)) + final residentCompilerInfoFile = residentCompilerInfoFileArg != null + ? File(maybeUriToFilename(residentCompilerInfoFileArg)) : defaultResidentServerInfoFile; - if (useResidentServer && residentServerInfoFile != null) { + if (useResidentServer && residentCompilerInfoFile != null) { try { // Ensure the parent directory exists. - if (!residentServerInfoFile.parent.existsSync()) { - residentServerInfoFile.parent.createSync(); + if (!residentCompilerInfoFile.parent.existsSync()) { + residentCompilerInfoFile.parent.createSync(); } // TODO(#49694) handle the case when executable is a kernel file executable = await generateKernel( executable, - residentServerInfoFile, + residentCompilerInfoFile, args, createCompileJitJson, ); @@ -359,11 +367,11 @@ class RunCommand extends DartdevCommand { try { await sendAndReceiveResponse( residentServerShutdownCommand, - residentServerInfoFile, + residentCompilerInfoFile, ); } catch (_) { } finally { - cleanupResidentServerInfo(residentServerInfoFile); + cleanupResidentServerInfo(residentCompilerInfoFile); } } return errorExitCode; diff --git a/pkg/dartdev/lib/src/resident_frontend_constants.dart b/pkg/dartdev/lib/src/resident_frontend_constants.dart index 2573404198da..1c7c2bda4dbb 100644 --- a/pkg/dartdev/lib/src/resident_frontend_constants.dart +++ b/pkg/dartdev/lib/src/resident_frontend_constants.dart @@ -4,7 +4,7 @@ /// The name of the subdirectory in .dart_tool where dartdev stores kernel files const dartdevKernelCache = 'kernel'; -const serverInfoOption = 'resident-server-info-file'; +const residentCompilerInfoFileOption = 'resident-compiler-info-file'; const residentOption = 'resident'; const responseSuccessString = 'success'; const responseErrorString = 'errorMessage'; diff --git a/pkg/dartdev/test/commands/compilation_server_test.dart b/pkg/dartdev/test/commands/compilation_server_test.dart index c1ae25f3794e..ed1020442f94 100644 --- a/pkg/dartdev/test/commands/compilation_server_test.dart +++ b/pkg/dartdev/test/commands/compilation_server_test.dart @@ -27,10 +27,13 @@ void main() { final result = await p.run([ 'compilation-server', 'shutdown', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', ]); - expect(result.stdout, contains('No server instance running')); + expect( + result.stdout, + contains('No resident frontend compiler instance running'), + ); expect(result.stderr, isEmpty); expect(result.exitCode, 0); expect(File(serverInfoFile).existsSync(), false); @@ -41,7 +44,7 @@ void main() { final serverInfoFile = path.join(p.dirPath, 'info'); final runResult = await p.run([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', p.relativeFilePath, ]); @@ -53,7 +56,7 @@ void main() { final result = await p.run([ 'compilation-server', 'shutdown', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', ]); expect(result.stdout, matches(compilationServerShutdownRegExp)); @@ -68,7 +71,7 @@ void main() { final runResult = await p.run([ 'compilation-server', 'start', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', ]); expect(runResult.stdout, matches(compilationServerStartRegExp)); @@ -79,7 +82,7 @@ void main() { final result = await p.run([ 'compilation-server', 'shutdown', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', ]); expect(result.stdout, matches(compilationServerShutdownRegExp)); @@ -89,14 +92,14 @@ void main() { }); test( - 'start and shutdown when passing a relative path to --resident-server-info-file', + 'start and shutdown when using legacy --resident-server-info-file option', () async { p = project(mainSrc: 'void main() {}'); final serverInfoFile = path.join(p.dirPath, 'info'); final runResult = await p.run([ 'compilation-server', 'start', - '--$serverInfoOption=${path.relative(serverInfoFile, from: p.dirPath)}', + '--resident-server-info-file=$serverInfoFile', ]); expect(runResult.stdout, matches(compilationServerStartRegExp)); @@ -107,7 +110,37 @@ void main() { final result = await p.run([ 'compilation-server', 'shutdown', - '--$serverInfoOption=${path.relative(serverInfoFile, from: p.dirPath)}', + '--resident-server-info-file=$serverInfoFile', + ]); + + expect(result.stdout, matches(compilationServerShutdownRegExp)); + expect(result.stderr, isEmpty); + expect(result.exitCode, 0); + expect(File(serverInfoFile).existsSync(), false); + }); + + test( + 'start and shutdown when passing a relative path to --resident-compiler-info-file', + () async { + p = project(mainSrc: 'void main() {}'); + final serverInfoFile = path.join(p.dirPath, 'info'); + final runResult = await p.run([ + 'compilation-server', + 'start', + '--$residentCompilerInfoFileOption', + path.relative(serverInfoFile, from: p.dirPath), + ]); + + expect(runResult.stdout, matches(compilationServerStartRegExp)); + expect(runResult.stderr, isEmpty); + expect(runResult.exitCode, 0); + expect(File(serverInfoFile).existsSync(), true); + + final result = await p.run([ + 'compilation-server', + 'shutdown', + '--$residentCompilerInfoFileOption', + path.relative(serverInfoFile, from: p.dirPath), ]); expect(result.stdout, matches(compilationServerShutdownRegExp)); diff --git a/pkg/dartdev/test/commands/run_test.dart b/pkg/dartdev/test/commands/run_test.dart index b743e6a3fe79..3a1540db313b 100644 --- a/pkg/dartdev/test/commands/run_test.dart +++ b/pkg/dartdev/test/commands/run_test.dart @@ -21,7 +21,7 @@ const dartVMServiceMessagePrefix = 'The Dart VM service is listening on http://127.0.0.1:'; final dartVMServiceRegExp = RegExp(r'The Dart VM service is listening on (http://127.0.0.1:.*)'); -const residentFrontendServerPrefix = +const residentFrontendCompilerPrefix = 'The Resident Frontend Compiler is listening at 127.0.0.1:'; const dtdMessagePrefix = 'The Dart Tooling Daemon (DTD) is available at:'; @@ -858,7 +858,7 @@ void residentRun() { serverInfoFile = path.join(serverInfoDirectory.dirPath, 'info'); final result = await serverInfoDirectory.run([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', serverInfoDirectory.relativeFilePath, ]); expect(result.exitCode, 0); @@ -885,7 +885,7 @@ void residentRun() { p = project(mainSrc: "void main() { print('Hello World'); }"); final result = await p.run([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', p.relativeFilePath, ]); Directory? kernelCache = p.findDirectory('.dart_tool/kernel'); @@ -895,19 +895,19 @@ void residentRun() { result.stdout, allOf( contains('Hello World'), - isNot(contains(residentFrontendServerPrefix)), + isNot(contains(residentFrontendCompilerPrefix)), ), ); expect(result.stderr, isEmpty); expect(kernelCache, isNot(null)); }); - test('--resident-server-info-file handles relative paths correctly', + test("'Hello World' with legacy --resident-server-info-file option", () async { p = project(mainSrc: "void main() { print('Hello World'); }"); final result = await p.run([ 'run', - '--$serverInfoOption=${path.relative(serverInfoFile, from: p.dirPath)}', + '--resident-server-info-file=$serverInfoFile', p.relativeFilePath, ]); Directory? kernelCache = p.findDirectory('.dart_tool/kernel'); @@ -917,7 +917,30 @@ void residentRun() { result.stdout, allOf( contains('Hello World'), - isNot(contains(residentFrontendServerPrefix)), + isNot(contains(residentFrontendCompilerPrefix)), + ), + ); + expect(result.stderr, isEmpty); + expect(kernelCache, isNot(null)); + }); + + test('--resident-compiler-info-file handles relative paths correctly', + () async { + p = project(mainSrc: "void main() { print('Hello World'); }"); + final result = await p.run([ + 'run', + '--$residentCompilerInfoFileOption', + path.relative(serverInfoFile, from: p.dirPath), + p.relativeFilePath, + ]); + Directory? kernelCache = p.findDirectory('.dart_tool/kernel'); + + expect(result.exitCode, 0); + expect( + result.stdout, + allOf( + contains('Hello World'), + isNot(contains(residentFrontendCompilerPrefix)), ), ); expect(result.stderr, isEmpty); @@ -933,7 +956,7 @@ void residentRun() { ); final result = await p.run([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', '--enable-experiment=test-experiment', p.relativeFilePath, ]); @@ -944,7 +967,7 @@ void residentRun() { result.stdout, allOf( contains('hello'), - isNot(contains(residentFrontendServerPrefix)), + isNot(contains(residentFrontendCompilerPrefix)), ), ); expect(result.exitCode, 0); @@ -958,12 +981,12 @@ void residentRun() { final runResult1 = await p.run([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', p.relativeFilePath, ]); final runResult2 = await p2.run([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', p2.relativeFilePath, ]); @@ -972,14 +995,14 @@ void residentRun() { runResult1.stdout, allOf( contains('1'), - isNot(contains(residentFrontendServerPrefix)), + isNot(contains(residentFrontendCompilerPrefix)), ), ); expect( runResult2.stdout, allOf( contains('2'), - isNot(contains(residentFrontendServerPrefix)), + isNot(contains(residentFrontendCompilerPrefix)), ), ); }); @@ -991,7 +1014,7 @@ void residentRun() { final runResult1 = await p.run([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', path.join(p.dirPath, 'lib/main.dart'), ]); expect(runResult1.exitCode, 0); @@ -1000,7 +1023,7 @@ void residentRun() { final runResult2 = await p.run([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', path.join(p.dirPath, 'bin/main.dart'), ]); expect(runResult2.exitCode, 0); @@ -1018,7 +1041,7 @@ void residentRun() { p.deleteFile('.dart_tool/package_config.json'); final runResult = await p.run([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', p.relativeFilePath, ]); @@ -1049,27 +1072,27 @@ void residentRun() { TestProject p2 = project(mainSrc: 'void main() {}'); final runResult1 = await p2.run([ 'run', - '--$serverInfoOption=$tempServerInfoFile', + '--$residentCompilerInfoFileOption=$tempServerInfoFile', p2.relativeFilePath, ]); await deleteDirectory(p2.dir); expect(runResult1.exitCode, 0); - expect(runResult1.stdout, contains(residentFrontendServerPrefix)); + expect(runResult1.stdout, contains(residentFrontendCompilerPrefix)); await p.run([ 'run', - '--$serverInfoOption=$tempServerInfoFile', + '--$residentCompilerInfoFileOption=$tempServerInfoFile', p.relativeFilePath, ]); final runResult2 = await p.run([ 'run', - '--$serverInfoOption=$tempServerInfoFile', + '--$residentCompilerInfoFileOption=$tempServerInfoFile', p.relativeFilePath, ]); expect(runResult2.exitCode, 0); expect(runResult2.stderr, isEmpty); - expect(runResult2.stdout, isNot(contains(residentFrontendServerPrefix))); + expect(runResult2.stdout, isNot(contains(residentFrontendCompilerPrefix))); }); test('VM flags are passed properly', () async { @@ -1109,7 +1132,7 @@ void residentRun() { await p.runWithVmService([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', '--observe=0', '--pause-isolates-on-start', // This should negate the above flag. @@ -1130,7 +1153,7 @@ void residentRun() { sawCFEMsg = false; await p.runWithVmService([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', '--observe=0', '--pause-isolates-on-start', // This should negate the above flag. @@ -1173,7 +1196,7 @@ void residentRun() { await p.runWithVmService([ 'run', - '--$serverInfoOption=$serverInfoFile', + '--$residentCompilerInfoFileOption=$serverInfoFile', '--observe=0/::1', '--pause-isolates-on-start', // This should negate the above flag. From c490c4bd4caf2013ec0b139b314e1c9a502e193e Mon Sep 17 00:00:00 2001 From: Derek Xu Date: Tue, 2 Apr 2024 22:07:47 +0000 Subject: [PATCH 3/8] [CLI] Make passing --resident a prerequisite for passing --resident-compiler-info-file TEST=test case added to pkg/dartdev/test/commands/compilation_server_test.dart Issue: https://github.com/dart-lang/sdk/issues/54245 Change-Id: I3d982a8d85b63fed8510144bade71e2c95ced121 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/359581 Reviewed-by: Ben Konyi Commit-Queue: Derek Xu --- pkg/dartdev/lib/src/commands/run.dart | 15 +++++--- .../commands/compilation_server_test.dart | 1 + pkg/dartdev/test/commands/run_test.dart | 35 +++++++++++++++++++ 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/pkg/dartdev/lib/src/commands/run.dart b/pkg/dartdev/lib/src/commands/run.dart index ea096b17aaaf..d9df823c2a87 100644 --- a/pkg/dartdev/lib/src/commands/run.dart +++ b/pkg/dartdev/lib/src/commands/run.dart @@ -326,14 +326,21 @@ class RunCommand extends DartdevCommand { final String? residentCompilerInfoFileArg = args[CompilationServerCommand.residentCompilerInfoFileFlag] ?? args[CompilationServerCommand.legacyResidentServerInfoFileFlag]; - final useResidentServer = - args.wasParsed(residentOption) || residentCompilerInfoFileArg != null; + final useResidentCompiler = args.wasParsed(residentOption); + if (residentCompilerInfoFileArg != null && !useResidentCompiler) { + log.stderr( + 'Error: the --resident flag must be passed whenever the ' + '--resident-compiler-info-file option is passed.', + ); + return errorExitCode; + } + DartExecutableWithPackageConfig executable; final hasExperiments = args.enabledExperiments.isNotEmpty; try { executable = await getExecutableForCommand( mainCommand, - allowSnapshot: !(useResidentServer || hasExperiments), + allowSnapshot: !(useResidentCompiler || hasExperiments), nativeAssets: nativeAssets, ); } on CommandResolutionFailedException catch (e) { @@ -345,7 +352,7 @@ class RunCommand extends DartdevCommand { ? File(maybeUriToFilename(residentCompilerInfoFileArg)) : defaultResidentServerInfoFile; - if (useResidentServer && residentCompilerInfoFile != null) { + if (useResidentCompiler && residentCompilerInfoFile != null) { try { // Ensure the parent directory exists. if (!residentCompilerInfoFile.parent.existsSync()) { diff --git a/pkg/dartdev/test/commands/compilation_server_test.dart b/pkg/dartdev/test/commands/compilation_server_test.dart index ed1020442f94..9d90a6f59b12 100644 --- a/pkg/dartdev/test/commands/compilation_server_test.dart +++ b/pkg/dartdev/test/commands/compilation_server_test.dart @@ -44,6 +44,7 @@ void main() { final serverInfoFile = path.join(p.dirPath, 'info'); final runResult = await p.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', p.relativeFilePath, ]); diff --git a/pkg/dartdev/test/commands/run_test.dart b/pkg/dartdev/test/commands/run_test.dart index 3a1540db313b..44254370c59f 100644 --- a/pkg/dartdev/test/commands/run_test.dart +++ b/pkg/dartdev/test/commands/run_test.dart @@ -858,6 +858,7 @@ void residentRun() { serverInfoFile = path.join(serverInfoDirectory.dirPath, 'info'); final result = await serverInfoDirectory.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', serverInfoDirectory.relativeFilePath, ]); @@ -881,10 +882,30 @@ void residentRun() { } catch (_) {} }); + test( + 'passing --resident is a prerequisite for passing --resident-compiler-info-file', + () async { + p = project(mainSrc: 'void main() {}'); + final result = await p.run([ + 'run', + '--$residentCompilerInfoFileOption=$serverInfoFile', + p.relativeFilePath, + ]); + + expect(result.exitCode, 255); + expect( + result.stderr, + contains( + 'Error: the --resident flag must be passed whenever the --resident-compiler-info-file option is passed.', + ), + ); + }); + test("'Hello World'", () async { p = project(mainSrc: "void main() { print('Hello World'); }"); final result = await p.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', p.relativeFilePath, ]); @@ -907,6 +928,7 @@ void residentRun() { p = project(mainSrc: "void main() { print('Hello World'); }"); final result = await p.run([ 'run', + '--resident', '--resident-server-info-file=$serverInfoFile', p.relativeFilePath, ]); @@ -929,6 +951,7 @@ void residentRun() { p = project(mainSrc: "void main() { print('Hello World'); }"); final result = await p.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption', path.relative(serverInfoFile, from: p.dirPath), p.relativeFilePath, @@ -956,6 +979,7 @@ void residentRun() { ); final result = await p.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', '--enable-experiment=test-experiment', p.relativeFilePath, @@ -981,11 +1005,13 @@ void residentRun() { final runResult1 = await p.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', p.relativeFilePath, ]); final runResult2 = await p2.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', p2.relativeFilePath, ]); @@ -1014,6 +1040,7 @@ void residentRun() { final runResult1 = await p.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', path.join(p.dirPath, 'lib/main.dart'), ]); @@ -1023,6 +1050,7 @@ void residentRun() { final runResult2 = await p.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', path.join(p.dirPath, 'bin/main.dart'), ]); @@ -1041,6 +1069,7 @@ void residentRun() { p.deleteFile('.dart_tool/package_config.json'); final runResult = await p.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', p.relativeFilePath, ]); @@ -1072,6 +1101,7 @@ void residentRun() { TestProject p2 = project(mainSrc: 'void main() {}'); final runResult1 = await p2.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$tempServerInfoFile', p2.relativeFilePath, ]); @@ -1081,11 +1111,13 @@ void residentRun() { await p.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$tempServerInfoFile', p.relativeFilePath, ]); final runResult2 = await p.run([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$tempServerInfoFile', p.relativeFilePath, ]); @@ -1132,6 +1164,7 @@ void residentRun() { await p.runWithVmService([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', '--observe=0', '--pause-isolates-on-start', @@ -1153,6 +1186,7 @@ void residentRun() { sawCFEMsg = false; await p.runWithVmService([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', '--observe=0', '--pause-isolates-on-start', @@ -1196,6 +1230,7 @@ void residentRun() { await p.runWithVmService([ 'run', + '--resident', '--$residentCompilerInfoFileOption=$serverInfoFile', '--observe=0/::1', '--pause-isolates-on-start', From e44538986281273c5cc70fbc24ebabc9112b7095 Mon Sep 17 00:00:00 2001 From: Jonas Termansen Date: Tue, 2 Apr 2024 22:54:19 +0000 Subject: [PATCH 4/8] [infra] Remove Goma support. Bug: b/296994239 Change-Id: Ic96571d2c16d7c252cb41673e8bdb16cdb4de754 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/360507 Commit-Queue: Jonas Termansen Reviewed-by: Alexander Thomas Reviewed-by: William Hesse --- build/config/clang/clang.gni | 6 ----- build/config/compiler/BUILD.gn | 7 +----- build/config/mac/mac_sdk.gni | 5 ++-- build/toolchain/android/BUILD.gn | 8 +------ build/toolchain/fuchsia/BUILD.gn | 8 +------ build/toolchain/goma.gni | 22 ------------------ build/toolchain/linux/BUILD.gn | 9 +------ build/toolchain/mac/BUILD.gn | 11 ++------- build/toolchain/win/BUILD.gn | 5 +--- tools/build.py | 40 ++++++++++---------------------- tools/gn.py | 37 ----------------------------- 11 files changed, 21 insertions(+), 137 deletions(-) delete mode 100644 build/toolchain/goma.gni diff --git a/build/config/clang/clang.gni b/build/config/clang/clang.gni index 49cc5097ac06..f6d9d897d505 100644 --- a/build/config/clang/clang.gni +++ b/build/config/clang/clang.gni @@ -2,13 +2,7 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. -import("../../toolchain/goma.gni") - _toolchain_cpu = host_cpu -if (host_os == "mac" && use_goma) { - # Goma does not support ARM64. - _toolchain_cpu = "x64" -} default_clang_prefix = rebase_path("//buildtools/${host_os}-${_toolchain_cpu}/clang/bin", diff --git a/build/config/compiler/BUILD.gn b/build/config/compiler/BUILD.gn index 551817d5e328..247de346cc5c 100644 --- a/build/config/compiler/BUILD.gn +++ b/build/config/compiler/BUILD.gn @@ -921,12 +921,7 @@ config("optimize_speed") { config("symbols") { if (is_win) { - import("//build/toolchain/goma.gni") - if (use_goma) { - cflags = [ "/Z7" ] # No PDB file - } else { - cflags = [ "/Zi" ] # Produce PDB file, no edit and continue. - } + cflags = [ "/Zi" ] # Produce PDB file, no edit and continue. ldflags = [ "/DEBUG" ] } else { cflags = [ diff --git a/build/config/mac/mac_sdk.gni b/build/config/mac/mac_sdk.gni index 25cd5b4d72ac..7d3f337b58df 100644 --- a/build/config/mac/mac_sdk.gni +++ b/build/config/mac/mac_sdk.gni @@ -2,7 +2,6 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. -import("//build/toolchain/goma.gni") import("//build/toolchain/rbe.gni") declare_args() { @@ -18,7 +17,7 @@ declare_args() { # mac_sdk_min is used. mac_sdk_path = "" - mac_enable_relative_sdk_path = use_rbe || use_goma + mac_enable_relative_sdk_path = use_rbe } find_sdk_args = [ @@ -26,7 +25,7 @@ find_sdk_args = [ mac_sdk_min, ] -if (use_rbe || use_goma) { +if (use_rbe) { find_sdk_args += [ "--create_symlink_at", diff --git a/build/toolchain/android/BUILD.gn b/build/toolchain/android/BUILD.gn index 79a672d1c492..728cb58a7cc8 100644 --- a/build/toolchain/android/BUILD.gn +++ b/build/toolchain/android/BUILD.gn @@ -5,7 +5,6 @@ import("//build/config/sysroot.gni") # Imports android/config.gni. import("//build/toolchain/ccache.gni") import("//build/toolchain/gcc_toolchain.gni") -import("//build/toolchain/goma.gni") import("//build/toolchain/rbe.gni") # The Android GCC toolchains share most of the same parameters, so we have this @@ -20,12 +19,7 @@ import("//build/toolchain/rbe.gni") # Same as gcc_toolchain template("android_toolchain") { gcc_toolchain(target_name) { - if (use_goma) { - assert(!use_ccache, "Goma and ccache can't be used together.") - assembler_prefix = "$goma_dir/gomacc " - compiler_prefix = "$goma_dir/gomacc " - link_prefix = "$goma_dir/gomacc " - } else if (use_rbe) { + if (use_rbe) { compiler_args = rewrapper_args + [ "--labels=type=compile,compiler=clang,lang=cpp" ] diff --git a/build/toolchain/fuchsia/BUILD.gn b/build/toolchain/fuchsia/BUILD.gn index 0e75d8a84299..2ddee0326ca4 100644 --- a/build/toolchain/fuchsia/BUILD.gn +++ b/build/toolchain/fuchsia/BUILD.gn @@ -5,15 +5,9 @@ import("//build/config/sysroot.gni") import("//build/toolchain/ccache.gni") import("//build/toolchain/gcc_toolchain.gni") -import("//build/toolchain/goma.gni") import("//build/toolchain/rbe.gni") -if (use_goma) { - assert(!use_ccache, "Goma and ccache can't be used together.") - assembler_prefix = "$goma_dir/gomacc " - compiler_prefix = "$goma_dir/gomacc " - link_prefix = "$goma_dir/gomacc " -} else if (use_ccache) { +if (use_ccache) { assembler_prefix = "ccache " compiler_prefix = "ccache " link_prefix = "ccache " diff --git a/build/toolchain/goma.gni b/build/toolchain/goma.gni deleted file mode 100644 index c0f4cf282528..000000000000 --- a/build/toolchain/goma.gni +++ /dev/null @@ -1,22 +0,0 @@ -# Copyright (c) 2013 The Chromium Authors. All rights reserved. -# Use of this source code is governed by a BSD-style license that can be -# found in the LICENSE file. - -# Defines the configuration of Goma. -# -# This is currently designed to match the GYP build exactly, so as not to break -# people during the transition. - -declare_args() { - # Set to true to enable distributed compilation using Goma. - use_goma = false - - # Set the default value based on the platform. - if (is_win) { - # Absolute directory containing the Goma source code. - goma_dir = "C:\goma\goma-win" - } else { - # Absolute directory containing the Goma source code. - goma_dir = getenv("HOME") + "/goma" - } -} diff --git a/build/toolchain/linux/BUILD.gn b/build/toolchain/linux/BUILD.gn index ec670afe15be..9cc6e0ad3274 100644 --- a/build/toolchain/linux/BUILD.gn +++ b/build/toolchain/linux/BUILD.gn @@ -5,7 +5,6 @@ import("//build/config/sysroot.gni") import("//build/toolchain/ccache.gni") import("//build/toolchain/gcc_toolchain.gni") -import("//build/toolchain/goma.gni") import("//build/toolchain/rbe.gni") declare_args() { @@ -17,13 +16,7 @@ declare_args() { riscv64_toolchain_prefix = "" } -if (use_goma) { - assert(!use_ccache, "Goma and ccache can't be used together.") - assembler_prefix = "$goma_dir/gomacc " - compiler_prefix = "$goma_dir/gomacc " - link_prefix = "$goma_dir/gomacc " - gcc_compiler_prefix = compiler_prefix -} else if (use_rbe) { +if (use_rbe) { compiler_args = rewrapper_args + [ "--labels=type=compile,compiler=clang,lang=cpp" ] diff --git a/build/toolchain/mac/BUILD.gn b/build/toolchain/mac/BUILD.gn index de58a27a65b6..21a228cfc31f 100644 --- a/build/toolchain/mac/BUILD.gn +++ b/build/toolchain/mac/BUILD.gn @@ -7,20 +7,14 @@ # Linux. import("//build/config/mac/mac_sdk.gni") -import("../goma.gni") assert(host_os == "mac") import("//build/config/sysroot.gni") -import("//build/toolchain/goma.gni") import("//build/toolchain/rbe.gni") import("//build/toolchain/signing.gni") -if (use_goma) { - assembler_prefix = "$goma_dir/gomacc " - compiler_prefix = "$goma_dir/gomacc " - link_prefix = "$goma_dir/gomacc " -} else if (use_rbe) { +if (use_rbe) { compiler_args = rewrapper_args + [ "--labels=type=compile,compiler=clang,lang=cpp" ] if (rbe_os != host_os || rbe_cpu != host_cpu) { @@ -38,8 +32,7 @@ if (use_goma) { link_prefix = "" } -# Goma doesn't support the host-arm64 toolchain, so continue using Rosetta. -if (host_cpu == "arm64" && !use_goma) { +if (host_cpu == "arm64") { rebased_clang_dir = rebase_path("//buildtools/mac-arm64/clang/bin", root_build_dir) } else { diff --git a/build/toolchain/win/BUILD.gn b/build/toolchain/win/BUILD.gn index e5ee9a4abf64..3c9d3794cec7 100644 --- a/build/toolchain/win/BUILD.gn +++ b/build/toolchain/win/BUILD.gn @@ -8,7 +8,6 @@ declare_args() { } import("//build/config/win/visual_studio_version.gni") -import("//build/toolchain/goma.gni") import("//build/toolchain/rbe.gni") # Should only be running on Windows. @@ -25,9 +24,7 @@ tool_wrapper_path = rebase_path("tool_wrapper.py", root_build_dir) ninja_path = rebase_path("//buildtools/ninja/ninja") -if (use_goma) { - compiler_prefix = "$goma_dir/gomacc.exe " -} else if (use_rbe) { +if (use_rbe) { compiler_args = rewrapper_args + [ "--labels=type=compile,compiler=clang-cl,lang=cpp" ] if (rbe_os != host_os || rbe_cpu != host_cpu) { diff --git a/tools/build.py b/tools/build.py index 488818f3293e..bd12647aba34 100755 --- a/tools/build.py +++ b/tools/build.py @@ -43,17 +43,12 @@ def BuildOptions(): other_group.add_argument("-j", type=int, - help='Ninja -j option for Goma/RBE builds.', + help='Ninja -j option for RBE builds.', default=200 if sys.platform == 'win32' else 1000) other_group.add_argument("-l", type=int, - help='Ninja -l option for Goma/RBE builds.', + help='Ninja -l option for RBE builds.', default=64) - other_group.add_argument("--no-start-goma", - help="Don't try to start goma", - default=False, - dest='no_start_rbe', - action='store_true') other_group.add_argument("--no-start-rbe", help="Don't try to start rbe", default=False, @@ -124,10 +119,6 @@ def NotifyBuildDone(build_config, success, start): # Ignore return code, if this command fails, it doesn't matter. os.system(command) -def UseGoma(out_dir): - args_gn = os.path.join(out_dir, 'args.gn') - return 'use_goma = true' in open(args_gn, 'r').read() - def UseRBE(out_dir): args_gn = os.path.join(out_dir, 'args.gn') @@ -136,23 +127,20 @@ def UseRBE(out_dir): # Try to start RBE, but don't bail out if we can't. Instead print an error # message, and let the build fail with its own error messages as well. -rbe_started = None +rbe_started = False bootstrap_path = None -def StartRBE(out_dir, use_goma, env): +def StartRBE(out_dir, env): global rbe_started, bootstrap_path - rbe = 'goma' if use_goma else 'rbe' + rbe = 'rbe' if rbe_started: - if rbe_started != rbe: - print(f'Cannot mix RBE and Goma') - return False return True args_gn_path = os.path.join(out_dir, 'args.gn') - rbe_dir = None if use_goma else 'buildtools/reclient' + rbe_dir = 'buildtools/reclient' with open(args_gn_path, 'r') as fp: for line in fp: - if ('goma_dir' if use_goma else 'rbe_dir') in line: + if 'rbe_dir' in line: words = line.split() rbe_dir = words[2][1:-1] # rbe_dir = "/path/to/rbe" if not rbe_dir: @@ -161,23 +149,21 @@ def StartRBE(out_dir, use_goma, env): if not os.path.exists(rbe_dir) or not os.path.isdir(rbe_dir): print(f'Could not find {rbe} at {rbe_dir}') return False - bootstrap = 'goma_ctl.py' if use_goma else 'bootstrap' + bootstrap = 'bootstrap' bootstrap_path = os.path.join(rbe_dir, bootstrap) bootstrap_command = [bootstrap_path] - if use_goma: - bootstrap_command = ['python3', bootstrap_path, 'ensure_start'] process = subprocess.Popen(bootstrap_command, env=env) process.wait() if process.returncode != 0: print(f"Failed to start {rbe}") return False - rbe_started = rbe + rbe_started = True return True def StopRBE(env): global rbe_started, bootstrap_path - if rbe_started != 'rbe': + if not rbe_started: return bootstrap_command = [bootstrap_path, '--shutdown'] process = subprocess.Popen(bootstrap_command, env=env) @@ -192,10 +178,8 @@ def BuildOneConfig(options, targets, target_os, mode, arch, sanitizer, env): command = ['buildtools/ninja/ninja', '-C', out_dir] if options.verbose: command += ['-v'] - use_rbe = UseRBE(out_dir) - use_goma = UseGoma(out_dir) - if use_rbe or use_goma: - if options.no_start_rbe or StartRBE(out_dir, use_goma, env): + if UseRBE(out_dir): + if options.no_start_rbe or StartRBE(out_dir, env): using_rbe = True command += [('-j%s' % str(options.j))] command += [('-l%s' % str(options.l))] diff --git a/tools/gn.py b/tools/gn.py index 65fbf625a2aa..3984e5917156 100755 --- a/tools/gn.py +++ b/tools/gn.py @@ -292,32 +292,6 @@ def ToGnArgs(args, mode, arch, target_os, sanitizer, verify_sdk_hash, if prefix != None: gn_args[arch + '_toolchain_prefix'] = prefix - goma_dir = os.environ.get('GOMA_DIR') - # Search for goma in depot_tools in path - goma_depot_tools_dir = None - for path in os.environ.get('PATH', '').split(os.pathsep): - if os.path.basename(path) == 'depot_tools': - cipd_bin = os.path.join(path, '.cipd_bin') - if os.path.isfile(os.path.join(cipd_bin, ExecutableName('gomacc'))): - goma_depot_tools_dir = cipd_bin - break - # Otherwise use goma from home directory. - # TODO(whesse): Remove support for goma installed in home directory. - # Goma will only be distributed through depot_tools. - goma_home_dir = os.path.join(os.getenv('HOME', ''), 'goma') - if args.goma and goma_dir: - gn_args['use_goma'] = True - gn_args['goma_dir'] = goma_dir - elif args.goma and goma_depot_tools_dir: - gn_args['use_goma'] = True - gn_args['goma_dir'] = goma_depot_tools_dir - elif args.goma and os.path.exists(goma_home_dir): - gn_args['use_goma'] = True - gn_args['goma_dir'] = goma_home_dir - else: - gn_args['use_goma'] = False - gn_args['goma_dir'] = None - gn_args['use_rbe'] = args.rbe # Code coverage requires -O0 to be set. @@ -344,9 +318,6 @@ def ProcessOsOption(os_name): def ProcessOptions(args): - if args.goma: - print('Warning: Goma will be discontinued in the near future') - args.rbe = False if args.verify_sdk_hash == None: args.verify_sdk_hash = not args.rbe if args.git_version == None: @@ -422,8 +393,6 @@ def ProcessOptions(args): (socket.getfqdn().endswith('.corp.google.com') or socket.getfqdn().endswith('.c.googlers.com')): print('You can speed up your build by following: go/dart-rbe') - if not args.goma: - print('Goma is no longer enabled by default since RBE is ready.') old_rbe_cfg = 'win-intel.cfg' if HOST_OS == 'win32' else 'linux-intel.cfg' new_rbe_cfg = 'windows.cfg' if HOST_OS == 'win32' else 'unix.cfg' if os.environ.get('RBE_cfg') == os.path.join(os.getcwd(), 'build', 'rbe', @@ -450,12 +419,6 @@ def ide_switch(host_os): def AddCommonGnOptionArgs(parser): """Adds arguments that will change the default GN arguments.""" - parser.add_argument('--goma', help='Use goma', action='store_true') - parser.add_argument('--no-goma', - help='Disable goma', - dest='goma', - action='store_false') - parser.add_argument('--rbe', help='Use rbe', action='store_true') parser.add_argument('--no-rbe', help='Disable rbe', From 5b262d8234409682ed76b3f9b000ed28edb00db2 Mon Sep 17 00:00:00 2001 From: Nicholas Shahan Date: Tue, 2 Apr 2024 22:54:23 +0000 Subject: [PATCH 5/8] [ddc] Delete `_emitPropertyGet` Avoids the use of one method with multiple code paths and returns to handle any situation because it becomes very hard to reason about what original source code leads to each path. Fixes: https://github.com/dart-lang/sdk/issues/54463 Change-Id: I8158ae2a79e0f627f0703e2e253b4406022fc84b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/357208 Reviewed-by: Sigmund Cherem Reviewed-by: Srujan Gaddam Reviewed-by: Bob Nystrom Commit-Queue: Nicholas Shahan --- pkg/dev_compiler/lib/src/kernel/compiler.dart | 106 ++++++++++-------- 1 file changed, 58 insertions(+), 48 deletions(-) diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index e397e5ee04b6..a8ace3c018b7 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -4875,8 +4875,32 @@ class ProgramCompiler extends ComputeOnceConstantVisitor @override js_ast.Expression visitInstanceGet(InstanceGet node) { - return _emitPropertyGet( - node.receiver, node.interfaceTarget, node.name.text); + // TODO(nshahan): Marking an end span for property accessors would improve + // source maps and hovering in the debugger. Unfortunately this is not + // possible as Kernel does not store this data. + var member = node.interfaceTarget; + var receiver = node.receiver; + var jsReceiver = _visitExpression(receiver); + if (_isNonStaticJsInteropCallMember(member)) { + // Historically DDC has treated this as a "callable class" and the access + // of `.call` as a no-op. + // + // This is here to preserve the existing behavior for the non-static + // JavaScript interop (including some failing cases) but could potentially + // be cleaned up as a breaking change. + return jsReceiver; + } + var memberName = node.name.text; + if (_isObjectGetter(memberName) && + _shouldCallObjectMemberHelper(receiver)) { + // The names of the static helper methods in the runtime must match the + // names of the Object instance getters. + return runtimeCall('#(#)', [memberName, jsReceiver]); + } + // Otherwise generate this as a normal typed property get. + var jsMemberName = + _emitMemberName(memberName, member: node.interfaceTarget); + return js_ast.PropertyAccess(jsReceiver, jsMemberName); } @override @@ -4894,8 +4918,34 @@ class ProgramCompiler extends ComputeOnceConstantVisitor @override js_ast.Expression visitInstanceTearOff(InstanceTearOff node) { - return _emitPropertyGet( - node.receiver, node.interfaceTarget, node.name.text); + var member = node.interfaceTarget; + var receiver = node.receiver; + var jsReceiver = _visitExpression(receiver); + if (_isNonStaticJsInteropCallMember(member)) { + // Historically DDC has treated this as a "callable class" and the tearoff + // of `.call` as a no-op. + // + // This is here to preserve the existing behavior for the non-static + // JavaScript interop (including some failing cases) but could potentially + // be cleaned up as a breaking change. + return jsReceiver; + } + var memberName = node.name.text; + if (_isObjectMethodTearoff(memberName) && + _shouldCallObjectMemberHelper(receiver)) { + // The names of the static helper methods in the runtime must start with + // the names of the Object instance methods. + var tearOffName = '${memberName}Tearoff'; + return runtimeCall('#(#)', [tearOffName, jsReceiver]); + } + var jsMemberName = _emitMemberName(memberName, member: member); + if (_reifyTearoff(member)) { + return runtimeCall('bind(#, #)', [jsReceiver, jsMemberName]); + } + var jsPropertyAccess = js_ast.PropertyAccess(jsReceiver, jsMemberName); + return isJsMember(member) + ? runtimeCall('tearoffInterop(#)', [jsPropertyAccess]) + : jsPropertyAccess; } /// Returns `true` when [member] is a `.call` member (field, getter or method) @@ -4962,50 +5012,6 @@ class ProgramCompiler extends ComputeOnceConstantVisitor return false; } - // TODO(54463): Refactor and specialize this code for each type of 'get'. - js_ast.Expression _emitPropertyGet( - Expression receiver, Member member, String memberName) { - var jsReceiver = _visitExpression(receiver); - if (_isNonStaticJsInteropCallMember(member)) { - // Historically DDC has treated this as a "callable class" and the - // tearoff of `.call` as a no-op. This is still needed here to preserve - // the existing behavior for the non-static JavaScript interop (including - // potentially failing cases). - return jsReceiver; - } - var jsName = _emitMemberName(memberName, member: member); - - // TODO(jmesserly): we need to mark an end span for property accessors so - // they can be hovered. Unfortunately this is not possible as Kernel does - // not store this data. - if (_isObjectMember(memberName) && - _shouldCallObjectMemberHelper(receiver)) { - if (_isObjectMethodTearoff(memberName)) { - if (memberName == 'toString') { - return runtimeCall('toStringTearoff(#)', [jsReceiver]); - } - if (memberName == 'noSuchMethod') { - return runtimeCall('noSuchMethodTearoff(#)', [jsReceiver]); - } - assert(false, 'Unexpected Object method tearoff: $memberName'); - } - // The names of the static helper methods in the runtime must match the - // names of the Object instance members. - return runtimeCall('#(#)', [memberName, jsReceiver]); - } - // Otherwise generate this as a normal typed property get. - if (_reifyTearoff(member)) { - return runtimeCall('bind(#, #)', [jsReceiver, jsName]); - } else if (member is Procedure && - !member.isAccessor && - isJsMember(member)) { - return runtimeCall( - 'tearoffInterop(#)', [js_ast.PropertyAccess(jsReceiver, jsName)]); - } else { - return js_ast.PropertyAccess(jsReceiver, jsName); - } - } - /// Return whether [member] returns a native object whose type needs to be /// null-checked in sound null-safety. /// @@ -7516,7 +7522,11 @@ bool _isObjectMember(String name) { return false; } +bool _isObjectGetter(String name) => + name == 'hashCode' || name == 'runtimeType'; + bool _isObjectMethodTearoff(String name) => + // "==" isn't in here because there is no syntax to tear it off. name == 'toString' || name == 'noSuchMethod'; bool _isObjectMethodCall(String name, Arguments args) { From 5543d88289a7bd807aff816b3f51fb255c3588b4 Mon Sep 17 00:00:00 2001 From: Konstantin Shcheglov Date: Tue, 2 Apr 2024 23:02:22 +0000 Subject: [PATCH 6/8] AE. Deprecate OnClause, use MixinOnClause instead. Change-Id: I938a1ff4046851c46c6ae4cebdff25e05e88300f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/360741 Reviewed-by: Brian Wilkerson Commit-Queue: Konstantin Shcheglov --- .../lib/src/computer/computer_highlights.dart | 12 +- .../dart/in_scope_completion_pass.dart | 24 ++-- .../remove_name_from_declaration_clause.dart | 2 +- .../lib/src/utilities/selection.dart | 8 +- .../tool/code_completion/code_metrics.dart | 16 +-- .../code_completion/relevance_metrics.dart | 16 +-- .../relevance_table_generator.dart | 16 +-- .../tool/code_completion/visitors.dart | 12 +- pkg/analyzer/CHANGELOG.md | 4 + pkg/analyzer/lib/dart/ast/ast.dart | 2 + pkg/analyzer/lib/dart/ast/visitor.dart | 42 +++++++ pkg/analyzer/lib/src/dart/analysis/index.dart | 18 +-- pkg/analyzer/lib/src/dart/ast/ast.dart | 115 ++++++++++-------- .../lib/src/dart/ast/to_source_visitor.dart | 7 ++ pkg/analyzer/lib/src/dart/ast/utilities.dart | 30 +++-- .../dart/resolver/named_type_resolver.dart | 4 +- .../src/dart/resolver/resolution_visitor.dart | 6 +- .../lib/src/error/inheritance_override.dart | 2 +- .../src/error/type_arguments_verifier.dart | 2 +- pkg/analyzer/lib/src/fasta/ast_builder.dart | 10 +- .../lib/src/generated/error_verifier.dart | 8 +- pkg/analyzer/lib/src/generated/resolver.dart | 12 +- pkg/analyzer/lib/src/lint/linter_visitor.dart | 18 ++- .../lib/src/summary2/element_builder.dart | 8 +- .../lib/src/summary2/reference_resolver.dart | 10 +- .../lib/src/test_utilities/find_node.dart | 4 +- .../test/src/dart/resolution/mixin_test.dart | 4 +- ..._class_constraint_deferred_class_test.dart | 4 +- ...lass_constraint_disallowed_class_test.dart | 8 +- ...r_class_constraint_non_interface_test.dart | 20 +-- .../test/src/fasta/ast_builder_test.dart | 4 +- .../src/summary/resolved_ast_printer.dart | 16 +-- pkg/analyzer/test/util/ast_type_matchers.dart | 4 +- .../lib/src/utilities/completion/optype.dart | 12 +- .../src/utilities/completion/optype_test.dart | 6 +- 35 files changed, 288 insertions(+), 198 deletions(-) diff --git a/pkg/analysis_server/lib/src/computer/computer_highlights.dart b/pkg/analysis_server/lib/src/computer/computer_highlights.dart index 6ec38ae65ab8..334a5febdcb0 100644 --- a/pkg/analysis_server/lib/src/computer/computer_highlights.dart +++ b/pkg/analysis_server/lib/src/computer/computer_highlights.dart @@ -1248,6 +1248,12 @@ class _DartUnitHighlightsComputerVisitor extends RecursiveAstVisitor { super.visitMixinDeclaration(node); } + @override + void visitMixinOnClause(MixinOnClause node) { + computer._addRegion_token(node.onKeyword, HighlightRegionType.BUILT_IN); + super.visitMixinOnClause(node); + } + @override void visitNamedType(NamedType node) { if (node.importPrefix case final importPrefix?) { @@ -1300,12 +1306,6 @@ class _DartUnitHighlightsComputerVisitor extends RecursiveAstVisitor { super.visitNullLiteral(node); } - @override - void visitOnClause(OnClause node) { - computer._addRegion_token(node.onKeyword, HighlightRegionType.BUILT_IN); - super.visitOnClause(node); - } - @override void visitPartDirective(PartDirective node) { computer._addRegion_node(node, HighlightRegionType.DIRECTIVE); diff --git a/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart b/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart index 41dbdd8c3108..fa69193089f1 100644 --- a/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart +++ b/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart @@ -1795,6 +1795,16 @@ class InScopeCompletionPass extends SimpleAstVisitor { } } + @override + void visitMixinOnClause(MixinOnClause node) { + var onKeyword = node.onKeyword; + if (offset <= onKeyword.end) { + keywordHelper.addKeyword(Keyword.ON); + } else { + _forTypeAnnotation(node); + } + } + @override void visitNamedExpression(NamedExpression node) { if (offset <= node.name.label.end) { @@ -1882,16 +1892,6 @@ class InScopeCompletionPass extends SimpleAstVisitor { } } - @override - void visitOnClause(OnClause node) { - var onKeyword = node.onKeyword; - if (offset <= onKeyword.end) { - keywordHelper.addKeyword(Keyword.ON); - } else { - _forTypeAnnotation(node); - } - } - @override void visitParenthesizedExpression(ParenthesizedExpression node) { var expression = node.expression; @@ -3671,9 +3671,9 @@ extension on AstNode { MethodDeclaration(:var metadata) => metadata.contains(child), MixinDeclaration(:var members, :var metadata) => members.contains(child) || metadata.contains(child), - ObjectPattern(:var fields) => fields.contains(child), - OnClause(:var superclassConstraints) => + MixinOnClause(:var superclassConstraints) => superclassConstraints.contains(child), + ObjectPattern(:var fields) => fields.contains(child), PartDirective(:var metadata) => metadata.contains(child), PartOfDirective(:var metadata) => metadata.contains(child), PatternVariableDeclaration(:var metadata) => metadata.contains(child), diff --git a/pkg/analysis_server/lib/src/services/correction/dart/remove_name_from_declaration_clause.dart b/pkg/analysis_server/lib/src/services/correction/dart/remove_name_from_declaration_clause.dart index 5340247fa80a..cf4b0f5999f5 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/remove_name_from_declaration_clause.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/remove_name_from_declaration_clause.dart @@ -35,7 +35,7 @@ class RemoveNameFromDeclarationClause extends ResolvedCorrectionProducer { } else if (clause is ImplementsClause) { clauseName = 'implements'; nameList = clause.interfaces; - } else if (clause is OnClause) { + } else if (clause is MixinOnClause) { clauseName = 'on'; nameList = clause.superclassConstraints; } else if (clause is WithClause) { diff --git a/pkg/analysis_server/lib/src/utilities/selection.dart b/pkg/analysis_server/lib/src/utilities/selection.dart index e5b21c1f6d27..9cf83a9aa65c 100644 --- a/pkg/analysis_server/lib/src/utilities/selection.dart +++ b/pkg/analysis_server/lib/src/utilities/selection.dart @@ -267,13 +267,13 @@ class _ChildrenFinder extends SimpleAstVisitor { } @override - void visitObjectPattern(ObjectPattern node) { - _fromList(node.fields); + void visitMixinOnClause(MixinOnClause node) { + _fromList(node.superclassConstraints); } @override - void visitOnClause(OnClause node) { - _fromList(node.superclassConstraints); + void visitObjectPattern(ObjectPattern node) { + _fromList(node.fields); } @override diff --git a/pkg/analysis_server/tool/code_completion/code_metrics.dart b/pkg/analysis_server/tool/code_completion/code_metrics.dart index 2d04916ac728..fcc35ba9219c 100644 --- a/pkg/analysis_server/tool/code_completion/code_metrics.dart +++ b/pkg/analysis_server/tool/code_completion/code_metrics.dart @@ -945,6 +945,14 @@ class CodeShapeDataCollector extends RecursiveAstVisitor { super.visitMixinDeclaration(node); } + @override + void visitMixinOnClause(MixinOnClause node) { + _visitChildren(node, { + 'superclassConstraints': node.superclassConstraints, + }); + super.visitMixinOnClause(node); + } + @override void visitNamedExpression(NamedExpression node) { _visitChildren(node, { @@ -985,14 +993,6 @@ class CodeShapeDataCollector extends RecursiveAstVisitor { super.visitNullLiteral(node); } - @override - void visitOnClause(OnClause node) { - _visitChildren(node, { - 'superclassConstraints': node.superclassConstraints, - }); - super.visitOnClause(node); - } - @override void visitParenthesizedExpression(ParenthesizedExpression node) { _visitChildren(node, { diff --git a/pkg/analysis_server/tool/code_completion/relevance_metrics.dart b/pkg/analysis_server/tool/code_completion/relevance_metrics.dart index 3d90f886d9b5..f7caba7726f5 100644 --- a/pkg/analysis_server/tool/code_completion/relevance_metrics.dart +++ b/pkg/analysis_server/tool/code_completion/relevance_metrics.dart @@ -1036,6 +1036,14 @@ class RelevanceDataCollector extends RecursiveAstVisitor { inGenericContext = wasInGenericContext; } + @override + void visitMixinOnClause(MixinOnClause node) { + for (var constraint in node.superclassConstraints) { + _recordDataForNode('OnClause (type)', constraint); + } + super.visitMixinOnClause(node); + } + @override void visitNamedExpression(NamedExpression node) { // Named expressions only occur in argument lists and are handled there. @@ -1066,14 +1074,6 @@ class RelevanceDataCollector extends RecursiveAstVisitor { super.visitNullLiteral(node); } - @override - void visitOnClause(OnClause node) { - for (var constraint in node.superclassConstraints) { - _recordDataForNode('OnClause (type)', constraint); - } - super.visitOnClause(node); - } - @override void visitParenthesizedExpression(ParenthesizedExpression node) { _recordDataForNode('ParenthesizedExpression (expression)', node.expression, diff --git a/pkg/analysis_server/tool/code_completion/relevance_table_generator.dart b/pkg/analysis_server/tool/code_completion/relevance_table_generator.dart index 32bb9e225172..b1fdeec76d06 100644 --- a/pkg/analysis_server/tool/code_completion/relevance_table_generator.dart +++ b/pkg/analysis_server/tool/code_completion/relevance_table_generator.dart @@ -966,6 +966,14 @@ class RelevanceDataCollector extends RecursiveAstVisitor { super.visitMixinDeclaration(node); } + @override + void visitMixinOnClause(MixinOnClause node) { + for (var constraint in node.superclassConstraints) { + _recordDataForNode('OnClause_superclassConstraint', constraint); + } + super.visitMixinOnClause(node); + } + @override void visitNamedExpression(NamedExpression node) { // Named expressions only occur in argument lists and are handled there. @@ -996,14 +1004,6 @@ class RelevanceDataCollector extends RecursiveAstVisitor { super.visitNullLiteral(node); } - @override - void visitOnClause(OnClause node) { - for (var constraint in node.superclassConstraints) { - _recordDataForNode('OnClause_superclassConstraint', constraint); - } - super.visitOnClause(node); - } - @override void visitParenthesizedExpression(ParenthesizedExpression node) { _recordDataForNode('ParenthesizedExpression_expression', node.expression, diff --git a/pkg/analysis_server/tool/code_completion/visitors.dart b/pkg/analysis_server/tool/code_completion/visitors.dart index b5c10faa8cc1..4c26ec0dd284 100644 --- a/pkg/analysis_server/tool/code_completion/visitors.dart +++ b/pkg/analysis_server/tool/code_completion/visitors.dart @@ -560,6 +560,12 @@ class ExpectedCompletionsVisitor extends RecursiveAstVisitor { super.visitMixinDeclaration(node); } + @override + void visitMixinOnClause(MixinOnClause node) { + safelyRecordKeywordCompletion(node.onKeyword); + super.visitMixinOnClause(node); + } + @override void visitNativeClause(NativeClause node) { safelyRecordKeywordCompletion(node.nativeKeyword); @@ -578,12 +584,6 @@ class ExpectedCompletionsVisitor extends RecursiveAstVisitor { super.visitNullLiteral(node); } - @override - void visitOnClause(OnClause node) { - safelyRecordKeywordCompletion(node.onKeyword); - super.visitOnClause(node); - } - @override void visitPartDirective(PartDirective node) { safelyRecordKeywordCompletion(node.partKeyword); diff --git a/pkg/analyzer/CHANGELOG.md b/pkg/analyzer/CHANGELOG.md index e148c576efa6..c43cc7345a4d 100644 --- a/pkg/analyzer/CHANGELOG.md +++ b/pkg/analyzer/CHANGELOG.md @@ -9,6 +9,10 @@ use `PropertyInducingElement? get variable2` instead. The reason for this is that when the property accessor is an augmentation without the corresponding declaration, there is no corresponding variable. +* Deprecated `ExtensionDeclaration.onKeyword` and `extendedType`. + Use `ExtensionOnClause? get onClause` instead. + Extension augmentations are not allowed to have `onClause`. +* Deprecated `OnClause`, use `MixinOnClause` instead. ## 6.4.1 * Patch for crash in ffi_verifier. diff --git a/pkg/analyzer/lib/dart/ast/ast.dart b/pkg/analyzer/lib/dart/ast/ast.dart index a0ac1beb6088..ca69904699f1 100644 --- a/pkg/analyzer/lib/dart/ast/ast.dart +++ b/pkg/analyzer/lib/dart/ast/ast.dart @@ -168,6 +168,7 @@ export 'package:analyzer/src/dart/ast/ast.dart' MethodInvocation, MethodReferenceExpression, MixinDeclaration, + MixinOnClause, NamedCompilationUnitMember, NamedExpression, NamedType, @@ -181,6 +182,7 @@ export 'package:analyzer/src/dart/ast/ast.dart' NullLiteral, NullShortableExpression, ObjectPattern, + // ignore:deprecated_member_use_from_same_package OnClause, ParenthesizedExpression, ParenthesizedPattern, diff --git a/pkg/analyzer/lib/dart/ast/visitor.dart b/pkg/analyzer/lib/dart/ast/visitor.dart index 772a9a396b46..1d3736486e24 100644 --- a/pkg/analyzer/lib/dart/ast/visitor.dart +++ b/pkg/analyzer/lib/dart/ast/visitor.dart @@ -515,6 +515,12 @@ class GeneralizingAstVisitor implements AstVisitor { R? visitMixinDeclaration(MixinDeclaration node) => visitNamedCompilationUnitMember(node); + @override + R? visitMixinOnClause(MixinOnClause node) { + // ignore:deprecated_member_use_from_same_package + return visitOnClause(node); + } + R? visitNamedCompilationUnitMember(NamedCompilationUnitMember node) => visitCompilationUnitMember(node); @@ -554,6 +560,7 @@ class GeneralizingAstVisitor implements AstVisitor { @override R? visitObjectPattern(ObjectPattern node) => visitDartPattern(node); + @Deprecated('Use visitMixinOnClause() instead') @override R? visitOnClause(OnClause node) => visitNode(node); @@ -1408,6 +1415,12 @@ class RecursiveAstVisitor implements AstVisitor { return null; } + @override + R? visitMixinOnClause(MixinOnClause node) { + // ignore:deprecated_member_use_from_same_package + return visitOnClause(node); + } + @override R? visitNamedExpression(NamedExpression node) { node.visitChildren(this); @@ -1456,6 +1469,7 @@ class RecursiveAstVisitor implements AstVisitor { return null; } + @Deprecated('Use visitMixinOnClause() instead') @override R? visitOnClause(OnClause node) { node.visitChildren(this); @@ -2148,6 +2162,12 @@ class SimpleAstVisitor implements AstVisitor { @override R? visitMixinDeclaration(MixinDeclaration node) => null; + @override + R? visitMixinOnClause(MixinOnClause node) { + // ignore:deprecated_member_use_from_same_package + return visitOnClause(node); + } + @override R? visitNamedExpression(NamedExpression node) => null; @@ -2172,6 +2192,7 @@ class SimpleAstVisitor implements AstVisitor { @override R? visitObjectPattern(ObjectPattern node) => null; + @Deprecated('Use visitMixinOnClause() instead') @override R? visitOnClause(OnClause node) => null; @@ -2696,6 +2717,12 @@ class ThrowingAstVisitor implements AstVisitor { @override R? visitMixinDeclaration(MixinDeclaration node) => _throw(node); + @override + R? visitMixinOnClause(MixinOnClause node) { + // ignore:deprecated_member_use_from_same_package + return visitOnClause(node); + } + @override R? visitNamedExpression(NamedExpression node) => _throw(node); @@ -2720,6 +2747,7 @@ class ThrowingAstVisitor implements AstVisitor { @override R? visitObjectPattern(ObjectPattern node) => _throw(node); + @Deprecated('Use visitMixinOnClause() instead') @override R? visitOnClause(OnClause node) => _throw(node); @@ -3758,6 +3786,12 @@ class TimedAstVisitor implements AstVisitor { return result; } + @override + T? visitMixinOnClause(MixinOnClause node) { + // ignore:deprecated_member_use_from_same_package + return visitOnClause(node); + } + @override T? visitNamedExpression(NamedExpression node) { stopwatch.start(); @@ -3822,6 +3856,7 @@ class TimedAstVisitor implements AstVisitor { return result; } + @Deprecated('Use visitMixinOnClause() instead') @override T? visitOnClause(OnClause node) { stopwatch.start(); @@ -4657,6 +4692,12 @@ class UnifyingAstVisitor implements AstVisitor { @override R? visitMixinDeclaration(MixinDeclaration node) => visitNode(node); + @override + R? visitMixinOnClause(MixinOnClause node) { + // ignore:deprecated_member_use_from_same_package + return visitOnClause(node); + } + @override R? visitNamedExpression(NamedExpression node) => visitNode(node); @@ -4686,6 +4727,7 @@ class UnifyingAstVisitor implements AstVisitor { @override R? visitObjectPattern(ObjectPattern node) => visitNode(node); + @Deprecated('Use visitMixinOnClause() instead') @override R? visitOnClause(OnClause node) => visitNode(node); diff --git a/pkg/analyzer/lib/src/dart/analysis/index.dart b/pkg/analyzer/lib/src/dart/analysis/index.dart index 3a112ad90f08..c0f72734fccb 100644 --- a/pkg/analyzer/lib/src/dart/analysis/index.dart +++ b/pkg/analyzer/lib/src/dart/analysis/index.dart @@ -855,6 +855,14 @@ class _IndexContributor extends GeneralizingAstVisitor { super.visitMixinDeclaration(node); } + @override + void visitMixinOnClause(MixinOnClause node) { + for (NamedType namedType in node.superclassConstraints) { + recordSuperType(namedType, IndexRelationKind.CONSTRAINS); + namedType.accept(this); + } + } + @override visitNamedType(NamedType node) { _recordImportPrefixedElement( @@ -866,14 +874,6 @@ class _IndexContributor extends GeneralizingAstVisitor { node.typeArguments?.accept(this); } - @override - void visitOnClause(OnClause node) { - for (NamedType namedType in node.superclassConstraints) { - recordSuperType(namedType, IndexRelationKind.CONSTRAINS); - namedType.accept(this); - } - } - @override void visitPartDirective(PartDirective node) { final partElement = node.element; @@ -1032,7 +1032,7 @@ class _IndexContributor extends GeneralizingAstVisitor { void _addSubtype(String name, {NamedType? superclass, WithClause? withClause, - OnClause? onClause, + MixinOnClause? onClause, ImplementsClause? implementsClause, required List memberNodes}) { List supertypes = []; diff --git a/pkg/analyzer/lib/src/dart/ast/ast.dart b/pkg/analyzer/lib/src/dart/ast/ast.dart index 5d55d87aa171..33429438ddde 100644 --- a/pkg/analyzer/lib/src/dart/ast/ast.dart +++ b/pkg/analyzer/lib/src/dart/ast/ast.dart @@ -26,6 +26,13 @@ import 'package:analyzer/src/generated/utilities_dart.dart'; import 'package:collection/collection.dart'; import 'package:meta/meta.dart'; +/// The "on" clause in a mixin declaration. +/// +/// onClause ::= +/// 'on' [NamedType] (',' [NamedType])* +@Deprecated('Use MixinOnClause instead') +typedef OnClause = MixinOnClause; + /// Two or more string literals that are implicitly concatenated because of /// being adjacent (separated only by whitespace). /// @@ -1405,6 +1412,8 @@ abstract class AstVisitor { R? visitMixinDeclaration(MixinDeclaration node); + R? visitMixinOnClause(MixinOnClause node); + R? visitNamedExpression(NamedExpression node); R? visitNamedType(NamedType node); @@ -1421,6 +1430,7 @@ abstract class AstVisitor { R? visitObjectPattern(ObjectPattern node); + @Deprecated('Use visitMixinOnClause() instead') R? visitOnClause(OnClause node); R? visitParenthesizedExpression(ParenthesizedExpression node); @@ -11643,7 +11653,7 @@ abstract final class MixinDeclaration implements NamedCompilationUnitMember { /// The on clause for the mixin, or `null` if the mixin doesn't have any /// superclass constraints. - OnClause? get onClause; + MixinOnClause? get onClause; /// The right curly bracket. Token get rightBracket; @@ -11668,7 +11678,7 @@ final class MixinDeclarationImpl extends NamedCompilationUnitMemberImpl final TypeParameterListImpl? typeParameters; @override - final OnClauseImpl? onClause; + final MixinOnClauseImpl? onClause; @override final ImplementsClauseImpl? implementsClause; @@ -11739,6 +11749,56 @@ final class MixinDeclarationImpl extends NamedCompilationUnitMemberImpl } } +/// The "on" clause in a mixin declaration. +/// +/// onClause ::= +/// 'on' [NamedType] (',' [NamedType])* +abstract final class MixinOnClause implements AstNode { + /// The token representing the `on` keyword. + Token get onKeyword; + + /// The list of the classes are superclass constraints for the mixin. + NodeList get superclassConstraints; +} + +final class MixinOnClauseImpl extends AstNodeImpl implements MixinOnClause { + @override + final Token onKeyword; + + final NodeListImpl _superclassConstraints = NodeListImpl._(); + + MixinOnClauseImpl({ + required this.onKeyword, + required List superclassConstraints, + }) { + _superclassConstraints._initialize(this, superclassConstraints); + } + + @override + Token get beginToken => onKeyword; + + @override + Token get endToken => _superclassConstraints.endToken ?? onKeyword; + + @override + NodeListImpl get superclassConstraints => + _superclassConstraints; + + @override + // TODO(paulberry): add commas. + ChildEntities get _childEntities => ChildEntities() + ..addToken('onKeyword', onKeyword) + ..addNodeList('superclassConstraints', superclassConstraints); + + @override + E? accept(AstVisitor visitor) => visitor.visitMixinOnClause(this); + + @override + void visitChildren(AstVisitor visitor) { + _superclassConstraints.accept(visitor); + } +} + /// A node that declares a single name within the scope of a compilation unit. abstract final class NamedCompilationUnitMember implements CompilationUnitMember { @@ -12730,57 +12790,6 @@ final class ObjectPatternImpl extends DartPatternImpl implements ObjectPattern { } } -/// The "on" clause in a mixin declaration. -/// -/// onClause ::= -/// 'on' [NamedType] (',' [NamedType])* -abstract final class OnClause implements AstNode { - /// The token representing the `on` keyword. - Token get onKeyword; - - /// The list of the classes are superclass constraints for the mixin. - NodeList get superclassConstraints; -} - -final class OnClauseImpl extends AstNodeImpl implements OnClause { - @override - final Token onKeyword; - - final NodeListImpl _superclassConstraints = NodeListImpl._(); - - /// Initializes a newly created on clause. - OnClauseImpl({ - required this.onKeyword, - required List superclassConstraints, - }) { - _superclassConstraints._initialize(this, superclassConstraints); - } - - @override - Token get beginToken => onKeyword; - - @override - Token get endToken => _superclassConstraints.endToken ?? onKeyword; - - @override - NodeListImpl get superclassConstraints => - _superclassConstraints; - - @override - // TODO(paulberry): add commas. - ChildEntities get _childEntities => ChildEntities() - ..addToken('onKeyword', onKeyword) - ..addNodeList('superclassConstraints', superclassConstraints); - - @override - E? accept(AstVisitor visitor) => visitor.visitOnClause(this); - - @override - void visitChildren(AstVisitor visitor) { - _superclassConstraints.accept(visitor); - } -} - /// A parenthesized expression. /// /// parenthesizedExpression ::= diff --git a/pkg/analyzer/lib/src/dart/ast/to_source_visitor.dart b/pkg/analyzer/lib/src/dart/ast/to_source_visitor.dart index bfcfae829af9..d386141cd573 100644 --- a/pkg/analyzer/lib/src/dart/ast/to_source_visitor.dart +++ b/pkg/analyzer/lib/src/dart/ast/to_source_visitor.dart @@ -929,6 +929,12 @@ class ToSourceVisitor implements AstVisitor { sink.write('}'); } + @override + void visitMixinOnClause(MixinOnClause node) { + sink.write('on '); + _visitNodeList(node.superclassConstraints, separator: ', '); + } + @override void visitNamedExpression(NamedExpression node) { _visitNode(node.name); @@ -983,6 +989,7 @@ class ToSourceVisitor implements AstVisitor { sink.write(')'); } + @Deprecated('Use visitMixinOnClause() instead') @override void visitOnClause(OnClause node) { sink.write('on '); diff --git a/pkg/analyzer/lib/src/dart/ast/utilities.dart b/pkg/analyzer/lib/src/dart/ast/utilities.dart index 14d1fff6fa12..7dfff74bfe5c 100644 --- a/pkg/analyzer/lib/src/dart/ast/utilities.dart +++ b/pkg/analyzer/lib/src/dart/ast/utilities.dart @@ -1054,6 +1054,14 @@ class AstComparator implements AstVisitor { isEqualTokens(node.rightBracket, other.rightBracket); } + @override + bool visitMixinOnClause(MixinOnClause node) { + var other = _other as MixinOnClause; + return isEqualTokens(node.onKeyword, other.onKeyword) && + _isEqualNodeLists( + node.superclassConstraints, other.superclassConstraints); + } + @override bool visitNamedExpression(NamedExpression node) { NamedExpression other = _other as NamedExpression; @@ -1114,12 +1122,10 @@ class AstComparator implements AstVisitor { isEqualTokens(node.rightParenthesis, other.rightParenthesis); } + @Deprecated('Use visitMixinOnClause() instead') @override bool visitOnClause(OnClause node) { - OnClause other = _other as OnClause; - return isEqualTokens(node.onKeyword, other.onKeyword) && - _isEqualNodeLists( - node.superclassConstraints, other.superclassConstraints); + return visitMixinOnClause(node); } @override @@ -2971,6 +2977,14 @@ class NodeReplacer extends ThrowingAstVisitor { return visitNode(node); } + @override + bool visitMixinOnClause(covariant MixinOnClauseImpl node) { + if (_replaceInList(node.superclassConstraints)) { + return true; + } + return visitNode(node); + } + @override bool visitNamedExpression(covariant NamedExpressionImpl node) { if (identical(node.name, _oldNode)) { @@ -3016,12 +3030,10 @@ class NodeReplacer extends ThrowingAstVisitor { @override bool visitNullLiteral(NullLiteral node) => visitNode(node); + @Deprecated('Use visitMixinOnClause() instead') @override - bool visitOnClause(covariant OnClauseImpl node) { - if (_replaceInList(node.superclassConstraints)) { - return true; - } - return visitNode(node); + bool visitOnClause(covariant MixinOnClauseImpl node) { + return visitMixinOnClause(node); } @override diff --git a/pkg/analyzer/lib/src/dart/resolver/named_type_resolver.dart b/pkg/analyzer/lib/src/dart/resolver/named_type_resolver.dart index f6b456156e7f..7d3361e88655 100644 --- a/pkg/analyzer/lib/src/dart/resolver/named_type_resolver.dart +++ b/pkg/analyzer/lib/src/dart/resolver/named_type_resolver.dart @@ -391,7 +391,7 @@ class NamedTypeResolver with ScopeHelpers { node, CompileTimeErrorCode.NULLABLE_TYPE_IN_IMPLEMENTS_CLAUSE, ); - } else if (parent is OnClause) { + } else if (parent is MixinOnClause) { errorReporter.atNode( node, CompileTimeErrorCode.NULLABLE_TYPE_IN_ON_CLAUSE, @@ -449,7 +449,7 @@ class NamedTypeResolver with ScopeHelpers { } else if (parent is ImplementsClause) { errorCode = CompileTimeErrorCode .IMPLEMENTS_TYPE_ALIAS_EXPANDS_TO_TYPE_PARAMETER; - } else if (parent is OnClause) { + } else if (parent is MixinOnClause) { errorCode = CompileTimeErrorCode.MIXIN_ON_TYPE_ALIAS_EXPANDS_TO_TYPE_PARAMETER; } else if (parent is WithClause) { diff --git a/pkg/analyzer/lib/src/dart/resolver/resolution_visitor.dart b/pkg/analyzer/lib/src/dart/resolver/resolution_visitor.dart index 7d0561f7b9a5..c95dc1f24592 100644 --- a/pkg/analyzer/lib/src/dart/resolver/resolution_visitor.dart +++ b/pkg/analyzer/lib/src/dart/resolver/resolution_visitor.dart @@ -1627,7 +1627,7 @@ class ResolutionVisitor extends RecursiveAstVisitor { void _resolveOnClause({ required Declaration? declaration, - required OnClauseImpl? clause, + required MixinOnClauseImpl? clause, }) { if (clause == null) return; @@ -1679,7 +1679,7 @@ class ResolutionVisitor extends RecursiveAstVisitor { return; case MixinElement(): if (clause is ImplementsClause || - clause is OnClause || + clause is MixinOnClause || clause is WithClause) { return; } @@ -1703,7 +1703,7 @@ class ResolutionVisitor extends RecursiveAstVisitor { } case ImplementsClause(): errorCode = CompileTimeErrorCode.IMPLEMENTS_NON_CLASS; - case OnClause(): + case MixinOnClause(): errorCode = CompileTimeErrorCode.MIXIN_SUPER_CLASS_CONSTRAINT_NON_INTERFACE; case WithClause(): diff --git a/pkg/analyzer/lib/src/error/inheritance_override.dart b/pkg/analyzer/lib/src/error/inheritance_override.dart index 8a16b8dfa400..4d7e693d3697 100644 --- a/pkg/analyzer/lib/src/error/inheritance_override.dart +++ b/pkg/analyzer/lib/src/error/inheritance_override.dart @@ -150,7 +150,7 @@ class _ClassVerifier { final Token classNameToken; final List members; final ImplementsClause? implementsClause; - final OnClause? onClause; + final MixinOnClause? onClause; final NamedType? superclass; final WithClause? withClause; diff --git a/pkg/analyzer/lib/src/error/type_arguments_verifier.dart b/pkg/analyzer/lib/src/error/type_arguments_verifier.dart index f8002a1683d7..c5d25ff04469 100644 --- a/pkg/analyzer/lib/src/error/type_arguments_verifier.dart +++ b/pkg/analyzer/lib/src/error/type_arguments_verifier.dart @@ -569,7 +569,7 @@ class TypeArgumentsVerifier { case ExtendsClause _: case GenericTypeAlias _: case ImplementsClause _: - case OnClause _: + case MixinOnClause _: case WithClause _: return false; } diff --git a/pkg/analyzer/lib/src/fasta/ast_builder.dart b/pkg/analyzer/lib/src/fasta/ast_builder.dart index 89e86437a511..dffe44aa3fdf 100644 --- a/pkg/analyzer/lib/src/fasta/ast_builder.dart +++ b/pkg/analyzer/lib/src/fasta/ast_builder.dart @@ -4772,7 +4772,7 @@ class AstBuilder extends StackListener { var implementsClause = pop(NullValues.IdentifierList) as ImplementsClauseImpl?; - var onClause = pop(NullValues.IdentifierList) as OnClauseImpl?; + var onClause = pop(NullValues.IdentifierList) as MixinOnClauseImpl?; var baseKeyword = pop(NullValues.Token) as Token?; var augmentKeyword = pop(NullValues.Token) as Token?; var typeParameters = pop() as TypeParameterListImpl?; @@ -4808,7 +4808,7 @@ class AstBuilder extends StackListener { errorCode: ParserErrorCode.EXPECTED_NAMED_TYPE_ON, ); push( - OnClauseImpl( + MixinOnClauseImpl( onKeyword: onKeyword, superclassConstraints: onTypes, ), @@ -5307,14 +5307,14 @@ class AstBuilder extends StackListener { final builder = _classLikeBuilder as _MixinDeclarationBuilder; var implementsClause = pop(NullValues.IdentifierList) as ImplementsClauseImpl?; - var onClause = pop(NullValues.IdentifierList) as OnClauseImpl?; + var onClause = pop(NullValues.IdentifierList) as MixinOnClauseImpl?; if (onClause != null) { final existingClause = builder.onClause; if (existingClause == null) { builder.onClause = onClause; } else { - builder.onClause = OnClauseImpl( + builder.onClause = MixinOnClauseImpl( onKeyword: existingClause.onKeyword, superclassConstraints: [ ...existingClause.superclassConstraints, @@ -6189,7 +6189,7 @@ class _MixinDeclarationBuilder extends _ClassLikeDeclarationBuilder { final Token? baseKeyword; final Token mixinKeyword; final Token name; - OnClauseImpl? onClause; + MixinOnClauseImpl? onClause; ImplementsClauseImpl? implementsClause; _MixinDeclarationBuilder({ diff --git a/pkg/analyzer/lib/src/generated/error_verifier.dart b/pkg/analyzer/lib/src/generated/error_verifier.dart index f25edd15e6ae..13a44f757cf6 100644 --- a/pkg/analyzer/lib/src/generated/error_verifier.dart +++ b/pkg/analyzer/lib/src/generated/error_verifier.dart @@ -3332,7 +3332,7 @@ class ErrorVerifier extends RecursiveAstVisitor NamedType? superclass, WithClause? withClause, ImplementsClause? implementsClause, - OnClause? onClause, + MixinOnClause? onClause, ) { if (superclass != null) { final type = superclass.type; @@ -4594,7 +4594,7 @@ class ErrorVerifier extends RecursiveAstVisitor /// /// See [CompileTimeErrorCode.MIXIN_SUPER_CLASS_CONSTRAINT_DISALLOWED_CLASS], /// [CompileTimeErrorCode.MIXIN_SUPER_CLASS_CONSTRAINT_DEFERRED_CLASS]. - bool _checkForOnClauseErrorCodes(OnClause? onClause) { + bool _checkForOnClauseErrorCodes(MixinOnClause? onClause) { if (onClause == null) { return false; } @@ -4913,7 +4913,7 @@ class ErrorVerifier extends RecursiveAstVisitor NamedType? superclass, WithClause? withClause, ImplementsClause? implementsClause, - OnClause? onClause) { + MixinOnClause? onClause) { void reportErrorsForSealedClassesAndMixins(List namedTypes) { for (NamedType namedType in namedTypes) { final type = namedType.type; @@ -5708,7 +5708,7 @@ class ErrorVerifier extends RecursiveAstVisitor /// Checks the class for problems with the superclass, mixins, or implemented /// interfaces. - void _checkMixinInheritance(MixinDeclaration node, OnClause? onClause, + void _checkMixinInheritance(MixinDeclaration node, MixinOnClause? onClause, ImplementsClause? implementsClause) { // Only check for all of the inheritance logic around clauses if there // isn't an error code such as "Cannot implement double" already. diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart index 872d92e09aee..2f462748ebaa 100644 --- a/pkg/analyzer/lib/src/generated/resolver.dart +++ b/pkg/analyzer/lib/src/generated/resolver.dart @@ -2897,6 +2897,12 @@ class ResolverVisitor extends ThrowingAstVisitor node.declaredElement!, node.implementsClause); } + @override + void visitMixinOnClause(MixinOnClause node) { + checkUnreachableNode(node); + node.visitChildren(this); + } + @override void visitNamedExpression(NamedExpression node, {DartType contextType = UnknownInferredType.instance}) { @@ -2942,12 +2948,6 @@ class ResolverVisitor extends ThrowingAstVisitor checkUnreachableNode(node); } - @override - void visitOnClause(OnClause node) { - checkUnreachableNode(node); - node.visitChildren(this); - } - @override void visitParenthesizedExpression(ParenthesizedExpression node, {DartType contextType = UnknownInferredType.instance}) { diff --git a/pkg/analyzer/lib/src/lint/linter_visitor.dart b/pkg/analyzer/lib/src/lint/linter_visitor.dart index 255f3baf6f0e..b1932b3fb03a 100644 --- a/pkg/analyzer/lib/src/lint/linter_visitor.dart +++ b/pkg/analyzer/lib/src/lint/linter_visitor.dart @@ -635,6 +635,12 @@ class LinterVisitor implements AstVisitor { node.visitChildren(this); } + @override + void visitMixinOnClause(MixinOnClause node) { + _runSubscriptions(node, registry._forMixinOnClause); + node.visitChildren(this); + } + @override void visitNamedExpression(NamedExpression node) { _runSubscriptions(node, registry._forNamedExpression); @@ -683,9 +689,10 @@ class LinterVisitor implements AstVisitor { node.visitChildren(this); } + @Deprecated('Use visitMixinOnClause() instead') @override void visitOnClause(OnClause node) { - _runSubscriptions(node, registry._forOnClause); + _runSubscriptions(node, registry._forMixinOnClause); node.visitChildren(this); } @@ -1203,6 +1210,7 @@ class NodeLintRegistry { final List<_Subscription> _forMethodDeclaration = []; final List<_Subscription> _forMethodInvocation = []; final List<_Subscription> _forMixinDeclaration = []; + final List<_Subscription> _forMixinOnClause = []; final List<_Subscription> _forNamedExpression = []; final List<_Subscription> _forNamedType = []; final List<_Subscription> _forNativeClause = []; @@ -1210,7 +1218,6 @@ class NodeLintRegistry { final List<_Subscription> _forNullAssertPattern = []; final List<_Subscription> _forNullCheckPattern = []; final List<_Subscription> _forNullLiteral = []; - final List<_Subscription> _forOnClause = []; final List<_Subscription> _forParenthesizedExpression = []; final List<_Subscription> _forParenthesizedPattern = []; @@ -1746,6 +1753,10 @@ class NodeLintRegistry { _forMixinDeclaration.add(_Subscription(linter, visitor, _getTimer(linter))); } + void addMixinOnClause(LintRule linter, AstVisitor visitor) { + _forMixinOnClause.add(_Subscription(linter, visitor, _getTimer(linter))); + } + void addNamedExpression(LintRule linter, AstVisitor visitor) { _forNamedExpression.add(_Subscription(linter, visitor, _getTimer(linter))); } @@ -1780,8 +1791,9 @@ class NodeLintRegistry { _forObjectPattern.add(_Subscription(linter, visitor, _getTimer(linter))); } + @Deprecated('Use addMixinOnClause() instead') void addOnClause(LintRule linter, AstVisitor visitor) { - _forOnClause.add(_Subscription(linter, visitor, _getTimer(linter))); + addMixinOnClause(linter, visitor); } void addParenthesizedExpression(LintRule linter, AstVisitor visitor) { diff --git a/pkg/analyzer/lib/src/summary2/element_builder.dart b/pkg/analyzer/lib/src/summary2/element_builder.dart index ff9c64ffb91a..738c33e7a65c 100644 --- a/pkg/analyzer/lib/src/summary2/element_builder.dart +++ b/pkg/analyzer/lib/src/summary2/element_builder.dart @@ -1026,13 +1026,13 @@ class ElementBuilder extends ThrowingAstVisitor { } @override - void visitNamedType(NamedType node) { - node.typeArguments?.accept(this); + void visitMixinOnClause(MixinOnClause node) { + node.superclassConstraints.accept(this); } @override - void visitOnClause(OnClause node) { - node.superclassConstraints.accept(this); + void visitNamedType(NamedType node) { + node.typeArguments?.accept(this); } @override diff --git a/pkg/analyzer/lib/src/summary2/reference_resolver.dart b/pkg/analyzer/lib/src/summary2/reference_resolver.dart index 2a0e32b88635..fce1cb522ed6 100644 --- a/pkg/analyzer/lib/src/summary2/reference_resolver.dart +++ b/pkg/analyzer/lib/src/summary2/reference_resolver.dart @@ -351,6 +351,11 @@ class ReferenceResolver extends ThrowingAstVisitor { scope = outerScope; } + @override + void visitMixinOnClause(MixinOnClause node) { + node.superclassConstraints.accept(this); + } + @override void visitNamedType(covariant NamedTypeImpl node) { Element? element; @@ -400,11 +405,6 @@ class ReferenceResolver extends ThrowingAstVisitor { } } - @override - void visitOnClause(OnClause node) { - node.superclassConstraints.accept(this); - } - @override void visitRecordTypeAnnotation(covariant RecordTypeAnnotationImpl node) { node.positionalFields.accept(this); diff --git a/pkg/analyzer/lib/src/test_utilities/find_node.dart b/pkg/analyzer/lib/src/test_utilities/find_node.dart index 787132e7c733..c79a5c4f6532 100644 --- a/pkg/analyzer/lib/src/test_utilities/find_node.dart +++ b/pkg/analyzer/lib/src/test_utilities/find_node.dart @@ -122,9 +122,9 @@ class FindNode { MixinDeclaration get singleMixinDeclaration => _single(); - NamedType get singleNamedType => _single(); + MixinOnClause get singleMixinOnClause => _single(); - OnClause get singleOnClause => _single(); + NamedType get singleNamedType => _single(); ParenthesizedExpression get singleParenthesizedExpression => _single(); diff --git a/pkg/analyzer/test/src/dart/resolution/mixin_test.dart b/pkg/analyzer/test/src/dart/resolution/mixin_test.dart index 3ec52fd32654..29c578ca5322 100644 --- a/pkg/analyzer/test/src/dart/resolution/mixin_test.dart +++ b/pkg/analyzer/test/src/dart/resolution/mixin_test.dart @@ -321,9 +321,9 @@ class B {} mixin M on A, B {} '''); - final node = findNode.singleOnClause; + final node = findNode.singleMixinOnClause; assertResolvedNodeText(node, r''' -OnClause +MixinOnClause onKeyword: on superclassConstraints NamedType diff --git a/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_deferred_class_test.dart b/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_deferred_class_test.dart index 36f10cfdd938..f84884be9744 100644 --- a/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_deferred_class_test.dart +++ b/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_deferred_class_test.dart @@ -25,9 +25,9 @@ mixin M on math.Random {} 48, 11), ]); - final node = findNode.singleOnClause; + final node = findNode.singleMixinOnClause; assertResolvedNodeText(node, r''' -OnClause +MixinOnClause onKeyword: on superclassConstraints NamedType diff --git a/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_disallowed_class_test.dart b/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_disallowed_class_test.dart index 848c60366753..5fae7f5f3cdf 100644 --- a/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_disallowed_class_test.dart +++ b/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_disallowed_class_test.dart @@ -31,9 +31,9 @@ mixin M on Enum {} 27, 4), ]); - final node = findNode.singleOnClause; + final node = findNode.singleMixinOnClause; assertResolvedNodeText(node, r''' -OnClause +MixinOnClause onKeyword: on superclassConstraints NamedType @@ -51,9 +51,9 @@ mixin M on int {} 11, 3), ]); - final node = findNode.singleOnClause; + final node = findNode.singleMixinOnClause; assertResolvedNodeText(node, r''' -OnClause +MixinOnClause onKeyword: on superclassConstraints NamedType diff --git a/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_non_interface_test.dart b/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_non_interface_test.dart index e65167714b65..d6ecf13e06ef 100644 --- a/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_non_interface_test.dart +++ b/pkg/analyzer/test/src/diagnostics/mixin_super_class_constraint_non_interface_test.dart @@ -25,9 +25,9 @@ mixin M on dynamic {} 7), ]); - final node = findNode.singleOnClause; + final node = findNode.singleMixinOnClause; assertResolvedNodeText(node, r''' -OnClause +MixinOnClause onKeyword: on superclassConstraints NamedType @@ -46,9 +46,9 @@ mixin M on E {} 1), ]); - final node = findNode.singleOnClause; + final node = findNode.singleMixinOnClause; assertResolvedNodeText(node, r''' -OnClause +MixinOnClause onKeyword: on superclassConstraints NamedType @@ -67,9 +67,9 @@ mixin M on A {} 1), ]); - final node = findNode.singleOnClause; + final node = findNode.singleMixinOnClause; assertResolvedNodeText(node, r''' -OnClause +MixinOnClause onKeyword: on superclassConstraints NamedType @@ -87,9 +87,9 @@ mixin M on Never {} 5), ]); - final node = findNode.singleOnClause; + final node = findNode.singleMixinOnClause; assertResolvedNodeText(node, r''' -OnClause +MixinOnClause onKeyword: on superclassConstraints NamedType @@ -108,9 +108,9 @@ mixin M on void {} 4), ]); - final node = findNode.singleOnClause; + final node = findNode.singleMixinOnClause; assertResolvedNodeText(node, r''' -OnClause +MixinOnClause onKeyword: on superclassConstraints NamedType diff --git a/pkg/analyzer/test/src/fasta/ast_builder_test.dart b/pkg/analyzer/test/src/fasta/ast_builder_test.dart index cc34815f9bd4..5fa2d8ad2b6c 100644 --- a/pkg/analyzer/test/src/fasta/ast_builder_test.dart +++ b/pkg/analyzer/test/src/fasta/ast_builder_test.dart @@ -1413,7 +1413,7 @@ mixin M on C implements A, (int, int), B {} MixinDeclaration mixinKeyword: mixin @11 name: M @17 - onClause: OnClause + onClause: MixinOnClause onKeyword: on @19 superclassConstraints NamedType @@ -1464,7 +1464,7 @@ mixin M on A, (int, int), B {} MixinDeclaration mixinKeyword: mixin @0 name: M @6 - onClause: OnClause + onClause: MixinOnClause onKeyword: on @8 superclassConstraints NamedType diff --git a/pkg/analyzer/test/src/summary/resolved_ast_printer.dart b/pkg/analyzer/test/src/summary/resolved_ast_printer.dart index d9a3040a9886..f0d3c0720c1e 100644 --- a/pkg/analyzer/test/src/summary/resolved_ast_printer.dart +++ b/pkg/analyzer/test/src/summary/resolved_ast_printer.dart @@ -1031,6 +1031,14 @@ class ResolvedAstPrinter extends ThrowingAstVisitor { }); } + @override + void visitMixinOnClause(MixinOnClause node) { + _sink.writeln('MixinOnClause'); + _sink.withIndent(() { + _writeNamedChildEntities(node); + }); + } + @override void visitNamedExpression(NamedExpression node) { _sink.writeln('NamedExpression'); @@ -1097,14 +1105,6 @@ class ResolvedAstPrinter extends ThrowingAstVisitor { }); } - @override - void visitOnClause(OnClause node) { - _sink.writeln('OnClause'); - _sink.withIndent(() { - _writeNamedChildEntities(node); - }); - } - @override void visitParenthesizedExpression(ParenthesizedExpression node) { _sink.writeln('ParenthesizedExpression'); diff --git a/pkg/analyzer/test/util/ast_type_matchers.dart b/pkg/analyzer/test/util/ast_type_matchers.dart index 835bf69ba7fb..6a1437e6a74b 100644 --- a/pkg/analyzer/test/util/ast_type_matchers.dart +++ b/pkg/analyzer/test/util/ast_type_matchers.dart @@ -185,6 +185,8 @@ const isMethodReferenceExpression = TypeMatcher(); const isMixinDeclaration = TypeMatcher(); +const isMixinOnClause = TypeMatcher(); + const isNamedCompilationUnitMember = TypeMatcher(); const isNamedExpression = TypeMatcher(); @@ -201,8 +203,6 @@ const isNormalFormalParameter = TypeMatcher(); const isNullLiteral = TypeMatcher(); -const isOnClause = TypeMatcher(); - const isParenthesizedExpression = TypeMatcher(); const isPartDirective = TypeMatcher(); diff --git a/pkg/analyzer_plugin/lib/src/utilities/completion/optype.dart b/pkg/analyzer_plugin/lib/src/utilities/completion/optype.dart index e7184fcf8ca8..17f9066c7bea 100644 --- a/pkg/analyzer_plugin/lib/src/utilities/completion/optype.dart +++ b/pkg/analyzer_plugin/lib/src/utilities/completion/optype.dart @@ -1174,6 +1174,12 @@ class _OpTypeAstVisitor extends GeneralizingAstVisitor { } } + @override + void visitMixinOnClause(MixinOnClause node) { + optype.completionLocation = 'MixinOnClause_superclassConstraint'; + optype.includeTypeNameSuggestions = true; + } + @override void visitNamedExpression(NamedExpression node) { if (identical(entity, node.expression)) { @@ -1271,12 +1277,6 @@ class _OpTypeAstVisitor extends GeneralizingAstVisitor { optype.completionLocation = 'ObjectPattern_fieldName'; } - @override - void visitOnClause(OnClause node) { - optype.completionLocation = 'OnClause_superclassConstraint'; - optype.includeTypeNameSuggestions = true; - } - @override void visitParenthesizedExpression(ParenthesizedExpression node) { if (identical(entity, node.expression)) { diff --git a/pkg/analyzer_plugin/test/src/utilities/completion/optype_test.dart b/pkg/analyzer_plugin/test/src/utilities/completion/optype_test.dart index e14bfc5f307c..aeb464f39bc8 100644 --- a/pkg/analyzer_plugin/test/src/utilities/completion/optype_test.dart +++ b/pkg/analyzer_plugin/test/src/utilities/completion/optype_test.dart @@ -2125,10 +2125,12 @@ void f(int a, {int b}) {} } Future test_onClause_beginning() async { - // OnClause MixinDeclaration + // MixinOnClause MixinDeclaration addTestSource('mixin M on ^\n{}'); await assertOpType( - completionLocation: 'OnClause_superclassConstraint', typeNames: true); + completionLocation: 'MixinOnClause_superclassConstraint', + typeNames: true, + ); } Future test_postfixExpression_inOperator() async { From c6a82ebf9d32f99bc1e0fb8ca2e3338731515b4a Mon Sep 17 00:00:00 2001 From: Konstantin Shcheglov Date: Wed, 3 Apr 2024 00:03:19 +0000 Subject: [PATCH 7/8] Macro. Send dart/textDocumentContentDidChange before analysis.errors Change-Id: I343d61208f3500e68219257459e167307a55ee78 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/360662 Reviewed-by: Brian Wilkerson Commit-Queue: Konstantin Shcheglov --- .../lib/src/analysis_server.dart | 24 ++-- .../test/analysis_server_base.dart | 6 +- .../abstract_lsp_over_legacy.dart | 124 ++++++++++++++++++ ...t_text_document_content_provider_test.dart | 30 ++++- 4 files changed, 166 insertions(+), 18 deletions(-) diff --git a/pkg/analysis_server/lib/src/analysis_server.dart b/pkg/analysis_server/lib/src/analysis_server.dart index 1348c348526e..6770740ef2c1 100644 --- a/pkg/analysis_server/lib/src/analysis_server.dart +++ b/pkg/analysis_server/lib/src/analysis_server.dart @@ -999,18 +999,6 @@ abstract class CommonServerContextManagerCallbacks var path = result.path; filesToFlush.add(path); - if (result is AnalysisResultWithErrors) { - if (analysisServer.isAnalyzed(path)) { - final serverErrors = server.doAnalysisError_listFromEngine(result); - recordAnalysisErrors(path, serverErrors); - } - } - - if (result is ResolvedUnitResult) { - analysisServer.filesResolvedSinceLastIdle.add(path); - handleResolvedUnitResult(result); - } - // If this is a virtual file and the client supports URIs, we need to notify // that it's been updated. var lspUri = analysisServer.uriConverter.toClientUri(result.path); @@ -1027,6 +1015,18 @@ abstract class CommonServerContextManagerCallbacks ); analysisServer.sendLspNotification(message); } + + if (result is AnalysisResultWithErrors) { + if (analysisServer.isAnalyzed(path)) { + final serverErrors = server.doAnalysisError_listFromEngine(result); + recordAnalysisErrors(path, serverErrors); + } + } + + if (result is ResolvedUnitResult) { + analysisServer.filesResolvedSinceLastIdle.add(path); + handleResolvedUnitResult(result); + } } void handleResolvedUnitResult(ResolvedUnitResult result); diff --git a/pkg/analysis_server/test/analysis_server_base.dart b/pkg/analysis_server/test/analysis_server_base.dart index 4fa06684c62f..185da8b22fbf 100644 --- a/pkg/analysis_server/test/analysis_server_base.dart +++ b/pkg/analysis_server/test/analysis_server_base.dart @@ -93,6 +93,8 @@ abstract class ContextResolutionTest with ResourceProviderMixin { final List _analysisGeneralServices = []; final Map> _analysisFileSubscriptions = {}; + void Function(Notification)? notificationListener; + Folder get sdkRoot => newFolder('/sdk'); Future addGeneralAnalysisSubscription( @@ -126,7 +128,9 @@ abstract class ContextResolutionTest with ResourceProviderMixin { return response; } - void processNotification(Notification notification) {} + void processNotification(Notification notification) { + notificationListener?.call(notification); + } Future removeGeneralAnalysisSubscription( GeneralAnalysisService service, diff --git a/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart b/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart index 50e24bfef827..e99ac6d4d897 100644 --- a/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart +++ b/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart @@ -7,15 +7,114 @@ import 'dart:convert'; import 'package:analysis_server/lsp_protocol/protocol.dart'; import 'package:analysis_server/protocol/protocol_constants.dart'; +import 'package:analysis_server/src/lsp/constants.dart'; import 'package:analysis_server/src/protocol/protocol_internal.dart'; import 'package:analysis_server/src/protocol_server.dart'; +import 'package:analyzer/file_system/file_system.dart'; +import 'package:analyzer/src/utilities/extensions/file_system.dart'; import 'package:analyzer_plugin/src/utilities/client_uri_converter.dart'; +import 'package:collection/collection.dart'; import 'package:path/path.dart' as path; import 'package:test/test.dart'; import '../analysis_server_base.dart'; import '../lsp/change_verifier.dart'; import '../lsp/request_helpers_mixin.dart'; +import '../services/completion/dart/text_expectations.dart'; +import '../utils/tree_string_sink.dart'; + +class EventsCollector { + final ContextResolutionTest test; + List events = []; + + EventsCollector(this.test) { + test.notificationListener = (notification) { + switch (notification.event) { + case ANALYSIS_NOTIFICATION_ERRORS: + events.add( + AnalysisErrorsParams.fromNotification(notification), + ); + case ANALYSIS_NOTIFICATION_FLUSH_RESULTS: + events.add( + AnalysisFlushResultsParams.fromNotification(notification), + ); + case LSP_NOTIFICATION_NOTIFICATION: + final params = LspNotificationParams.fromNotification(notification); + events.add(params.lspNotification); + default: + throw StateError(notification.event); + } + }; + } + + List take() { + final result = events; + events = []; + return result; + } +} + +class EventsPrinter { + final EventsPrinterConfiguration configuration; + final ResourceProvider resourceProvider; + final TreeStringSink sink; + + EventsPrinter({ + required this.configuration, + required this.resourceProvider, + required this.sink, + }); + + void write(List events) { + for (final event in events) { + switch (event) { + case AnalysisErrorsParams(): + sink.writelnWithIndent('AnalysisErrors'); + sink.withIndent(() { + _writelnFile(name: 'file', event.file); + if (event.errors.isNotEmpty) { + sink.writelnWithIndent('errors: notEmpty'); + } else { + sink.writelnWithIndent('errors: empty'); + } + }); + case AnalysisFlushResultsParams(): + sink.writeElements( + 'AnalysisFlushResults', + event.files.sorted(), + _writelnFile, + ); + case NotificationMessage(): + switch (event.method) { + case CustomMethods.dartTextDocumentContentDidChange: + sink.writelnWithIndent(event.method); + var params = + event.params as DartTextDocumentContentDidChangeParams; + sink.withIndent(() { + sink.writeIndentedLine(() { + sink.write('uri: '); + sink.write(params.uri); + }); + }); + } + default: + throw UnimplementedError('${event.runtimeType}'); + } + } + } + + void _writelnFile(String path, {String? name}) { + sink.writeIndentedLine(() { + if (name != null) { + sink.write('$name: '); + } + final file = resourceProvider.getFile(path); + sink.write(file.posixPath); + }); + } +} + +class EventsPrinterConfiguration {} abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest with @@ -61,6 +160,31 @@ abstract class LspOverLegacyTest extends PubPackageAnalysisServerTest ); } + Future assertEventsText( + EventsCollector collector, + String expected, + ) async { + await pumpEventQueue(times: 5000); + + final buffer = StringBuffer(); + final sink = TreeStringSink(sink: buffer, indent: ''); + + final events = collector.take(); + EventsPrinter( + configuration: EventsPrinterConfiguration(), + resourceProvider: resourceProvider, + sink: sink, + ).write(events); + + final actual = buffer.toString(); + if (actual != expected) { + print('-------- Actual --------'); + print('$actual------------------------'); + TextExpectationsCollector.add(actual); + } + expect(actual, expected); + } + /// Creates a legacy request with an auto-assigned ID. Request createLegacyRequest(RequestParams params) { return params.toRequest('${_nextLspRequestId++}'); diff --git a/pkg/analysis_server/test/lsp_over_legacy/dart_text_document_content_provider_test.dart b/pkg/analysis_server/test/lsp_over_legacy/dart_text_document_content_provider_test.dart index 9a216de4cf42..b00817134f3d 100644 --- a/pkg/analysis_server/test/lsp_over_legacy/dart_text_document_content_provider_test.dart +++ b/pkg/analysis_server/test/lsp_over_legacy/dart_text_document_content_provider_test.dart @@ -2,7 +2,7 @@ // for details. 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:analysis_server/protocol/protocol_generated.dart'; +import 'package:analysis_server/src/protocol_server.dart'; import 'package:test/test.dart'; import 'package:test_reflective_loader/test_reflective_loader.dart'; @@ -65,15 +65,35 @@ class A {} await waitForTasksFinished(); await enableCustomUriSupport(); + final collector = EventsCollector(this); + // Verify initial contents of the macro. var macroGeneratedContent = await getDartTextDocumentContent(testFileMacroUri); expect(macroGeneratedContent!.content, contains('void foo() {')); - // Modify the file and expect a change event. - newFile(testFilePath, content.replaceAll('void foo() {', 'void foo2() {')); - await dartTextDocumentContentDidChangeNotifications - .firstWhere((notification) => notification.uri == testFileMacroUri); + // Modify the file, changing its API signature. + // So, the macro runs, and the macro generated file changes. + newFile( + testFilePath, + content.replaceAll( + 'void foo() {}', + 'void foo2() {}', + ), + ); + + // Note, 'dart/textDocumentContentDidChange' before 'AnalysisErrors'. + // So, the IDE does not discard errors. + await assertEventsText(collector, r''' +AnalysisErrors + file: /home/test/lib/test.dart + errors: empty +dart/textDocumentContentDidChange + uri: dart-macro+file:///home/test/lib/test.dart +AnalysisErrors + file: /home/test/lib/test.macro.dart + errors: empty +'''); // Verify updated contents of the macro. macroGeneratedContent = await getDartTextDocumentContent(testFileMacroUri); From 769cfb32baf779bba7e7b67b50f330e46c0640ea Mon Sep 17 00:00:00 2001 From: Brian Wilkerson Date: Wed, 3 Apr 2024 00:30:00 +0000 Subject: [PATCH 8/8] Use the budget to control whether overrides are suggested I believe that this is the right thing to do from a UX perspective, but this change could potentially cause flakiness in tests. There's support in the LSP path for setting a bigger budget, but not for legacy based tests (at least not yet). Change-Id: I76353fd3e4cb4cf8fb7a55c27a3353415143dc06 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/360740 Reviewed-by: Keerti Parthasarathy Commit-Queue: Brian Wilkerson --- .../completion/dart/completion_manager.dart | 2 +- .../completion/dart/completion_state.dart | 34 +++++++++++-------- .../dart/in_scope_completion_pass.dart | 6 ++-- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart b/pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart index 126018ceed88..76d6223c827d 100644 --- a/pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart +++ b/pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart @@ -159,7 +159,7 @@ class DartCompletionManager { if (selection == null) { throw AbortCompletion(); } - var state = CompletionState(request, selection); + var state = CompletionState(request, selection, budget); var pass = InScopeCompletionPass( state: state, collector: collector, diff --git a/pkg/analysis_server/lib/src/services/completion/dart/completion_state.dart b/pkg/analysis_server/lib/src/services/completion/dart/completion_state.dart index ea4b2f81bd28..5e961810dbba 100644 --- a/pkg/analysis_server/lib/src/services/completion/dart/completion_state.dart +++ b/pkg/analysis_server/lib/src/services/completion/dart/completion_state.dart @@ -14,35 +14,41 @@ class CompletionState { /// The completion request being processed. final DartCompletionRequest request; - /// The selection at the time completion was requested. The selection is - /// required to have a length of zero. + /// The selection at the time completion was requested. + /// + /// The selection is required to have a length of zero. final Selection selection; + /// The budget controlling how much time can be spent computing completion + /// suggestions. + final CompletionBudget budget; + /// Initialize a newly created completion state. - CompletionState(this.request, this.selection) : assert(selection.length == 0); + CompletionState(this.request, this.selection, this.budget) + : assert(selection.length == 0); - /// Returns the type of value required by the context in which completion was + /// The type of value required by the context in which completion was /// requested. DartType? get contextType => request.contextType; - /// Return the [ClassMember] that encloses the completion location, or `null` - /// if the completion location isn't in a class member. + /// The [ClassMember] that encloses the completion location, or `null` if the + /// completion location isn't in a class member. ClassMember? get enclosingMember { return selection.coveringNode.thisOrAncestorOfType(); } - /// Return `true` if the completion location is inside an instance member, and - /// hence there is a binding for `this`. + /// Whether the completion location is inside an instance member, and hence + /// whether there is a binding for `this`. bool get inInstanceScope { var member = enclosingMember; return member != null && !member.isStatic; } - /// Returns the element of the library containing the completion location. + /// The element of the library containing the completion location. LibraryElement get libraryElement => request.libraryElement; - /// Return the type of `this` at the completion location, or `null` - /// if the completion location doesn't allow `this` to be used. + /// The type of `this` at the completion location, or `null` if the completion + /// location doesn't allow `this` to be used. DartType? get thisType { AstNode? node = selection.coveringNode; while (node != null) { @@ -70,8 +76,8 @@ class CompletionState { return null; } - /// Return `true` if the given `feature` is enabled in the library containing - /// the selection. + /// Whether the given `feature` is enabled in the library containing the + /// selection. bool isFeatureEnabled(Feature feature) { return libraryElement.featureSet.isEnabled(feature); } @@ -79,7 +85,7 @@ class CompletionState { // TODO(brianwilkerson): Move to 'package:analysis_server/src/utilities/extensions/ast.dart' extension on ClassMember { - /// Return `true` if this member is a static member. + /// Whether this member is a static member. bool get isStatic { var self = this; if (self is MethodDeclaration) { diff --git a/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart b/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart index fa69193089f1..888bc45916f1 100644 --- a/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart +++ b/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart @@ -3468,8 +3468,10 @@ class InScopeCompletionPass extends SimpleAstVisitor { required InterfaceElement? element, bool skipAt = false, }) { - // TODO(brianwilkerson): Check whether there's sufficient remaining time - // before computing suggestions for overrides. + if (state.budget.isEmpty) { + // Don't suggest overrides if the time budget has already been spent. + return; + } if (suggestOverrides && element != null) { overrideHelper.computeOverridesFor( interfaceElement: element,