Improvements to the division migrator (#41)

* Improvements to the division migrator

Resolves #32 and resolves #33.

* Refactor special color function handle into fn

* Fix remaining comments

* Add comment and changelog entry

Author: jathak

CHANGELOG.md

@@ -1,3 +1,10 @@
+## 1.0.0-alpha.3
+
+* Division migrator: Treat most slash operations within argument lists as
+  division.
+* Division migrator: Only migrate slash operations to slash-list calls in a
+  non-plain-CSS context.
+
 ## 1.0.0-alpha.2
 
 * Internal changes only.

lib/src/migrators/division.dart

@@ -5,6 +5,7 @@
 // https://opensource.org/licenses/MIT.
 
 import 'package:args/args.dart';
+import 'package:sass/sass.dart';
 
 // The sass package's API is not necessarily stable. It is being imported with
 // the Sass team's explicit knowledge and approval. See
@@ -49,6 +50,13 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
   /// True when the current node is expected to evaluate to a number.
   var _expectsNumericResult = false;
 
+  /// Allows division within this argument invocation.
+  @override
+  void visitArgumentInvocation(ArgumentInvocation invocation) {
+    _withContext(() => super.visitArgumentInvocation(invocation),
+        isDivisionAllowed: true);
+  }
+
   /// If this is a division operation, migrates it.
   ///
   /// If this is any other operator, allows division within its left and right
@@ -65,6 +73,15 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
     }
   }
 
+  /// Allows division within a function call's arguments, with special handling
+  /// for new-syntax color functions.
+  @override
+  void visitFunctionExpression(FunctionExpression node) {
+    visitInterpolation(node.name);
+    if (_tryColorFunction(node)) return;
+    visitArgumentInvocation(node.arguments);
+  }
+
   /// Disallows division within this list.
   @override
   void visitListExpression(ListExpression node) {
@@ -105,6 +122,48 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
         isDivisionAllowed: true);
   }
 
+  /// Migrates [node] and returns true if it is a new-syntax color function or
+  /// returns false if it is any other function.
+  bool _tryColorFunction(FunctionExpression node) {
+    if (!["rgb", "rgba", "hsl", "hsla"].contains(node.name.asPlain)) {
+      return false;
+    }
+
+    ListExpression channels;
+    if (node.arguments.positional.length == 1 &&
+        node.arguments.named.isEmpty &&
+        node.arguments.positional.first is ListExpression) {
+      channels = node.arguments.positional.first;
+    } else if (node.arguments.positional.isEmpty &&
+        node.arguments.named.containsKey(r'$channels') &&
+        node.arguments.named.length == 1 &&
+        node.arguments.named[r'$channels'] is ListExpression) {
+      channels = node.arguments.named[r'$channels'];
+    }
+    if (channels == null ||
+        channels.hasBrackets ||
+        channels.separator != ListSeparator.space ||
+        channels.contents.length != 3 ||
+        channels.contents.last is! BinaryOperationExpression) {
+      return false;
+    }
+
+    var last = channels.contents.last as BinaryOperationExpression;
+    if (last.left is! NumberExpression || last.right is! NumberExpression) {
+      // Handles cases like `rgb(10 20 30/2 / 0.5)`, since converting `30/2` to
+      // `divide(30, 20)` would cause `/ 0.5` to be interpreted as division.
+      _patchSpacesToCommas(channels);
+      _patchOperatorToComma(last);
+    }
+    _withContext(() {
+      channels.contents[0].accept(this);
+      channels.contents[1].accept(this);
+      last.left.accept(this);
+    }, isDivisionAllowed: true);
+    last.right.accept(this);
+    return true;
+  }
+
   /// Visits a `/` operation [node] and migrates it to either the `division`
   /// function or the `slash-list` function.
   ///
