Improve the suggested replacements for unary minus in /-as-division (#1888)

Closes #1887

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

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();
 }

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;
 }

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();
   }
 

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();
   }
 }

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",

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",

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);
+    }
+  }
+}

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> {
+  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);
+}

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.

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.

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

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