refactor value equality checks

src/builtin/list.rs

@@ -6,7 +6,7 @@ use crate::{
     args::CallArgs,
     common::{Brackets, ListSeparator, QuoteKind},
     error::SassResult,
-    parse::{HigherIntermediateValue, Parser, ValueVisitor},
+    parse::Parser,
     unit::Unit,
     value::{Number, Value},
 };
@@ -244,14 +244,7 @@ fn index(mut args: CallArgs, parser: &mut Parser<'_>) -> SassResult<Value> {
     let value = parser.arg(&mut args, 1, "value")?;
     // TODO: find a way to propagate any errors here
     // Potential input to fuzz: index(1px 1in 1cm, 96px + 1rem)
-    let index = match list.into_iter().position(|v| {
+    let index = match list.into_iter().position(|v| v.equals(&value)) {
-        ValueVisitor::new(parser, args.span())
-            .equal(
-                HigherIntermediateValue::Literal(v),
-                HigherIntermediateValue::Literal(value.clone()),
-            )
-            .map_or(false, |v| v.is_true())
-    }) {
         Some(v) => Number::from(v + 1),
         None => return Ok(Value::Null),
     };

src/builtin/map.rs

@@ -23,7 +23,7 @@ fn map_get(mut args: CallArgs, parser: &mut Parser<'_>) -> SassResult<Value> {
                 .into())
         }
     };
-    Ok(map.get(&key, args.span(), parser)?.unwrap_or(Value::Null))
+    Ok(map.get(&key)?.unwrap_or(Value::Null))
 }
 
 fn map_has_key(mut args: CallArgs, parser: &mut Parser<'_>) -> SassResult<Value> {
@@ -41,7 +41,7 @@ fn map_has_key(mut args: CallArgs, parser: &mut Parser<'_>) -> SassResult<Value>
                 .into())
         }
     };
-    Ok(Value::bool(map.get(&key, args.span(), parser)?.is_some()))
+    Ok(Value::bool(map.get(&key)?.is_some()))
 }
 
 fn map_keys(mut args: CallArgs, parser: &mut Parser<'_>) -> SassResult<Value> {

src/parse/value/eval.rs

@@ -84,8 +84,8 @@ impl<'a, 'b: 'a> ValueVisitor<'a, 'b> {
             Op::Rem => self.rem(val1, val2)?,
             Op::And => Self::and(val1, val2)?,
             Op::Or => Self::or(val1, val2)?,
-            Op::Equal => self.equal(val1, val2)?,
+            Op::Equal => self.equal(val1, val2),
-            Op::NotEqual => self.not_equal(val1, val2)?,
+            Op::NotEqual => self.not_equal(val1, val2),
             Op::GreaterThan => self.greater_than(val1, val2)?,
             Op::GreaterThanEqual => self.greater_than_or_equal(val1, val2)?,
             Op::LessThan => self.less_than(val1, val2)?,
@@ -672,11 +672,7 @@ impl<'a, 'b: 'a> ValueVisitor<'a, 'b> {
         Ok(if left.is_true() { left } else { right })
     }
 
