From 2652a60bccd80f5d50772ae1b6defddb0e4503cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=AA=E3=81=A4=E3=81=8D?= Date: Wed, 16 Nov 2022 05:30:51 +0000 Subject: [PATCH 1/6] Do not throw ProtocolErrorType.PARAMS for custom functions and importers --- bin/dart_sass_embedded.dart | 9 ++---- lib/src/importer/base.dart | 14 ++------- lib/src/importer/file.dart | 4 +-- test/file_importer_test.dart | 28 +++++++++++------ test/function_test.dart | 60 +++++++++--------------------------- test/importer_test.dart | 20 +++++------- 6 files changed, 45 insertions(+), 90 deletions(-) diff --git a/bin/dart_sass_embedded.dart b/bin/dart_sass_embedded.dart index 0f762c1..90b5524 100644 --- a/bin/dart_sass_embedded.dart +++ b/bin/dart_sass_embedded.dart @@ -55,13 +55,8 @@ void main(List 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()) { diff --git a/lib/src/importer/base.dart b/lib/src/importer/base.dart index b1a6ae6..9b751f8 100644 --- a/lib/src/importer/base.dart +++ b/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. @@ -28,18 +26,10 @@ abstract class ImporterBase extends sass.Importer { try { parsedUrl = Uri.parse(url); } on FormatException catch (error) { - sendAndThrow(paramsError("$field is invalid: $error")); + throw "$field is invalid: $error"; } 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 '$field must be absolute, was "$parsedUrl"'; } } diff --git a/lib/src/importer/file.dart b/lib/src/importer/file.dart index caa4290..e00be9e 100644 --- a/lib/src/importer/file.dart +++ b/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 @@ -47,8 +46,7 @@ class FileImporter extends ImporterBase { var url = parseAbsoluteUrl("FileImportResponse.file_url", response.fileUrl); if (url.scheme != 'file') { - sendAndThrow(paramsError( - 'FileImportResponse.file_url must be a file: URL, was "$url"')); + throw 'FileImportResponse.file_url must be a file: URL, was "$url"'; } return _filesystemImporter.canonicalize(url); diff --git a/test/file_importer_test.dart b/test/file_importer_test.dart index fcd0d40..f1114b8 100644 --- a/test/file_importer_test.dart +++ b/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,7 +74,7 @@ void main() { ..id = request.id ..fileUrl = "")); - await _expectImportParamsError( + await _expectImportError( process, 'FileImportResponse.file_url must be absolute, was ""'); await process.kill(); }); @@ -73,7 +85,7 @@ void main() { ..id = request.id ..fileUrl = "foo")); - await _expectImportParamsError( + await _expectImportError( process, 'FileImportResponse.file_url must be absolute, was "foo"'); await process.kill(); }); @@ -84,7 +96,7 @@ void main() { ..id = request.id ..fileUrl = "other:foo")); - await _expectImportParamsError(process, + await _expectImportError(process, 'FileImportResponse.file_url must be 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 _expectImportParamsError( - EmbeddedProcess process, Object message) async { - await expectLater(process.outbound, - emits(isProtocolError(errorId, ProtocolErrorType.PARAMS, message))); - +Future _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'")); } diff --git a/test/function_test.dart b/test/function_test.dart index d240366..427f042 100644 --- a/test/function_test.dart +++ b/test/function_test.dart @@ -21,74 +21,34 @@ 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, '"" is missing "("'); 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, '"foo" is missing "("'); 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, '"foo(\$bar" doesn\'t end with ")"'); 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, '"foo() " doesn\'t end with ")"'); 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, 'Expected identifier.'); await _process.kill(); }); }); @@ -1989,3 +1949,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 _expectFunctionError( + EmbeddedProcess process, Object message) async { + var failure = getCompileFailure(await process.outbound.next); + expect(failure.message, equals(message)); +} diff --git a/test/importer_test.dart b/test/importer_test.dart index 75d4173..2a9be74 100644 --- a/test/importer_test.dart +++ b/test/importer_test.dart @@ -64,7 +64,7 @@ void main() { }); group("canonicalization", () { - group("emits a protocol error", () { + group("emits a compile failure", () { test("for a canonicalize response with an empty URL", () async { process.inbound.add(compileString("@import 'other'", importers: [ InboundMessage_CompileRequest_Importer()..importerId = 1 @@ -76,7 +76,7 @@ void main() { ..id = request.id ..url = "")); - await _expectImportParamsError( + await _expectImportError( process, 'CanonicalizeResponse.url must be absolute, was ""'); await process.kill(); }); @@ -92,7 +92,7 @@ void main() { ..id = request.id ..url = "relative")); - await _expectImportParamsError(process, + await _expectImportError(process, 'CanonicalizeResponse.url must be absolute, was "relative"'); await process.kill(); }); @@ -216,7 +216,7 @@ void main() { }); group("importing", () { - group("emits a protocol error", () { + group("emits a compile failure", () { test("for an import result with a relative sourceMapUrl", () async { process.inbound.add(compileString("@import 'other'", importers: [ InboundMessage_CompileRequest_Importer()..importerId = 1 @@ -230,7 +230,7 @@ void main() { ..success = (InboundMessage_ImportResponse_ImportSuccess() ..sourceMapUrl = "relative"))); - await _expectImportParamsError( + await _expectImportError( process, 'ImportResponse.success.source_map_url must be absolute, was ' '"relative"'); @@ -510,14 +510,10 @@ Future _canonicalize(EmbeddedProcess process) async { ..url = "custom:other")); } -/// 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 _expectImportParamsError( - EmbeddedProcess process, Object message) async { - await expectLater(process.outbound, - emits(isProtocolError(errorId, ProtocolErrorType.PARAMS, message))); - +Future _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'")); } From dcf88c120c6bb7cd5ce8f458e6e58ba35ff37929 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=AA=E3=81=A4=E3=81=8D?= Date: Thu, 17 Nov 2022 07:01:37 +0000 Subject: [PATCH 2/6] Update function parsing error message --- lib/src/host_callable.dart | 21 ++++----------------- test/function_test.dart | 30 +++++++++++++++++++----------- 2 files changed, 23 insertions(+), 28 deletions(-) diff --git a/lib/src/host_callable.dart b/lib/src/host_callable.dart index 203ca75..3e1367e 100644 --- a/lib/src/host_callable.dart +++ b/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) { + sass.Callable? callable; + callable = sass.Callable.host(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; } diff --git a/test/function_test.dart b/test/function_test.dart index 427f042..26e5e80 100644 --- a/test/function_test.dart +++ b/test/function_test.dart @@ -24,31 +24,36 @@ void main() { 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 _expectFunctionError(_process, '"" is missing "("'); + 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 _expectFunctionError(_process, '"foo" is missing "("'); + 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 _expectFunctionError(_process, '"foo(\$bar" doesn\'t end with ")"'); + 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 _expectFunctionError(_process, '"foo() " doesn\'t end with ")"'); + 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 _expectFunctionError(_process, 'Expected identifier.'); + await _expectFunctionError( + _process, r'Invalid signature "foo($)": Expected identifier.'); await _process.kill(); }); }); @@ -1808,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.'); }); }); }); From 5f9df21137817bfb88a11c20a079d8365e43eb75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=AA=E3=81=A4=E3=81=8D?= Date: Thu, 17 Nov 2022 09:14:15 +0000 Subject: [PATCH 3/6] Update importer error message --- lib/src/importer/base.dart | 8 ++++---- lib/src/importer/file.dart | 5 ++--- lib/src/importer/host.dart | 6 +++--- test/file_importer_test.dart | 8 ++++---- test/importer_test.dart | 10 ++++------ 5 files changed, 17 insertions(+), 20 deletions(-) diff --git a/lib/src/importer/base.dart b/lib/src/importer/base.dart index 9b751f8..2e08a99 100644 --- a/lib/src/importer/base.dart +++ b/lib/src/importer/base.dart @@ -19,17 +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) { - throw "$field is invalid: $error"; + throw '$source must return a URL, was "$url"'; } if (parsedUrl.scheme.isNotEmpty) return parsedUrl; - throw '$field must be absolute, was "$parsedUrl"'; + throw '$source must return an absolute URL, was "$parsedUrl"'; } } diff --git a/lib/src/importer/file.dart b/lib/src/importer/file.dart index e00be9e..22b1b98 100644 --- a/lib/src/importer/file.dart +++ b/lib/src/importer/file.dart @@ -43,10 +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') { - throw '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); diff --git a/lib/src/importer/host.dart b/lib/src/importer/host.dart index 33aaeff..925a77a 100644 --- a/lib/src/importer/host.dart +++ b/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: diff --git a/test/file_importer_test.dart b/test/file_importer_test.dart index f1114b8..194b63f 100644 --- a/test/file_importer_test.dart +++ b/test/file_importer_test.dart @@ -75,7 +75,7 @@ void main() { ..fileUrl = "")); await _expectImportError( - process, 'FileImportResponse.file_url must be absolute, was ""'); + process, 'The file importer must return an absolute URL, was ""'); await process.kill(); }); @@ -85,8 +85,8 @@ void main() { ..id = request.id ..fileUrl = "foo")); - await _expectImportError( - 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(); }); @@ -97,7 +97,7 @@ void main() { ..fileUrl = "other:foo")); await _expectImportError(process, - 'FileImportResponse.file_url must be a file: URL, was "other:foo"'); + 'The file importer must return a file: URL, was "other:foo"'); await process.kill(); }); }); diff --git a/test/importer_test.dart b/test/importer_test.dart index 2a9be74..384ceee 100644 --- a/test/importer_test.dart +++ b/test/importer_test.dart @@ -77,7 +77,7 @@ void main() { ..url = "")); await _expectImportError( - process, 'CanonicalizeResponse.url must be absolute, was ""'); + process, 'The importer must return an absolute URL, was ""'); await process.kill(); }); @@ -93,7 +93,7 @@ void main() { ..url = "relative")); await _expectImportError(process, - 'CanonicalizeResponse.url must be absolute, was "relative"'); + 'The importer must return an absolute URL, was "relative"'); await process.kill(); }); }); @@ -230,10 +230,8 @@ void main() { ..success = (InboundMessage_ImportResponse_ImportSuccess() ..sourceMapUrl = "relative"))); - await _expectImportError( - process, - 'ImportResponse.success.source_map_url must be absolute, was ' - '"relative"'); + await _expectImportError(process, + 'The importer must return an absolute URL, was "relative"'); await process.kill(); }); }); From cd9b80cb92a42e93c7a7a40ecdaddbbfa2327f13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=AA=E3=81=A4=E3=81=8D?= Date: Fri, 18 Nov 2022 21:01:05 +0000 Subject: [PATCH 4/6] Trigger CI From 809330369dc40cf2cad5579d3887ab916c273fc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=AA=E3=81=A4=E3=81=8D?= Date: Thu, 1 Dec 2022 03:36:14 +0000 Subject: [PATCH 5/6] Update to match dart-sass PR --- lib/src/host_callable.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/src/host_callable.dart b/lib/src/host_callable.dart index 3e1367e..7f274ef 100644 --- a/lib/src/host_callable.dart +++ b/lib/src/host_callable.dart @@ -25,8 +25,8 @@ import 'utils.dart'; sass.Callable hostCallable(Dispatcher dispatcher, FunctionRegistry functions, int compilationId, String signature, {int? id}) { - sass.Callable? callable; - callable = sass.Callable.host(signature, (arguments) { + late sass.Callable callable; + callable = sass.Callable.fromSignature(signature, (arguments) { var protofier = Protofier(dispatcher, functions, compilationId); var request = OutboundMessage_FunctionCallRequest() ..compilationId = compilationId @@ -36,7 +36,7 @@ sass.Callable hostCallable(Dispatcher dispatcher, FunctionRegistry functions, if (id != null) { request.functionId = id; } else { - request.name = callable!.name; + request.name = callable.name; } // ignore: deprecated_member_use From fd0a7bb5982f502b12f1935ea9c3f3ea1598d6a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=AA=E3=81=A4=E3=81=8D?= Date: Thu, 8 Dec 2022 11:08:05 -0800 Subject: [PATCH 6/6] Trigger CI