improve invalid prod. diagnostics, untangle ebc

It seems like the reording logic may have lived in ebc at one time, though I
can't be sure about that. The logic that checks and warns for ungrounded
productions generated warnings related to chunking and used chunking trace
settings, even though it's fully possible to trigger these from regular `sp`
commands (and there is an additional call in RL, as well).

Move all of the chunking-related warnings and settings back to ebc. Instead of
setting `m_failure_type` on the agent's chunking manager, return the error code
from `reorder_and_validate_lhs_and_rhs` and check that in ebc to trigger logging
and agent stopping logic. Place the logic moved there into a new dedicated
method. Update other consumers of this method to check for `reorder_success`
instead of `true`, as well.

Put the reorder failure logging behind `OM_WARNINGS`, which is on by default and
is (correctly) not specific to chunking/ebc.

Rename `EBCFailureType` and `display_ebc_error` to reflect that fact that they
they aren't chunking-specific.

Update unit tests to check the warnings printed for ungrounded productions.

Core/SoarKernel/src/explanation_based_chunking/ebc.cpp

@@ -99,7 +99,6 @@ void Explanation_Based_Chunker::reinit()
     m_prod_name                         = NULL;
     chunk_free_problem_spaces           = NIL;
     chunky_problem_spaces               = NIL;
-    m_failure_type                      = ebc_success;
     m_rule_type                         = ebc_no_rule;
     m_learning_on_for_instantiation     = ebc_settings[SETTING_EBC_LEARNING_ON];
 }

Core/SoarKernel/src/explanation_based_chunking/ebc.h

@@ -101,7 +101,6 @@ class Explanation_Based_Chunker
         /* Determines whether learning is on for a particular instantiation
          * based on the global learning settings and whether the state chunky */
         bool set_learning_for_instantiation(instantiation* inst);
-        void set_failure_type(EBCFailureType pFailure_type) {m_failure_type = pFailure_type; };
         void set_rule_type(ebc_rule_type pRuleType) {m_rule_type = pRuleType; };
         void reset_chunks_this_d_cycle() { chunks_this_d_cycle = 0; justifications_this_d_cycle = 0;};
 
@@ -200,7 +199,6 @@ class Explanation_Based_Chunker
         Symbol*             m_prod_name;
         ProductionType      m_prod_type;
         bool                m_should_print_name, m_should_print_prod;
-        EBCFailureType      m_failure_type;
 
         /* Core tables used by EBC during identity assignment during instantiation
          * creation. The data stored within them is temporary and cleared after use. */
@@ -250,6 +248,7 @@ class Explanation_Based_Chunker
         void            remove_chunk_instantiation();
         void            remove_from_chunk_cond_set(chunk_cond_set* set, chunk_cond* cc);
         bool            reorder_and_validate_chunk();
+        void            report_reorder_errors(ProdReorderFailureType reorder_result);
         void            deallocate_failed_chunk();
         void            clean_up(uint64_t pClean_up_id, soar_timer* pTimer = NULL);
         bool            add_chunk_to_rete();

Core/SoarKernel/src/explanation_based_chunking/ebc_build.cpp

@@ -772,7 +772,6 @@ void Explanation_Based_Chunker::learn_rule_from_instance(instantiation* inst, in
     }
 
     m_inst = inst;
-    m_failure_type = ebc_success;
 
     if (!can_learn_from_instantiation()) { m_inst = NULL; return; }
 
