Deprecate incorrect HSL units

CHANGELOG.md

@@ -1,3 +1,42 @@
+## 1.32.0
+
+* Deprecate passing non-`%` numbers as lightness and saturation to `hsl()`,
+  `hsla()`, `color.adjust()`, and `color.change()`. This matches the CSS
+  specification, which also requires `%` for all lightness and saturation
+  parameters. See [the Sass website][color-units] for more details.
+
+* Deprecate passing numbers with units other than `deg` as the hue to `hsl()`,
+  `hsla()`, `adjust-hue()`, `color.adjust()`, and `color.change()`. Unitless
+  numbers *are* still allowed here, since they're allowed by CSS. See [the Sass
+  website][color-units] for more details.
+
+* Improve error messages about incompatible units.
+
+* Properly mark some warnings emitted by `sass:color` functions as deprecation
+  warnings.
+
+### Dart API
+
+* Rename `SassNumber.valueInUnits()` to `SassNumber.coerceValue()`. The old name
+  remains, but is now deprecated.
+
+* Rename `SassNumber.coerceValueToUnit()`, a shorthand for
+  `SassNumber.coerceValue()` that takes a single numerator unit.
+
+* Add `SassNumber.coerceToMatch()` and `SassNumber.coerceValueToMatch()`, which
+  work like `SassNumber.coerce()` and `SassNumber.coerceValue()` but take a
+  `SassNumber` whose units should be matched rather than taking the units
+  explicitly. These generate better error messages than `SassNumber.coerce()`
+  and `SassNumber.coerceValue()`.
+
+* Add `SassNumber.convertToMatch()` and `SassNumber.convertValueToMatch()`,
+  which work like `SassNumber.coerceToMatch()` and
+  `SassNumber.coerceValueToMatch()` except they throw exceptions when converting
+  unitless values to or from units.
+
+* Add `SassNumber.compatibleWithUnit()`, which returns whether the number can be
+  coerced to a single numerator unit.
+
 ## 1.31.0
 
 * Add support for parsing `clamp()` as a special math function, the same way

lib/src/functions/color.dart

