Why is starlarkstruct.Struct constructor field quoted by the .String() method?

[gbouv]

This line here: https://github.com/google/starlark-go/blob/d7da88764354917a82dfa84a8ae1cb04f107ab78/starlarkstruct/struct.go#L140

When I do

stringDict := starlark.StringDict{
   "attr": "hello"
}
myStruct := starlarkstruct.FromStringDict(starlarkstruct.Default, stringDict)
fmt.Println(myStruct.String()) //-> 'struct(attr="hello")'

However when I provide my own constructor:

stringDict := starlark.StringDict{
   "attr": "hello"
}
myStruct := starlarkstruct.FromStringDict(starlarkstruct.String('struct2'), stringDict)
fmt.Println(myStruct.String()) //-> '\"struct2\"(attr="hello")'   <<< note the additional quotes in here

I would expect that if the struct constructor is a starlark.String, we call GoString() instead of String() to be consistent with what we do for the Default constructor. It would also have the huge advantage of "stringifying" any struct object in valid starlark code.

In short, I'd suggest we replace https://github.com/google/starlark-go/blob/d7da88764354917a82dfa84a8ae1cb04f107ab78/starlarkstruct/struct.go#L135-L141 with something like

switch constructor := s.constructor.(type) {
case starlark.String:
    buf.WriteString(constructor.GoString()) // avoid String()'s quotation
default:
    buf.WriteString(constructor.String())
}

The metapoint here is that I don't really understand what type the constructor can be, other than starlark.String... Is there really some use cases for the constructor being a starlark.Int, or even a *starlark.Dict??

[adonovan]

The metapoint here is that I don't really understand what type the constructor can be, other than starlark.String... Is there really some use cases for the constructor being a starlark.Int, or even a *starlark.Dict??

No, there was never any expectation that one might use int or dict as constructors, but one might define a new Starlark type that is similar to a class or type symbol in Python. This would allow different flavors of struct values to carry around arbitrary state or behavior that could be used by application-defined built-ins, and would provide better hygiene (namespaces, unforgeable capabilities, etc) than mere strings.

But I think the change you suggest makes some sense. If the constructor is a string, let's assume it's an identifier and not quote it.

[gbouv]

Thanks for your quick response @adonovan, and for the clarification around the constructor being something else that a string. That makes sense to me.

I'll submit a PR to change the Struct String () method as proposed above. I need to go through the CLA agreement first as it will be my first contribution to this repo. I'll ping you on this thread when the change is up

[gbouv]

@adonovan the PR is up and I signed the CLA: https://github.com/google/starlark-go/pull/449. Feel free to take a look whenever you have some time and let me know what you think :)