Migrate to multiplication when possible (#198)

CHANGELOG.md

@@ -1,3 +1,15 @@
+## 1.5.0
+
+### Division Migrator
+
+* When migrating division where the divisor is a common constant value, the
+  migrator will now convert it to multiplication, rather than `math.div`.
+
+  For example: `$variable / 2` would be migrated to `$variable * 0.5`.
+
+  To disable this and migrate all division to `math.div`, pass
+  `--no-multiplication`.
+
 ## 1.4.5
 
 * Glob syntax will no longer be resolved if a file with that literal name

lib/src/migrators/division.dart

@@ -32,26 +32,35 @@ More info: https://sass-lang.com/d/slash-div""";
     ..addFlag('pessimistic',
         abbr: 'p',
         help: "Only migrate / expressions that are unambiguously division.",
-        negatable: false);
+        negatable: false)
+    ..addFlag('multiplication',
+        help: 'Migrate / expressions with certain constant divisors to use '
+            'multiplication instead.',
+        defaultsTo: true);
 
   bool get isPessimistic => argResults!['pessimistic'] as bool;
+  bool get useMultiplication => argResults!['multiplication'] as bool;
 
   @override
   Map<Uri, String> migrateFile(
       ImportCache importCache, Stylesheet stylesheet, Importer importer) {
     var visitor = _DivisionMigrationVisitor(
-        importCache, this.isPessimistic, migrateDependencies);
+        importCache, isPessimistic, useMultiplication, migrateDependencies);
     var result = visitor.run(stylesheet, importer);
     missingDependencies.addAll(visitor.missingDependencies);
     return result;
   }
 }
 
+/// The set of constant divisors that should be migrated to multiplication.
+const _allowedDivisors = {2, 4, 5, 8, 10, 20, 40, 50, 80, 100, 1000};
+
 class _DivisionMigrationVisitor extends MigrationVisitor {
   final bool isPessimistic;
+  final bool useMultiplication;
 
-  _DivisionMigrationVisitor(
-      ImportCache importCache, this.isPessimistic, bool migrateDependencies)
+  _DivisionMigrationVisitor(ImportCache importCache, this.isPessimistic,
+      this.useMultiplication, bool migrateDependencies)
       : super(importCache, migrateDependencies);
 
   /// True when division is allowed by the context the current node is in.
@@ -260,9 +269,8 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
   /// Visits a `/` operation [node] and migrates it to either the `division`
   /// function or the `slash-list` function.
   ///
-  /// Returns true the `/` was migrated to either function call, and false if
-  /// the `/` is ambiguous and a warning was emitted instead (pessimistic mode
-  /// only).
+  /// Returns true the `/` was migrated to either function call (indicating that
+  /// parentheses surrounding this operation should be removed).
   bool _visitSlashOperation(BinaryOperationExpression node) {
     if ((!_isDivisionAllowed && _onlySlash(node)) ||
         _isDefinitelyNotNumber(node)) {
@@ -276,17 +284,17 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
         _visitSlashListArguments(node);
       }
       return true;
-    } else if (_expectsNumericResult ||
-        _isDefinitelyNumber(node) ||
-        !isPessimistic) {
+    }
+    if (_expectsNumericResult || _isDefinitelyNumber(node) || !isPessimistic) {
       // Definitely division
+      _withContext(() => super.visitBinaryOperationExpression(node),
+          expectsNumericResult: true);
+      if (_tryMultiplication(node)) return false;
       addPatch(patchBefore(node, "${_builtInPrefix('math')}div("));
       addPatch(patchAfter(node, ")"));
       _patchParensIfAny(node.left);
       _patchOperatorToComma(node);
       _patchParensIfAny(node.right);
-      _withContext(() => super.visitBinaryOperationExpression(node),
-          expectsNumericResult: true);
       return true;
     } else {
       emitWarning("Could not determine whether this is division", node.span);
@@ -295,6 +303,27 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
     }
   }
 
+  /// Given a division operation [node], patches it to use multiplication
+  /// instead if the reciprocal of the divisor can be accurately represented as
+  /// a decimal.
+  ///
+  /// Returns true if patched and false otherwise.
+  bool _tryMultiplication(BinaryOperationExpression node) {
+    if (!useMultiplication) return false;
+    var divisor = node.right;
+    if (divisor is! NumberExpression) return false;
+    if (divisor.unit != null) return false;
+    if (!_allowedDivisors.contains(divisor.value)) return false;
+    var operatorSpan = node.left.span
+        .extendThroughWhitespace()
+        .end
+        .pointSpan()
+        .extendIfMatches('/');
+    addPatch(Patch(operatorSpan, '*'));
+    addPatch(Patch(node.right.span, '${1 / divisor.value}'));
+    return true;
+  }
+
   /// Visits the arguments of a `/` operation that is being converted into a
   /// call to `slash-list`, converting slashes to commas and removing
   /// unnecessary interpolation.

