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

Closes #1887

Author: nex3

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