Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement first class mixins #2073

Merged
merged 36 commits into from
Oct 5, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
ef23769
initial impl of first class mixins
connorskees Aug 25, 2023
0b9f3a8
add mixin type to js api
connorskees Aug 26, 2023
63c1f5e
add first class mixins to embedded protocol
connorskees Aug 26, 2023
f9badfa
support AsyncBuiltInCallable in accepts-content
connorskees Aug 26, 2023
010ad35
generate evaluate file
connorskees Aug 26, 2023
b908e27
bump ci
connorskees Aug 29, 2023
7d6757c
merge main
connorskees Sep 1, 2023
f715c22
revert embedded protocol changes
connorskees Sep 2, 2023
c0eced3
Style nits
nex3 Sep 6, 2023
91ac279
some pr review
connorskees Sep 7, 2023
194e171
Revert "revert embedded protocol changes"
connorskees Sep 7, 2023
154e12e
remove redundant content cast
connorskees Sep 8, 2023
ca57cb0
correctly pass content through meta.apply
connorskees Sep 8, 2023
4093914
correctly pass through acceptsContent value
connorskees Sep 8, 2023
eb33c56
pr review
connorskees Sep 14, 2023
eeb14ee
refactor embedded registry
connorskees Sep 14, 2023
1cfd14b
merge main
connorskees Sep 14, 2023
5e1774c
merge main
connorskees Sep 16, 2023
faba24a
await _applyMixin
connorskees Sep 19, 2023
52a1e6f
Merge branch 'main' of https://github.com/sass/dart-sass into feat/fi…
connorskees Sep 20, 2023
ff3d80e
Merge branch 'main' of https://github.com/sass/dart-sass into feat/fi…
connorskees Sep 21, 2023
89ef731
add doc comment to _applyMixin
connorskees Sep 21, 2023
93b812a
run synchronize
connorskees Sep 22, 2023
6e85d50
bump ci
connorskees Sep 25, 2023
2738c00
merge main
connorskees Sep 26, 2023
80fd8bb
Update pubspec and changelog
nex3 Sep 28, 2023
9485b4f
merge main
connorskees Sep 29, 2023
3144e02
nits
connorskees Sep 30, 2023
f1c1d5a
lazily evaluate file spans
connorskees Oct 3, 2023
895b2ca
Merge branch 'main' of https://github.com/sass/dart-sass into feat/fi…
connorskees Oct 3, 2023
96bbca2
bump ci
connorskees Oct 3, 2023
679e1bd
bump ci
connorskees Oct 4, 2023
65f2d2d
Merge branch 'main' of https://github.com/sass/dart-sass into feat/fi…
connorskees Oct 4, 2023
75ccdfb
take Object self in JS constructor
connorskees Oct 4, 2023
6277d5d
bump ci
connorskees Oct 4, 2023
f1b4ff3
add SassMixin to ESM export
connorskees Oct 5, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions lib/src/callable/async_built_in.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class AsyncBuiltInCallable implements AsyncCallable {
/// The callback to run when executing this callable.
final Callback _callback;

bool hasContent = false;

/// Creates a function with a single [arguments] declaration and a single
/// [callback].
///
Expand All @@ -52,7 +54,7 @@ class AsyncBuiltInCallable implements AsyncCallable {
/// defined.
AsyncBuiltInCallable.mixin(String name, String arguments,
FutureOr<void> callback(List<Value> arguments),
{Object? url})
{Object? url, bool hasContent = false})
: this.parsed(name,
ArgumentDeclaration.parse('@mixin $name($arguments) {', url: url),
(arguments) async {
Expand All @@ -62,11 +64,12 @@ class AsyncBuiltInCallable implements AsyncCallable {
// quickly so it's easier to just return Sass's `null` for mixins and
// simply ignore it at the call site.
return sassNull;
});
}, hasContent: hasContent);

/// Creates a callable with a single [arguments] declaration and a single
/// [callback].
AsyncBuiltInCallable.parsed(this.name, this._arguments, this._callback);
AsyncBuiltInCallable.parsed(this.name, this._arguments, this._callback,
{this.hasContent = false});

