Logic error in ActionWarehouse set intersection

[dschwen]

Description of the enhancement or error report

ActionWarehouse uses std::set_intersection, wher one of the arguments is task_set defined by

  const auto & ordered_names = _syntax.getSortedTaskSet();
  for (const auto & task_set : ordered_names)

The fact that getSortedTaskSet() returns an std::vector<std::vector<std::string>> is obfuscated by the auto keywords and the fact that the method name is a blatant lie!

std::set_intersection requires the ranges defined by the two iterator sets to be ordered. GCC actually check this now in debug mode, causing @tonkmr's #9379 to fail (https://www.moosebuild.org/job/96999/).

Rationale for the enhancement or information for reproducing the error

Broken code.

Identified impact

Fix bug.

[dschwen]

Broken in 01352b4

[permcody]

I worked with Roy on this and approved the change. It's not an easy fix. The problem (as the original PR states) is that MOOSE relies heavily on order despite the fact that it shouldn't anywhere where it's not explicitly declared. The old behavior created all sorts of nasty behavior with DistributedMesh that was not easy to track down. As we move forward it appears that we may need to rely on implicit ordering for a long time.

I suggest that we rename the method to make it more clear as to what's occurring here. We can't go back to using sets anytime soon.

[permcody]

BTW - If there's a compiler flag to enable extra checks in debug clang, I'd be very interested in adding them. GCC still reins king of debug container checking.

[dschwen]

The obvious fix would be to copy the returned vectors into sets inside the for loop and use those in the intersection.

[permcody]

Right, but you may not realize you aren't getting sets vectors, especially with auto and the misleading name, hence the error. I'm more than willing to change the API since I don't see us going back.

[dschwen]

Looking at the code

const std::vector<std::vector<std::string>> &
Syntax::getSortedTaskSet()
{
  return _tasks.getSortedValuesSets();
}

we need to go deeper.

getSortedValuesSets() actually pretends(?) to return a sorted vector. That function is a beast (@bwspenc , did you write this?). I wonder if it contains a small logic error. It surprises me that this makes only one single (new) test fail.

[permcody]

No I originally wrote this code. I started from stack overflow and then modified it to work with MOOSE. It's been touched by Ben and Roy and many others over the years.

[dschwen]

with

--- a/framework/include/utils/DependencyResolver.h
+++ b/framework/include/utils/DependencyResolver.h
@@ -360,6 +360,8 @@ DependencyResolver<T>::getSortedValuesSets()
     _ordered_items.push_back(next_set);
   }
 
+  if (!std::is_sorted(_ordered_items.begin(), _ordered_items.end()))
+    mooseError("Returned items in DependencyResolver<T>::getSortedValuesSets() are not sorted");
   return _ordered_items;
 }
 

Every. Single. Test. Fails.

[permcody]

Yeah, ordered as in ordered by dependencies, not "ordered" from a "sorted" point of view. The current design is that it's a "stable ordering" meaning that all items remain in their original order unless an explicit dependency is added.

[dschwen]

Looks like nothing is going to happen here.