should sqlitex.Save() check *perr for a nil pointer inside the error, not just *perr == nil ?

[glycerine]

Hi Ross!

I wonder if https://github.com/zombiezen/go-sqlite/blob/v1.3.0/sqlitex/savepoint.go#L86

should be using something like isNilInterface() to be checking *errp for nil-ness? That is, instead of

        if *errp == nil && recoverP == nil {
            // Success path. Release the savepoint successfully.

should it be?

        if isNilInterface(*errp) && recoverP == nil {
            // Success path. Release the savepoint successfully.

where isNilInterface is something like

// isNilInterface uses reflect to to return true iff the face                                                                   
// contains a nil pointer, map, array, slice, or channel.                                                              
func isNilInterface(face interface{}) bool {
    if face == nil {
        return true
    }
    switch reflect.TypeOf(face).Kind() {
    case reflect.Ptr, reflect.Array, reflect.Map, reflect.Slice, reflect.Chan:
        return reflect.ValueOf(face).IsNil()
    }
    return false
}

My worry is that the variable perr might have an address, but still have nil value pointer inside the interface.

Refer to https://go.dev/doc/faq#nil_error , and for concreteness, this demonstration:

package main

func checkForNil(perr *error) {
    if perr == nil || *perr == nil {
        println("detected nil")
    } else {
        println("we have non-nil interface")
    }
}

type MyError struct{}

func (e *MyError) Error() string { return "hmm" }

func getMyError() error {
    var p *MyError // nil by default
    return p // we may think we are returning nil, but the returned error (an interface wrapping p) will != nil
}

func main() {
    //ok: prints "detected nil"                                                                                        
    //var err error                                                                                                    

    //problem: prints "we have non-nil interface"                                                                      
    err := getMyError()
    checkForNil(&err)
}

[zombiezen]

I believe the code is WAI. As the doc example shows, sqlitex.Save is meant to be defer-called on the named return value address.

Do you have some code that uses sqlitex.Save that is acting in a surprising way? That would help me understand why you're asking about this.

[glycerine]

package main

import (
    "zombiezen.com/go/sqlite"
    "zombiezen.com/go/sqlite/sqlitex"
)

var problem bool // false

func doSomeMoreWork() error {
    var p *MyError // (nil by default)

    if (problem) {
    p = &MyError{}
    }

    return p // we may think we are returning nil, but the returned error (an interface wrapping p) will != nil
}

func doWork(conn *sqlite.Conn) (err error) {
        defer sqlitex.Save(conn)(&err)

        // ... do work in the save-point transaction
        err = doSomeMoreWork()
        return
}

func main() {
    var conn *sqlite.Conn
    // ... initialize conn ...

    doWork(conn)
    // we are surpised when the savepoint rolled back instead of committed. Even though problem was false.
}

[glycerine]

This has been a common bug in many systems I have worked on. Thus it even has a place in the Go FAQ.

[zombiezen]

Understood. I don't think that it's the place of sqlitex.Save to try to perform deep inspection for this type of error, and I can imagine this potentially breaking legitimate uses in a subtle way. I'm going to keep this as-is.