Provide better fix for determining when dummies are needed

opencog/atoms/pattern/PatternLink.cc

@@ -768,6 +768,35 @@ bool PatternLink::is_virtual(const Handle& clause)
 
 /* ================================================================= */
 
+/// Return true if the term has variables in it that do not yet appear
+/// in any non-evaluatable mandatory clause. If there are such
+/// variables, we either have to find non-evaluatable terms that hold
+/// them, or add those variables directly, as mandatory terms.
+bool PatternLink::need_dummies(const PatternTermPtr& ptm)
+{
+	// Are there any variables below us?
+	// Do they already appear in some existing mandatory term?
+	// If not, then we have to go fishing for fixed terms,
+	// or add dummies.
+	HandleSet vset = get_free_variables(ptm->getHandle());
+	for (const Handle& v: vset)
+	{
+		bool found_this_v = false;
+		for (const PatternTermPtr& man : _pat.pmandatory)
+		{
+			HandleSet vman = get_free_variables(man->getHandle());
+			if (vman.end() != vman.find(v))
+			{
+				found_this_v = true;
+				break;
+			}
+		}
+		if (not found_this_v)
+			return true;
+	}
+	return false;
+}
+
 /// Add implicit fixed terms. The pattern might consist only of
 /// evaluatable clauses. Unless we find some constant Atom inside of
 /// it, we risk having to perform a very large search trying to find
@@ -1128,21 +1157,12 @@ void PatternLink::make_term_tree_recursive(const PatternTermPtr& root,
 	if ((parent->getHandle() == nullptr or parent->hasEvaluatable())
 	    and not ptm->isQuoted() and can_evaluate(h))
 	{
-
-		// Yuck. Ugly, ugly hack to pass the FowardChainerUTest.
-		// Need to remove this ASAP.
-		bool already_have_fixed = false;
-		for (const PatternTermPtr& man : _pat.pmandatory)
-			if (not man->hasAnyEvaluatable())
-			{
-				already_have_fixed = true;
-				break;
-			}
+		if (not need_dummies(ptm)) return;
 
 		// If its an AndLink, make sure that all of the children are
 		// evaluatable. The problem is .. users insert AndLinks into
 		// random places...
-		if (not already_have_fixed and AND_LINK == t)
+		if (AND_LINK == t)
 		{
 			for (const PatternTermPtr& ptc : ptm->getOutgoingSet())
 				if (ptc->isQuoted() or not can_eval_or_present(ptc->getHandle()))
@@ -1153,7 +1173,7 @@ void PatternLink::make_term_tree_recursive(const PatternTermPtr& root,
 		}
 
 		// If the evaluatables have literal-ish subterms, add those.
-		if (not already_have_fixed and add_unaries(ptm)) return;
+		if (add_unaries(ptm)) return;
 
 		// If the above failed, then try adding dummy variables.
 		add_dummies(ptm);

opencog/atoms/pattern/PatternLink.h

@@ -133,8 +133,9 @@ class PatternLink : public PrenexLink
 
 	void locate_cacheable(const PatternTermSeq& clauses);
 
-	void add_dummies(const PatternTermPtr&);
+	bool need_dummies(const PatternTermPtr&);
 	bool add_unaries(const PatternTermPtr&);
+	void add_dummies(const PatternTermPtr&);
 
 	void make_connectivity_map(void);
 	void make_map_recursive(const Handle&, const PatternTermPtr&);