From 13cc7d2da48476c2a09a540062bfce853f782db4 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Thu, 16 Feb 2023 13:34:57 -0800 Subject: [PATCH] Improve the suggested replacements for unary minus in /-as-division (#1888) Closes #1887 --- CHANGELOG.md | 5 + lib/src/ast/sass/argument_invocation.dart | 22 ++- .../ast/sass/expression/binary_operation.dart | 33 +++-- lib/src/ast/sass/expression/list.dart | 18 ++- .../ast/sass/expression/unary_operation.dart | 10 ++ lib/src/visitor/async_evaluate.dart | 4 +- lib/src/visitor/evaluate.dart | 6 +- lib/src/visitor/expression_to_calc.dart | 53 +++++++ lib/src/visitor/replace_expression.dart | 136 ++++++++++++++++++ pkg/sass_api/CHANGELOG.md | 7 + pkg/sass_api/lib/sass_api.dart | 1 + pkg/sass_api/pubspec.yaml | 4 +- pubspec.yaml | 2 +- 13 files changed, 280 insertions(+), 21 deletions(-) create mode 100644 lib/src/visitor/expression_to_calc.dart create mode 100644 lib/src/visitor/replace_expression.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d6fa2cb..15a8098e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## 1.58.2 + +* Print better `calc()`-based suggestions for `/`-as-division expression that + contain calculation-incompatible constructs like unary minus. + ## 1.58.1 * Emit a unitless hue when serializing `hsl()` colors. The `deg` unit is diff --git a/lib/src/ast/sass/argument_invocation.dart b/lib/src/ast/sass/argument_invocation.dart index 25af571c..0514ad8a 100644 --- a/lib/src/ast/sass/argument_invocation.dart +++ b/lib/src/ast/sass/argument_invocation.dart @@ -5,7 +5,9 @@ import 'package:meta/meta.dart'; import 'package:source_span/source_span.dart'; +import '../../value/list.dart'; import 'expression.dart'; +import 'expression/list.dart'; import 'node.dart'; /// A set of arguments passed in to a function or mixin. @@ -46,12 +48,24 @@ class ArgumentInvocation implements SassNode { keywordRest = null; String toString() { + var rest = this.rest; + var keywordRest = this.keywordRest; var components = [ - ...positional, - for (var name in named.keys) "\$$name: ${named[name]}", - if (rest != null) "$rest...", - if (keywordRest != null) "$keywordRest..." + for (var argument in positional) _parenthesizeArgument(argument), + for (var entry in named.entries) + "\$${entry.key}: ${_parenthesizeArgument(entry.value)}", + if (rest != null) "${_parenthesizeArgument(rest)}...", + if (keywordRest != null) "${_parenthesizeArgument(keywordRest)}..." ]; return "(${components.join(', ')})"; } + + /// Wraps [argument] in parentheses if necessary. + String _parenthesizeArgument(Expression argument) => + argument is ListExpression && + argument.separator == ListSeparator.comma && + !argument.hasBrackets && + argument.contents.length > 1 + ? "($argument)" + : argument.toString(); } diff --git a/lib/src/ast/sass/expression/binary_operation.dart b/lib/src/ast/sass/expression/binary_operation.dart index d3b45d92..b93a8e02 100644 --- a/lib/src/ast/sass/expression/binary_operation.dart +++ b/lib/src/ast/sass/expression/binary_operation.dart @@ -8,6 +8,7 @@ import 'package:source_span/source_span.dart'; import '../../../visitor/interface/expression.dart'; import '../expression.dart'; +import 'list.dart'; /// A binary operator, as in `1 + 2` or `$this and $other`. /// @@ -64,8 +65,11 @@ class BinaryOperationExpression implements Expression { var buffer = StringBuffer(); var left = this.left; // Hack to make analysis work. - var leftNeedsParens = left is BinaryOperationExpression && - left.operator.precedence < operator.precedence; + var leftNeedsParens = (left is BinaryOperationExpression && + left.operator.precedence < operator.precedence) || + (left is ListExpression && + !left.hasBrackets && + left.contents.length > 1); if (leftNeedsParens) buffer.writeCharCode($lparen); buffer.write(left); if (leftNeedsParens) buffer.writeCharCode($rparen); @@ -75,8 +79,12 @@ class BinaryOperationExpression implements Expression { buffer.writeCharCode($space); var right = this.right; // Hack to make analysis work. - var rightNeedsParens = right is BinaryOperationExpression && - right.operator.precedence <= operator.precedence; + var rightNeedsParens = (right is BinaryOperationExpression && + right.operator.precedence <= operator.precedence && + !(right.operator == operator && operator.isAssociative)) || + (right is ListExpression && + !right.hasBrackets && + right.contents.length > 1); if (rightNeedsParens) buffer.writeCharCode($lparen); buffer.write(right); if (rightNeedsParens) buffer.writeCharCode($rparen); @@ -93,10 +101,10 @@ enum BinaryOperator { singleEquals('single equals', '=', 0), /// The disjunction operator, `or`. - or('or', 'or', 1), + or('or', 'or', 1, associative: true), /// The conjunction operator, `and`. - and('and', 'and', 2), + and('and', 'and', 2, associative: true), /// The equality operator, `==`. equals('equals', '==', 3), @@ -117,13 +125,13 @@ enum BinaryOperator { lessThanOrEquals('less than or equals', '<=', 4), /// The addition operator, `+`. - plus('plus', '+', 5), + plus('plus', '+', 5, associative: true), /// The subtraction operator, `-`. minus('minus', '-', 5), /// The multiplication operator, `*`. - times('times', '*', 6), + times('times', '*', 6, associative: true), /// The division operator, `/`. dividedBy('divided by', '/', 6), @@ -142,7 +150,14 @@ enum BinaryOperator { /// An operator with higher precedence binds tighter. final int precedence; - const BinaryOperator(this.name, this.operator, this.precedence); + /// Whether this operation has the [associative property]. + /// + /// [associative property]: https://en.wikipedia.org/wiki/Associative_property + final bool isAssociative; + + const BinaryOperator(this.name, this.operator, this.precedence, + {bool associative = false}) + : isAssociative = associative; String toString() => name; } diff --git a/lib/src/ast/sass/expression/list.dart b/lib/src/ast/sass/expression/list.dart index 53a10f3f..5361f68d 100644 --- a/lib/src/ast/sass/expression/list.dart +++ b/lib/src/ast/sass/expression/list.dart @@ -37,12 +37,26 @@ class ListExpression implements Expression { String toString() { var buffer = StringBuffer(); - if (hasBrackets) buffer.writeCharCode($lbracket); + if (hasBrackets) { + buffer.writeCharCode($lbracket); + } else if (contents.isEmpty || + (contents.length == 1 && separator == ListSeparator.comma)) { + buffer.writeCharCode($lparen); + } + buffer.write(contents .map((element) => _elementNeedsParens(element) ? "($element)" : element.toString()) .join(separator == ListSeparator.comma ? ", " : " ")); - if (hasBrackets) buffer.writeCharCode($rbracket); + + if (hasBrackets) { + buffer.writeCharCode($rbracket); + } else if (contents.isEmpty) { + buffer.writeCharCode($rparen); + } else if (contents.length == 1 && separator == ListSeparator.comma) { + buffer.write(",)"); + } + return buffer.toString(); } diff --git a/lib/src/ast/sass/expression/unary_operation.dart b/lib/src/ast/sass/expression/unary_operation.dart index 201e1a82..3a3df897 100644 --- a/lib/src/ast/sass/expression/unary_operation.dart +++ b/lib/src/ast/sass/expression/unary_operation.dart @@ -8,6 +8,8 @@ import 'package:source_span/source_span.dart'; import '../../../visitor/interface/expression.dart'; import '../expression.dart'; +import 'binary_operation.dart'; +import 'list.dart'; /// A unary operator, as in `+$var` or `not fn()`. /// @@ -30,7 +32,15 @@ class UnaryOperationExpression implements Expression { String toString() { var buffer = StringBuffer(operator.operator); if (operator == UnaryOperator.not) buffer.writeCharCode($space); + var operand = this.operand; + var needsParens = operand is BinaryOperationExpression || + operand is UnaryOperationExpression || + (operand is ListExpression && + !operand.hasBrackets && + operand.contents.length > 1); + if (needsParens) buffer.write($lparen); buffer.write(operand); + if (needsParens) buffer.write($rparen); return buffer.toString(); } } diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index 2e4377a7..7eb5f4f7 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -41,6 +41,7 @@ import '../utils.dart'; import '../util/multi_span.dart'; import '../util/nullable.dart'; import '../value.dart'; +import 'expression_to_calc.dart'; import 'interface/css.dart'; import 'interface/expression.dart'; import 'interface/modifiable_css.dart'; @@ -2229,7 +2230,8 @@ class _EvaluateVisitor "Using / for division outside of calc() is deprecated " "and will be removed in Dart Sass 2.0.0.\n" "\n" - "Recommendation: ${recommendation(node)} or calc($node)\n" + "Recommendation: ${recommendation(node)} or " + "${expressionToCalc(node)}\n" "\n" "More info and automated migrator: " "https://sass-lang.com/d/slash-div", diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index acae7482..45d5986a 100644 --- a/lib/src/visitor/evaluate.dart +++ b/lib/src/visitor/evaluate.dart @@ -5,7 +5,7 @@ // DO NOT EDIT. This file was generated from async_evaluate.dart. // See tool/grind/synchronize.dart for details. // -// Checksum: d5cb0fe933051782cbfb79ee3d65bc4353471f11 +// Checksum: d84fe267879d0fb034853a0a8a5105b2919916ec // // ignore_for_file: unused_import @@ -50,6 +50,7 @@ import '../utils.dart'; import '../util/multi_span.dart'; import '../util/nullable.dart'; import '../value.dart'; +import 'expression_to_calc.dart'; import 'interface/css.dart'; import 'interface/expression.dart'; import 'interface/modifiable_css.dart'; @@ -2219,7 +2220,8 @@ class _EvaluateVisitor "Using / for division outside of calc() is deprecated " "and will be removed in Dart Sass 2.0.0.\n" "\n" - "Recommendation: ${recommendation(node)} or calc($node)\n" + "Recommendation: ${recommendation(node)} or " + "${expressionToCalc(node)}\n" "\n" "More info and automated migrator: " "https://sass-lang.com/d/slash-div", diff --git a/lib/src/visitor/expression_to_calc.dart b/lib/src/visitor/expression_to_calc.dart new file mode 100644 index 00000000..c434046f --- /dev/null +++ b/lib/src/visitor/expression_to_calc.dart @@ -0,0 +1,53 @@ +// Copyright 2023 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 '../ast/sass.dart'; +import 'replace_expression.dart'; + +/// Converts [expression] to an equivalent `calc()`. +/// +/// This assumes that [expression] already returns a number. It's intended for +/// use in end-user messaging, and may not produce directly evaluable +/// expressions. +CalculationExpression expressionToCalc(Expression expression) => + CalculationExpression.calc( + expression.accept(const _MakeExpressionCalculationSafe()), + expression.span); + +/// A visitor that replaces constructs that can't be used in a calculation with +/// those that can. +class _MakeExpressionCalculationSafe with ReplaceExpressionVisitor { + const _MakeExpressionCalculationSafe(); + + Expression visitCalculationExpression(CalculationExpression node) => node; + + Expression visitBinaryOperationExpression(BinaryOperationExpression node) => node + .operator == + BinaryOperator.modulo + // `calc()` doesn't support `%` for modulo but Sass doesn't yet support the + // `mod()` calculation function because there's no browser support, so we have + // to work around it by wrapping the call in a Sass function. + ? FunctionExpression( + 'max', ArgumentInvocation([node], const {}, node.span), node.span, + namespace: 'math') + : super.visitBinaryOperationExpression(node); + + Expression visitInterpolatedFunctionExpression( + InterpolatedFunctionExpression node) => + node; + + Expression visitUnaryOperationExpression(UnaryOperationExpression node) { + // `calc()` doesn't support unary operations. + if (node.operator == UnaryOperator.plus) { + return node.operand; + } else if (node.operator == UnaryOperator.minus) { + return BinaryOperationExpression( + BinaryOperator.times, NumberExpression(-1, node.span), node.operand); + } else { + // Other unary operations don't produce numbers, so keep them as-is to + // give the user a more useful syntax error after serialization. + return super.visitUnaryOperationExpression(node); + } + } +} diff --git a/lib/src/visitor/replace_expression.dart b/lib/src/visitor/replace_expression.dart new file mode 100644 index 00000000..d34775c2 --- /dev/null +++ b/lib/src/visitor/replace_expression.dart @@ -0,0 +1,136 @@ +// Copyright 2023 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 'package:tuple/tuple.dart'; + +import '../ast/sass.dart'; +import '../exception.dart'; +import 'interface/expression.dart'; + +/// A visitor that recursively traverses each expression in a SassScript AST and +/// replaces its contents with the values returned by nested recursion. +/// +/// In addition to the methods from [ExpressionVisitor], this has more general +/// protected methods that can be overridden to add behavior for a wide variety +/// of AST nodes: +/// +/// * [visitArgumentInvocation] +/// * [visitSupportsCondition] +/// * [visitInterpolation] +/// +/// {@category Visitor} +mixin ReplaceExpressionVisitor implements ExpressionVisitor { + Expression visitCalculationExpression(CalculationExpression node) => + CalculationExpression(node.name, + node.arguments.map((argument) => argument.accept(this)), node.span); + + Expression visitBinaryOperationExpression(BinaryOperationExpression node) => + BinaryOperationExpression( + node.operator, node.left.accept(this), node.right.accept(this)); + + Expression visitBooleanExpression(BooleanExpression node) => node; + + Expression visitColorExpression(ColorExpression node) => node; + + Expression visitFunctionExpression( + FunctionExpression node) => + FunctionExpression( + node.originalName, visitArgumentInvocation(node.arguments), node.span, + namespace: node.namespace); + + Expression visitInterpolatedFunctionExpression( + InterpolatedFunctionExpression node) => + InterpolatedFunctionExpression(visitInterpolation(node.name), + visitArgumentInvocation(node.arguments), node.span); + + Expression visitIfExpression(IfExpression node) => + IfExpression(visitArgumentInvocation(node.arguments), node.span); + + Expression visitListExpression(ListExpression node) => ListExpression( + node.contents.map((item) => item.accept(this)), node.separator, node.span, + brackets: node.hasBrackets); + + Expression visitMapExpression(MapExpression node) => MapExpression( + node.pairs.map( + (pair) => Tuple2(pair.item1.accept(this), pair.item2.accept(this))), + node.span); + + Expression visitNullExpression(NullExpression node) => node; + + Expression visitNumberExpression(NumberExpression node) => node; + + Expression visitParenthesizedExpression(ParenthesizedExpression node) => + ParenthesizedExpression(node.expression.accept(this), node.span); + + Expression visitSelectorExpression(SelectorExpression node) => node; + + Expression visitStringExpression(StringExpression node) => + StringExpression(visitInterpolation(node.text), quotes: node.hasQuotes); + + Expression visitSupportsExpression(SupportsExpression node) => + SupportsExpression(visitSupportsCondition(node.condition)); + + Expression visitUnaryOperationExpression(UnaryOperationExpression node) => + UnaryOperationExpression( + node.operator, node.operand.accept(this), node.span); + + Expression visitValueExpression(ValueExpression node) => node; + + Expression visitVariableExpression(VariableExpression node) => node; + + /// Replaces each expression in an [invocation]. + /// + /// The default implementation of the visit methods calls this to replace any + /// argument invocation in an expression. + @protected + ArgumentInvocation visitArgumentInvocation(ArgumentInvocation invocation) => + ArgumentInvocation( + invocation.positional.map((expression) => expression.accept(this)), + { + for (var entry in invocation.named.entries) + entry.key: entry.value.accept(this) + }, + invocation.span, + rest: invocation.rest?.accept(this), + keywordRest: invocation.keywordRest?.accept(this)); + + /// Replaces each expression in [condition]. + /// + /// The default implementation of the visit methods call this to visit any + /// [SupportsCondition] they encounter. + @protected + SupportsCondition visitSupportsCondition(SupportsCondition condition) { + if (condition is SupportsOperation) { + return SupportsOperation( + visitSupportsCondition(condition.left), + visitSupportsCondition(condition.right), + condition.operator, + condition.span); + } else if (condition is SupportsNegation) { + return SupportsNegation( + visitSupportsCondition(condition.condition), condition.span); + } else if (condition is SupportsInterpolation) { + return SupportsInterpolation( + condition.expression.accept(this), condition.span); + } else if (condition is SupportsDeclaration) { + return SupportsDeclaration(condition.name.accept(this), + condition.value.accept(this), condition.span); + } else { + throw SassException( + "BUG: Unknown SupportsCondition $condition.", condition.span); + } + } + + /// Replaces each expression in an [interpolation]. + /// + /// The default implementation of the visit methods call this to visit any + /// interpolation in an expression. + @protected + Interpolation visitInterpolation(Interpolation interpolation) => + Interpolation( + interpolation.contents + .map((node) => node is Expression ? node.accept(this) : node), + interpolation.span); +} diff --git a/pkg/sass_api/CHANGELOG.md b/pkg/sass_api/CHANGELOG.md index 06c7fc29..e4f02e68 100644 --- a/pkg/sass_api/CHANGELOG.md +++ b/pkg/sass_api/CHANGELOG.md @@ -1,3 +1,10 @@ +## 5.1.0 + +* Add `BinaryOperation.isAssociative`. + +* Add a `ReplaceExpressionVisitor`, which recursively visits all expressions in + an AST and rebuilds them with replacement components. + ## 5.0.1 * No user-visible changes. diff --git a/pkg/sass_api/lib/sass_api.dart b/pkg/sass_api/lib/sass_api.dart index c839afe1..21c82fda 100644 --- a/pkg/sass_api/lib/sass_api.dart +++ b/pkg/sass_api/lib/sass_api.dart @@ -25,6 +25,7 @@ export 'package:sass/src/visitor/interface/statement.dart'; export 'package:sass/src/visitor/recursive_ast.dart'; export 'package:sass/src/visitor/recursive_selector.dart'; export 'package:sass/src/visitor/recursive_statement.dart'; +export 'package:sass/src/visitor/replace_expression.dart'; export 'package:sass/src/visitor/statement_search.dart'; /// Parses [text] as a CSS identifier and returns the result. diff --git a/pkg/sass_api/pubspec.yaml b/pkg/sass_api/pubspec.yaml index cd8fb44a..5e607eac 100644 --- a/pkg/sass_api/pubspec.yaml +++ b/pkg/sass_api/pubspec.yaml @@ -2,7 +2,7 @@ name: sass_api # Note: Every time we add a new Sass AST node, we need to bump the *major* # version because it's a breaking change for anyone who's implementing the # visitor interface(s). -version: 5.0.1 +version: 5.1.0 description: Additional APIs for Dart Sass. homepage: https://github.com/sass/dart-sass @@ -10,7 +10,7 @@ environment: sdk: ">=2.17.0 <3.0.0" dependencies: - sass: 1.58.1 + sass: 1.58.2 dev_dependencies: dartdoc: ^5.0.0 diff --git a/pubspec.yaml b/pubspec.yaml index c7c9b78c..546b7c02 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,5 +1,5 @@ name: sass -version: 1.58.1 +version: 1.58.2-dev description: A Sass implementation in Dart. homepage: https://github.com/sass/dart-sass