Panic when trying to use uninitialized or deleted Pigosat objects

optimize_test.go

@@ -67,7 +67,7 @@ func checkFeasibleRecord(t *testing.T, v parameters, args <-chan arguments) {
 		// Each call to IsFeasible is paried with a go RecordSolution. Thus
 		// we're looking for pairs of arguments.
 		if count%2 == 0 {
-			if arg.status == NotReady { // sentinel
+			if arg.status == -1 { // sentinel
 				return
 			}
 			last = arg
@@ -91,7 +91,7 @@ func TestMinimize(t *testing.T) {
 				m := newMinimizer(lo, hi, opt, t)
 				go checkFeasibleRecord(t, m.params, m.args)
 				min, optimal, feasible := Minimize(m)
-				m.args <- arguments{status: NotReady} // sentinel
+				m.args <- arguments{status: -1} // sentinel
 				if opt <= hi && min != opt {
 					t.Errorf("%+v: min=%d", m.params, min)
 				}

pigosat.go

@@ -31,10 +31,6 @@ func PicosatVersion() string { return "960" }
 
 // Return values for Pigosat.Solve's status.
 const (
-	// NotReady is a solver status only used in PiGoSAT and it means the
-	// underlying data structures of the solver have not been initialized or
-	// their memory was previously freed.
-	NotReady = -1
 	// PicoSAT cannot determine the satisfiability of the formula.
 	Unknown = 0
 	// The formula is satisfiable.
@@ -45,6 +41,11 @@ const (
 
 // Struct Pigosat must be created with NewPigosat and stores the state of the
 // solver. Once initialized by NewPigosat, it is safe for concurrent use.
+//
+// You do not need to call Delete on your Pigosat object (unless you plan to use
+// runtime.SetFinalizer). However if you do, or if you create a Pigosat object
+// without NewPigosat (don't do that), all Pigosat methods will panic (except
+// Delete, which will be a no-op).
 type Pigosat struct {
 	// Pointer to the underlying C struct.
 	p    *C.PicoSAT
@@ -135,14 +136,22 @@ func NewPigosat(options *Options) (*Pigosat, error) {
 // Delete may be called when you are done using a Pigosat instance, after which
 // it cannot be used again. However, you only need to call this method if the
 // instance's finalizer was reset using runtime.SetFinalizer (if you're not
-// sure, it's always safe to call Delete again). Most users will not need this
+// sure, it's always safe to call Delete again*). Most users will not need this
 // method.
+//
+// *Delete called on an uninitialized or already-Deleted object is a no-op,
+// unlike all other Pigosat methods, which cannot be called after Delete.
 func (p *Pigosat) Delete() {
-	if p == nil || p.p == nil {
+	// Don't use p.ready() here so that if you use Delete, you don't have to
+	// also do runtime.SetFinalizer(p, nil)
+	if p == nil {
 		return
 	}
 	p.lock.Lock()
 	defer p.lock.Unlock()
+	if p.p == nil {
+		return
+	}
 	// void picosat_reset (PicoSAT *);
 	C.picosat_reset(p.p)
 	p.p = nil
@@ -152,37 +161,47 @@ func (p *Pigosat) Delete() {
 	runtime.SetFinalizer(p, nil)
 }
 
+// ready readies a Pigosat object for use in a public method. It obtains the
+// appropriate lock and returns the appropriate unlocking method, so it can be
+// used like
+//     defer p.ready(readonly)()
+// where readonly should be true if the method does not write to p and must be
+// false if the method does write to p. If p is uninitialized or deleted,
+// ready panics.
+func (p *Pigosat) ready(readonly bool) (unlock func()) {
+	if readonly {
+		p.lock.RLock()
+		unlock = p.lock.RUnlock
+	} else {
+		p.lock.Lock()
+		unlock = p.lock.Unlock
+	}
+	if p.p == nil {
+		defer unlock()
+		panic("Attempted to use a deleted Pigosat object")
+	}
+	return
+}
+
 // Variables returns the number of variables in the formula: The m in the DIMACS
 // header "p cnf <m> n".
 func (p *Pigosat) Variables() int {
-	if p == nil || p.p == nil {
-		return 0
-	}
-	p.lock.RLock()
-	defer p.lock.RUnlock()
+	defer p.ready(true)()
 	// int picosat_variables (PicoSAT *);
 	return int(C.picosat_variables(p.p))
 }
 
 // AddedOriginalClauses returns the number of clauses in the formula: The n in
 // the DIMACS header "p cnf m <n>".
 func (p *Pigosat) AddedOriginalClauses() int {
-	if p == nil || p.p == nil {
-		return 0
-	}
-	p.lock.RLock()
-	defer p.lock.RUnlock()
+	defer p.ready(true)()
 	// int picosat_added_original_clauses (PicoSAT *);
 	return int(C.picosat_added_original_clauses(p.p))
 }
 
 // Seconds returns the time spent in the PicoSAT library.
 func (p *Pigosat) Seconds() time.Duration {
-	if p == nil || p.p == nil {
-		return 0
-	}
-	p.lock.RLock()
-	defer p.lock.RUnlock()
+	defer p.ready(true)()
 	// double picosat_seconds (PicoSAT *);
 	return time.Duration(float64(C.picosat_seconds(p.p)) * float64(time.Second))
 }
@@ -196,11 +215,7 @@ func (p *Pigosat) Seconds() time.Duration {
 // the clause, and causes AddClauses to skip reading the rest of the slice. Nil
 // slices are ignored and skipped.
 func (p *Pigosat) AddClauses(clauses [][]int32) {
-	if p == nil || p.p == nil {
-		return
-	}
-	p.lock.Lock()
-	defer p.lock.Unlock()
+	defer p.ready(false)()
 	var had0 bool
 	for _, clause := range clauses {
 		if len(clause) == 0 {
@@ -244,11 +259,7 @@ func (p *Pigosat) blocksol(sol []bool) {
 //        // Do stuff with status, solution
 //    }
 func (p *Pigosat) Solve() (status int, solution []bool) {
-	if p == nil || p.p == nil {
-		return NotReady, nil
-	}
-	p.lock.Lock()
-	defer p.lock.Unlock()
+	defer p.ready(false)()
 	// int picosat_sat (PicoSAT *, int decision_limit);
 	status = int(C.picosat_sat(p.p, -1))
 	if status == Unsatisfiable || status == Unknown {
@@ -277,11 +288,7 @@ func (p *Pigosat) Solve() (status int, solution []bool) {
 // length. There is no need to call BlockSolution after calling Pigosat.Solve,
 // which calls it automatically for every Satisfiable solution.
 func (p *Pigosat) BlockSolution(solution []bool) error {
-	if p == nil || p.p == nil {
-		return nil
-	}
-	p.lock.Lock()
-	defer p.lock.Unlock()
+	defer p.ready(false)()
 	if n := int(C.picosat_variables(p.p)); len(solution) != n+1 {
 		return fmt.Errorf("solution length %d, but have %d variables",
 			len(solution), n)

pigosat_test.go

@@ -303,32 +303,38 @@ func TestMeasureAllCalls(t *testing.T) {
 	}
 }
 
-// Test that nil method calls are no ops
-func TestNil(t *testing.T) {
+// Assert that function f panics when called. test is a string identifying which
+// input data are being tested. method is a string identifying which method is
+// being tested in f.
+func assertPanics(t *testing.T, test, method string, f func()) {
+	defer func() {
+		if r := recover(); r == nil {
+			t.Errorf("Test %s: %s failed to panic", test, method)
+		}
+	}()
+	f()
+}
+
+// Test that method calls on uninitialized or deleted objects panic
+// (except Delete, which is a no-op)
+func TestUninitializedOrDeleted(t *testing.T) {
 	var a, b *Pigosat
 	b, _ = NewPigosat(nil)
 	b.Delete()
 	for name, p := range map[string]*Pigosat{"uninit": a, "deleted": b} {
-		// No panicking
-		p.Delete()
-		p.AddClauses([][]int32{{1}, {2}})
-		if p.Variables() != 0 {
-			t.Errorf("Test %s: Expected 0 vars, got %d", name, p.Variables())
-		}
-		if c := p.AddedOriginalClauses(); c != 0 {
-			t.Errorf("Test %s: Expected 0 clauses, got %d", name, c)
-		}
-		if p.Seconds() != 0 {
-			t.Errorf("Test %s: Expected 0 seconds, got %v", name, p.Seconds())
-		}
-		if status, solution := p.Solve(); status != NotReady || solution != nil {
-			t.Errorf("Test %s: Expected status %d and nil solution, got %d and %v",
-				name, NotReady, status, solution)
-		}
-		if err := p.BlockSolution([]bool{}); err != nil {
-			t.Errorf("Test %s: Expected no-op BlockSolution, got error: %v",
-				name, err)
-		}
+		assertPanics(t, name, "AddClauses", func() {
+			p.AddClauses([][]int32{{1}, {2}})
+		})
+		assertPanics(t, name, "Variables", func() { p.Variables() })
+		assertPanics(t, name, "AddedOriginalClauses", func() {
+			p.AddedOriginalClauses()
+		})
+		assertPanics(t, name, "Seconds", func() { p.Seconds() })
+		assertPanics(t, name, "Solve", func() { p.Solve() })
+		assertPanics(t, name, "BlockSolution", func() {
+			p.BlockSolution([]bool{})
+		})
+		p.Delete() // No panicking; do last for clean uninitialized-object test
 	}
 }