/// Returns the argument declaration and Dart callback for the given
/// positional and named arguments.
Expand Down
9 changes: 6 additions & 3 deletions lib/src/callable/built_in.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ final class BuiltInCallable implements Callable, AsyncBuiltInCallable {
/// The overloads declared for this callable.
final List<(ArgumentDeclaration, Callback)> _overloads;

bool hasContent = false;
connorskees marked this conversation as resolved.
Show resolved Hide resolved

/// Creates a function with a single [arguments] declaration and a single
/// [callback].
///
Expand Down Expand Up @@ -48,18 +50,19 @@ final class BuiltInCallable implements Callable, AsyncBuiltInCallable {
/// defined.
BuiltInCallable.mixin(
String name, String arguments, void callback(List<Value> arguments),
{Object? url})
{Object? url, bool hasContent = false})
: this.parsed(name,
ArgumentDeclaration.parse('@mixin $name($arguments) {', url: url),
(arguments) {
callback(arguments);
return sassNull;
});
}, hasContent: hasContent);

/// Creates a callable with a single [arguments] declaration and a single
/// [callback].
BuiltInCallable.parsed(this.name, ArgumentDeclaration arguments,
Value callback(List<Value> arguments))
Value callback(List<Value> arguments),
{this.hasContent = false})
: _overloads = [(arguments, callback)];

/// Creates a function with multiple implementations.
Expand Down
14 changes: 14 additions & 0 deletions lib/src/functions/meta.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'dart:collection';

import 'package:collection/collection.dart';

import '../ast/sass/statement/mixin_rule.dart';
import '../callable.dart';
import '../util/map.dart';
import '../value.dart';
Expand Down Expand Up @@ -45,6 +46,7 @@ final global = UnmodifiableListView([
sassNull => "null",
SassNumber() => "number",
SassFunction() => "function",
SassMixin() => "mixin",
SassCalculation() => "calculation",
SassString() => "string",
_ => throw "[BUG] Unknown value type ${arguments[0]}"
Expand Down Expand Up @@ -77,6 +79,18 @@ final local = UnmodifiableListView([
? argument
: SassString(argument.toString(), quotes: false)),
ListSeparator.comma);
}),
_function("accepts-content", r"$mixin", (arguments) {
var mixin = arguments[0].assertMixin("mixin");
bool acceptsContent = switch (mixin.callable) {
AsyncBuiltInCallable(hasContent: var hasContent) ||
BuiltInCallable(hasContent: var hasContent) ||
UserDefinedCallable(declaration: MixinRule(hasContent: var hasContent)) =>
hasContent,
AsyncCallable() =>
throw UnsupportedError("Unknown callable type $mixin."),
connorskees marked this conversation as resolved.
Show resolved Hide resolved
};
return SassBoolean(acceptsContent);
})
]);

