Fix lifetime and copy issues with NodeDfsIterable (#4569)

When running node_traversal_black with ASan and UBSan, there were two
distinct issues being reported. UBSan was complaining that we were
assigning an invalid value to a Boolean. This happened because
d_initialized in NodeDfsIterator was uninitialized and the default
copy constructor tried to copy it. This commit removes that data member.
ASan was complainig that NodeDfsIterator::begin() was trying to access
a skip function that had been freed. In particular, this happened when
NodeDfsIterable was used in a range-based for loop like so:

for (auto n : NodeDfsIterable(n).inPostorder())
{
  ...
}
The problem here is that the lifetime of a temporary within the range
expression is not extended (the lifetime of the result of the range
expression is extended, however) [0]. This is a problem because
NodeDfsIterable(n) is a temporary and inPostorder() returns a
reference to that temporary. It makes sense that the compiler cannot
possibly know that the reference from inPostorder() corresponds to
NodeDfsIterable(n), so it cannot extend its lifetime. See [1] for more
details on the problem with chained functions.

This commit fixes the issue by replacing the fluent interface of
NodeDfsIterable by a constructor with default arguments. Additionally,
it introduces an enum to represent the visit order to avoid having a
nondescript Boolean argument.

[0] https://en.cppreference.com/w/cpp/language/range-for#Temporary_range_expression
[1] http://cpptruths.blogspot.com/2018/10/chained-functions-break-reference.html?m=1

1 files changed, 16 insertions, 16 deletions

diff --git a/test/unit/expr/node_traversal_black.h b/test/unit/expr/node_traversal_black.h
index b751a0999..7e08f209d 100644
--- a/test/unit/expr/node_traversal_black.h
+++ b/test/unit/expr/node_traversal_black.h
@@ -58,7 +58,7 @@ class NodePostorderTraversalBlack : public CxxTest::TestSuite
     Node eb = d_nodeManager->mkConst(false);
     Node cnd = d_nodeManager->mkNode(XOR, tb, eb);
 
-    auto traversal = NodeDfsIterable(cnd).inPostorder();
+    auto traversal = NodeDfsIterable(cnd, VisitOrder::POSTORDER);
     NodeDfsIterator i = traversal.begin();
     NodeDfsIterator end = traversal.end();
     TS_ASSERT_EQUALS(*i, tb);
@@ -79,7 +79,7 @@ class NodePostorderTraversalBlack : public CxxTest::TestSuite
     Node eb = d_nodeManager->mkConst(false);
     Node cnd = d_nodeManager->mkNode(XOR, tb, eb);
 
-    auto traversal = NodeDfsIterable(cnd).inPostorder();
+    auto traversal = NodeDfsIterable(cnd, VisitOrder::POSTORDER);
     NodeDfsIterator i = traversal.begin();
     NodeDfsIterator end = traversal.end();
     TS_ASSERT_EQUALS(*(i++), tb);
@@ -108,7 +108,7 @@ class NodePostorderTraversalBlack : public CxxTest::TestSuite
     Node cnd = d_nodeManager->mkNode(XOR, tb, eb);
 
     size_t count = 0;
-    for (auto i : NodeDfsIterable(cnd).inPostorder())
+    for (auto i : NodeDfsIterable(cnd, VisitOrder::POSTORDER))
     {
       ++count;
     }
@@ -122,7 +122,7 @@ class NodePostorderTraversalBlack : public CxxTest::TestSuite
     Node cnd = d_nodeManager->mkNode(XOR, tb, eb);
 
     size_t count = 0;
-    for (auto i : NodeDfsIterable(cnd).inPostorder())
+    for (auto i : NodeDfsIterable(cnd, VisitOrder::POSTORDER))
     {
       if (i.isConst())
       {
@@ -139,7 +139,7 @@ class NodePostorderTraversalBlack : public CxxTest::TestSuite
     Node cnd = d_nodeManager->mkNode(XOR, tb, eb);
     Node top = d_nodeManager->mkNode(XOR, cnd, cnd);
 
-    auto traversal = NodeDfsIterable(top).inPostorder();
+    auto traversal = NodeDfsIterable(top, VisitOrder::POSTORDER);
 
     size_t count = std::count_if(traversal.begin(),
                                  traversal.end(),
@@ -155,7 +155,7 @@ class NodePostorderTraversalBlack : public CxxTest::TestSuite
     Node top = d_nodeManager->mkNode(XOR, cnd, cnd);
     std::vector<TNode> expected = {tb, eb, cnd, top};
 
-    auto traversal = NodeDfsIterable(top).inPostorder();
+    auto traversal = NodeDfsIterable(top, VisitOrder::POSTORDER);
 
     std::vector<TNode> actual;
     std::copy(traversal.begin(), traversal.end(), std::back_inserter(actual));
@@ -170,8 +170,8 @@ class NodePostorderTraversalBlack : public CxxTest::TestSuite
     Node top = d_nodeManager->mkNode(XOR, cnd, cnd);
     std::vector<TNode> expected = {top};
 
-    auto traversal = NodeDfsIterable(top).inPostorder().skipIf(
-        [&cnd](TNode n) { return n == cnd; });
+    auto traversal = NodeDfsIterable(
+        top, VisitOrder::POSTORDER, [&cnd](TNode n) { return n == cnd; });
 
     std::vector<TNode> actual;
     std::copy(traversal.begin(), traversal.end(), std::back_inserter(actual));
@@ -204,7 +204,7 @@ class NodePreorderTraversalBlack : public CxxTest::TestSuite
     Node eb = d_nodeManager->mkConst(false);
     Node cnd = d_nodeManager->mkNode(XOR, tb, eb);
 
-    auto traversal = NodeDfsIterable(cnd).inPreorder();
+    auto traversal = NodeDfsIterable(cnd, VisitOrder::PREORDER);
     NodeDfsIterator i = traversal.begin();
     NodeDfsIterator end = traversal.end();
     TS_ASSERT_EQUALS(*i, cnd);
@@ -225,7 +225,7 @@ class NodePreorderTraversalBlack : public CxxTest::TestSuite
     Node eb = d_nodeManager->mkConst(false);
     Node cnd = d_nodeManager->mkNode(XOR, tb, eb);
 
-    auto traversal = NodeDfsIterable(cnd).inPreorder();
+    auto traversal = NodeDfsIterable(cnd, VisitOrder::PREORDER);
     NodeDfsIterator i = traversal.begin();
     NodeDfsIterator end = traversal.end();
     TS_ASSERT_EQUALS(*(i++), cnd);
@@ -241,7 +241,7 @@ class NodePreorderTraversalBlack : public CxxTest::TestSuite
     Node cnd = d_nodeManager->mkNode(XOR, tb, eb);
 
     size_t count = 0;
-    for (auto i : NodeDfsIterable(cnd).inPreorder())
+    for (auto i : NodeDfsIterable(cnd, VisitOrder::PREORDER))
     {
       ++count;
     }
@@ -255,7 +255,7 @@ class NodePreorderTraversalBlack : public CxxTest::TestSuite
     Node cnd = d_nodeManager->mkNode(XOR, tb, eb);
 
     size_t count = 0;
-    for (auto i : NodeDfsIterable(cnd).inPreorder())
+    for (auto i : NodeDfsIterable(cnd, VisitOrder::PREORDER))
     {
       if (i.isConst())
       {
@@ -272,7 +272,7 @@ class NodePreorderTraversalBlack : public CxxTest::TestSuite
     Node cnd = d_nodeManager->mkNode(XOR, tb, eb);
     Node top = d_nodeManager->mkNode(XOR, cnd, cnd);
 
-    auto traversal = NodeDfsIterable(top).inPreorder();
+    auto traversal = NodeDfsIterable(top, VisitOrder::PREORDER);
 
     size_t count = std::count_if(traversal.begin(),
                                  traversal.end(),
@@ -288,7 +288,7 @@ class NodePreorderTraversalBlack : public CxxTest::TestSuite
     Node top = d_nodeManager->mkNode(XOR, cnd, cnd);
     std::vector<TNode> expected = {top, cnd, tb, eb};
 
-    auto traversal = NodeDfsIterable(top).inPreorder();
+    auto traversal = NodeDfsIterable(top, VisitOrder::PREORDER);
 
     std::vector<TNode> actual;
     std::copy(traversal.begin(), traversal.end(), std::back_inserter(actual));
@@ -303,8 +303,8 @@ class NodePreorderTraversalBlack : public CxxTest::TestSuite
     Node top = d_nodeManager->mkNode(XOR, cnd, cnd);
     std::vector<TNode> expected = {top, cnd, eb};
 
-    auto traversal = NodeDfsIterable(top).inPreorder().skipIf(
-        [&tb](TNode n) { return n == tb; });
+    auto traversal = NodeDfsIterable(
+        top, VisitOrder::PREORDER, [&tb](TNode n) { return n == tb; });
 
     std::vector<TNode> actual;
     std::copy(traversal.begin(), traversal.end(), std::back_inserter(actual));