enforce append-like semantics for *Container more consistently

The copy-on-write/rowCache changes require that functions that
modify containers be able to generate new containers. Once that
became possible, some significant pool of other operations
started relying on it -- for instance, operations might return
a new container even though they're in theory "in place" operations.

I developed a tool for checking for unused function return
values (github.com/molecula/noticeme), and ran it on this, and
picked out the places where `*Container` values were generated
but not used, and some of them seem to be potentially-real
bugs, and a few are probably harmless. Updated code to make
those diagnostics go away.

go.sum

@@ -328,6 +328,7 @@ golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGm
 golang.org/x/tools v0.0.0-20190226205152-f727befe758c/go.mod h1:9Yl7xja0Znq3iFh3HoIrodX9oNMXvdceNzlUR8zjMvY=
 golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs=
 golang.org/x/tools v0.0.0-20190524140312-2c0ae7006135/go.mod h1:RgjU9mgBXZiqYHBnxXauZ1Gv1EHHAz9KjViQ78xBX0Q=
+golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e h1:aZzprAO9/8oim3qStq3wc1Xuxx4QmAGriC4VU4ojemQ=
 golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
 golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=

roaring/btree_test.go

@@ -440,7 +440,7 @@ func benchmarkGetSeq(b *testing.B, n int) {
 	b.ReportAllocs()
 	for i := 0; i < b.N; i++ {
 		for j := 0; j < n; j++ {
-			r.Get(uint64(j))
+			_, _ = r.Get(uint64(j))
 		}
 	}
 	b.StopTimer()
@@ -518,7 +518,7 @@ func benchmarkGetRnd(b *testing.B, n int) {
 	b.ReportAllocs()
 	for i := 0; i < b.N; i++ {
 		for _, v := range a {
-			r.Get(uint64(v))
+			_, _ = r.Get(uint64(v))
 		}
 	}
 	b.StopTimer()

roaring/containers_btree.go

@@ -188,7 +188,7 @@ func (btc *bTreeContainers) Repair() {
 // (new-container, write). If write is true, the container is used to
 // replace the given container.
 func (btc *bTreeContainers) Update(key uint64, fn func(*Container, bool) (*Container, bool)) {
-	btc.tree.Put(key, fn)
+	_, _ = btc.tree.Put(key, fn)
 	btc.lastKey = ^uint64(0)
 	btc.lastContainer = nil
 }

roaring/roaring.go

@@ -1568,7 +1568,7 @@ func (b *Bitmap) Shift(n int) (*Bitmap, error) {
 		}
 		o, carry := shift(ci)
 		if lastCarry {
-			o.add(0)
+			o, _ = o.add(0)
 		}
 		if o.N() > 0 {
 			output.Containers.Put(ki, o)
@@ -4543,7 +4543,7 @@ func unionRunRun(a, b *Container) *Container {
 	}
 	output.setN(n)
 	if len(output.runs()) > runMaxSize {
-		output.runToBitmap()
+		output = output.runToBitmap()
 	}
 	return output
 }
@@ -5072,14 +5072,14 @@ func differenceArrayArray(a, b *Container) *Container {
 	for i, j := 0, 0; i < na; {
 		va := aa[i]
 		if j >= nb {
-			output.add(va)
+			output, _ = output.add(va)
 			i++
 			continue
 		}
 
 		vb := ab[j]
 		if va < vb {
-			output.add(va)
+			output, _ = output.add(va)
 			i++
 		} else if va > vb {
 			j++
@@ -6482,14 +6482,15 @@ func (b *Bitmap) DifferenceInPlace(others ...*Bitmap) {
 			if targetKey == iKey {
 				// note: a nil container is valid, and has N == 0.
 				if iContainer.N() != 0 {
-					if curContainer.frozen() {
-						curContainer = curContainer.Clone()
-						b.Containers.Put(targetKey, curContainer)
-					}
-					curContainer.differenceInPlace(iContainer)
+					// Note: This Thaw() may be unnecessary, but some of the
+					// differenceInPlace code may be assuming the container is
+					// always writable.
+					curContainer = curContainer.Thaw().differenceInPlace(iContainer)
 					if curContainer.N() == 0 {
 						removeContainerKeys = append(removeContainerKeys, targetKey)
 						break
+					} else {
+						b.Containers.Put(targetKey, curContainer)
 					}
 				}
 				iIter.hasNext = iIter.iter.Next()
@@ -6504,43 +6505,44 @@ func (b *Bitmap) DifferenceInPlace(others ...*Bitmap) {
 	target.Containers.Repair()
 }
 
-func (c *Container) differenceInPlace(other *Container) {
+func (c *Container) differenceInPlace(other *Container) *Container {
 	if other == nil {
-		return
+		return c
 	}
 	if other.isArray() {
 		if c.isArray() {
-			differenceArrayArrayInPlace(c, other)
+			return differenceArrayArrayInPlace(c, other)
 		} else if c.isBitmap() {
-			differenceBitmapArrayInPlace(c, other)
+			return differenceBitmapArrayInPlace(c, other)
 		} else if c.isRun() {
-			differenceRunArrayInPlace(c, other)
+			return differenceRunArrayInPlace(c, other)
 		}
 	} else if other.isBitmap() {
 		if c.isArray() {
-			differenceArrayBitmapInPlace(c, other)
+			return differenceArrayBitmapInPlace(c, other)
 		} else if c.isBitmap() {
-			differenceBitmapBitmapInPlace(c, other)
+			return differenceBitmapBitmapInPlace(c, other)
 		} else if c.isRun() {
-			differenceRunBitmapInPlace(c, other)
+			return differenceRunBitmapInPlace(c, other)
 		}
 	} else if other.isRun() {
 		if c.isArray() {
-			differenceArrayRunInPlace(c, other)
+			return differenceArrayRunInPlace(c, other)
 		} else if c.isBitmap() {
-			differenceBitmapRunInPlace(c, other)
+			return differenceBitmapRunInPlace(c, other)
 		} else if c.isRun() {
-			differenceRunRunInPlace(c, other)
+			return differenceRunRunInPlace(c, other)
 		}
 	}
+	return c
 }
 
-func differenceArrayArrayInPlace(c, other *Container) {
+func differenceArrayArrayInPlace(c, other *Container) *Container {
 	statsHit("differenceInPlace/ArrayArray")
 	aa, ab := c.array(), other.array()
 	na, nb := len(aa), len(ab)
 	if na == 0 || nb == 0 {
-		return
+		return c
 	}
 	n := 0
 	for i, j := 0, 0; i < na; {
@@ -6565,15 +6567,16 @@ func differenceArrayArrayInPlace(c, other *Container) {
 	}
 	aa = aa[:n]
 	c.setArray(aa)
+	return c
 }
 
-func differenceArrayBitmapInPlace(c, other *Container) {
+func differenceArrayBitmapInPlace(c, other *Container) *Container {
 	statsHit("differenceInPlace/ArrayBitmap")
 	aa := c.array()
 	n := 0
 	bitmap := other.bitmap()
 	if len(aa) == 0 || len(bitmap) == 0 {
-		return
+		return c
 	}
 	for _, va := range aa {
 		bmidx := va / 64
@@ -6588,16 +6591,17 @@ func differenceArrayBitmapInPlace(c, other *Container) {
 	}
 	aa = aa[:n]
 	c.setArray(aa)
+	return c
 }
 
-func differenceArrayRunInPlace(c, other *Container) {
+func differenceArrayRunInPlace(c, other *Container) *Container {
 	statsHit("differenceInPlace/ArrayRun")
 
 	i := 0 // array index
 	j := 0 // run index
 	aa, rb := c.array(), other.runs()
 	if len(aa) == 0 || len(rb) == 0 {
-		return
+		return c
 	}
 	n := 0
 
@@ -6632,14 +6636,15 @@ func differenceArrayRunInPlace(c, other *Container) {
 	}
 	aa = aa[:n]
 	c.setArray(aa)
+	return c
 }
 
-func differenceBitmapArrayInPlace(c, other *Container) {
+func differenceBitmapArrayInPlace(c, other *Container) *Container {
 	statsHit("differenceInPlace/BitmapArray")
 	bitmap := c.bitmap()
 	ab := other.array()
 	if len(bitmap) == 0 || len(ab) == 0 {
-		return
+		return c
 	}
 
 	n := c.N()
@@ -6651,18 +6656,19 @@ func differenceBitmapArrayInPlace(c, other *Container) {
 	}
 	c.setN(n)
 	if n < ArrayMaxSize {
-		c.bitmapToArray() // With This Work
+		c = c.bitmapToArray() // With This Work
 	}
+	return c
 }
 
-func differenceBitmapBitmapInPlace(c, other *Container) {
+func differenceBitmapBitmapInPlace(c, other *Container) *Container {
 	statsHit("differenceInPlace/BitmapBitmap")
 	// local variables added to prevent BCE checks in loop
 	// see https://go101.org/article/bounds-check-elimination.html
 	a := c.bitmap()
 	b := other.bitmap()
 	if len(a) == 0 || len(b) == 0 {
-		return
+		return c
 	}
 
 	var (
@@ -6677,25 +6683,27 @@ func differenceBitmapBitmapInPlace(c, other *Container) {
 	}
 	c.setN(n)
 	if n < ArrayMaxSize {
-		c.bitmapToArray() // Will this work?
+		c = c.bitmapToArray()
 	}
+	return c
 }
 
-func differenceBitmapRunInPlace(c, other *Container) {
+func differenceBitmapRunInPlace(c, other *Container) *Container {
 	statsHit("differenceInPlace/BitmapRun")
 	if len(c.bitmap()) == 0 {
-		return
+		return c
 	}
 	for _, run := range other.runs() {
 		c.bitmapZeroRange(uint64(run.Start), uint64(run.Last)+1)
 	}
+	return c
 }
 
-func differenceRunArrayInPlace(c, other *Container) {
+func differenceRunArrayInPlace(c, other *Container) *Container {
 	statsHit("differenceInPlace/RunArray")
 	ra, ab := c.runs(), other.array()
 	if len(ra) == 0 || len(ab) == 0 {
-		return
+		return c
 	}
 	runs := make([]Interval16, 0, len(ra))
 	bidx := 0
@@ -6745,14 +6753,14 @@ RUNLOOP:
 	for _, run := range runs {
 		c.n += int32(run.Last-run.Start) + 1
 	}
-	c.optimize()
+	return c.optimize()
 }
 
-func differenceRunBitmapInPlace(c, other *Container) {
+func differenceRunBitmapInPlace(c, other *Container) *Container {
 	statsHit("differenceInPlace/RunBitmap")
 	ra := c.runs()
 	if len(ra) == 0 || len(other.bitmap()) == 0 {
-		return
+		return c
 	}
 	// If a is full, difference is the flip of b.
 	if len(ra) > 0 && ra[0].Start == 0 && ra[0].Last == 65535 {
@@ -6765,7 +6773,7 @@ func differenceRunBitmapInPlace(c, other *Container) {
 		c.setMapped(false)
 		c.setBitmap(bitmap)
 		c.setN(c.count())
-		return
+		return c
 	}
 	runs := make([]Interval16, 0, len(ra))
 	for _, inputRun := range ra {
@@ -6811,18 +6819,19 @@ func differenceRunBitmapInPlace(c, other *Container) {
 		c.n += int32(run.Last-run.Start) + 1
 	}
 	if c.N() < ArrayMaxSize && int32(len(runs)) > c.N()/2 {
-		c.runToArray()
+		c = c.runToArray()
 	} else if len(runs) > runMaxSize {
-		c.runToBitmap()
+		c = c.runToBitmap()
 	}
+	return c
 }
 
-func differenceRunRunInPlace(c, other *Container) {
+func differenceRunRunInPlace(c, other *Container) *Container {
 	statsHit("differenceInPlace/RunRun")
 
 	ra, rb := c.runs(), other.runs()
 	if len(ra) == 0 || len(rb) == 0 {
-		return
+		return c
 	}
 	apos := 0 // current a-run index
 	bpos := 0 // current b-run index
@@ -6882,6 +6891,7 @@ func differenceRunRunInPlace(c, other *Container) {
 	for _, run := range runs {
 		c.n += int32(run.Last-run.Start) + 1
 	}
+	return c
 }
 
 //RBF exports to be reconsidered as we progress

roaring/roaring_internal_test.go

@@ -135,19 +135,19 @@ func TestRunCountRange(t *testing.T) {
 	if cnt != 0 {
 		t.Fatalf("should get 0 from empty container, but got: %v", cnt)
 	}
-	c.add(5)
-	c.add(6)
-	c.add(7)
+	c, _ = c.add(5)
+	c, _ = c.add(6)
+	c, _ = c.add(7)
 
 	cnt = RunCountRange(c.runs(), 2, 9)
 	if cnt != 3 {
 		t.Fatalf("should get 3 from interval within range, but got: %v", cnt)
 	}
 
-	c.add(8)
-	c.add(9)
-	c.add(10)
-	c.add(11)
+	c, _ = c.add(8)
+	c, _ = c.add(9)
+	c, _ = c.add(10)
+	c, _ = c.add(11)
 
 	cnt = RunCountRange(c.runs(), 4, 8)
 	if cnt != 3 {
@@ -199,17 +199,17 @@ func TestRunCountRange(t *testing.T) {
 		t.Fatalf("should get 6 from interval equal to range, but got: %v", cnt)
 	}
 
-	c.add(17)
-	c.add(19)
-	c.add(18)
+	c, _ = c.add(17)
+	c, _ = c.add(19)
+	c, _ = c.add(18)
 
 	cnt = RunCountRange(c.runs(), 1, 22)
 	if cnt != 10 {
 		t.Fatalf("should get 10 from multiple ranges in interval, but got: %v", cnt)
 	}
 
-	c.add(13)
-	c.add(14)
+	c, _ = c.add(13)
+	c, _ = c.add(14)
 
 	cnt = RunCountRange(c.runs(), 6, 18)
 	if cnt != 9 {
@@ -227,17 +227,17 @@ func TestRunContains(t *testing.T) {
 	if c.runContains(5) {
 		t.Fatalf("empty run container should not contain 5")
 	}
-	c.add(5)
+	c, _ = c.add(5)
 	if !c.runContains(5) {
 		t.Fatalf("run container with 5 should contain 5")
 	}
 
-	c.add(6)
-	c.add(7)
+	c, _ = c.add(6)
+	c, _ = c.add(7)
 
-	c.add(9)
-	c.add(10)
-	c.add(11)
+	c, _ = c.add(9)
+	c, _ = c.add(10)
+	c, _ = c.add(11)
 
 	if !c.runContains(10) {
 		t.Fatalf("run container with 10 in second run should contain 10")
@@ -282,7 +282,7 @@ func TestIntersectionCountArrayBitmap3(t *testing.T) {
 	if res.N() != res.count() || res.N() != MaxContainerVal+1 {
 		t.Fatalf("test #2 intersectCountBitmapRun fail orig: %v new: %v exp: %v", res.N(), res.count(), MaxContainerVal+1)
 	}
-	b.bitmapToRun(0)
+	b = b.bitmapToRun(0)
 	res = intersectRunRun(a, b)
 	n := intersectionCountRunRun(a, b)
 	if res.N() != res.count() || res.N() != MaxContainerVal+1 || res.N() != int32(n) {
@@ -613,7 +613,7 @@ func TestIntersectBitmapRunBitmap(t *testing.T) {
 		b.setN(4097)
 		ret := intersectBitmapRun(a, b)
 		if ret.isArray() {
-			ret.arrayToBitmap()
+			ret = ret.arrayToBitmap()
 		}
 		if !reflect.DeepEqual(ret.bitmap(), exp) {
 			t.Fatalf("test #%v expected %v, but got %v", i, exp, ret.bitmap())
@@ -710,9 +710,9 @@ func TestUnionMixed(t *testing.T) {
 			res := union(tt.c1, tt.c2)
 			// convert to array for comparison
 			if res.isBitmap() {
-				res.bitmapToArray()
+				res = res.bitmapToArray()
 			} else if res.isRun() {
-				res.runToArray()
+				res = res.runToArray()
 			}
 			if !reflect.DeepEqual(res.array(), tt.exp) {
 				t.Fatalf("test %s expected %v, but got %v", tt.name, tt.exp, res.array())
@@ -1304,11 +1304,11 @@ func TestBitmapToRun(t *testing.T) {
 	for i, test := range tests {
 		a := NewContainerBitmap(-1, test.bitmap)
 		x := a.bitmap()
-		a.bitmapToRun(0)
+		a = a.bitmapToRun(0)
 		if !reflect.DeepEqual(a.runs(), test.exp) {
 			t.Fatalf("test #%v expected %v, but got %v", i, test.exp, a.runs())
 		}
-		a.runToBitmap()
+		a = a.runToBitmap()
 		if !reflect.DeepEqual(a.bitmap(), x) {
 			t.Fatalf("test #%v expected %v, but got %v", i, a.bitmap(), x)
 		}
@@ -1633,7 +1633,7 @@ func MakeBitmap(start []uint64) []uint64 {
 func MakeLastBitSet() []uint64 {
 	obj := NewFileBitmap(65535)
 	c := obj.container(0)
-	c.arrayToBitmap()
+	c = c.arrayToBitmap()
 	return c.bitmap()
 }
 
@@ -2924,8 +2924,7 @@ func unionInPlaceWrapper(a, b *Container) *Container {
 
 func differenceInPlaceWrapper(a, b *Container) *Container {
 	a = a.Clone()
-	// this should probably return its new value, but currently does not
-	a.differenceInPlace(b)
+	a = a.differenceInPlace(b)
 	return a
 }
 
@@ -3859,7 +3858,7 @@ func BenchmarkUnionBitmapBitmapInPlace(b *testing.B) {
 	b1 := newTestBitmapContainer()
 	b2 := newTestBitmapContainer()
 	for n := 0; n < b.N; n++ {
-		unionBitmapBitmapInPlace(b1, b2)
+		b1 = unionBitmapBitmapInPlace(b1, b2)
 	}
 }
 
@@ -4356,7 +4355,7 @@ func BenchmarkUnionRunRunInPlace(bm *testing.B) {
 					brun := doContainer(ContainerRun, br.fn())
 
 					abmp := arun.runToBitmap()
-					unionBitmapRunInPlace(abmp, brun)
+					_ = unionBitmapRunInPlace(abmp, brun)
 				}
 			})
 
@@ -4365,7 +4364,7 @@ func BenchmarkUnionRunRunInPlace(bm *testing.B) {
 					arun := doContainer(ContainerRun, ar.fn())
 					brun := doContainer(ContainerRun, br.fn())
 
-					unionRunRunInPlace(arun, brun)
+					_ = unionRunRunInPlace(arun, brun)
 				}
 			})
 		}

roaring/unmarshal_binary.go

@@ -60,7 +60,7 @@ func (b *Bitmap) UnmarshalBinary(data []byte) (err error) {
 		}
 		newC.setMapped(true)
 		if !b.preferMapping {
-			newC.unmapOrClone()
+			newC = newC.unmapOrClone()
 		}
 		b.Containers.Put(itrKey, newC)
 		itrKey, itrCType, itrN, itrLen, itrPointer, itrErr = itr.Next()
@@ -152,7 +152,7 @@ func InspectBinary(data []byte, mapped bool, info *BitmapInfo) (b *Bitmap, mappe
 		}
 		newC.setMapped(true)
 		if !mapped {
-			newC.unmapOrClone()
+			newC = newC.unmapOrClone()
 		}
 		newC.flags |= flagPristine
 		if newC.flags&flagMapped != 0 {