google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.26k stars 204 forks source link

Expand functionality for custom types #429

Closed gebv closed 1 year ago

gebv commented 1 year ago

I want to add support for binary\unary ops for custom value (implements starlark.Value)

https://github.com/google/starlark-go/blob/f738f5508c12fe5a9fae44bbdf07a94ddcf5030e/starlark/eval.go#L720

I want to add the ability to override starlark.Binary, starlark.Unary. Will you accept this PR?

PS How do I add a comparison for different types? Why can't it be done at the interface level?

pseudo code

// starlark/eval.go
var BinaryDefault = // exists function starlark.Binary
var Binary func(op syntax.Token, x, y Value) (Value, error) = BinaryDefault

// my code
func main() {

   starlark.Binary = // my custom logic for binary ops
} 
adonovan commented 1 year ago

You can do this already: see https://github.com/google/starlark-go/blob/master/starlark/value.go#L33 and https://github.com/google/starlark-go/blob/master/starlark/value.go#L278-L289.

gebv commented 1 year ago

Ok, let me give you an example @adonovan

In Binary\Unary, binding to types, not interfaces https://github.com/google/starlark-go/blob/f738f5508c12fe5a9fae44bbdf07a94ddcf5030e/starlark/eval.go#L805

If make starlark.Value in container, does not work (because Binary binds to types and not interfaces) https://go.dev/play/p/wFjVyyRlyJp

If the developer will be able to set custom logic in to starlark.Basic/starlark.Unary (like http.DefaultClient) it would be much more convenient

adonovan commented 1 year ago

I'm sorry, I don't understand what you mean by "binding to types and not interfaces". You need to define a Binary method on your Wrap type, as in this example: https://go.dev/play/p/zgqEQU82_JP

gebv commented 1 year ago

The main idea is starlark.Binary and starlark.Unary hard related to types (starlark.String, starlark.Int, etc). In general this is not convenient. For types that wraped over basic types And starlark.String has not implement starlark.HasBinary

My proposal makes minimal changes. Will allow not to depend on code that hard related to types. Will allow to write your own implementation of starlark.Binary https://github.com/google/starlark-go/pull/430/files#diff-372e699d4615af081f3c24181c777ecc9d956711af32cc5d997976592ed482ecR699-R704

adonovan commented 1 year ago

The main idea is starlark.Binary and starlark.Unary hard related to types (starlark.String, starlark.Int, etc). In general this is not convenient. For types that wrapped over basic types

I'm still not sure what problem you are trying to solve. Perhaps you are saying that the interface-based approach to testing whether a given Go value supports a particular Starlark operator is too restrictive, since a proxy type (such as your wrapper type) must commit at compile time to support all the operators that any particular wrapped value may support. That's certainly a valid criticism. But I can't even review your PR until I understand the problem.

gebv commented 1 year ago

Starlark is a tool for creating tools. My problem is very specific to the project where I use starlark. Little bit no comfortable when in "tool for creating tool" for example for different types I cannot write my own comparison rule. Or inherit the base type and expand its functionality. Or something else on a fundamental level. If add the ability to override starlark.Binary\starlark.Unary\starlark.Compare - it will be easier to write custom logic

PR is draft. To show that the changes are minimal, does not break the API. It remains only to add there tests.

adonovan commented 1 year ago

for different types I cannot write my own comparison rule.

Have you tried implementing the Comparable interface and defining an equivalence relation?

Or inherit the base type and expand its functionality.

Go does not have inheritance.

If add the ability to override starlark.Binary\starlark.Unary\starlark.Compare

I understand that you want to change the definition of these functions, but what exactly would you write instead? What could you achieve that way that you cannot achieve by implementing methods on your data types? Perhaps the answer is that you wish to change the behavior for existing types, such as to declare that 2 + 2 = 5, or that tuple int = vector scalar product. Unfortunately that's not one of the dimensions of extensibility supported by the design.