maurice/snix
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

refactor(tvix/eval): Don't (ab)use PartialEq for Nix equality

Browse source 
Using rust's PartialEq trait to implement Nix equality semantics is
reasonably fraught with peril, both because the actual laws are
different than what nix expects, and (more importantly) because certain
things actually require extra context to compare for equality (for
example, thunks need to be forced). This converts the manual PartialEq
impl for Value (and all its descendants) to a *derived* PartialEq
impl (which requires a lot of extra PartialEq derives on miscellanious
other types within the codebase), and converts the previous
nix-semantics equality comparison into a new `nix_eq` method. This
returns an EvalResult, even though it can't currently return an error,
to allow it to fail when eg forcing thunks (which it will do soon).

Since the PartialEq impls for Value and NixAttrs are now quite boring,
this converts the generated proptests for those into handwritten ones
that cover `nix_eq` instead

Change-Id: If3da7171f88c22eda5b7a60030d8b00c3b76f672
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6650
Autosubmit: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This commit is contained in:
Griffin Smith 2022-09-18 14:04:40 -04:00 • committed by clbot
parent 0e5baae7ad
commit 915ff5ac2a
11 changed files with 196 additions and 118 deletions
Download patch file Download diff file Expand all files Collapse all files

122
tvix/eval/src/value/attrs.rs
View file

@ -18,7 +18,7 @@ use super::Value;
#[cfg(test)]
mod tests;
#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq)]
enum AttrsRep {
Empty,
Map(BTreeMap<NixString, Value>),
@ -71,7 +71,7 @@ impl AttrsRep {
}
#[repr(transparent)]
#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq)]
pub struct NixAttrs(AttrsRep);
impl Display for NixAttrs {
@ -97,58 +97,6 @@ impl Display for NixAttrs {
}
}
impl PartialEq for NixAttrs {
fn eq(&self, other: &Self) -> bool {
match (&self.0, &other.0) {
(AttrsRep::Empty, AttrsRep::Empty) => true,
// It is possible to create an empty attribute set that
// has Map representation like so: ` { ${null} = 1; }`.
//
// Preventing this would incur a cost on all attribute set
// construction (we'd have to check the actual number of
// elements after key construction). In practice this
// probably does not happen, so it's better to just bite
// the bullet and implement this branch.
(AttrsRep::Empty, AttrsRep::Map(map)) | (AttrsRep::Map(map), AttrsRep::Empty) => {
map.is_empty()
}
// Other specialised representations (KV ...) definitely
// do not match `Empty`.
(AttrsRep::Empty, _) | (_, AttrsRep::Empty) => false,
(
AttrsRep::KV {
name: n1,
value: v1,
},
AttrsRep::KV {
name: n2,
value: v2,
},
) => n1 == n2 && v1 == v2,
(AttrsRep::Map(map), AttrsRep::KV { name, value })
| (AttrsRep::KV { name, value }, AttrsRep::Map(map)) => {
if map.len() != 2 {
return false;
}
if let (Some(m_name), Some(m_value)) =
(map.get(&NixString::NAME), map.get(&NixString::VALUE))
{
return name == m_name && value == m_value;
}
false
}
(AttrsRep::Map(m1), AttrsRep::Map(m2)) => m1 == m2,
}
}
}
#[cfg(feature = "arbitrary")]
mod arbitrary {
use super::*;
@ -337,6 +285,72 @@ impl NixAttrs {
pub(crate) fn from_map(map: BTreeMap<NixString, Value>) -> Self {
NixAttrs(AttrsRep::Map(map))
}
/// Compare `self` against `other` for equality using Nix equality semantics
pub fn nix_eq(&self, other: &Self) -> Result<bool, ErrorKind> {
match (&self.0, &other.0) {
(AttrsRep::Empty, AttrsRep::Empty) => Ok(true),
// It is possible to create an empty attribute set that
// has Map representation like so: ` { ${null} = 1; }`.
//
// Preventing this would incur a cost on all attribute set
// construction (we'd have to check the actual number of
// elements after key construction). In practice this
// probably does not happen, so it's better to just bite
// the bullet and implement this branch.
(AttrsRep::Empty, AttrsRep::Map(map)) | (AttrsRep::Map(map), AttrsRep::Empty) => {
Ok(map.is_empty())
}
// Other specialised representations (KV ...) definitely
// do not match `Empty`.
(AttrsRep::Empty, _) | (_, AttrsRep::Empty) => Ok(false),
(
AttrsRep::KV {
name: n1,
value: v1,
},
AttrsRep::KV {
name: n2,
value: v2,
},
) => Ok(n1.nix_eq(n2)? && v1.nix_eq(v2)?),
(AttrsRep::Map(map), AttrsRep::KV { name, value })
| (AttrsRep::KV { name, value }, AttrsRep::Map(map)) => {
if map.len() != 2 {
return Ok(false);
}
if let (Some(m_name), Some(m_value)) =
(map.get(&NixString::NAME), map.get(&NixString::VALUE))
{
return Ok(name.nix_eq(m_name)? && value.nix_eq(m_value)?);
}
Ok(false)
}
(AttrsRep::Map(m1), AttrsRep::Map(m2)) => {
if m1.len() != m2.len() {
return Ok(false);
}
for (k, v1) in m1 {
if let Some(v2) = m2.get(k) {
if !v1.nix_eq(v2)? {
return Ok(false);
}
} else {
return Ok(false);
}
}
Ok(true)
}
}
}
}
/// In Nix, name/value attribute pairs are frequently constructed from