pubspec.yaml

@@ -1,5 +1,5 @@
 name: sass_migrator
-version: 1.4.5
+version: 1.5.0
 description: A tool for running migrations on Sass files
 author: Jennifer Thakar <[email protected]>
 homepage: https://github.com/sass/migrator

test/cli_dart_test.dart

@@ -37,15 +37,9 @@ void main() {
 
     await (await runMigrator(["division", "test-*.scss"])).shouldExit(0);
 
-    await d
-        .file("test-1.scss", '@use "sass:math";\n\na {b: math.div(1, 2)}')
-        .validate();
-    await d
-        .file("test-2.scss", '@use "sass:math";\n\nc {d: math.div(1, 2)}')
-        .validate();
-    await d
-        .file("test-3.scss", '@use "sass:math";\n\ne {f: math.div(1, 2)}')
-        .validate();
+    await d.file("test-1.scss", 'a {b: (1 * 0.5)}').validate();
+    await d.file("test-2.scss", 'c {d: (1 * 0.5)}').validate();
+    await d.file("test-3.scss", 'e {f: (1 * 0.5)}').validate();
   });
 
   test("allows recursive glob arguments", () async {
@@ -58,9 +52,9 @@ void main() {
     await (await runMigrator(["division", "**.scss"])).shouldExit(0);
 
     await d.dir('dir', [
-      d.file("test-1.scss", '@use "sass:math";\n\na {b: math.div(1, 2)}'),
-      d.file("test-2.scss", '@use "sass:math";\n\nc {d: math.div(1, 2)}'),
-      d.file("test-3.scss", '@use "sass:math";\n\ne {f: math.div(1, 2)}')
+      d.file("test-1.scss", 'a {b: (1 * 0.5)}'),
+      d.file("test-2.scss", 'c {d: (1 * 0.5)}'),
+      d.file("test-3.scss", 'e {f: (1 * 0.5)}')
     ]).validate();
   });
 
@@ -69,9 +63,7 @@ void main() {
 
     await (await runMigrator(["division", "[dir]/test.scss"])).shouldExit(0);
 
-    await d.dir('[dir]', [
-      d.file("test.scss", '@use "sass:math";\n\na {b: math.div(1, 2)}')
-    ]).validate();
+    await d.dir('[dir]', [d.file("test.scss", 'a {b: (1 * 0.5)}')]).validate();
   });
 
   group("with --dry-run", () {

test/migrators/division/always_division.hrx

@@ -1,5 +1,5 @@
 <==> arguments
---pessimistic
+--pessimistic --no-multiplication
 
 <==> input/entrypoint.scss
 @function six-divided-by-three() {

test/migrators/division/multiplication.hrx

@@ -0,0 +1,25 @@
+<==> input/entrypoint.scss
+$a: 2000px / 2;
+$b: $a / 4;
+$c: $a / 5;
+$d: ($a / 8);
+$e: ($a / 10);
+$f: ($a / 20);
+$g: $a/40;
+$h: ($a/50);
+$i: ($a + $b)/80;
+$j: ($a / 2)/100;
+$k: $a/2/1000;
+
+<==> output/entrypoint.scss
+$a: 2000px * 0.5;
+$b: $a * 0.25;
+$c: $a * 0.2;
+$d: ($a * 0.125);
+$e: ($a * 0.1);
+$f: ($a * 0.05);
+$g: $a*0.025;
+$h: ($a*0.02);
+$i: ($a + $b)*0.0125;
+$j: ($a * 0.5)*0.01;
+$k: $a*0.5*0.001;

test/migrators/division/not_multiplication.hrx

@@ -0,0 +1,17 @@
+<==> input/entrypoint.scss
+$a: 2000px / 3;
+$b: $a / 7;
+$c: $a / 16;
+$d: $a / 25;
+$e: $a / 10000;
+$f: $a / 2px;
+
+<==> output/entrypoint.scss
+@use "sass:math";
+
+$a: math.div(2000px, 3);
+$b: math.div($a, 7);
+$c: math.div($a, 16);
+$d: math.div($a, 25);
+$e: math.div($a, 10000);
+$f: math.div($a, 2px);