Invoke callback for partial rule in chain. - #273
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates ds::chain_t::execute to invoke the provided callback not only for newly-derived facts, but also for newly-generated intermediate (“partial”) rules produced during chaining.
Changes:
- Track newly-generated partial rules during a single
execute()run via atemp_rulesset (deduplication). - Invoke the
callbackwhen a new partial rule is encountered (in addition to the existing fact-callback behavior).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (callback(rule)) { | ||
| break_all = true; | ||
| } | ||
| } while (false); | ||
| } |
There was a problem hiding this comment.
When callback(rule) returns true here, break_all is set but chain_recursive continues and still iterates over facts below (and can recurse further). This violates the documented “stop searching” behavior and differs from the premises_count()==0 branch which returns immediately. Consider returning immediately (and/or checking break_all before the facts loop / before recursing) once the callback requests termination.
| auto new_rule = std::unique_ptr<rule_t>(reinterpret_cast<rule_t*>(operator new(rule->data_size()))); | ||
| memcpy(new_rule->head(), rule->head(), rule->data_size()); | ||
| if (rules.find(new_rule) != rules.end() || temp_rules.find(new_rule) != temp_rules.end()) { | ||
| break; | ||
| } | ||
| temp_rules.emplace(std::move(new_rule)); | ||
| if (callback(rule)) { | ||
| break_all = true; | ||
| } |
There was a problem hiding this comment.
This block allocates/copies new_rule and stores it in temp_rules to deduplicate callbacks, but then invokes callback(rule) instead of using the stable copied instance. For intermediate rules, rule can point into the scratch buffer and may be overwritten later in the search, which is risky if callers retain the pointer. Prefer invoking the callback with the stored copy (or otherwise documenting/enforcing that the pointer is only valid during the callback).
| temp_rules.emplace(std::move(new_rule)); | ||
| if (callback(rule)) { | ||
| break_all = true; | ||
| } |
There was a problem hiding this comment.
New behavior: callbacks are now invoked for newly-generated partial rules (non-zero premises) and deduplicated via temp_rules. There are existing chain_t tests, but none assert that partial-rule callbacks fire (or fire only once) and that callback returning true stops the search promptly. Adding a focused unit test would help prevent regressions in this new observable behavior.
No description provided.