-    pub fn equal(
+    pub fn equal(&self, left: HigherIntermediateValue, right: HigherIntermediateValue) -> Value {
-        &self,
-        left: HigherIntermediateValue,
-        right: HigherIntermediateValue,
-    ) -> SassResult<Value> {
         let left = match left {
             HigherIntermediateValue::Literal(v) => v,
             v => panic!("{:?}", v),
@@ -685,60 +681,10 @@ impl<'a, 'b: 'a> ValueVisitor<'a, 'b> {
             HigherIntermediateValue::Literal(v) => v,
             v => panic!("{:?}", v),
         };
-        Ok(Value::bool(match left {
+        Value::bool(left.equals(&right))
-            Value::String(s1, ..) => match right {
-                Value::String(s2, ..) => s1 == s2,
-                _ => false,
-            },
-            Value::Dimension(n, unit) => match right {
-                Value::Dimension(n2, unit2) => {
-                    if !unit.comparable(&unit2) {
-                        false
-                    } else if unit == unit2 {
-                        n == n2
-                    } else if unit == Unit::None || unit2 == Unit::None {
-                        false
-                    } else {
-                        n == (n2
-                            * UNIT_CONVERSION_TABLE[unit.to_string().as_str()]
-                                [unit2.to_string().as_str()]
-                            .clone())
-                    }
-                }
-                _ => false,
-            },
-            Value::List(list1, sep1, brackets1) => match right {
-                Value::List(list2, sep2, brackets2) => {
-                    if sep1 != sep2 || brackets1 != brackets2 || list1.len() != list2.len() {
-                        false
-                    } else {
-                        let mut equals = true;
-                        for (a, b) in list1.into_iter().zip(list2) {
-                            if !self
-                                .equal(
-                                    HigherIntermediateValue::Literal(a),
-                                    HigherIntermediateValue::Literal(b),
-                                )?
-                                .is_true()
-                            {
-                                equals = false;
-                                break;
-                            }
-                        }
-                        equals
-                    }
-                }
-                _ => false,
-            },
-            s => s == right,
-        }))
     }
 
-    fn not_equal(
+    fn not_equal(&self, left: HigherIntermediateValue, right: HigherIntermediateValue) -> Value {
-        &self,
-        left: HigherIntermediateValue,
-        right: HigherIntermediateValue,
-    ) -> SassResult<Value> {
         let left = match left {
             HigherIntermediateValue::Literal(v) => v,
             v => panic!("{:?}", v),
@@ -747,53 +693,7 @@ impl<'a, 'b: 'a> ValueVisitor<'a, 'b> {
             HigherIntermediateValue::Literal(v) => v,
             v => panic!("{:?}", v),
         };
-        Ok(Value::bool(match left {
+        Value::bool(left.not_equals(&right))
-            Value::String(s1, ..) => match right {
-                Value::String(s2, ..) => s1 != s2,
-                _ => true,
-            },
-            Value::Dimension(n, unit) => match right {
-                Value::Dimension(n2, unit2) => {
-                    if !unit.comparable(&unit2) {
-                        true
-                    } else if unit == unit2 {
-                        n != n2
-                    } else if unit == Unit::None || unit2 == Unit::None {
-                        true
-                    } else {
-                        n != (n2
-                            * UNIT_CONVERSION_TABLE[unit.to_string().as_str()]
-                                [unit2.to_string().as_str()]
-                            .clone())
-                    }
-                }
-                _ => true,
-            },
-            Value::List(list1, sep1, brackets1) => match right {
-                Value::List(list2, sep2, brackets2) => {
-                    if sep1 != sep2 || brackets1 != brackets2 || list1.len() != list2.len() {
-                        true
-                    } else {
-                        let mut equals = false;
-                        for (a, b) in list1.into_iter().zip(list2) {
-                            if self
-                                .not_equal(
-                                    HigherIntermediateValue::Literal(a),
-                                    HigherIntermediateValue::Literal(b),
-                                )?
-                                .is_true()
-                            {
-                                equals = true;
-                                break;
-                            }
-                        }
-                        equals
-                    }
-                }
-                _ => true,
-            },
-            s => s != right,
-        }))
     }
 
     fn cmp(

src/value/map.rs

@@ -1,11 +1,8 @@
 use std::{slice::Iter, vec::IntoIter};
 
-use codemap::Span;
-
 use crate::{
     common::{Brackets, ListSeparator},
     error::SassResult,
-    parse::{HigherIntermediateValue, Parser, ValueVisitor},
     value::Value,
 };
 
@@ -17,20 +14,9 @@ impl SassMap {
         SassMap(Vec::new())
     }
 
-    pub fn get(
+    pub fn get(self, key: &Value) -> SassResult<Option<Value>> {
-        self,
-        key: &Value,
-        span: Span,
-        parser: &mut Parser<'_>,
-    ) -> SassResult<Option<Value>> {
         for (k, v) in self.0 {
-            if ValueVisitor::new(parser, span)
+            if k.equals(&key) {
-                .equal(
-                    HigherIntermediateValue::Literal(k),
-                    HigherIntermediateValue::Literal(key.clone()),
-                )?
-                .is_true()
-            {
                 return Ok(Some(v));
             }
         }
@@ -38,7 +24,7 @@ impl SassMap {
     }
 
     pub fn remove(&mut self, key: &Value) {
-        self.0.retain(|(ref k, ..)| k != key);
+        self.0.retain(|(ref k, ..)| k.not_equals(key));
     }
 
     pub fn merge(&mut self, other: SassMap) {
@@ -74,7 +60,7 @@ impl SassMap {
     /// Returns true if the key already exists
     pub fn insert(&mut self, key: Value, value: Value) -> bool {
         for (ref k, ref mut v) in &mut self.0 {
-            if k == &key {
+            if k.equals(&key) {
                 *v = value;
                 return true;
             }

src/value/mod.rs

@@ -8,7 +8,7 @@ use crate::{
     error::SassResult,
     parse::Parser,
     selector::Selector,
-    unit::Unit,
+    unit::{Unit, UNIT_CONVERSION_TABLE},
     utils::hex_char_for,
     {Cow, Token},
 };
@@ -23,6 +23,7 @@ mod map;
 mod number;
 mod sass_function;
 
+// todo: remove Eq and PartialEq impls
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub(crate) enum Value {
     Important,
@@ -248,6 +249,90 @@ impl Value {
         }
     }
 
+    pub fn equals(&self, other: &Self) -> bool {
+        match self {
+            Value::String(s1, ..) => match other {
+                Value::String(s2, ..) => s1 == s2,
+                _ => false,
+            },
+            Value::Dimension(n, unit) => match other {
+                Value::Dimension(n2, unit2) => {
+                    if !unit.comparable(&unit2) {
+                        false
+                    } else if unit == unit2 {
+                        n == n2
+                    } else if unit == &Unit::None || unit2 == &Unit::None {
+                        false
+                    } else {
+                        n == &(n2.clone()
+                            * UNIT_CONVERSION_TABLE[unit.to_string().as_str()]
+                                [unit2.to_string().as_str()]
+                            .clone())
+                    }
+                }
+                _ => false,
+            },
+            Value::List(list1, sep1, brackets1) => match other {
+                Value::List(list2, sep2, brackets2) => {
+                    if sep1 != sep2 || brackets1 != brackets2 || list1.len() != list2.len() {
+                        false
+                    } else {
+                        for (a, b) in list1.into_iter().zip(list2) {
+                            if !a.equals(b) {
+                                return false;
+                            }
+                        }
+                        true
+                    }
+                }
+                _ => false,
+            },
+            s => s == other,
+        }
+    }
+
+    pub fn not_equals(&self, other: &Self) -> bool {
+        match self {
+            Value::String(s1, ..) => match other {
+                Value::String(s2, ..) => s1 != s2,
+                _ => true,
+            },
+            Value::Dimension(n, unit) => match other {
+                Value::Dimension(n2, unit2) => {
+                    if !unit.comparable(&unit2) {
+                        true
+                    } else if unit == unit2 {
+                        n != n2
+                    } else if unit == &Unit::None || unit2 == &Unit::None {
+                        true
+                    } else {
+                        n != &(n2.clone()
+                            * UNIT_CONVERSION_TABLE[unit.to_string().as_str()]
+                                [unit2.to_string().as_str()]
+                            .clone())
+                    }
+                }
+                _ => true,
+            },
+            Value::List(list1, sep1, brackets1) => match other {
+                Value::List(list2, sep2, brackets2) => {
+                    if sep1 != sep2 || brackets1 != brackets2 || list1.len() != list2.len() {
+                        true
+                    } else {
+                        for (a, b) in list1.iter().zip(list2) {
+                            if a.not_equals(b) {
+                                return true;
+                            }
+                        }
+                        false
+                    }
+                }
+                _ => true,
+            },
+            s => s != other,
+        }
+    }
+
     // TODO:
     // https://github.com/sass/dart-sass/blob/d4adea7569832f10e3a26d0e420ae51640740cfb/lib/src/ast/sass/expression/list.dart#L39
     pub fn inspect(&self, span: Span) -> SassResult<Cow<'static, str>> {

tests/map.rs

@@ -182,3 +182,8 @@ test!(
     "$a: (foo: red, ); a {\n  color: inspect($a);\n}\n",
     "a {\n  color: (foo: red);\n}\n"
 );
+test!(
+    map_merge_not_exactly_equal,
+    "a {\n  color: inspect(map-merge((0cm: a), (0mm: b)));;\n}\n",
+    "a {\n  color: (0cm: b);\n}\n"
+);