godbus / dbus

Native Go bindings for D-Bus
BSD 2-Clause "Simplified" License
976 stars 225 forks source link

Type `dbus.Error` is not comparable. #317

Open tomkcook opened 2 years ago

tomkcook commented 2 years ago

Type dbus.Error is not comparable. This means that any attempt to compare an error returned from a dbus call with any other error will compile but result in a runtime panic.

guelfey commented 2 years ago

dbus.Error is a direct mapping of what a dbus error message looks like, but since it can include a body of arbitrary length and elements, it's represented as a []interface{} which is indeed not comparable. This is not easily changeable in a compatible way; if you want to check for a specific error, you can use it's Name as well as dbus.Store.

Regarding runtime panic: do you have a minimal example of what you mean? As far as I'm aware, this should be a compiler error in all cases. E.g.:

package dbus

import (
    "testing"
)

func TestCompareError(t *testing.T) {
    a := Error{
        Name: "foo",
        Body: []interface{}{1},
    }
    b := Error{
        Name: "bar",
        Body: []interface{}{"baz"},
    }
    _ = a == b
}

gives me ./error_test.go:16:8: invalid operation: a == b (struct containing []interface {} cannot be compared).

tomkcook commented 2 years ago

While this trivial comparison produces a compile error, as soon as you treat the objects involved as errors you lose this protection. This stand-alone test demonstrates the problem:

package compare

import (
        "testing"
)

type Error struct {
        Name string
        Body []interface{}
}

func (e Error) Error() string {
        if len(e.Body) >= 1 {
                s, ok := e.Body[0].(string)
                if ok {
                        return s
                }
        }
        return e.Name
}

func foo() error {
        return Error{
                Name: "foo",
                Body: []interface{}{1},
        }
}

func TestCompareError(t *testing.T) {
        _ = foo() == foo()
}
guelfey commented 2 years ago

Fair point, I didn't have that in mind. This is unfortunate, but I don't see this as an issue that would justify a breaking API change: Comparing errors directly (besides to some special values like io.EOF) is a bad idea in the general case. Specifically here, it only causes problems if both are dbus.Error, and in this case you anyway have to write your own logic on how you define equality. If one of them isn't a dbus.Error, there's no problem with the comparison.

guelfey commented 2 years ago

See e.g.

package dbus

import (
    "fmt"
    "io"
    "testing"
)

func TestCompareError(t *testing.T) {
    var a error = Error{
        Name: "foo",
        Body: []interface{}{1},
    }
    var b error = io.EOF

    fmt.Println(a == b)
}
=== RUN   TestCompareError
false
--- PASS: TestCompareError (0.00s)
PASS