@@ -1024,7 +1023,6 @@ void Explanation_Based_Chunker::clean_up (uint64_t pClean_up_id, soar_timer* pTi
     m_chunk_inst                        = NULL;
     m_prod_name                         = NULL;
     m_rule_type                         = ebc_no_rule;
-    m_failure_type                      = ebc_success;
 
     clear_symbol_identity_map();
     if (ebc_settings[SETTING_EBC_LEARNING_ON])

Core/SoarKernel/src/explanation_based_chunking/ebc_repair.cpp

@@ -19,7 +19,7 @@ void delete_ungrounded_symbol_list(agent* thisAgent, matched_symbol_list** uncon
 {
     matched_symbol_list* lSyms = *unconnected_syms;
     chunk_element* lSym;
-   
+
     //for(matched_symbol_list::iterator it = lSyms->begin(), end = lSyms->end(); it != end; ++it)
     for (auto it = lSyms->begin(); it != lSyms->end(); it++)
     {
@@ -306,11 +306,12 @@ bool Explanation_Based_Chunker::reorder_and_validate_chunk()
 {
     matched_symbol_list* unconnected_syms = new matched_symbol_list();
 
-    reorder_and_validate_lhs_and_rhs(thisAgent, &m_lhs, &m_rhs, false, unconnected_syms, true, true);
+    auto reorder_result = reorder_and_validate_lhs_and_rhs(thisAgent, &m_lhs, &m_rhs, false, unconnected_syms, true, true);
 
-    if (m_failure_type != ebc_success)
+    if (reorder_result != reorder_success)
     {
-        if ((m_failure_type == ebc_failed_unconnected_conditions) || (m_failure_type == ebc_failed_reordering_rhs))
+        report_reorder_errors(reorder_result);
+        if ((reorder_result == reorder_failed_unconnected_conditions) || (reorder_result == reorder_failed_reordering_rhs))
         {
             thisAgent->outputManager->display_soar_feedback(thisAgent, ebc_progress_repairing, thisAgent->trace_settings[TRACE_CHUNKS_WARNINGS_SYSPARAM]);
 
@@ -320,7 +321,7 @@ bool Explanation_Based_Chunker::reorder_and_validate_chunk()
             delete_ungrounded_symbol_list(thisAgent, &unconnected_syms);
             unconnected_syms = new matched_symbol_list();
             thisAgent->outputManager->display_soar_feedback(thisAgent, ebc_progress_validating, thisAgent->trace_settings[TRACE_CHUNKS_WARNINGS_SYSPARAM]);
-            if (reorder_and_validate_lhs_and_rhs(thisAgent, &m_lhs, &m_rhs, false, unconnected_syms, false, false))
+            if (reorder_and_validate_lhs_and_rhs(thisAgent, &m_lhs, &m_rhs, false, unconnected_syms, false, false) == reorder_success)
             {
                 delete_ungrounded_symbol_list(thisAgent, &unconnected_syms);
                 if (thisAgent->trace_settings[TRACE_CHUNKS_WARNINGS_SYSPARAM])
@@ -341,3 +342,41 @@ bool Explanation_Based_Chunker::reorder_and_validate_chunk()
     return true;
 }
 
+void Explanation_Based_Chunker::report_reorder_errors(ProdReorderFailureType reorder_result) {
+    const char* message;
+    switch (reorder_result) {
+        case reorder_failed_reordering_rhs: {
+            message = "Chunking issue detected. Soar has learned a rule with ungrounded action(s). Repair required.";
+            break;
+        }
+        case reorder_failed_unconnected_conditions: {
+            message = "Chunking issue detected. Soar has learned a rule with with ungrounded condition(s). Repair required.\n"
+                "   This is likely caused by a condition that tested a working memory element \n"
+                "   that was created in the sub-state but later became connected to the \n"
+                "   super-state because it was a child of an identifier that was an element\n"
+                "   of a previous result in that same sub-state.";
+            break;
+        }
+        case reorder_failed_no_roots: {
+            message = "Chunking issue detected. Soar has learned a rule with no conditions that match a goal state.";
+            break;
+        }
+        case reorder_failed_negative_relational_test_bindings: {
+            message = "Chunking issue detected. Soar has learned a rule with negative relational test bindings.";
+            break;
+        }
+        case reorder_success: {
+            // this is checked in the caller and should never happen
+            return;
+        }
+    }
+    if (thisAgent->trace_settings[TRACE_CHUNKS_WARNINGS_SYSPARAM])
+    {
+        print_current_built_rule(message);
+    }
+    if (ebc_settings[SETTING_EBC_INTERRUPT_WARNING])
+    {
+        thisAgent->stop_soar = true;
+        thisAgent->reason_for_stopping = message;
+    }
+}

Core/SoarKernel/src/output_manager/output_errors.cpp

@@ -17,50 +17,42 @@
 #include "ebc.h"
 #include "xml.h"
 
-void Output_Manager::display_ebc_error(agent* thisAgent, EBCFailureType pErrorType, const char* pString1, const char* pString2)
+void Output_Manager::display_reorder_error(agent* thisAgent, ProdReorderFailureType pErrorType, const char* pString1, const char* pString2)
 {
-    if (!thisAgent->trace_settings[TRACE_CHUNKS_WARNINGS_SYSPARAM]) return;
+    if (!thisAgent->outputManager->settings[OM_WARNINGS]) return;
     switch (pErrorType)
     {
-        case ebc_failed_reordering_rhs:
+        case reorder_failed_reordering_rhs:
         {
-//            thisAgent->explanationBasedChunker->print_current_built_rule("Attempted to add an invalid rule:");
-
-            printa_sf(thisAgent,  "%eThe following RHS actions contain variables that are not tested\n"
+            printa_sf(thisAgent,  "%eAttempted to add rule with ungrounded action(s).\n"
+                                  "The following RHS actions contain variables that are not tested\n"
                                   "in a positive condition on the LHS: \n\n"
                                   "%s\n", pString2);
             break;
         }
-        case ebc_failed_unconnected_conditions:
+        case reorder_failed_unconnected_conditions:
         {
-//            thisAgent->explanationBasedChunker->print_current_built_rule("Attempted to add an invalid rule:");
-
             printa_sf(thisAgent,"%eConditions on the LHS contain tests that are not connected \n"
                                 "to a goal: %s\n\n", pString2);
-            printa(thisAgent,   "   This is likely caused by a condition that tested a working memory element \n"
-                                "   that was created in the sub-state but later became connected to the \n"
-                                "   super-state because it was a child of an identifier that was an element\n"
-                                "   of a previous result in that same sub-state.\n");
             break;
         }
-        case ebc_failed_no_roots:
+        case reorder_failed_no_roots:
         {
-            thisAgent->explanationBasedChunker->print_current_built_rule("Attempted to add an invalid rule:");
-//            printa_sf(thisAgent,"\nChunking has created an invalid rule: %s\n\n", pString1);
-            printa(   thisAgent,  "   None of the conditions reference a goal state.\n");
+            printa_sf(thisAgent, "Error: production %s has no positive conditions that reference a goal state.\n"\
+                "Did you forget to add \"^type state\" or \"^superstate nil\"?\n",
+                thisAgent->name_of_production_being_reordered);
             break;
         }
-        case ebc_failed_negative_relational_test_bindings:
+        case reorder_failed_negative_relational_test_bindings:
         {
             thisAgent->explanationBasedChunker->print_current_built_rule("Attempted to add an invalid rule:");
-//            printa_sf(thisAgent,"\nChunking has created an invalid rule: %s\n\n", pString1);
             printa(thisAgent,  "   Unbound relational test in negative condition of rule \n");
             break;
         }
         default:
         {
             thisAgent->explanationBasedChunker->print_current_built_rule("Attempted to add an invalid rule:");
-            printa(thisAgent, "\nUnspecified chunking failure. That's weird.  Should report.\n\n");
+            printa(thisAgent, "\nUnspecified reordering/validation failure. That's weird. Should report.\n\n");
             printa_sf(thisAgent, "        %s\n", pString1);
         }
     }

Core/SoarKernel/src/output_manager/output_manager.h

@@ -237,7 +237,7 @@ class Output_Manager
         /* A single function to print all pre-formatted Soar error messages.  Added
          * to make other code cleaner and easier to parse */
         void display_soar_feedback(agent* thisAgent, SoarCannedMessageType pErrorType, bool shouldPrint = true);
-        void display_ebc_error(agent* thisAgent, EBCFailureType pErrorType, const char* pString1 = NULL, const char* pString2 = NULL);
+        void display_reorder_error(agent* thisAgent, ProdReorderFailureType pErrorType, const char* pString1 = NULL, const char* pString2 = NULL);
         void display_ambiguous_command_error(agent* thisAgent, std::list< std::string > matched_objects_str);
 
         /* -- Should be moved elsewhere -- */

Core/SoarKernel/src/parsing/parser.cpp

@@ -1119,7 +1119,7 @@ condition* parse_tail_of_conds_for_one_id(agent* thisAgent, Lexer* lexer,
     if (lexer->current_lexeme.type == R_PAREN_LEXEME)
     {
         if (is_state_or_impasse_cond) {
-            thisAgent->outputManager->printa_sf(thisAgent,  "Expected attribute-value test "\
+            thisAgent->outputManager->printa_sf(thisAgent,  "Error: Expected attribute-value test "\
                 "after state/impasse test. Did you forget to add \"^type state\" or \"^superstate nil\"?\n");
             return nullptr;
         }
@@ -2460,7 +2460,7 @@ production* parse_production(agent* thisAgent, const char* prod_string, unsigned
     for (lhs_bottom = lhs; lhs_bottom->next != NIL; lhs_bottom = lhs_bottom->next);
 
     thisAgent->name_of_production_being_reordered = name->sc->name;
-    if (!reorder_and_validate_lhs_and_rhs(thisAgent, &lhs_top, &rhs, true))
+    if (reorder_and_validate_lhs_and_rhs(thisAgent, &lhs_top, &rhs, true) != reorder_success)
     {
         abort_parse_production(thisAgent, name, &documentation, &lhs, &rhs);
         return NIL;

Core/SoarKernel/src/reinforcement_learning/reinforcement_learning.cpp

@@ -602,7 +602,7 @@ Symbol* rl_build_template_instantiation(agent* thisAgent, instantiation* my_temp
 
         // make new production
         thisAgent->name_of_production_being_reordered = new_name_symbol->sc->name;
-        if (new_action && reorder_and_validate_lhs_and_rhs(thisAgent, &cond_top, &new_action, false))
+        if (new_action && reorder_and_validate_lhs_and_rhs(thisAgent, &cond_top, &new_action, false) == reorder_success)
         {
             production* new_production = make_production(thisAgent, USER_PRODUCTION_TYPE, new_name_symbol, my_template->name->sc->name, &cond_top, &new_action, false, NULL);
 

Core/SoarKernel/src/shared/enums.h

@@ -230,12 +230,12 @@ enum EBCTraceType {
     ebc_explanation_trace
 };
 
-enum EBCFailureType {
-    ebc_success,
-    ebc_failed_no_roots,
-    ebc_failed_negative_relational_test_bindings,
-    ebc_failed_reordering_rhs,
-    ebc_failed_unconnected_conditions
+enum ProdReorderFailureType {
+    reorder_success,
+    reorder_failed_no_roots,
+    reorder_failed_negative_relational_test_bindings,
+    reorder_failed_reordering_rhs,
+    reorder_failed_unconnected_conditions
 };
 
 enum EBCExplainStatus {

Core/SoarKernel/src/soar_representation/production.cpp

@@ -348,22 +348,22 @@ bool action_is_in_tc(action* a, tc_number tc)
  * so EBC can first try to fix unconnected conditions before creating
  * the production. */
 
-bool reorder_and_validate_lhs_and_rhs(agent*        thisAgent,
-                                      condition**   lhs_top,
-                                      action**      rhs_top,
-                                      bool          reorder_nccs,
-                              matched_symbol_list*  ungrounded_syms,
-                                     bool           add_ungrounded_lhs,
-                                     bool           add_ungrounded_rhs)
+ProdReorderFailureType reorder_and_validate_lhs_and_rhs(agent*        thisAgent,
+                                                        condition**   lhs_top,
+                                                        action**      rhs_top,
+                                                        bool          reorder_nccs,
+                                                matched_symbol_list*  ungrounded_syms,
+                                                        bool          add_ungrounded_lhs,
+                                                        bool          add_ungrounded_rhs)
 {
     tc_number tc;
-    bool lhs_good = false;
 
     thisAgent->symbolManager->reset_variable_generator(*lhs_top, *rhs_top);
     tc = get_new_tc_number(thisAgent);
     add_bound_variables_in_condition_list(thisAgent, *lhs_top, tc, NIL);
 
-    if (! reorder_action_list(thisAgent, rhs_top, tc, ungrounded_syms, add_ungrounded_rhs))
+    auto rhs_reorder_result = reorder_action_list(thisAgent, rhs_top, tc, ungrounded_syms, add_ungrounded_rhs);
+    if (rhs_reorder_result != reorder_success)
     {
         /* If there are problems on the LHS, we need the ungrounded_syms
          * from them, before we return.  So we call, reorder_lhs too.
@@ -372,11 +372,9 @@ bool reorder_and_validate_lhs_and_rhs(agent*        thisAgent,
         {
             reorder_lhs(thisAgent, lhs_top, reorder_nccs, ungrounded_syms);
         }
-        return false;
+        return rhs_reorder_result;
     }
-    lhs_good = reorder_lhs(thisAgent, lhs_top, reorder_nccs, ungrounded_syms, add_ungrounded_lhs);
-    if (!lhs_good) return false;
-    return true;
+    return reorder_lhs(thisAgent, lhs_top, reorder_nccs, ungrounded_syms, add_ungrounded_lhs);
 }
 
 /* Note:  The rete load command will create a production without calling this.  Changes made here may

Core/SoarKernel/src/soar_representation/production.h

@@ -156,13 +156,13 @@ void add_all_variables_in_condition_list(agent* thisAgent, condition* cond_list,
     say.  Normally deallocate_production() should be invoked only via
     the production_remove_ref() macro.
 ------------------------------------------------------------------- */
-bool reorder_and_validate_lhs_and_rhs(agent*                    thisAgent,
-                                      condition**               lhs_top,
-                                      action**                  rhs_top,
-                                      bool                      reorder_nccs,
-                                      matched_symbol_list*      ungrounded_syms = NULL,
-                                      bool                      add_ungrounded_lhs = false,
-                                      bool                      add_ungrounded_rhs = false
+ProdReorderFailureType reorder_and_validate_lhs_and_rhs(agent*                    thisAgent,
+                                                        condition**               lhs_top,
+                                                        action**                  rhs_top,
+                                                        bool                      reorder_nccs,
+                                                        matched_symbol_list*      ungrounded_syms = NULL,
+                                                        bool                      add_ungrounded_lhs = false,
+                                                        bool                      add_ungrounded_rhs = false
 );
 
 production* make_production(agent* thisAgent, ProductionType type,