Do not throw ProtocolErrorType.PARAMS for custom functions and importers (#118)

Author: ntkme

bin/dart_sass_embedded.dart

@@ -55,13 +55,8 @@ void main(List<String> args) {
           _decodeImporter(dispatcher, request, importer) ??
           (throw mandatoryError("Importer.importer")));
 
-      var globalFunctions = request.globalFunctions.map((signature) {
-        try {
-          return hostCallable(dispatcher, functions, request.id, signature);
-        } on sass.SassException catch (error) {
-          throw paramsError('CompileRequest.global_functions: $error');
-        }
-      });
+      var globalFunctions = request.globalFunctions.map((signature) =>
+          hostCallable(dispatcher, functions, request.id, signature));
 
       late sass.CompileResult result;
       switch (request.whichInput()) {

lib/src/host_callable.dart

@@ -7,7 +7,6 @@ import 'dart:cli';
 import 'dart:io';
 
 import 'package:sass_api/sass_api.dart' as sass;
-import 'package:source_span/source_span.dart';
 
 import 'dispatcher.dart';
 import 'embedded_sass.pb.dart';
@@ -26,21 +25,8 @@ import 'utils.dart';
 sass.Callable hostCallable(Dispatcher dispatcher, FunctionRegistry functions,
     int compilationId, String signature,
     {int? id}) {
-  var openParen = signature.indexOf('(');
-  if (openParen == -1) {
-    throw sass.SassException('"$signature" is missing "("',
-        SourceFile.fromString(signature).span(0));
-  }
-
-  if (!signature.endsWith(")")) {
-    throw sass.SassException('"$signature" doesn\'t end with ")"',
-        SourceFile.fromString(signature).span(signature.length));
-  }
-
-  var name = signature.substring(0, openParen);
-  return sass.Callable.function(
-      name, signature.substring(openParen + 1, signature.length - 1),
-      (arguments) {
+  late sass.Callable callable;
+  callable = sass.Callable.fromSignature(signature, (arguments) {
     var protofier = Protofier(dispatcher, functions, compilationId);
     var request = OutboundMessage_FunctionCallRequest()
       ..compilationId = compilationId
@@ -50,7 +36,7 @@ sass.Callable hostCallable(Dispatcher dispatcher, FunctionRegistry functions,
     if (id != null) {
       request.functionId = id;
     } else {
-      request.name = name;
+      request.name = callable.name;
     }
 
     // ignore: deprecated_member_use
@@ -74,4 +60,5 @@ sass.Callable hostCallable(Dispatcher dispatcher, FunctionRegistry functions,
       throw error.message;
     }
   });
+  return callable;
 }

lib/src/importer/base.dart

@@ -6,8 +6,6 @@ import 'package:meta/meta.dart';
 import 'package:sass_api/sass_api.dart' as sass;
 
 import '../dispatcher.dart';
-import '../embedded_sass.pb.dart' hide SourceSpan;
-import '../utils.dart';
 
 /// An abstract base class for importers that communicate with the host in some
 /// way.
@@ -21,25 +19,17 @@ abstract class ImporterBase extends sass.Importer {
   /// Parses [url] as a [Uri] and throws an error if it's invalid or relative
   /// (including root-relative).
   ///
-  /// The [field] name is used in the error message if one is thrown.
+  /// The [source] name is used in the error message if one is thrown.
   @protected
-  Uri parseAbsoluteUrl(String field, String url) {
+  Uri parseAbsoluteUrl(String source, String url) {
     Uri parsedUrl;
     try {
       parsedUrl = Uri.parse(url);
     } on FormatException catch (error) {
-      sendAndThrow(paramsError("$field is invalid: $error"));
+      throw '$source must return a URL, was "$url"';
     }
 
     if (parsedUrl.scheme.isNotEmpty) return parsedUrl;
-    sendAndThrow(paramsError('$field must be absolute, was "$parsedUrl"'));
-  }
-
-  /// Sends [error] to the remote endpoint, and also throws it so that the Sass
-  /// compilation fails.
-  @protected
-  Never sendAndThrow(ProtocolError error) {
-    dispatcher.sendError(error);
-    throw "Protocol error: ${error.message}";
+    throw '$source must return an absolute URL, was "$parsedUrl"';
   }
 }

lib/src/importer/file.dart

@@ -9,7 +9,6 @@ import 'package:sass_api/sass_api.dart' as sass;
 
 import '../dispatcher.dart';
 import '../embedded_sass.pb.dart' hide SourceSpan;
-import '../utils.dart';
 import 'base.dart';
 
 /// A filesystem importer to use for most implementation details of
@@ -44,11 +43,9 @@ class FileImporter extends ImporterBase {
 
       switch (response.whichResult()) {
         case InboundMessage_FileImportResponse_Result.fileUrl:
-          var url =
-              parseAbsoluteUrl("FileImportResponse.file_url", response.fileUrl);
+          var url = parseAbsoluteUrl("The file importer", response.fileUrl);
           if (url.scheme != 'file') {
-            sendAndThrow(paramsError(
-                'FileImportResponse.file_url must be a file: URL, was "$url"'));
+            throw 'The file importer must return a file: URL, was "$url"';
           }
 
           return _filesystemImporter.canonicalize(url);

lib/src/importer/host.dart

@@ -35,7 +35,7 @@ class HostImporter extends ImporterBase {
 
       switch (response.whichResult()) {
         case InboundMessage_CanonicalizeResponse_Result.url:
-          return parseAbsoluteUrl("CanonicalizeResponse.url", response.url);
+          return parseAbsoluteUrl("The importer", response.url);
 
         case InboundMessage_CanonicalizeResponse_Result.error:
           throw response.error;
@@ -60,8 +60,8 @@ class HostImporter extends ImporterBase {
           return sass.ImporterResult(response.success.contents,
               sourceMapUrl: response.success.sourceMapUrl.isEmpty
                   ? null
-                  : parseAbsoluteUrl("ImportResponse.success.source_map_url",
-                      response.success.sourceMapUrl),
+                  : parseAbsoluteUrl(
+                      "The importer", response.success.sourceMapUrl),
               syntax: syntaxToSyntax(response.success.syntax));
 
         case InboundMessage_ImportResponse_Result.error:

test/file_importer_test.dart

@@ -54,6 +54,18 @@ void main() {
           "InboundMessage_CanonicalizeResponse.");
       await process.kill();
     });
+  });
+
+  group("emits a compile failure", () {
+    late OutboundMessage_FileImportRequest request;
+
+    setUp(() async {
+      process.inbound.add(compileString("@import 'other'", importers: [
+        InboundMessage_CompileRequest_Importer()..fileImporterId = 1
+      ]));
+
+      request = getFileImportRequest(await process.outbound.next);
+    });
 
     group("for a FileImportResponse with a URL", () {
       test("that's empty", () async {
@@ -62,8 +74,8 @@ void main() {
             ..id = request.id
             ..fileUrl = ""));
 
-        await _expectImportParamsError(
-            process, 'FileImportResponse.file_url must be absolute, was ""');
+        await _expectImportError(
+            process, 'The file importer must return an absolute URL, was ""');
         await process.kill();
       });
 
@@ -73,8 +85,8 @@ void main() {
             ..id = request.id
             ..fileUrl = "foo"));
 
-        await _expectImportParamsError(
-            process, 'FileImportResponse.file_url must be absolute, was "foo"');
+        await _expectImportError(process,
+            'The file importer must return an absolute URL, was "foo"');
         await process.kill();
       });
 
@@ -84,8 +96,8 @@ void main() {
             ..id = request.id
             ..fileUrl = "other:foo"));
 
-        await _expectImportParamsError(process,
-            'FileImportResponse.file_url must be a file: URL, was "other:foo"');
+        await _expectImportError(process,
+            'The file importer must return a file: URL, was "other:foo"');
         await process.kill();
       });
     });
@@ -268,14 +280,10 @@ void main() {
   });
 }
 
