Improve handling of errors in @for loop ranges.

Author: kaj

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)

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());

rsass/src/parser/mod.rs

@@ -31,7 +31,9 @@ use self::value::{
 };
 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::{
+    Callable, FormalArgs, Item, Name, Selectors, SrcRange, SrcValue, Value,
+};
 use crate::value::ListSeparator;
 #[cfg(test)]
 use crate::value::{Numeric, Unit};
@@ -389,27 +391,35 @@ 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,
+        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, ignore_comments)(input)?;
+    let (input, to) = terminated(single_value_p, ignore_comments)(input)?;
     let (input, body) = body_block(input)?;
     Ok((
         input,
-        Item::For {
-            name: name.into(),
-            from: Box::new(from),
-            to: Box::new(to),
-            inclusive,
+        Item::For(
+            name.into(),
+            SrcRange {
+                from,
+                to,
+                inclusive,
+            },
             body,
-        },
+        ),
     ))
 }
 
+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> {
     let (input, cond) = terminated(value_expression, opt_spacelike)(input)?;
     let (input, body) = body_block(input)?;

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>),
 

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,9 @@ 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::variabledeclaration::parser::*;
 }

rsass/src/sass/srcrange.rs

@@ -0,0 +1,55 @@
+use crate::{
+    value::{Numeric, ValueRange},
+    Error, Invalid, ScopeRef,
+};
+
+use super::SrcValue;
+
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd)]
+pub struct SrcRange {
+    /// The start value for the iteration.
+    pub from: SrcValue,
+    /// The end value for the iteration.
+    pub to: SrcValue,
+    /// True if the end should be included in the range.
+    pub 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))
+    }
+}

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())))
+    }
+}

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;

rsass/src/value/range.rs

@@ -1,89 +1,35 @@
-use super::{Number, Numeric, UnitSet};
+use super::{Numeric, UnitSet};
 use crate::css::Value;
-use std::fmt;
 
 pub struct ValueRange {
-    from: Number,
-    to: Number,
-    step: Number,
+    from: i64,
+    to: i64,
+    step: i64,
     unit: UnitSet,
 }
 
 impl ValueRange {
-    pub fn new(
-        from: Value,
-        to: Value,
-        inclusive: bool,
-    ) -> Result<Self, RangeError> {
-        let from =
-            from.numeric_value().map_err(RangeError::FromNotNumeric)?;
-        let to = to.numeric_value().map_err(RangeError::ToNotNumeric)?;
-
-        let to = if from.is_no_unit() || to.is_no_unit() {
-            to.value
-        } else if let Some(scaled) = to.as_unitset(&from.unit) {
-            scaled
-        } else {
-            return Err(RangeError::IncompatibleUnits(from.unit, to.unit));
-        };
-        let step = if to >= from.value {
-            Number::from(1)
-        } else {
-            Number::from(-1)
-        };
-        let to = if inclusive { to + step.clone() } else { to };
-        Ok(Self {
-            from: from.value,
+    pub fn new(from: i64, to: i64, inclusive: bool, unit: UnitSet) -> Self {
+        let step = if to >= from { 1 } else { -1 };
+        let to = if inclusive { to + step } else { to };
+        Self {
+            from,
             to,
             step,
-            unit: from.unit,
-        })
+            unit,
+        }
     }
 }
 
 impl Iterator for ValueRange {
     type Item = Value;
     fn next(&mut self) -> Option<Value> {
-        if self.from.partial_cmp(&self.to)
-            == Number::from(0).partial_cmp(&self.step)
-        {
-            let result =
-                Numeric::new(self.from.clone(), self.unit.clone()).into();
-            self.from = self.from.clone() + self.step.clone();
+        if self.from.partial_cmp(&self.to) == 0.partial_cmp(&self.step) {
+            let result = Numeric::new(self.from, self.unit.clone()).into();
+            self.from += self.step;
             Some(result)
         } else {
             None
         }
     }
 }
-
-#[derive(Debug)]
-pub enum RangeError {
-    FromNotNumeric(Value),
-    ToNotNumeric(Value),
-    IncompatibleUnits(UnitSet, UnitSet),
-}
-
-impl fmt::Display for RangeError {
-    fn fmt(&self, out: &mut fmt::Formatter) -> fmt::Result {
-        match self {
-            Self::FromNotNumeric(v) => {
-                write!(
-                    out,
-                    "Bad Range: from needs to be numeric, got {}",
-                    v.format(Default::default()),
-                )
-            }
-            Self::ToNotNumeric(v) => {
-                write!(
-                    out,
-                    "Bad Range: to needs to be numeric, got {}",
-                    v.format(Default::default())
-                )
-            }
-            Self::IncompatibleUnits(a, b) => {
-                write!(out, "for loop needs compatible units, got {a}..{b}")
-            }
-        }
-    }
-}

rsass/src/value/unitset.rs

@@ -137,7 +137,7 @@ impl UnitSet {
             }
         }
         self.units.retain(|(_u, p)| *p != 0);
-        factor.into()
+        factor
     }
 }