spy16 / parens

Parens is a highly flexible and embeddable LISP toolkit. :computer:
33 stars 3 forks source link

Return error from Equals() method. #23

Closed lthibault closed 4 years ago

lthibault commented 4 years ago

@spy16 I've encountered the need to return an errors from the Equals() method in all of my Wetware datatypes. Again, the reason is that attribute lookups on Cap'n Proto structs can fail.

spy16 commented 4 years ago

Hmm, I am actually not sure if this would be a typical scenario 🤔 ..

What about Equals defined on Wetware side as below?

func Equals(v1, v2 parens.Any) (bool, error) {
   if e, ok := v1.(interface { Equals(other parens.Any) (bool, error) }); ok {
       return e.Equals(v2)
   }
   return parens.Equal(v1, v2), nil
}
lthibault commented 4 years ago

I'd expect this to be about as typical as e.g. Count returning an error. One common use-case would be when something like (= "foo" -1) doesn't really mean anything. I know most languages will just say "nope, they're not equal", but it also makes sense for many languages to treat this as a type error. I think Parens should support that.

Returning to my specific issue, here's a representative example from the Wetware codebase. My goal is to make this less panicky.

// Equals returns true if 'other' is string and has same value.
func (s String) Equals(other parens.Any) bool {
    str, err := s.v.Str()
    if err != nil {
        panic(err)
    }

    o, ok := other.(String)
    if !ok {
        return false
    }

    otherStr, err := o.v.Str()
    if err != nil {
        panic(err)
    }

    return str == otherStr
}

The example you suggest ought to work, so I suppose this is really a discussion about scope. Do we want to provide built-in functions like parens.Equals? If so, should we try to make them work on the broadest possible set of values? (I personally think "yes" and "yes" -- perhaps we should even add Equals to the Any interface?).

lthibault commented 4 years ago

@spy16 Actually, how do you feel about replacing Equals with a Comp method?

var ErrIncomparableTypes = errors.New("incomparable types")

type Comparable interface {
    // Comp returns 0 if the comparable c is equal to other, -1 if c < other, and 1 if c > other.
    Comp(other parens.Value) (int, error) 
}
spy16 commented 4 years ago

I think it was initially this interface. But changed it to the Equals only approach since this didn't work for some types (func types and arbitrary go types ?🤔).. can't remember exactly why.

How do you see this behaving for different types ? (Primitive types are obvious. But composite types and other go interop types)

lthibault commented 4 years ago

@spy16 Yeah, after posting this I hit a bit of a snag, namely: Comp forces you to impose an ordering on types. For things like parens.String, it doesn't make much sense. For instance, "hello" and "world" are not equal, but is one meaningfully less than the other? I think not.

I actually think we should have both Comparable and EqualityProvider types:

var ErrIncomparableTypes = errors.New("incomparable types")

type EqualityProvider interface {
    Equals(other parens.Any) (bool, error)
}

type Comparable interface {
    Comp(other parens.Any) (int, error)
}

A parens.Eq function could use one or the other, depending on what is provided by the type:

func Eq(a, b parens.Any) (bool, error) {
    if eq, ok := a.(EqualityProvider); ok {
       return eq.Equals(b) 
    }

    if eq, ok := b.(EqualityProvider); ok {
        return eq.Equals(a)
    }

    if cmp, ok := a.(Comparable); ok {
        i, err := cmp.Comp(b)
        return i == 0, err
    }

    if cmp, ok := b.(Comparable); ok {
        i, err := cmp.Comp(a)
        return i == 0, err
    }

    return false, ErrIncomparableType
}

How do you see this behaving for different types ? (Primitive types are obvious. But composite types and other go interop types)

Can't we just defer to the user? Wetware defines Comp and Equals on composite types, using custom logic. Works like a charm:

P.S.: At the risk of beating a dead horse: returning an error from these methods is really helpful.

spy16 commented 4 years ago

I am being bit skeptical because I want to make sure the builtin data types and functions are as lean as possible. This is because, I think we can't really cover all requirements of different use-cases and it is best to defer any non-essential feature designs to the user - In the best case scenario, users get to compose simple types and functions to build complex usecase, in the worst case scenario it is easy to re-implement the data types themselves without breaking lot of expectations from parens itself.

That said, I think comparison is an essential and straightforward requirement and we can add it to parens itself. Can you update the PR with the Comparable and proposed Eq implementation? ..

lthibault commented 4 years ago

I've taken a stab at implementing Comparable, along with Eq and friends. Let me know what you think!