@@ -115,9 +174,14 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
     if ((!_isDivisionAllowed && _onlySlash(node)) ||
         _isDefinitelyNotNumber(node)) {
       // Definitely not division
-      addPatch(patchBefore(node, "slash-list("));
-      addPatch(patchAfter(node, ")"));
-      _visitSlashListArguments(node);
+      if (_isDivisionAllowed || _containsInterpolation(node)) {
+        // We only want to convert a non-division slash operation to a
+        // slash-list call when it's in a non-plain-CSS context to avoid
+        // unnecessary function calls within plain CSS.
+        addPatch(patchBefore(node, "slash-list("));
+        addPatch(patchAfter(node, ")"));
+        _visitSlashListArguments(node);
+      }
       return true;
     } else if (_expectsNumericResult ||
         _isDefinitelyNumber(node) ||
@@ -126,7 +190,7 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
       addPatch(patchBefore(node, "divide("));
       addPatch(patchAfter(node, ")"));
       _patchParensIfAny(node.left);
-      _patchSlashToComma(node);
+      _patchOperatorToComma(node);
       _patchParensIfAny(node.right);
       _withContext(() => super.visitBinaryOperationExpression(node),
           expectsNumericResult: true);
@@ -145,7 +209,7 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
     if (node is BinaryOperationExpression &&
         node.operator == BinaryOperator.dividedBy) {
       _visitSlashListArguments(node.left);
-      _patchSlashToComma(node);
+      _patchOperatorToComma(node);
       _visitSlashListArguments(node.right);
     } else if (node is StringExpression &&
         node.text.contents.length == 1 &&
@@ -218,8 +282,27 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
         node is StringExpression;
   }
 
+  /// Returns true if [node] contains an interpolation.
+  bool _containsInterpolation(Expression node) {
+    if (node is ParenthesizedExpression) return _containsInterpolation(node);
+    if (node is BinaryOperationExpression) {
+      return _containsInterpolation(node.left) ||
+          _containsInterpolation(node.right);
+    }
+    return node is StringExpression && node.text.asPlain == null;
+  }
+
+  /// Converts a space-separated list [node] to a comma-separated list.
+  void _patchSpacesToCommas(ListExpression node) {
+    for (var i = 0; i < node.contents.length - 1; i++) {
+      var start = node.contents[i].span.end;
+      var end = node.contents[i + 1].span.start;
+      addPatch(Patch(start.file.span(start.offset, end.offset), ", "));
+    }
+  }
+
   /// Adds a patch replacing the operator of [node] with ", ".
-  void _patchSlashToComma(BinaryOperationExpression node) {
+  void _patchOperatorToComma(BinaryOperationExpression node) {
     var start = node.left.span.end;
     var end = node.right.span.start;
     addPatch(Patch(start.file.span(start.offset, end.offset), ", "));

pubspec.yaml

@@ -1,8 +1,8 @@
 name: sass_migrator
-version: 1.0.0-alpha.2
+version: 1.0.0-alpha.3
 description: A tool for running migrations on Sass files
 author: Jennifer Thakar <[email protected]>
-homepage: https://github.com/sass/sass_migrator
+homepage: https://github.com/sass/migrator
 
 environment:
   sdk: '>=2.2.0 <3.0.0'

test/migrators/division/always_division.hrx

@@ -6,7 +6,12 @@
   @return 6px / 3px;
 }
 
+@function identity($x) {
+  @return $x;
+}
+
 a {
+  // Arithmetic on number literals
   b: (4px + 2px) / 3px;
   c: 6px/3px + 1;
   d: (6px / 3px);
@@ -24,14 +29,24 @@ a {
   l: 3 / $x > 1;
   m: 3 / $x <= 1;
   n: 3 / $x >= 1;
+
+  // Function calls
+  o: identity(6px / 3px);
+  p: rgba(10, 20, 30/2, 0.5);
+  q: rgb(10 20 30/2 / 0.5);
 }
 
 <==> output/entrypoint.scss
 @function six-divided-by-three() {
   @return divide(6px, 3px);
 }
 
+@function identity($x) {
+  @return $x;
+}
+
 a {
+  // Arithmetic on number literals
   b: divide(4px + 2px, 3px);
   c: divide(6px, 3px) + 1;
   d: divide(6px, 3px);
@@ -49,4 +64,9 @@ a {
   l: divide(3, $x) > 1;
   m: divide(3, $x) <= 1;
   n: divide(3, $x) >= 1;
+
+  // Function calls
+  o: identity(divide(6px, 3px));
+  p: rgba(10, 20, divide(30, 2), 0.5);
+  q: rgb(10, 20, divide(30, 2), 0.5);
 }

test/migrators/division/maybe_division.hrx

@@ -5,6 +5,7 @@ a {
   d: 3 / $x == 4;
   e: fn() / 3;
   f: 3 / $x;
+  g: fn(3 / $x);
 }
 
 <==> output/entrypoint.scss
@@ -14,4 +15,5 @@ a {
   d: divide(3, $x) == 4;
   e: divide(fn(), 3);
   f: divide(3, $x);
+  g: fn(divide(3, $x));
 }

test/migrators/division/never_division.hrx

@@ -2,22 +2,11 @@
 :foo(#{6/3}) {
   b: 6px / 3px;
   c: 6px / 3px / 2;
-  d: #{4px + 2px} / 3px;
-  e: bold 6px/3px sans-serif;
-  f: (bold 6px/3px sans-serif);
-  g: (six / three);
-  h: 6 / three;
-  i: #{$x} / #{6px / 3px} / #{2};
+  d: bold 6px/3px sans-serif;
+  e: (bold 6px/3px sans-serif);
+  f: 6 / three;
+  g: rgb(10 20 30 / 0.5);
 }
 
-<==> output/entrypoint.scss
-:foo(#{slash-list(6, 3)}) {
-  b: slash-list(6px, 3px);
-  c: slash-list(6px, 3px, 2);
-  d: slash-list(4px + 2px, 3px);
-  e: bold slash-list(6px, 3px) sans-serif;
-  f: (bold slash-list(6px, 3px) sans-serif);
-  g: slash-list(six, three);
-  h: slash-list(6, three);
-  i: slash-list($x, slash-list(6px, 3px), 2);
-}
+<==> log.txt
+Nothing to migrate!

test/migrators/division/slash_list.hrx

@@ -0,0 +1,17 @@
+<==> input/entrypoint.scss
+a {
+  b: #{4px + 2px} / 3px;
+  c: (six / three);
+  $d: 6 / three;
+  e: #{$x} / #{6px / 3px} / #{2};
+  $f: #{$x} / #{6px / 3px} / #{2};
+}
+
+<==> output/entrypoint.scss
+a {
+  b: slash-list(4px + 2px, 3px);
+  c: slash-list(six, three);
+  $d: slash-list(6, three);
+  e: slash-list($x, 6px / 3px, 2);
+  $f: slash-list($x, divide(6px, 3px), 2);
+}