Expand Down
1 change: 1 addition & 0 deletions lib/src/js.dart
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ void main() {
exports.CalculationInterpolation = calculationInterpolationClass;
exports.SassColor = colorClass;
exports.SassFunction = functionClass;
exports.SassMixin = mixinClass;
exports.SassList = listClass;
exports.SassMap = mapClass;
exports.SassNumber = numberClass;
Expand Down
1 change: 1 addition & 0 deletions lib/src/js/exports.dart
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class Exports {
external set SassBoolean(JSClass function);
external set SassColor(JSClass function);
external set SassFunction(JSClass function);
external set SassMixin(JSClass mixin);
external set SassList(JSClass function);
external set SassMap(JSClass function);
external set SassNumber(JSClass function);
Expand Down
2 changes: 2 additions & 0 deletions lib/src/js/value.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export 'value/color.dart';
export 'value/function.dart';
export 'value/list.dart';
export 'value/map.dart';
export 'value/mixin.dart';
export 'value/number.dart';
export 'value/string.dart';

Expand Down Expand Up @@ -42,6 +43,7 @@ final JSClass valueClass = () {
'assertColor': (Value self, [String? name]) => self.assertColor(name),
'assertFunction': (Value self, [String? name]) => self.assertFunction(name),
'assertMap': (Value self, [String? name]) => self.assertMap(name),
'assertMixin': (Value self, [String? name]) => self.assertMixin(name),
'assertNumber': (Value self, [String? name]) => self.assertNumber(name),
'assertString': (Value self, [String? name]) => self.assertString(name),
'tryMap': (Value self) => self.tryMap(),
Expand Down
22 changes: 22 additions & 0 deletions lib/src/js/value/mixin.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2021 Google Inc. Use of this source code is governed by an
// MIT-style license that can be found in the LICENSE file or at
// https://opensource.org/licenses/MIT.

import 'package:node_interop/js.dart';

import '../../callable.dart';
import '../../value.dart';
import '../reflection.dart';
import '../utils.dart';

/// The JavaScript `SassMixin` class.
final JSClass mixinClass = () {
var jsClass = createJSClass('sass.SassMixin', () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it is related to the browser failure, but I think the argument is missing Object self here when looking at all other values.

jsThrow(JsError(
'It is not possible to construct a SassMixin through the JavaScript API'));
});

getJSClass(SassMixin(Callable('f', '', (_) => sassNull)))
.injectSuperclass(jsClass);
Comment on lines +19 to +20
Copy link
Contributor

@ntkme ntkme Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this does not make much sense. If I understand it correctly the reason we have this for Sass Function type is to allow the "JS" type to inherit the prototype of dart-converted-JS type? If so here the argument passed into SassMixin (the dart type) should be some argument necessary to construct an actual Mixin object.

@nex3 I don't fully understand how the JS-interop works so please correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind. Looks like Mixin just takes AsyncCallable so this is probably fine.

return jsClass;
}();
9 changes: 9 additions & 0 deletions lib/src/value.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import 'value/color.dart';
import 'value/function.dart';
import 'value/list.dart';
import 'value/map.dart';
import 'value/mixin.dart';
import 'value/number.dart';
import 'value/string.dart';
import 'visitor/interface/value.dart';
Expand All @@ -27,6 +28,7 @@ export 'value/color.dart';
export 'value/function.dart';
export 'value/list.dart';
export 'value/map.dart';
export 'value/mixin.dart';
export 'value/null.dart';
export 'value/number.dart' hide conversionFactor;
export 'value/string.dart';
Expand Down Expand Up @@ -177,6 +179,13 @@ abstract class Value {
SassFunction assertFunction([String? name]) =>
throw SassScriptException("$this is not a function reference.", name);

/// Throws a [SassScriptException] if [this] isn't a mixin reference.
///
/// If this came from a mixin argument, [name] is the argument name
connorskees marked this conversation as resolved.
Show resolved Hide resolved
/// (without the `$`). It's used for error reporting.
SassMixin assertMixin([String? name]) =>
throw SassScriptException("$this is not a mixin reference.", name);

/// Throws a [SassScriptException] if [this] isn't a map.
///
/// If this came from a function argument, [name] is the argument name
Expand Down
38 changes: 38 additions & 0 deletions lib/src/value/mixin.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2016 Google Inc. Use of this source code is governed by an
// MIT-style license that can be found in the LICENSE file or at
// https://opensource.org/licenses/MIT.

import 'package:meta/meta.dart';

import '../callable.dart';
import '../visitor/interface/value.dart';
import '../value.dart';

/// A SassScript mixin reference.
///
/// A mixin reference captures a mixin from the local environment so that
/// it may be passed between modules.
///
/// {@category Value}
@sealed
connorskees marked this conversation as resolved.
Show resolved Hide resolved
class SassMixin extends Value {
/// The callable that this mixin invokes.
///
/// Note that this is typed as an [AsyncCallable] so that it will work with
/// both synchronous and asynchronous evaluate visitors, but in practice the
/// synchronous evaluate visitor will crash if this isn't a [Callable].
final AsyncCallable callable;
connorskees marked this conversation as resolved.
Show resolved Hide resolved

SassMixin(this.callable);

/// @nodoc
@internal
T accept<T>(ValueVisitor<T> visitor) => visitor.visitMixin(this);

SassMixin assertMixin([String? name]) => this;

bool operator ==(Object other) =>
other is SassMixin && callable == other.callable;

int get hashCode => callable.hashCode;
}
92 changes: 91 additions & 1 deletion lib/src/visitor/async_evaluate.dart
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,19 @@ final class _EvaluateVisitor
});
}, url: "sass:meta"),

