fixes #51

We were creating a recursive range, this fixes that by removing calls to `update_deps` which was pointless anyways
nascentxyz · Dec 10, 2023 · fda559b · fda559b
1 parent 1fa6730
commit fda559b
Show file tree

Hide file tree

Showing 16 changed files with 851 additions and 674 deletions.
diff --git a/crates/graph/src/nodes/context/var/ranging.rs b/crates/graph/src/nodes/context/var/ranging.rs
@@ -1,3 +1,4 @@
+use std::collections::BTreeSet;
 use crate::{
     nodes::{Concrete, ContextNode, ContextVarNode},
     range::{range_string::ToRangeString, Range},
@@ -145,7 +146,8 @@ impl ContextVarNode {
         analyzer: &mut (impl GraphBackend + AnalyzerBackend),
         mut new_min: Elem<Concrete>,
     ) -> Result<(), GraphError> {
-        if new_min.contains_node((*self).into()) {
+        assert!(self.latest_version(analyzer) == *self);
+        if new_min.recursive_dependent_on(analyzer)?.contains(self) {
             if let Some(prev) = self.previous_or_inherited_version(analyzer) {
                 new_min.filter_recursion((*self).into(), prev.into());
             } else {
@@ -154,11 +156,12 @@ impl ContextVarNode {
         }
 
         tracing::trace!(
-            "setting range minimum: {} (node idx: {}), current:\n{:#?}, new_min:\n{:#?}",
+            "setting range minimum: {} (node idx: {}), current:{}, new_min:{}, deps: {:#?}",
             self.display_name(analyzer)?,
             self.0,
-            self.range_min(analyzer)?,
-            new_min
+            self.range_min(analyzer)?.unwrap(),
+            new_min,
+            new_min.recursive_dependent_on(analyzer)?
         );
 
         if self.is_concrete(analyzer)? {
@@ -185,14 +188,15 @@ impl ContextVarNode {
         analyzer: &mut (impl GraphBackend + AnalyzerBackend),
         mut new_max: Elem<Concrete>,
     ) -> Result<(), GraphError> {
-        if new_max.contains_node((*self).into()) {
+        assert!(self.latest_version(analyzer) == *self);
+        if new_max.recursive_dependent_on(analyzer)?.contains(self) {
             if let Some(prev) = self.previous_or_inherited_version(analyzer) {
                 new_max.filter_recursion((*self).into(), prev.into());
             }
         }
 
         tracing::trace!(
-            "setting range maximum: {:?}, {}, current:\n{:#?}, new:\n{:#?}",
+            "setting range maximum: {:?}, {}, current: {}, new: {:#?}",
             self,
             self.display_name(analyzer)?,
             self.ref_range(analyzer)?.unwrap().range_max(), // .unwrap()
@@ -239,7 +243,8 @@ impl ContextVarNode {
         analyzer: &mut (impl GraphBackend + AnalyzerBackend),
         mut new_min: Elem<Concrete>,
     ) -> Result<bool, GraphError> {
-        if new_min.contains_node((*self).into()) {
+        assert!(self.latest_version(analyzer) == *self);
+        if new_min.recursive_dependent_on(analyzer)?.contains(self) {
             if let Some(prev) = self.previous_version(analyzer) {
                 new_min.filter_recursion((*self).into(), prev.into());
             }
@@ -267,7 +272,8 @@ impl ContextVarNode {
         analyzer: &mut (impl GraphBackend + AnalyzerBackend),
         mut new_max: Elem<Concrete>,
     ) -> Result<bool, GraphError> {
-        if new_max.contains_node((*self).into()) {
+        assert!(self.latest_version(analyzer) == *self);
+        if new_max.recursive_dependent_on(analyzer)?.contains(self) {
             if let Some(prev) = self.previous_version(analyzer) {
                 new_max.filter_recursion((*self).into(), prev.into());
             }

diff --git a/crates/graph/src/nodes/context/variables.rs b/crates/graph/src/nodes/context/variables.rs
@@ -201,6 +201,7 @@ impl ContextNode {
         loc: Loc,
         analyzer: &mut (impl GraphBackend + AnalyzerBackend),
     ) -> Result<ContextVarNode, GraphError> {
+        let var = var.latest_version(analyzer);
         if let Some(ctx) = var.maybe_ctx(analyzer) {
             if ctx != *self {
                 let mut new_cvar = var.latest_version(analyzer).underlying(analyzer)?.clone();

diff --git a/crates/graph/src/range/elem/concrete.rs b/crates/graph/src/range/elem/concrete.rs
@@ -32,6 +32,10 @@ impl RangeElem<Concrete> for RangeConcrete<Concrete> {
     //  Elem::Concrete(self.clone())
     // }
 
+    fn has_cycle(&self, seen: &mut Vec<ContextVarNode>, analyzer: &impl GraphBackend) -> Result<bool, Self::GraphError> {
+        Ok(false)
+    }
+
     fn flatten(
         &self,
         _maximize: bool,
@@ -153,4 +157,6 @@ impl RangeElem<Concrete> for RangeConcrete<Concrete> {
     ) -> Result<bool, GraphError> {
         Ok(false)
     }
+
+    fn recursive_dependent_on(&self, _: &impl GraphBackend) -> Result<Vec<ContextVarNode>, GraphError> { Ok(vec![]) }
 }
diff --git a/crates/graph/src/range/elem/elem_enum.rs b/crates/graph/src/range/elem/elem_enum.rs
@@ -411,6 +411,26 @@ impl RangeElem<Concrete> for Elem<Concrete> {
         }
     }
 
+    fn recursive_dependent_on(&self, analyzer: &impl GraphBackend) -> Result<Vec<ContextVarNode>, GraphError> {
+        match self {
+            Self::Reference(d) => d.recursive_dependent_on(analyzer),
+            Self::Concrete(_) => Ok(vec![]),
+            Self::Expr(expr) => expr.recursive_dependent_on(analyzer),
+            Self::ConcreteDyn(d) => d.recursive_dependent_on(analyzer),
+            Self::Null => Ok(vec![]),
+        }
+    }
+
+    fn has_cycle(&self, seen: &mut Vec<ContextVarNode>, analyzer: &impl GraphBackend) -> Result<bool, Self::GraphError> {
+        match self {
+            Self::Reference(d) => d.has_cycle(seen, analyzer),
+            Self::Concrete(_) => Ok(false),
+            Self::Expr(expr) => expr.has_cycle(seen, analyzer),
+            Self::ConcreteDyn(d) => d.has_cycle(seen, analyzer),
+            Self::Null => Ok(false),
+        }
+    }
+
     fn update_deps(&mut self, mapping: &BTreeMap<ContextVarNode, ContextVarNode>) {
         match self {
             Self::Reference(d) => d.update_deps(mapping),

diff --git a/crates/graph/src/range/elem/elem_trait.rs b/crates/graph/src/range/elem/elem_trait.rs
@@ -52,6 +52,10 @@ pub trait RangeElem<T> {
     /// Traverses the range expression and finds all nodes that are dynamically pointed to
     /// and returns it in a vector.
     fn dependent_on(&self) -> Vec<ContextVarNode>;
+
+    fn recursive_dependent_on(&self, analyzer: &impl GraphBackend) -> Result<Vec<ContextVarNode>, Self::GraphError>;
+
+    fn has_cycle(&self, seen: &mut Vec<ContextVarNode>, analyzer: &impl GraphBackend) -> Result<bool, Self::GraphError>;
     /// Traverses the range expression and updates stale pointers from older versions
     /// of a variable to a newer version.
     ///

diff --git a/crates/graph/src/range/elem/expr.rs b/crates/graph/src/range/elem/expr.rs
@@ -99,6 +99,7 @@ impl RangeElem<Concrete> for RangeExpr<Concrete> {
         maximize: bool,
         analyzer: &impl GraphBackend,
     ) -> Result<Elem<Concrete>, GraphError> {
+        // println!("flattening expr: {}", Elem::Expr(self.clone()));
         Ok(Elem::Expr(RangeExpr::new(
             self.lhs.flatten(maximize, analyzer)?,
             self.op,
@@ -116,6 +117,18 @@ impl RangeElem<Concrete> for RangeExpr<Concrete> {
         deps
     }
 
+    fn recursive_dependent_on(&self, analyzer: &impl GraphBackend) -> Result<Vec<ContextVarNode>, GraphError> {
+        let mut deps = self.lhs.recursive_dependent_on(analyzer)?;
+        deps.extend(self.rhs.recursive_dependent_on(analyzer)?);
+        Ok(deps)
+    }
+
+    fn has_cycle(&self, seen: &mut Vec<ContextVarNode>, analyzer: &impl GraphBackend) -> Result<bool, GraphError> {
+        let lhs_has_cycle = self.lhs.has_cycle(seen, analyzer)?;
+        let rhs_has_cycle = self.rhs.has_cycle(seen, analyzer)?;
+        Ok(lhs_has_cycle || rhs_has_cycle)
+    }
+
     fn update_deps(&mut self, mapping: &BTreeMap<ContextVarNode, ContextVarNode>) {
         self.lhs.update_deps(mapping);
         self.rhs.update_deps(mapping);

diff --git a/crates/graph/src/range/elem/map_or_array.rs b/crates/graph/src/range/elem/map_or_array.rs
@@ -59,11 +59,47 @@ impl RangeElem<Concrete> for RangeDyn<Concrete> {
         deps
     }
 
+    fn recursive_dependent_on(&self, analyzer: &impl GraphBackend) -> Result<Vec<ContextVarNode>, GraphError> {
+        let mut deps: Vec<ContextVarNode> = self.len.recursive_dependent_on(analyzer)?;
+        deps.extend(
+            self.val
+                .values()
+                .map(|val| val.recursive_dependent_on(analyzer))
+                .collect::<Result<Vec<Vec<_>>, _>>()?
+                .iter()
+                .flatten()
+                .collect::<Vec<_>>()
+        );
+        deps.extend(
+            self.val
+                .keys()
+                .map(|key| key.recursive_dependent_on(analyzer))
+                .collect::<Result<Vec<Vec<_>>, _>>()?
+                .iter()
+                .flatten()
+                .collect::<Vec<_>>()
+        );
+        Ok(deps)
+    }
+
+    fn has_cycle(&self, seen: &mut Vec<ContextVarNode>, analyzer: &impl GraphBackend) -> Result<bool, GraphError> {
+        let mut has_cycle = false;
+        has_cycle = has_cycle || self.len.has_cycle(seen, analyzer)?;
+        self.val
+            .iter()
+            .try_for_each(|(_, val)| {
+                has_cycle = has_cycle ||  val.has_cycle(seen, analyzer)?;
+                Ok(())
+            })?;
+        Ok(has_cycle)
+    }
+
     fn flatten(
         &self,
         maximize: bool,
         analyzer: &impl GraphBackend,
     ) -> Result<Elem<Concrete>, GraphError> {
+        // println!("flattening range dyn");
         Ok(Elem::ConcreteDyn(Box::new(Self {
             minimized: None,
             maximized: None,

diff --git a/crates/graph/src/range/elem/reference.rs b/crates/graph/src/range/elem/reference.rs
@@ -52,6 +52,29 @@ impl RangeElem<Concrete> for Reference<Concrete> {
     fn dependent_on(&self) -> Vec<ContextVarNode> {
         vec![self.idx.into()]
     }
+
+    fn recursive_dependent_on(&self, analyzer: &impl GraphBackend) -> Result<Vec<ContextVarNode>, GraphError> {
+        let mut deps = ContextVarNode(self.idx.index()).dependent_on(analyzer, true)?;
+        deps.push(ContextVarNode(self.idx.index()));
+        Ok(deps)
+    }
+
+    fn has_cycle(&self, seen: &mut Vec<ContextVarNode>, analyzer: &impl GraphBackend) -> Result<bool, GraphError> {
+        let cvar = ContextVarNode::from(self.idx);
+        let mut has_cycle = false;
+        if seen.contains(&cvar) {
+            Ok(true)
+        } else {
+            seen.push(cvar);
+            if let Some(range) = cvar.ref_range(analyzer)? {
+                has_cycle = has_cycle || range.min.has_cycle(seen, analyzer)?;
+                has_cycle = has_cycle || range.max.has_cycle(seen, analyzer)?;
+                Ok(has_cycle)
+            } else {
+                Ok(false)
+            }
+        }
+    }
 
     fn update_deps(&mut self, mapping: &BTreeMap<ContextVarNode, ContextVarNode>) {
         if let Some(new) = mapping.get(&ContextVarNode::from(self.idx)) {
@@ -65,6 +88,7 @@ impl RangeElem<Concrete> for Reference<Concrete> {
         analyzer: &impl GraphBackend,
     ) -> Result<Elem<Concrete>, GraphError> {
         let cvar = ContextVarNode::from(self.idx);
+        // println!("flattening reference: {} (idx_{})", cvar.display_name(analyzer)?, self.idx.index());
         if cvar.is_fundamental(analyzer)? {
             return Ok(Elem::Reference(Reference::new(
                 cvar.global_first_version(analyzer).into(),

diff --git a/crates/graph/src/range/solc_range.rs b/crates/graph/src/range/solc_range.rs
@@ -67,26 +67,40 @@ impl SolcRange {
         deps.into_iter().map(ContextVarNode::from).collect()
     }
 
+    pub fn recursive_dependent_on(&self, analyzer: &impl GraphBackend,) -> Result<Vec<ContextVarNode>, GraphError> {
+        let mut deps = self.range_min().recursive_dependent_on(analyzer)?;
+        deps.extend(self.range_max().recursive_dependent_on(analyzer)?);
+        deps.dedup();
+
+        Ok(deps)
+    }
+
     /// Update a particular context variable with the latest version
     pub fn update_deps(
         &mut self,
         node: ContextVarNode,
         ctx: ContextNode,
         analyzer: &impl GraphBackend,
     ) {
+        assert!(node.latest_version(analyzer) == node);
         let deps = self.dependent_on();
         let mapping: BTreeMap<ContextVarNode, ContextVarNode> = deps
             .into_iter()
             .filter(|dep| !dep.is_const(analyzer).unwrap())
             .map(|dep| {
                 let latest = dep.latest_version_in_ctx(ctx, analyzer).unwrap();
+                // let dep_depends_on_node = dep.range(analyzer).unwrap().unwrap().recursive_dependent_on(analyzer).unwrap();
+                // let dep_depends_on_node = dep_depends_on_node.contains(&node) || dep_depends_on_node.contains(&);
                 if latest == node {
                     if let Some(prev) = latest.previous_version(analyzer) {
+                        println!("replacing: {} with {}", dep.0, prev.0);
                         (dep, prev)
                     } else {
+                        println!("replacing: {} with {}", dep.0, dep.0);
                         (dep, dep)
                     }
                 } else {
+                    println!("replacing: {} with {}", dep.0, latest.0);
                     (dep, latest)
                 }
             })

diff --git a/crates/pyrometer/tests/test_data/loops.sol b/crates/pyrometer/tests/test_data/loops.sol
@@ -28,4 +28,32 @@ contract For {
         require(x == 10);
         return x;
     }
+
+    function complicated_while_loop(uint256 amount) public returns (uint256) {
+        uint256 x = amount;
+        amount -= x;
+        return amount;
+        // uint256 balance = 1;
+        // uint256 amountToRedeem;
+        // if (amount > balance) {
+        //     amountToRedeem = balance;
+        // } else {
+        //     amountToRedeem = amount;
+        // }
+        // amount -= amountToRedeem;
+
+        // return amount;
+    }
+
+    function loop_op_assign(uint256 value) internal pure {
+        uint256 temp = value;
+        uint256 digits;
+        while (temp != 0) {
+            digits++;
+            temp /= 10;
+        }
+    }
 }
+
+
+