Improve handling of errors in @for loop ranges.

Author: kaj

Diff for: CHANGELOG.md

@@ -21,6 +21,7 @@ project adheres to
 * All numbers are now represented as f64 (#203).
   This removes integration with `num-rational`, `num-bigint`,
   `num-integer` and `num-traits`.
+* Improved `@for` loop evaluation and error handling (#206).
 * Msrv is now 1.63.0 for rsass (and 1.74 for rsass-cli).
 
 ### Other changes:

Diff for: rsass/src/error.rs

@@ -1,7 +1,7 @@
-use crate::css::InvalidCss;
+use crate::css::{is_not, InvalidCss};
 use crate::input::{LoadError, SourcePos};
+use crate::output::{Format, Formatted};
 use crate::parser::ParseError;
-use crate::value::RangeError;
 use crate::ScopeError;
 use std::{fmt, io};
 
@@ -29,8 +29,6 @@ pub enum Error {
     ///
     /// The bool is true for a used module and false for an import.
     ImportLoop(bool, SourcePos, Option<SourcePos>),
-    /// A range error
-    BadRange(RangeError),
     /// Error parsing sass data.
     ParseError(ParseError),
     /// Something bad at a specific position.
@@ -90,7 +88,6 @@ impl fmt::Display for Error {
                 writeln!(out, "{what}")?;
                 pos.show(out)
             }
-            Self::BadRange(ref err) => err.fmt(out),
             Self::IoError(ref err) => err.fmt(out),
         }
     }
@@ -112,11 +109,6 @@ impl From<ParseError> for Error {
         Self::ParseError(e)
     }
 }
