Formalize solver state machine in propt and decision_proceduret#8852
Formalize solver state machine in propt and decision_proceduret#8852tautschnig wants to merge 4 commits intodiffblue:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR formalizes and enforces a solver/decision-procedure “state machine” to make model/conflict queries fail-fast when called in invalid states, and to consistently invalidate solver results when the underlying solver is mutated.
Changes:
- Add
propt::statustand track solver state transitions inprop_solve()plus invalidation viaclear_status(). - Add
decision_proceduret::get_status()tracking of the latest solve result, and document the intended state machine. - Update SAT solver backends to call
clear_status()atlcnf()entry; add CaDiCaL unit tests and CaDiCaL precondition checks for state-dependent APIs.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| unit/solvers/sat/satcheck_cadical.cpp | Adds unit tests for propt state transitions using CaDiCaL. |
| src/solvers/sat/satcheck_picosat.cpp | Invalidates propt solver state on clause addition (lcnf). |
| src/solvers/sat/satcheck_minisat2.cpp | Invalidates propt solver state on clause addition (lcnf). |
| src/solvers/sat/satcheck_minisat.cpp | Invalidates propt solver state on clause addition (lcnf). |
| src/solvers/sat/satcheck_lingeling.cpp | Invalidates propt solver state on clause addition (lcnf). |
| src/solvers/sat/satcheck_ipasir.cpp | Invalidates propt solver state on clause addition (lcnf). |
| src/solvers/sat/satcheck_glucose.cpp | Invalidates propt solver state on clause addition (lcnf). |
| src/solvers/sat/satcheck_cadical.cpp | Enforces SAT/UNSAT-only preconditions for model/conflict queries; invalidates state on lcnf. |
| src/solvers/sat/satcheck_booleforce.cpp | Invalidates propt solver state on clause addition (lcnf). |
| src/solvers/sat/dimacs_cnf.cpp | Invalidates propt solver state on clause “addition” for DIMACS dumping. |
| src/solvers/sat/cnf_clause_list.cpp | Invalidates propt solver state on clause addition to clause-list CNF. |
| src/solvers/sat/cnf.h | Documents legacy vs canonical status tracking (cnf_solvert::status vs propt::solver_state). |
| src/solvers/prop/prop.h | Documents propt state machine; adds statust, get_status(), and invalidation hooks in mutating APIs. |
| src/solvers/prop/prop.cpp | Updates prop_solve() to set solver_state based on solve result. |
| src/solvers/decision_procedure.h | Documents decision procedure state machine; adds get_status() / latest_result. |
| src/solvers/decision_procedure.cpp | Tracks latest solve result in operator(). |
Comments suppressed due to low confidence (1)
src/solvers/decision_procedure.cpp:43
- Per the documented state machine, adding constraints should transition back to UNKNOWN, but
set_to_true/falsedon’t invalidatelatest_result. After a SAT result, callers can add constraints andget_status()will still reportD_SATISFIABLEeven though the model is no longer valid. Consider resettinglatest_result(or a dedicated UNKNOWN status) in these wrappers, and ensure other mutation entry points (e.g.,handle()or directset_to()implementations) also invalidate the status consistently.
void decision_proceduret::set_to_true(const exprt &expr)
{
set_to(expr, true);
}
void decision_proceduret::set_to_false(const exprt &expr)
{
set_to(expr, false);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add a statust enum (UNKNOWN, SAT, UNSAT, ERROR) to propt that tracks the solver state. prop_solve() sets the state based on the result, and mutations (l_set_to, lcnf) transition back to UNKNOWN. This formalises the contract that l_get() is only valid after a SAT result, and is_in_conflict() only after UNSAT. Similarly, decision_proceduret now tracks its latest result, accessible via get_status(), documenting that get() is only valid after D_SATISFIABLE. The cnf_solvert::statust enum is retained for now as legacy state tracking in do_prop_solve implementations; the canonical state is the new propt::solver_state. As an exemplar, satcheck_cadical's lcnf() calls clear_status() to transition back to UNKNOWN when clauses are added. Other solver backends can follow the same pattern. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
All SAT solver backends now call clear_status() at the start of their lcnf() method, transitioning the solver state from SAT/UNSAT back to UNKNOWN when new clauses are added. This ensures the state machine documented in propt is consistently enforced across all backends. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
l_get() now requires the solver to be in the SAT state, and is_in_conflict() requires the UNSAT state. This enforces the state machine contract documented in propt. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
75ef883 to
99e1c5f
Compare
Replace the TO_BE_DOCUMENTED placeholder with a description of the class purpose, its API surface, and its incremental-use model. Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8852 +/- ##
========================================
Coverage 80.01% 80.01%
========================================
Files 1700 1700
Lines 188338 188373 +35
Branches 73 73
========================================
+ Hits 150695 150723 +28
- Misses 37643 37650 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
As suggested by @kroening in #8849, this formalizes the solver state machine in
proptanddecision_proceduret.Commit 1: Document and track solver state machine
propt::statustenum (UNKNOWN,SAT,UNSAT,ERROR) and tracks state transitions:prop_solve()sets state based on resultl_set_to()andlcnf()overloads callclear_status()to transition SAT/UNSAT → UNKNOWNdecision_proceduret::get_status()trackinglatest_resultCommit 2: Call
clear_status()in all solver backendslcnf()implementations now callclear_status()at entry, ensuring the state machine is consistently enforced across all backends.Commit 3: Enforce state preconditions
l_get()requiresSATstate,is_in_conflict()requiresUNSATstate (viaPRECONDITION)Note:
cnf_solvert::statustis retained for now (marked as legacy/superseded) since it is used by alldo_prop_solveimplementations. Unifying the two can be done as a follow-up.