image/draw: blank images after go1.18

[creack]

Go version

go1.23.1 amd64/linux

Output of go env in your module/workspace:

GO111MODULE='on'
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/creack/.cache/go-build'
GOENV='/home/creack/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/creack/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/creack/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/creack/goroot'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/creack/goroot/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.1'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/creack/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build411067699=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Pulled on old-ish (5years) project and tried to use it, but it is now broken. https://github.com/creack/bug

I bisected go major version and found that it started to break at go1.18, go1.17 was still working fine. Now the result is a blank image.

Simple project using stdlin only:

$ go list -f '{{ .Imports }}'
[bytes errors image image/color image/draw io io/ioutil unicode/utf8]

Checking using go test.

What did you see happen?

All tests fail with blank images.

What did you expect to see?

Passing tests.

[gabyhelp]

Related Issues and Documentation

• runtime/cgo: Compilation Error #58320 (closed)
• affected/package: #51201 (closed)
• some issues here ! #39105 (closed)
• bug #68989 (closed)
• Update funny #60279 (closed)
• [Invalid] During tests, minor changes to code results in [no test files] #50871 (closed)
• test #61922 (closed)
• go bug #53266 (closed)
• test001 #53494 (closed)
• go bug #34172 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[randall77]

It would be helpful to bisect to a single CL if you can do that.
There was an image/draw change in 1.18, see https://tip.golang.org/doc/go1.18#minor_library_changes. Not sure if that is related to your code or not.

[ianlancetaylor]

The behavior change was indeed caused by https://go.dev/cl/340049. CC @nigeltao

[nigeltao]

Here's what's happening. bug.Gray embeds a from-the-standard-library image.Gray:

package bug

type Gray struct {
	// real holds the real pixel version of the image.
	*image.Gray

	// Other fields not shown.
}

It also 'overrides' the Set method:

package bug

func (p *Gray) Set(x, y int, c color.Color) {
	// Discard pixels outside the image.
	if !(image.Point{x, y}.In(p.Gray.Rect)) {
		return
	}
	p.Gray.Set(x, y, c)
	p.SetBraille(x, y, p.ColorModel().Convert(c))
}

Overrides is in 'quotes' because Go isn't really object-oriented the way C++ or Java is.

Note that bug.Gray's Set method calls the embedded image.Gray's Set method and does other things - it calls SetBraille.

---

Now, what happened in Go 1.18 is that there's new image.RGBA64Image and draw.RGBA64Image interfaces and that image/draw will use them (if your destination image implements that interface).

With Go 1.18 (and later), a bug.Gray automatically implements the draw.RGBA64Image interface (even though the package bug source code hasn't changed) because the embedded image.Gray implements this interface. But bug.Gray doesn't 'override' SetRGBA64, so when image/draw calls SetRGBA64, it doesn't do the other things - it doesn't call SetBraille.

---

@ianlancetaylor and others can correct me if I'm wrong, but I don't actually think this can be 'fixed' in the standard library, if we still want to keep the RGBA64Image optimizations that landed in Go 1.8.

Instead, I think that package bug needs to change, either by (1) not actually using embedding or (2) explicitly having bug.Gray implement SetRGBA64. Or both, but you only need at least one of those two.

Personally, I think that, in hindsight, Go's embedding feature is a mistake and methods should instead always be explicitly 'forwarded' to struct fields. Especially as doing so would avoid surprises like this. I would recommend (1), like this:

@@ -66,7 +66,7 @@ func (cm Threshold) Inverse() Threshold {
 // Each braille character represents 2x4 actual pixels.
 type Gray struct {
        // real holds the real pixel version of the image.
-       *image.Gray
+       Gray *image.Gray
 
        // content holds the braille representation of the image.
        // Not using stdlib's single dim slice as benchmark shows
@@ -108,6 +108,9 @@ func NewGray(r image.Rectangle) *Gray {
        return img
 }
 
+func (p *Gray) At(x, y int) color.Color { return p.Gray.At(x, y) }
+func (p *Gray) Bounds() image.Rectangle { return p.Gray.Bounds() }
+
 // Clear all pixels.
 func (p *Gray) Clear() {
        // The for loop is more efficient that a copy of empty element.

This patch makes the package bug tests pass for me.

[ianlancetaylor]

Thanks for the analysis. I agree that this is not a bug in Go. The Go compatibility guarantee permits us to add methods to existing types. Any embedding of a type in the standard library must consider this possibility.