-impl From<RangeError> for Error {
-    fn from(e: RangeError) -> Self {
-        Self::BadRange(e)
-    }
-}
 
 impl From<LoadError> for Error {
     fn from(err: LoadError) -> Self {
@@ -163,6 +155,26 @@ pub enum Invalid {
 }
 
 impl Invalid {
+    pub(crate) fn not<'a, T>(v: &'a T, what: &str) -> Self
+    where
+        Formatted<'a, T>: std::fmt::Display,
+    {
+        Self::AtError(is_not(v, what))
+    }
+    pub(crate) fn expected_to<'a, T>(value: &'a T, cond: &str) -> Self
+    where
+        Formatted<'a, T>: std::fmt::Display,
+    {
+        Self::AtError(format!(
+            "Expected {} to {}.",
+            Formatted {
+                value,
+                format: Format::introspect()
+            },
+            cond
+        ))
+    }
+
     /// Combine this with a position to get an [`Error`].
     pub fn at(self, pos: SourcePos) -> Error {
         Error::Invalid(self, pos)

Diff for: rsass/src/output/transform.rs

@@ -7,7 +7,6 @@ use crate::css::{self, AtRule, Import, SelectorCtx, Value};
 use crate::error::ResultPos;
 use crate::input::{Context, Loader, Parsed, SourceKind};
 use crate::sass::{get_global_module, Expose, Item, UseAs};
-use crate::value::ValueRange;
 use crate::{Error, Invalid, ScopeRef};
 
 pub fn handle_parsed(
@@ -315,18 +314,8 @@ fn handle_item(
             }
             scope.restore_local_values(pushed);
         }
-        Item::For {
-            ref name,
-            ref from,
-            ref to,
-            inclusive,
-            ref body,
-        } => {
-            let range = ValueRange::new(
-                from.evaluate(scope.clone())?,
-                to.evaluate(scope.clone())?,
-                *inclusive,
-            )?;
+        Item::For(ref name, ref range, ref body) => {
+            let range = range.evaluate(scope.clone())?;
             check_body(body, BodyContext::Control)?;
             for value in range {
                 let scope = ScopeRef::sub(scope.clone());

Diff for: rsass/src/parser/mod.rs

@@ -30,8 +30,12 @@ use self::value::{
     value_expression,
 };
 use crate::input::{SourceFile, SourceName, SourcePos};
-use crate::sass::parser::{variable_declaration2, variable_declaration_mod};
-use crate::sass::{Callable, FormalArgs, Item, Name, Selectors, Value};
+use crate::sass::parser::{
+    src_range, variable_declaration2, variable_declaration_mod,
+};
+use crate::sass::{
+    Callable, FormalArgs, Item, Name, Selectors, SrcValue, Value,
+};
 use crate::value::ListSeparator;
 #[cfg(test)]
 use crate::value::{Numeric, Unit};
@@ -387,27 +391,18 @@ fn each_loop2(input: Span) -> PResult<Item> {
 /// A for loop after the initial `@for`.
 fn for_loop2(input: Span) -> PResult<Item> {
     let (input, name) = delimited(tag("$"), name, ignore_comments)(input)?;
-    let (input, from) = delimited(
-        terminated(tag("from"), ignore_comments),
-        single_value,
-        ignore_comments,
-    )(input)?;
-    let (input, inclusive) = terminated(
-        alt((value(true, tag("through")), value(false, tag("to")))),
-        ignore_comments,
-    )(input)?;
-    let (input, to) = terminated(single_value, ignore_comments)(input)?;
+    let (input, range) = src_range(input)?;
     let (input, body) = body_block(input)?;
-    Ok((
-        input,
-        Item::For {
-            name: name.into(),
-            from: Box::new(from),
-            to: Box::new(to),
-            inclusive,
-            body,
-        },
-    ))
+    Ok((input, Item::For(name.into(), range, body)))
+}
+
+/// A single `SrcValue`.
+///
+/// That is, a single sass value with source position.
+pub fn single_value_p(input: Span) -> PResult<SrcValue> {
+    let (rest, value) = single_value(input)?;
+    let pos = input.up_to(&rest).to_owned();
+    Ok((rest, SrcValue::new(value, pos)))
 }
 
 fn while_loop2(input: Span) -> PResult<Item> {

Diff for: rsass/src/sass/item.rs

@@ -1,5 +1,5 @@
 use super::{
-    CallArgs, Callable, Name, SassString, Selectors, Value,
+    CallArgs, Callable, Name, SassString, Selectors, SrcRange, Value,
     VariableDeclaration,
 };
 use crate::input::SourcePos;
@@ -62,18 +62,7 @@ pub enum Item {
     /// The value may be or evaluate to a list.
     Each(Vec<Name>, Value, Vec<Item>),
     /// An `@for` loop directive.
-    For {
-        /// The name of the iteration variable.
-        name: Name,
-        /// The start value for the iteration.
-        from: Box<Value>,
-        /// The end value for the iteration.
-        to: Box<Value>,
-        /// True if the end should be included in the range.
-        inclusive: bool,
-        /// The body of the loop.
-        body: Vec<Item>,
-    },
+    For(Name, SrcRange, Vec<Item>),
     /// An `@while` loop directive.
     While(Value, Vec<Item>),
 

Diff for: rsass/src/sass/mod.rs

@@ -18,6 +18,8 @@ mod item;
 mod mixin;
 mod name;
 mod selectors;
+mod srcrange;
+mod srcvalue;
 mod string;
 mod value;
 mod variabledeclaration;
@@ -36,6 +38,10 @@ pub use self::string::{SassString, StringPart};
 pub use self::value::{BinOp, Value};
 pub use self::variabledeclaration::VariableDeclaration;
 
+pub(crate) use srcrange::SrcRange;
+pub(crate) use srcvalue::SrcValue;
+
 pub(crate) mod parser {
+    pub(crate) use super::srcrange::parser::*;
     pub(crate) use super::variabledeclaration::parser::*;
 }

Diff for: rsass/src/sass/srcrange.rs

@@ -0,0 +1,80 @@
+use super::SrcValue;
+use crate::value::{Numeric, ValueRange};
+use crate::{Error, Invalid, ScopeRef};
+
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd)]
+pub struct SrcRange {
+    from: SrcValue,
+    to: SrcValue,
+    inclusive: bool,
+}
+
+impl SrcRange {
+    pub fn evaluate(&self, scope: ScopeRef) -> Result<ValueRange, Error> {
+        let (from, unit) = self.from.eval_map(scope.clone(), |v| {
+            let v = v
+                .numeric_value()
+                .map_err(|v| Invalid::not(&v, "a number"))?;
+            let unit = v.unit;
+            let v = v.value.into_integer().map_err(|e| {
+                Invalid::not(&Numeric::new(e, unit.clone()), "an int")
+            })?;
+
+            Ok((v, unit))
+        })?;
+        let to = self.to.eval_map(scope.clone(), |v| {
+            let v = v
+                .numeric_value()
+                .map_err(|v| Invalid::not(&v, "a number"))?;
+
+            let v = if unit.is_none() || v.is_no_unit() {
+                v.value
+            } else if let Some(scaled) = v.as_unitset(&unit) {
+                scaled
+            } else {
+                return Err(Invalid::expected_to(
+                    &v,
+                    &format!("have unit {unit}"),
+                ));
+            };
+
+            let v = v.into_integer().map_err(|e| {
+                Invalid::not(&Numeric::new(e, unit.clone()), "an int")
+            })?;
+
+            Ok(v)
+        })?;
+        Ok(ValueRange::new(from, to, self.inclusive, unit))
+    }
+}
+
+pub mod parser {
+    use super::SrcRange;
+    use crate::parser::util::ignore_comments;
+    use crate::parser::{single_value_p, PResult, Span};
+    use nom::branch::alt;
+    use nom::bytes::complete::tag;
+    use nom::combinator::value;
+    use nom::sequence::{delimited, terminated};
+
+    pub fn src_range(input: Span) -> PResult<SrcRange> {
+        let (input, from) = delimited(
+            terminated(tag("from"), ignore_comments),
+            single_value_p,
+            ignore_comments,
+        )(input)?;
+        let (input, inclusive) = terminated(
+            alt((value(true, tag("through")), value(false, tag("to")))),
+            ignore_comments,
+        )(input)?;
+        let (input, to) = terminated(single_value_p, ignore_comments)(input)?;
+        Ok((
+            input,
+            SrcRange {
+                from,
+                to,
+                inclusive,
+            },
+        ))
+    }
+}

Diff for: rsass/src/sass/srcvalue.rs

@@ -0,0 +1,24 @@
+use super::Value;
+use crate::{css, input::SourcePos, Error, Invalid, ScopeRef};
+
+/// A value with a specific source position
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd)]
+pub struct SrcValue {
+    value: Value,
+    pos: SourcePos,
+}
+
+impl SrcValue {
+    pub fn new(value: Value, pos: SourcePos) -> Self {
+        Self { value, pos }
+    }
+    pub fn eval_map<T, F>(&self, scope: ScopeRef, f: F) -> Result<T, Error>
+    where
+        F: Fn(css::Value) -> Result<T, Invalid>,
+    {
+        // TODO: The position should be applied to err from evaluate as well!
+        self.value
+            .evaluate(scope)
+            .and_then(|v| f(v).map_err(|e| e.at(self.pos.clone())))
+    }
+}

Diff for: rsass/src/value/mod.rs

@@ -17,4 +17,4 @@ pub use self::operator::{BadOp, Operator};
 pub use self::quotes::Quotes;
 pub use self::unit::{CssDimension, Dimension, Unit};
 pub use self::unitset::{CssDimensionSet, UnitSet};
-pub(crate) use range::{RangeError, ValueRange};
+pub(crate) use range::ValueRange;