-/// Asserts that [process] emits a [ProtocolError] params error with the given
+/// Asserts that [process] emits a [CompileFailure] result with the given
 /// [message] on its protobuf stream and causes the compilation to fail.
-Future<void> _expectImportParamsError(
-    EmbeddedProcess process, Object message) async {
-  await expectLater(process.outbound,
-      emits(isProtocolError(errorId, ProtocolErrorType.PARAMS, message)));
-
+Future<void> _expectImportError(EmbeddedProcess process, Object message) async {
   var failure = getCompileFailure(await process.outbound.next);
-  expect(failure.message, equals('Protocol error: $message'));
+  expect(failure.message, equals(message));
   expect(failure.span.text, equals("'other'"));
 }

test/function_test.dart

@@ -21,74 +21,39 @@ void main() {
     _process = await EmbeddedProcess.start();
   });
 
-  group("emits a protocol error for a custom function with a signature", () {
+  group("emits a compile failure for a custom function with a signature", () {
     test("that's empty", () async {
       _process.inbound.add(compileString("a {b: c}", functions: [r""]));
-      await expectParamsError(
-          _process,
-          0,
-          'CompileRequest.global_functions: Error: "" is missing "("\n'
-          '  ╷\n'
-          '1 │ \n'
-          '  │ ^\n'
-          '  ╵\n'
-          '  - 1:1  root stylesheet');
+      await _expectFunctionError(
+          _process, r'Invalid signature "": Expected identifier.');
       await _process.kill();
     });
 
     test("that's just a name", () async {
       _process.inbound.add(compileString("a {b: c}", functions: [r"foo"]));
-      await expectParamsError(
-          _process,
-          0,
-          'CompileRequest.global_functions: Error: "foo" is missing "("\n'
-          '  ╷\n'
-          '1 │ foo\n'
-          '  │ ^^^\n'
-          '  ╵\n'
-          '  - 1:1  root stylesheet');
+      await _expectFunctionError(
+          _process, r'Invalid signature "foo": expected "(".');
       await _process.kill();
     });
 
     test("without a closing paren", () async {
       _process.inbound.add(compileString("a {b: c}", functions: [r"foo($bar"]));
-      await expectParamsError(
-          _process,
-          0,
-          'CompileRequest.global_functions: Error: "foo(\$bar" doesn\'t end with ")"\n'
-          '  ╷\n'
-          '1 │ foo(\$bar\n'
-          '  │         ^\n'
-          '  ╵\n'
-          '  - 1:9  root stylesheet');
+      await _expectFunctionError(
+          _process, r'Invalid signature "foo($bar": expected ")".');
       await _process.kill();
     });
 
     test("with text after the closing paren", () async {
       _process.inbound.add(compileString("a {b: c}", functions: [r"foo() "]));
-      await expectParamsError(
-          _process,
-          0,
-          'CompileRequest.global_functions: Error: "foo() " doesn\'t end with ")"\n'
-          '  ╷\n'
-          '1 │ foo() \n'
-          '  │       ^\n'
-          '  ╵\n'
-          '  - 1:7  root stylesheet');
+      await _expectFunctionError(
+          _process, r'Invalid signature "foo() ": expected no more input.');
       await _process.kill();
     });
 
     test("with invalid arguments", () async {
       _process.inbound.add(compileString("a {b: c}", functions: [r"foo($)"]));
-      await expectParamsError(
-          _process,
-          0,
-          'CompileRequest.global_functions: Error: Expected identifier.\n'
-          '  ╷\n'
-          '1 │ @function foo(\$) {\n'
-          '  │                ^\n'
-          '  ╵\n'
-          '  - 1:16  root stylesheet');
+      await _expectFunctionError(
+          _process, r'Invalid signature "foo($)": Expected identifier.');
       await _process.kill();
     });
   });
@@ -1848,25 +1813,28 @@ void main() {
         }
 
         test("that's empty", () async {
-          await expectSignatureError("", '"" is missing "("');
+          await expectSignatureError(
+              "", r'Invalid signature "": Expected identifier.');
         });
 
         test("that's just a name", () async {
-          await expectSignatureError("foo", '"foo" is missing "("');
+          await expectSignatureError(
+              "foo", r'Invalid signature "foo": expected "(".');
         });
 
         test("without a closing paren", () async {
           await expectSignatureError(
-              r"foo($bar", '"foo(\$bar" doesn\'t end with ")"');
+              r"foo($bar", r'Invalid signature "foo($bar": expected ")".');
         });
 
         test("with text after the closing paren", () async {
-          await expectSignatureError(
-              r"foo() ", '"foo() " doesn\'t end with ")"');
+          await expectSignatureError(r"foo() ",
+              r'Invalid signature "foo() ": expected no more input.');
         });
 
         test("with invalid arguments", () async {
-          await expectSignatureError(r"foo($)", 'Expected identifier.');
+          await expectSignatureError(
+              r"foo($)", r'Invalid signature "foo($)": Expected identifier.');
         });
       });
     });
@@ -1989,3 +1957,11 @@ Value _hsl(num hue, num saturation, num lightness, double alpha) => Value()
     ..saturation = saturation * 1.0
     ..lightness = lightness * 1.0
     ..alpha = alpha);
+
+/// Asserts that [process] emits a [CompileFailure] result with the given
+/// [message] on its protobuf stream and causes the compilation to fail.
+Future<void> _expectFunctionError(
+    EmbeddedProcess process, Object message) async {
+  var failure = getCompileFailure(await process.outbound.next);
+  expect(failure.message, equals(message));
+}