@@ -120,6 +120,7 @@ final global = UnmodifiableListView([
   _function("adjust-hue", r"$color, $degrees", (arguments) {
     var color = arguments[0].assertColor("color");
     var degrees = arguments[1].assertNumber("degrees");
+    _checkAngle(degrees);
     return color.changeHsl(hue: color.hue + degrees.value);
   }),
 
@@ -231,9 +232,11 @@ final module = BuiltInModule("color", functions: [
       }
 
       var result = _functionString("invert", arguments.take(1));
-      warn("Passing a number to color.invert() is deprecated.\n"
+      warn(
+          "Passing a number to color.invert() is deprecated.\n"
           "\n"
-          "Recommendation: $result");
+          "Recommendation: $result",
+          deprecation: true);
       return result;
     }
 
@@ -255,9 +258,11 @@ final module = BuiltInModule("color", functions: [
   _function("grayscale", r"$color", (arguments) {
     if (arguments[0] is SassNumber) {
       var result = _functionString("grayscale", arguments.take(1));
-      warn("Passing a number to color.grayscale() is deprecated.\n"
+      warn(
+          "Passing a number to color.grayscale() is deprecated.\n"
           "\n"
-          "Recommendation: $result");
+          "Recommendation: $result",
+          deprecation: true);
       return result;
     }
 
@@ -306,9 +311,11 @@ final module = BuiltInModule("color", functions: [
           !argument.hasQuotes &&
           argument.text.contains(_microsoftFilterStart)) {
         var result = _functionString("alpha", arguments);
-        warn("Using color.alpha() for a Microsoft filter is deprecated.\n"
+        warn(
+            "Using color.alpha() for a Microsoft filter is deprecated.\n"
             "\n"
-            "Recommendation: $result");
+            "Recommendation: $result",
+            deprecation: true);
         return result;
       }
 
@@ -322,9 +329,11 @@ final module = BuiltInModule("color", functions: [
           argument.text.contains(_microsoftFilterStart))) {
         // Support the proprietary Microsoft alpha() function.
         var result = _functionString("alpha", arguments);
-        warn("Using color.alpha() for a Microsoft filter is deprecated.\n"
+        warn(
+            "Using color.alpha() for a Microsoft filter is deprecated.\n"
             "\n"
-            "Recommendation: $result");
+            "Recommendation: $result",
+            deprecation: true);
         return result;
       }
 
@@ -337,9 +346,11 @@ final module = BuiltInModule("color", functions: [
   _function("opacity", r"$color", (arguments) {
     if (arguments[0] is SassNumber) {
       var result = _functionString("opacity", arguments);
-      warn("Passing a number to color.opacity() is deprecated.\n"
+      warn(
+          "Passing a number to color.opacity() is deprecated.\n"
           "\n"
-          "Recommendation: $result");
+          "Recommendation: $result",
+          deprecation: true);
       return result;
     }
 
@@ -437,9 +448,11 @@ SassColor _updateComponents(List<Value> arguments,
   ///
   /// [max] should be 255 for RGB channels, 1 for the alpha channel, and 100
   /// for saturation, lightness, whiteness, and blackness.
-  num getParam(String name, num max, {bool assertPercent = false}) {
+  num getParam(String name, num max,
+      {bool checkPercent = false, bool assertPercent = false}) {
     var number = keywords.remove(name)?.assertNumber(name);
     if (number == null) return null;
+    if (!scale && checkPercent) _checkPercent(number, name);
     if (scale || assertPercent) number.assertUnit("%", name);
     if (scale) max = 100;
     return number.valueInRange(change ? 0 : -max, max, name);
@@ -449,9 +462,13 @@ SassColor _updateComponents(List<Value> arguments,
   var red = getParam("red", 255);
   var green = getParam("green", 255);
   var blue = getParam("blue", 255);
-  var hue = scale ? null : keywords.remove("hue")?.assertNumber("hue")?.value;
-  var saturation = getParam("saturation", 100);
-  var lightness = getParam("lightness", 100);
+
+  var hueNumber = scale ? null : keywords.remove("hue")?.assertNumber("hue");
+  if (hueNumber != null) _checkAngle(hueNumber, "hue");
+  var hue = hueNumber?.value;
+
+  var saturation = getParam("saturation", 100, checkPercent: true);
+  var lightness = getParam("lightness", 100, checkPercent: true);
   var whiteness = getParam("whiteness", 100, assertPercent: true);
   var blackness = getParam("blackness", 100, assertPercent: true);
 
@@ -600,6 +617,10 @@ Value _hsl(String name, List<Value> arguments) {
   var saturation = arguments[1].assertNumber("saturation");
   var lightness = arguments[2].assertNumber("lightness");
 
+  _checkAngle(hue, "hue");
+  _checkPercent(saturation, "saturation");
+  _checkPercent(lightness, "lightness");
+
   return SassColor.hsl(
       hue.value,
       saturation.value.clamp(0, 100),
@@ -609,6 +630,56 @@ Value _hsl(String name, List<Value> arguments) {
           : _percentageOrUnitless(alpha.assertNumber("alpha"), 1, "alpha"));
 }
 
+/// Prints a deprecation warning if [hue] has a unit other than `deg`.
+void _checkAngle(SassNumber angle, [String name]) {
+  if (!angle.hasUnits || angle.hasUnit('deg')) return;
+
+  var message = StringBuffer()
+    ..writeln("\$$name: Passing a unit other than deg is deprecated.")
+    ..writeln();
+
+  if (angle.compatibleWithUnit('deg')) {
+    message
+      ..writeln(
+          "You're passing $angle, which is currently (incorrectly) converted "
+          "to ${SassNumber(angle.value, 'deg')}.")
+      ..writeln("Soon, it will instead be correctly converted to "
+          "${angle.coerce(['deg'], [])}.")
+      ..writeln();
+
+    var actualUnit = angle.numeratorUnits.first;
+    message
+      ..writeln("To preserve current behavior: \$$name * 1deg/1$actualUnit")
+      ..writeln("To migrate to new behavior: 0deg + \$$name")
+      ..writeln();
+  } else {
+    message
+      ..writeln("To preserve current behavior: \$$name${_removeUnits(angle)}")
+      ..writeln();
+  }
+
+  message.write("See https://sass-lang.com/d/color-units");
+  warn(message.toString(), deprecation: true);
+}
+
+/// Prints a deprecation warning if [number] doesn't have unit `%`.
+void _checkPercent(SassNumber number, String name) {
+  if (number.hasUnit('%')) return;
+
+  warn("\$$name: Passing a number without unit % is deprecated.\n"
+      "\n"
+      "To preserve current behavior: \$$name${_removeUnits(number)} * 1%",
+       deprecation: true);
+}
+
+/// Returns the right-hand side of an expression that would remove all units
+/// from `$number` but leaves the value the same.
+///
+/// Used for constructing deprecation messages.
+String _removeUnits(SassNumber number) =>
+    number.denominatorUnits.map((unit) => " * 1$unit").join() +
+    number.numeratorUnits.map((unit) => " / 1$unit").join();
+
 /// Create an HWB color from the given [arguments].
 Value _hwb(List<Value> arguments) {
   var alpha = arguments.length > 3 ? arguments[3] : null;

lib/src/functions/math.dart

@@ -41,20 +41,17 @@ final _clamp = _function("clamp", r"$min, $number, $max", (arguments) {
   var min = arguments[0].assertNumber("min");
   var number = arguments[1].assertNumber("number");
   var max = arguments[2].assertNumber("max");
-  if (min.hasUnits == number.hasUnits && number.hasUnits == max.hasUnits) {
-    if (min.greaterThanOrEquals(max).isTruthy) return min;
-    if (min.greaterThanOrEquals(number).isTruthy) return min;
-    if (number.greaterThanOrEquals(max).isTruthy) return max;
-    return number;
-  }
 
-  var arg2 = min.hasUnits != number.hasUnits ? number : max;
-  var arg2Name = min.hasUnits != number.hasUnits ? "\$number" : "\$max";
-  var unit1 = min.hasUnits ? "has unit ${min.unitString}" : "is unitless";
-  var unit2 = arg2.hasUnits ? "has unit ${arg2.unitString}" : "is unitless";
-  throw SassScriptException(
-      "\$min $unit1 but $arg2Name $unit2. Arguments must all have units or all "
-      "be unitless.");
+  // Even though we don't use the resulting values, `convertValueToMatch`
+  // generates more user-friendly exceptions than [greaterThanOrEquals] since it
+  // has more context about parameter names.
+  number.convertValueToMatch(min, "number", "min");
+  max.convertValueToMatch(min, "max", "min");
+
+  if (min.greaterThanOrEquals(max).isTruthy) return min;
+  if (min.greaterThanOrEquals(number).isTruthy) return min;
+  if (number.greaterThanOrEquals(max).isTruthy) return max;
+  return number;
 });
 
 final _floor = _numberFunction("floor", (value) => value.floor());
@@ -94,27 +91,16 @@ final _hypot = _function("hypot", r"$numbers...", (arguments) {
     throw SassScriptException("At least one argument must be passed.");
   }
 
-  var numeratorUnits = numbers[0].numeratorUnits;
-  var denominatorUnits = numbers[0].denominatorUnits;
   var subtotal = 0.0;
   for (var i = 0; i < numbers.length; i++) {
     var number = numbers[i];
-    if (number.hasUnits == numbers[0].hasUnits) {
-      number = number.coerce(numeratorUnits, denominatorUnits);
-      subtotal += math.pow(number.value, 2);
-    } else {
-      var unit1 = numbers[0].hasUnits
-          ? "has unit ${numbers[0].unitString}"
-          : "is unitless";
-      var unit2 =
-          number.hasUnits ? "has unit ${number.unitString}" : "is unitless";
-      throw SassScriptException(
-          "Argument 1 $unit1 but argument ${i + 1} $unit2. Arguments must all "
-          "have units or all be unitless.");
-    }
+    var value = number.convertValueToMatch(
+        numbers[0], "numbers[${i + 1}]", "numbers[1]");
+    subtotal += math.pow(value, 2);
   }
   return SassNumber.withUnits(math.sqrt(subtotal),
-      numeratorUnits: numeratorUnits, denominatorUnits: denominatorUnits);
+      numeratorUnits: numbers[0].numeratorUnits,
+      denominatorUnits: numbers[0].denominatorUnits);
 });
 
 ///
@@ -232,42 +218,36 @@ final _atan = _function("atan", r"$number", (arguments) {
 final _atan2 = _function("atan2", r"$y, $x", (arguments) {
   var y = arguments[0].assertNumber("y");
   var x = arguments[1].assertNumber("x");
-  if (y.hasUnits != x.hasUnits) {
-    var unit1 = y.hasUnits ? "has unit ${y.unitString}" : "is unitless";
-    var unit2 = x.hasUnits ? "has unit ${x.unitString}" : "is unitless";
-    throw SassScriptException(
-        "\$y $unit1 but \$x $unit2. Arguments must all have units or all be "
-        "unitless.");
-  }
 
-  x = x.coerce(y.numeratorUnits, y.denominatorUnits);
-  var xValue = _fuzzyRoundIfZero(x.value);
+  var xValue = _fuzzyRoundIfZero(x.convertValueToMatch(y, 'x', 'y'));
   var yValue = _fuzzyRoundIfZero(y.value);
   var atan2 = math.atan2(yValue, xValue) * 180 / math.pi;
   return SassNumber.withUnits(atan2, numeratorUnits: ['deg']);
 });
 
 final _cos = _function("cos", r"$number", (arguments) {
-  var number = _coerceToRad(arguments[0].assertNumber("number"));
-  return SassNumber(math.cos(number.value));
+  var value =
+      arguments[0].assertNumber("number").coerceValueToUnit("rad", "number");
+  return SassNumber(math.cos(value));
 });
 
 final _sin = _function("sin", r"$number", (arguments) {
-  var number = _coerceToRad(arguments[0].assertNumber("number"));
-  var numberValue = _fuzzyRoundIfZero(number.value);
-  return SassNumber(math.sin(numberValue));
+  var value = _fuzzyRoundIfZero(
+      arguments[0].assertNumber("number").coerceValueToUnit("rad", "number"));
+  return SassNumber(math.sin(value));
 });
 
 final _tan = _function("tan", r"$number", (arguments) {
-  var number = _coerceToRad(arguments[0].assertNumber("number"));
+  var value =
+      arguments[0].assertNumber("number").coerceValueToUnit("rad", "number");
   var asymptoteInterval = 0.5 * math.pi;
   var tanPeriod = 2 * math.pi;
-  if (fuzzyEquals((number.value - asymptoteInterval) % tanPeriod, 0)) {
+  if (fuzzyEquals((value - asymptoteInterval) % tanPeriod, 0)) {
     return SassNumber(double.infinity);
-  } else if (fuzzyEquals((number.value + asymptoteInterval) % tanPeriod, 0)) {
+  } else if (fuzzyEquals((value + asymptoteInterval) % tanPeriod, 0)) {
     return SassNumber(double.negativeInfinity);
   } else {
-    var numberValue = _fuzzyRoundIfZero(number.value);
+    var numberValue = _fuzzyRoundIfZero(value);
     return SassNumber(math.tan(numberValue));
   }
 });
@@ -322,15 +302,6 @@ num _fuzzyRoundIfZero(num number) {
   return number.isNegative ? -0.0 : 0;
 }
 
-SassNumber _coerceToRad(SassNumber number) {
-  try {
-    return number.coerce(['rad'], []);
-  } on SassScriptException catch (error) {
-    if (!error.message.startsWith('Incompatible units')) rethrow;
-    throw SassScriptException('\$number: Expected ${number} to be an angle.');
-  }
-}
-
 /// Returns a [Callable] named [name] that transforms a number's value
 /// using [transform] and preserves its units.
 BuiltInCallable _numberFunction(String name, num transform(num value)) {

lib/src/utils.dart

@@ -37,6 +37,11 @@ String pluralize(String name, int number, {String plural}) {
   return '${name}s';
 }
 
+/// Returns `a $word` or `an $word` depending on whether [word] starts with a
+/// vowel.
+String a(String word) =>
+    [$a, $e, $i, $o, $u].contains(word.codeUnitAt(0)) ? "an $word" : "a $word";
+
 /// Returns a bulleted list of items in [bullets].
 String bulletedList(Iterable<String> bullets) {
   return bullets.map((element) {