Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type dbus.Error is not comparable. #317

Open
tomkcook opened this issue Mar 3, 2022 · 4 comments
Open

Type dbus.Error is not comparable. #317

tomkcook opened this issue Mar 3, 2022 · 4 comments

Comments

@tomkcook
Copy link
Contributor

tomkcook commented Mar 3, 2022

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
Copy link
Member

guelfey commented Mar 16, 2022

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
Copy link
Contributor Author

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
Copy link
Member

guelfey commented Apr 24, 2022

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
Copy link
Member

guelfey commented Apr 24, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants