From e5139f694e2b23028ca68e5c7f1d3b8cc71fc669 Mon Sep 17 00:00:00 2001 From: David Morgan Date: Wed, 7 Aug 2024 11:17:47 +0200 Subject: [PATCH] Differentiate types of request explicitly, remove ListOfService. --- pkgs/_macro_host/lib/macro_host.dart | 70 ++++++++----------- pkgs/_macro_host/test/macro_host_test.dart | 19 +++-- .../_macro_server/test/macro_server_test.dart | 8 +-- pkgs/macro_service/lib/src/macro_service.dart | 2 +- .../lib/src/macro_service.g.dart | 24 +++++++ schemas/macro_service.schema.json | 13 ++++ 6 files changed, 80 insertions(+), 56 deletions(-) diff --git a/pkgs/_macro_host/lib/macro_host.dart b/pkgs/_macro_host/lib/macro_host.dart index d2db0e32..dcff2f5a 100644 --- a/pkgs/_macro_host/lib/macro_host.dart +++ b/pkgs/_macro_host/lib/macro_host.dart @@ -14,32 +14,19 @@ import 'package:macro_service/macro_service.dart'; /// /// Tools that want to support macros, such as the Analyzer and the CFE, can /// do so by running a `MacroHost` and providing their own `HostService`. -class MacroHost implements HostService { +class MacroHost { + final _HostService _hostService; final MacroServer macroServer; - final ListOfServices services; final MacroBuilder macroBuilder = MacroBuilder(); final MacroRunner macroRunner = MacroRunner(); - // TODO(davidmorgan): this should be per macro, as part of tracking per-macro - // lifecycle state. - Completer>? _macroPhases; - - MacroHost._(this.macroServer, this.services) { - services.services.insert(0, this); - } + MacroHost._(this.macroServer, this._hostService); - /// Starts a macro host serving the provided [service]. - /// - /// The service passed in should handle introspection RPCs, it does not need - /// to handle others. - /// - /// TODO(davidmorgan): make this split clearer, it should be in the protocol - /// definition somewhere which requests the host handles. - static Future serve({required HostService service}) async { - final listOfServices = ListOfServices(); - listOfServices.services.add(service); - final server = await MacroServer.serve(service: listOfServices); - return MacroHost._(server, listOfServices); + /// Starts a macro host with introspection queries handled by [queryService]. + static Future serve({required QueryService queryService}) async { + final hostService = _HostService(queryService); + final server = await MacroServer.serve(service: hostService); + return MacroHost._(server, hostService); } /// Whether [name] is a macro according to that package's `pubspec.yaml`. @@ -54,11 +41,13 @@ class MacroHost implements HostService { Uri packageConfig, QualifiedName name) async { // TODO(davidmorgan): track macro lifecycle, correctly run once per macro // code change including if queried multiple times before response returns. - if (_macroPhases != null) return _macroPhases!.future; - _macroPhases = Completer(); + if (_hostService._macroPhases != null) { + return _hostService._macroPhases!.future; + } + _hostService._macroPhases = Completer(); final macroBundle = await macroBuilder.build(packageConfig, [name]); macroRunner.start(macroBundle: macroBundle, endpoint: macroServer.endpoint); - return _macroPhases!.future; + return _hostService._macroPhases!.future; } /// Sends [request] to the macro with [name]. @@ -70,34 +59,35 @@ class MacroHost implements HostService { name, HostRequest.augmentRequest(request)); return response.asAugmentResponse; } +} + +class _HostService implements HostService { + final QueryService queryService; + // TODO(davidmorgan): this should be per macro, as part of tracking per-macro + // lifecycle state. + Completer>? _macroPhases; + + _HostService(this.queryService); /// Handle requests that are for the host. @override - Future handle(MacroRequest request) async { + Future handle(MacroRequest request) async { switch (request.type) { case MacroRequestType.macroStartedRequest: _macroPhases!.complete(request .asMacroStartedRequest.macroDescription.runsInPhases .toSet()); return Response.macroStartedResponse(MacroStartedResponse()); + case MacroRequestType.queryRequest: + return Response.queryResponse( + await queryService.handle(request.asQueryRequest)); default: - return null; + return Response.errorResponse(ErrorResponse(error: 'unsupported')); } } } -// TODO(davidmorgan): this is used to handle some requests in the host while -// letting some fall through to the passed in service. Differentiate in a -// better way. -class ListOfServices implements HostService { - List services = []; - - @override - Future handle(MacroRequest request) async { - for (final service in services) { - final result = await service.handle(request); - if (result != null) return result; - } - throw StateError('No service handled: $request'); - } +/// Service provided by the frontend the host integrates with. +abstract interface class QueryService { + Future handle(QueryRequest request); } diff --git a/pkgs/_macro_host/test/macro_host_test.dart b/pkgs/_macro_host/test/macro_host_test.dart index 4639720d..0a3c463e 100644 --- a/pkgs/_macro_host/test/macro_host_test.dart +++ b/pkgs/_macro_host/test/macro_host_test.dart @@ -12,8 +12,8 @@ import 'package:test/test.dart'; void main() { group(MacroHost, () { test('hosts a macro, receives augmentations', () async { - final service = TestMacroHostService(); - final host = await MacroHost.serve(service: service); + final service = TestQueryService(); + final host = await MacroHost.serve(queryService: service); final macroName = QualifiedName( 'package:_test_macros/declare_x_macro.dart#DeclareXImplementation'); @@ -29,8 +29,8 @@ void main() { }); test('hosts a macro, responds to queries', () async { - final service = TestMacroHostService(); - final host = await MacroHost.serve(service: service); + final service = TestQueryService(); + final host = await MacroHost.serve(queryService: service); final macroName = QualifiedName( 'package:_test_macros/query_class.dart#QueryClassImplementation'); @@ -48,13 +48,10 @@ void main() { }); } -class TestMacroHostService implements HostService { +class TestQueryService implements QueryService { @override - Future handle(MacroRequest request) async { - if (request.type == MacroRequestType.queryRequest) { - return Response.queryResponse(QueryResponse( - model: Model(uris: {'package:foo/foo.dart': Library()}))); - } - return null; + Future handle(QueryRequest request) async { + return QueryResponse( + model: Model(uris: {'package:foo/foo.dart': Library()})); } } diff --git a/pkgs/_macro_server/test/macro_server_test.dart b/pkgs/_macro_server/test/macro_server_test.dart index 8c4b3a1c..a2b5ff40 100644 --- a/pkgs/_macro_server/test/macro_server_test.dart +++ b/pkgs/_macro_server/test/macro_server_test.dart @@ -13,7 +13,7 @@ import 'package:test/test.dart'; void main() { group(MacroServer, () { test('serves a macro service', () async { - final service = TestMacroHostService(); + final service = TestHostService(); final server = await MacroServer.serve(service: service); await MacroClient.run( @@ -26,18 +26,18 @@ void main() { }); } -class TestMacroHostService implements HostService { +class TestHostService implements HostService { final StreamController _macroStartedRequestsController = StreamController(); Stream get macroStartedRequests => _macroStartedRequestsController.stream; @override - Future handle(MacroRequest request) async { + Future handle(MacroRequest request) async { if (request.type == MacroRequestType.macroStartedRequest) { _macroStartedRequestsController.add(request.asMacroStartedRequest); return Response.macroStartedResponse(MacroStartedResponse()); } - return null; + return Response.errorResponse(ErrorResponse(error: 'unimplemented')); } } diff --git a/pkgs/macro_service/lib/src/macro_service.dart b/pkgs/macro_service/lib/src/macro_service.dart index 2b8b74e0..5b6725ab 100644 --- a/pkgs/macro_service/lib/src/macro_service.dart +++ b/pkgs/macro_service/lib/src/macro_service.dart @@ -10,7 +10,7 @@ abstract interface class HostService { /// /// Returns `null` if the request is of a type not handled by this service /// instance. - Future handle(MacroRequest request); + Future handle(MacroRequest request); } /// Service provided by the macro to the host. diff --git a/pkgs/macro_service/lib/src/macro_service.g.dart b/pkgs/macro_service/lib/src/macro_service.g.dart index aa3ecfe8..b1161d86 100644 --- a/pkgs/macro_service/lib/src/macro_service.g.dart +++ b/pkgs/macro_service/lib/src/macro_service.g.dart @@ -28,6 +28,18 @@ extension type AugmentResponse.fromJson(Map node) { (node['augmentations'] as List).cast(); } +/// Request could not be handled. +extension type ErrorResponse.fromJson(Map node) { + ErrorResponse({ + String? error, + }) : this.fromJson({ + if (error != null) 'error': error, + }); + + /// The error. + String get error => node['error'] as String; +} + /// A macro host server endpoint. TODO(davidmorgan): this should be a oneOf supporting different types of connection. TODO(davidmorgan): it's not clear if this belongs in this package! But, where else? extension type HostEndpoint.fromJson(Map node) { HostEndpoint({ @@ -157,6 +169,7 @@ extension type QueryResponse.fromJson(Map node) { enum ResponseType { unknown, augmentResponse, + errorResponse, macroStartedResponse, queryResponse; } @@ -165,6 +178,8 @@ extension type Response.fromJson(Map node) { static Response augmentResponse(AugmentResponse augmentResponse) => Response.fromJson( {'type': 'AugmentResponse', 'value': augmentResponse.node}); + static Response errorResponse(ErrorResponse errorResponse) => + Response.fromJson({'type': 'ErrorResponse', 'value': errorResponse.node}); static Response macroStartedResponse( MacroStartedResponse macroStartedResponse) => Response.fromJson( @@ -175,6 +190,8 @@ extension type Response.fromJson(Map node) { switch (node['type'] as String) { case 'AugmentResponse': return ResponseType.augmentResponse; + case 'ErrorResponse': + return ResponseType.errorResponse; case 'MacroStartedResponse': return ResponseType.macroStartedResponse; case 'QueryResponse': @@ -191,6 +208,13 @@ extension type Response.fromJson(Map node) { return AugmentResponse.fromJson(node['value'] as Map); } + ErrorResponse get asErrorResponse { + if (node['type'] != 'ErrorResponse') { + throw StateError('Not a ErrorResponse.'); + } + return ErrorResponse.fromJson(node['value'] as Map); + } + MacroStartedResponse get asMacroStartedResponse { if (node['type'] != 'MacroStartedResponse') { throw StateError('Not a MacroStartedResponse.'); diff --git a/schemas/macro_service.schema.json b/schemas/macro_service.schema.json index f7188bd2..0a9b5d2f 100644 --- a/schemas/macro_service.schema.json +++ b/schemas/macro_service.schema.json @@ -35,6 +35,16 @@ } } }, + "ErrorResponse": { + "type": "object", + "description": "Request could not be handled.", + "properties": { + "error": { + "description": "The error.", + "type": "string" + } + } + }, "HostEndpoint": { "type": "object", "description": "A macro host server endpoint. TODO(davidmorgan): this should be a oneOf supporting different types of connection. TODO(davidmorgan): it's not clear if this belongs in this package! But, where else?", @@ -133,6 +143,9 @@ { "$ref": "#/$defs/AugmentResponse" }, + { + "$ref": "#/$defs/ErrorResponse" + }, { "$ref": "#/$defs/MacroStartedResponse" },