Improve mutatable list documentation

[kortschak]

This is a development of the change suggested at https://groups.google.com/g/cel-go-discuss/c/6sA9XhqVRek/m/-uB0yuAoIQAJ including documentation additions to clarify what uses are valid.

The first commit makes all non-valid calls panic via a nil-pointer deref and highlighted that there are calls to .Type() in the runtime, so that method was added.

The second commit improves the error reporting to the user at the cost of some stub methods; with this change, an invalid use is reported like:

2022/03/21 19:35:43 codelab.go:108: internal error: invalid use of mutable list

rather than with a runtime error:

2022/03/21 13:45:29 codelab.go:107: internal error: runtime error: invalid memory address or nil pointer 

making it easier for users to find the error and bug reports/discussion list queries simpler to handle.

I did start out implementing testing for the panic behaviour, but it got pretty contrived.

Please take a look.

[TristonianJones]

In general we try not to panic. There are ways to make these calls work as well. For example, a naive way would be to first convert ToImmutableList and then invoke the appropriate method. I think that would be preferred (and would fix the prior behavior of accessing nil memory), even if it's inefficient. Thoughts?

[kortschak]

Yeah, I guess that's a design decision.

When I was originally looking at it I saw the possibility of automatically immutifying the list, so for example this would become func (l *mutableList) Contains(val ref.Val) ref.Val { l.ToImmutableList().Contains(val) }. This obviously become very inefficient if used, but is reasonable (note: this cost could be mitigated by caching in a field of *mutableList so that a sequence of non-mutating method calls do the right thing in a less costly way). I also think it's reasonable though to return an error to the user if they use a function in a way that is explicitly documented to not work as this code currently does; essentially we are mimicking a type system via that mechanism in the same way that reflect does.

I'm happy to go either way, though tend to lean toward the harsh prevention of people doing the wrong thing.

[TristonianJones]

When I'm writing an application, I agree with you about erroring as soon as possible. However, as a common library author it's usually much better to degrade gracefully than to cause a failure in the application that uses it. I'll admit, I never expected someone outside the CEL team to use this function when I wrote it. Oops :)

I think, perhaps, the following would be just as good, and a better user experience if someone were to use the MutableList in a way other than what I had originally intended.

// a/common/types/list.go
func NewMutableList(adapter ref.TypeAdapter) traits.Lister {
       var mutableValues []ref.Val
        return &mutableList{
               baseList: &baseList{
                       TypeAdapter: adapter,
                       value:       mutableValues,
                       size:        0,
                       get:         func(i int) interface{} { return mutableValues[i] },
               },
               mutableValues: mutableValues,
        }
 }
 
 // mutableList aggregates values into its internal storage. For use with internal CEL variables only.
 type mutableList struct {
        *baseList
        mutableValues []ref.Val
 }

func (l *mutableList) Add(other ref.Val) ref.Val {
        otherList, ok := other.(traits.Lister)
	if !ok {
		return MaybeNoSuchOverloadErr(otherList)
	}
        for i := IntZero; i < otherList.Size().(Int); i++ {
                l.size++
                l.mutableValues = append(l.mutableValues, otherList.Get(i))
        }
        return l

[kortschak]

Sure. I am playing with going through and implementing as many of the methods in *mutableList as a cheap to do so and, most of them are very cheap to implement, with a fall back to an implicit conversion that's cached. I'll send this for discussion when I'm happy with the docs/tests for it (Briefly, only ConvertToType, ConvertToNative and Value result in an implicit ToImmutableList call` which seems reasonable since they are essentially terminals in a process on a mutable list while the rest are either very cheap or are likely valuable to have while working on the mutable list).

[TristonianJones]

@kortschak I think the small example I provided might actually do everything that you would expect a traits.Lister to do.

[kortschak]

Yes, you're absolutely right!

[kortschak]

The warnings also go away. I have added a minor optimisation for the case where the other list is also a *mutableList making use of the runtime's awareness of size classes.

[TristonianJones]

/gcbrun

[TristonianJones]

/gcbrun

[kortschak]

Thanks for the discussion here. It was very helpful.

[TristonianJones]

Thanks for the contribution!