-
Notifications
You must be signed in to change notification settings - Fork 234
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
Improve mutatable list documentation #516
Conversation
* Document all methods that are available. * Make returned type indicate expectations. * Make unavailable methods panic.
common/types/list.go
Outdated
mutableValues: []ref.Val{}, | ||
} | ||
} | ||
|
||
func (*mutableList) Contains(ref.Val) ref.Val { panic("invalid use of mutable list") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kortschak I think the small example I provided might actually do everything that you would expect a traits.Lister
to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're absolutely right!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Also provide fast path list concatenation when other list is a *mutableList.
/gcbrun |
1 similar comment
/gcbrun |
Thanks for the discussion here. It was very helpful. |
Thanks for the contribution! |
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:
rather than with a runtime error:
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.