lthibault
commented
4 years ago On a high level, this all looks 👍
I have a few general design notes for the future, which I'll share below. Any specific problems or blocking issues will be mentioned directly in source comments.
Design notes
1. Value interface
I know we've discussed this before, but part of me is tempted to go all-in on the Value interface, with the main goal of reducing type assertions.
The general idea is to define a rich Value interface that contains both required and optional methods. Values that do not implement a given optional method would simply return ErrNotImplemented, allowing the runtime to take appropriate action.
To illustrate, it might look something like this:
var ErrNotImplemented = errors.New("not implemented")
type Value interface {
Source() string
Eval(env Env) (Value, error)
Invoke(env Env, args ...Value) (Value, error)
Compare(other Value) (bool, error) # note the added error value
// ...
}Obviously this adds a bit of boilerplate when implementing new values, but the advantages are:
- Flexibility
- Performance (because no reflection)
- Readability (because "reflection is never clear" and
Valuefunctionality is no longer split between multiple interfaces)
Regarding flexibility, a motivating example might be useful. One thing I would like to do is to automatically cast/convert types during operations on heterogeneous types, or during numerical overflows. For example:
(+ 0.00125 22)
;; prints `22.00125`, a floatI don't believe there's a simple way of doing this right now. An approach I like a lot is to define an Add(other Value) (Value, error) method to Value. The + function would then be implemented as follows:
func Add(v0, v1 Value) (Value, error) {
return v0.Add(v1)
}And Float64.Add might be implemented as follows:
func (f Float64) Add(other Value) (Value, error) {
switch v.(type) {
case Int64, Float64:
Float64(f + v), nil
default:
// N.B.: this allows the builtin `Float64` to gracefully handle
// user-defined types. Addition is commutative, so we can defer
// to the other value's implementation of Add.
other.Add(f)
}
}If I someday want to add a BigInt type, I now only need to ensure its Add method (and eventual Multiply, Divide, ShiftLeft and ShiftRight) methods correctly handle builtins.
With regards to performance, such an object model has the potential to greatly improve member access expressions (see: https://github.com/spy16/sabre/issues/12). Instead of relying on reflection, Value can define the following methods:
type Value interface {
// ...
// Bind a value to an attribute name
Bind(name string, v Value) error
// Resolve an attribute by name
Resolve(name string, defaultValue Value) (Value, error)
// ...
}Now, implementations are free to provide member access with e.g. a map[string]Value, although the Any type can still use reflection under the hood.
BTW: I also feel quite strongly that things like IsHashable are an anti-pattern. How can Sabre users create their own hashable types and use them in maps? I'm planning on building immutable datastructures, so there's no reason a list or map can't be used as a key.
The proposed approach would allow us to specify a Hash() (Value, error) method, such that an implementation can return a hash value that is different from the concrete value.
One last thing: if the Value interface gets too big, we can always fall back on Value.Resolve. For example, a map implementation can could call someKey.Resolve("Hash", nil) to check if the key's type implements a custom hashing method. If it does not, it can attempt to hash the concrete value. This would allow us to have many optional methods in our object model, without cluttering the Value interface, and without the drawbacks of reflection.
We don't have to do any of this now, but I'm interested in keeping this discussion going. I'm more and more convinced some form of object model is a must-have.
2. Core utilities
I would really like to see functions like containerString in some sort of package. Sabre's mission is to provide a batteries-included toolchain for building a lisp dialect in Go. Language internals are part of that contract, so I believe this is in-scope.
Same goes for evalValueList, newEvalErr, getPosition, etc. My project requires a lot of custom types, so these are all very useful.
3. Value.Source() should be replaced with fmt.Stringer
Like you, I'm increasingly convinced that Value should provide its string representation. That said, I think the String() string method is preferable to Source() string for two reasons:
First, fmt.Stringer means we can directly print values using the fmt or log packages:
// good
log.Printf("error encountered in %s at %s%s\n%s", filePath, lineNo, colNo, myValue)
// bad
log.Printf("error encountered in %s at %s%s\n%s", filePath, lineNo, colNo, myValue.Source())Second, Source() is arguably incorrect. Lisp is often referred to as a "syntax-free" language, since it's primary syntax is the set of data-structures itself, not their homoiconic string representations. This is arguably a detail, but it's nonetheless "more correct" to say "I'm returning a string representation of lisp source" than "I'm returning the source-code of this memory object". In other words, strings of symbols are serializations of lisp source.
4. Set should be implemented with a map.
I suspect there might be an issue with this suggestion, but I still thought I'd bring it up. I think the Set type should be backed by a hash map. Any reason why you used []Value instead of map[Value]struct{}?
5. Container types should be interfaces.
This is how Clojure does it. Using interfaces makes it much easier to implement custom types, which is in keeping with Sabre's positioning as a "build your own lisp toolkit".
I would like to see an abstract interface for List, Vector, Map, Set and Module, along with an unexported, default implementation for each. Notwithstanding the above object-model suggestions, the interface for the current Map implementation would be:
type Map interface {
Value
Get(Value) Value
Set(Value, Value)
Keys() Values
Values() Values
}We could then have default constructors for each interface (perhaps in a builtin package, or something). For Map, it would look something like this:
package builtin
func NewMap() core.Map {
return hashMap{Data: make(map[core.Value]core.Value)}
}Note also: I don't think it's appropriate for the exported type to be called HashMap. The Hash prefix is an implementation detail. Users of Sabre may wish to create maps based on trees, or other non-hash-based approaches. Users should care about behavior, not internals. Can we call it Map, instead?
spy16
Still work-in-progress. Highlights of the refactor here:
corepackage.readerpackage.sabreroot package provides the same interface for using sabre as before. (i.e.,New(),ReadEvalStr,ReadEval,ValueOfetc.)I was experimenting with the idea discussed in #16 .. But most likely i will go back to the old
Valueinterface itself.Feel free to provide feedback and design inputs if any.
Upcoming changes:
Set,HashMapandModuletosabrepackage instead ofcore? (just a thought.. since these are not really very well implemented at the moment and are not really necessary for pure LISP like usage)Analyseas public function to allow adding custom special forms? (I don't want to expose since either the special form is very generic in which case users can raise a PR and we can merge it into Sabre. If not, it probably can be implemented as a macro/function)..