BuiltInCallable.function("module-mixins", r"$module", (arguments) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 is it expected that those functions are exposed as global aliases as well and not just as module functions or is it a mistake ? The spec PR does not mention those aliases).

The metaFunctions variable defined here is not split between local and global lists (probably because when it was introduced, there were no local-only functions).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that metaMixins below is local-only (there is no global aliases for those mixins)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they aren't expected to be exposed globally.

var namespace = arguments[0].assertString("module");
var module = _environment.modules[namespace.text];
if (module == null) {
throw 'There is no module with namespace "${namespace.text}".';
}

return SassMap({
for (var (name, value) in module.mixins.pairs)
SassString(name): SassMixin(value)
});
}, url: "sass:meta"),

BuiltInCallable.function(
"get-function", r"$name, $css: false, $module: null", (arguments) {
var name = arguments[0].assertString("name");
Expand All @@ -439,6 +452,20 @@ final class _EvaluateVisitor
return SassFunction(callable);
}, url: "sass:meta"),

BuiltInCallable.function("get-mixin", r"$name, $module: null",
(arguments) {
var name = arguments[0].assertString("name");
var module = arguments[1].realNull?.assertString("module");

var callable = _addExceptionSpan(
_callableNode!,
() => _environment.getMixin(name.text.replaceAll("_", "-"),
namespace: module?.text));
if (callable == null) throw "Mixin not found: $name";

return SassMixin(callable);
}, url: "sass:meta"),

AsyncBuiltInCallable.function("call", r"$function, $args...",
(arguments) async {
var function = arguments[0];
Expand Down Expand Up @@ -512,7 +539,70 @@ final class _EvaluateVisitor
configuration: configuration,
namesInErrors: true);
_assertConfigurationIsEmpty(configuration, nameInError: true);
}, url: "sass:meta")
}, url: "sass:meta"),
BuiltInCallable.mixin("apply", r"$mixin, $args...", (arguments) async {
nex3 marked this conversation as resolved.
Show resolved Hide resolved
var mixin = arguments[0];
var args = arguments[1] as SassArgumentList;

var callableNode = _callableNode!;
var invocation = ArgumentInvocation([], {}, callableNode.span,
rest: ValueExpression(args, callableNode.span),
keywordRest: args.keywords.isEmpty
? null
: ValueExpression(
SassMap({
for (var (name, value) in args.keywords.pairs)
SassString(name, quotes: false): value
}),
callableNode.span));
connorskees marked this conversation as resolved.
Show resolved Hide resolved

var callable = mixin.assertMixin("mixin").callable;
// ignore: unnecessary_type_check
if (callable is AsyncCallable) {
connorskees marked this conversation as resolved.
Show resolved Hide resolved
switch (callable) {
case BuiltInCallable() when _environment.content != null:
throw _exception("Mixin doesn't accept a content block.");
connorskees marked this conversation as resolved.
Show resolved Hide resolved

case BuiltInCallable():
await _runBuiltInCallable(invocation, callable, callableNode);

case UserDefinedCallable<AsyncEnvironment>(
declaration: MixinRule(hasContent: false)
)
when _environment.content != null:
throw MultiSpanSassRuntimeException(
"Mixin doesn't accept a content block.",
callableNode.span,
"invocation",
{callable.declaration.arguments.spanWithName: "declaration"},
_stackTrace(callableNode.span));

case UserDefinedCallable<AsyncEnvironment>():
var contentCallable = _environment.content.andThen((content) =>
UserDefinedCallable(
content as CallableDeclaration, _environment.closure(),
inDependency: _inDependency));
connorskees marked this conversation as resolved.
Show resolved Hide resolved
_runUserDefinedCallable(invocation, callable, callableNode,
() async {
_environment.withContent(contentCallable, () async {
_environment.asMixin(() async {
for (var statement in callable.declaration.children) {
await _addErrorSpan(
callableNode, () => statement.accept(this));
}
});
});
});

case _:
throw UnsupportedError("Unknown callable type $mixin.");
}
} else {
throw SassScriptException(
"The mixin ${callable.name} is asynchronous.\n"
"This is probably caused by a bug in a Sass plugin.");
}
}, url: "sass:meta", hasContent: true),
];

var metaModule = BuiltInModule("meta",
Expand Down