nex-go
nex-go copied to clipboard
Refactor types
Addresses concerns mentioned in #50
Changes:
Makes large changes to how we handle types, both in terms of the API and internally. In https://github.com/PretendoNetwork/nex-go/pull/42 we made Go's built in types into our own types, but all the types we made were structs. This includes simple types, like the built in ones, which only had a single field to hold the "real" value. This was caused by a misunderstanding of the type system and how pointer receivers worked with interfaces/generics, deeming type aliases nonviable.
This was incorrect, type aliases ARE viable. PR makes the following changes:
- Renames all
Primitivetypes to just their type names. AddingPrimitivewas both a mouthful and unnecessary - Converts all basic (built in) types into type aliases
- Converts non-basic, but still simple, types into type aliases
- Converts both
ListandMapinto type aliases- NOTE:
MapKEYS MUST NOW IMPLEMENT THEcomparableTYPE CONSTRAINT. THIS MEANSMapANDListTHEMSELVES CANNOT BE KEYS, NOR CAN *ANY* TYPE WHICH HAS THOSE AS FIELDS ANYWHERE IN ITS TREE
- NOTE:
- Refactors
StationURLto be more accurately implemented in terms of the API - Adds several sanity checks in
StationURLto prevent badly formatted URLs from reaching clients - Adds support for application-specific parameters in
StationURL. This is not used by NEX, but is used by Rendez-Vous titles like WATCH_DOGS - Removes our reliance on pointer receivers everywhere. In cases where a types state does not need to be changed, a regular method receiver is used
- This also means all
Newmethods for types no longer return pointers by default
- This also means all
- Introduces a new pattern for a private
ensureFieldsmethod existing on complex types that may need to ensure some fields are initialized before use. This is required for using complex types inMapandListtypes
This allows for a more streamlined way of interacting with these types, allowing for code such as:
a := types.NewUInt32(10)
fmt.Println(a + 10) // 20
a |= 10
fmt.Println(a) // 10
Where our custom types can be used more-or-less like built in types, rather than using an arbitrary Value field or method, and needing to export many methods for basic operations like bitwise on our numeric types.
This PR DOES NOT:
- Allow for the
Equalsmethods to accept anything other than a pointer to the same type - ~~Make the
Listtype into a type alias~~ It does now - ~~Make the
Maptype into a type alias~~ It does now
The Equals method issue can be resolved, by changing the signature to Equals(o any) bool, and then checking for more types. This just requires some extra checks, and I'm not sure how useful it would be? With type aliases on simple types we can just use the == operator now, rather than Equals (which is now really only useful when you JUST have an RVType and don't know its type?)
Edit: Map and List are now type aliases as well
The List type can somewhat easily be turned into a type alias like so:
type List[T RVType] []T
However this has 2 caveats:
- We lose access to the
Typefield. Which means we can no longer use it to create new instances of theListelement type. However there is a way around this, which is mentioned later - The element type MUST be a pointer, not the raw value. We already do this, but it's important to keep in mind
The Map type is a bit different. To make it into a type alias, we have to use the map basic type. This type REQUIRES that the key type implements the comparable constraint. What this means is that the type can be evaluated with the == operator, which isn't always guaranteed for custom types depending on how they're implemented
Given that, the Map type can also be made into a type alias like so:
type Map[K comparable, V RVType] map[K]V
However this has 3 caveats:
- We lose access to the key and value type fields. Which means we can no longer use it to create new instances of the
Mapelement types. However there is a way around this, which is mentioned later - The
Mapvalues MUST be a pointer, not the raw value. We already do this, but it's important to keep in mind - The
Mapkey MUST be able to safely be evaluated with the==operator, which currently NONE of our struct-based types are. Our type alias types ARE, so types such asUInt32can safely be used, but ONLY when NOT used as pointers. SoMap[UInt32, *Something]would work, butMap[*UInt32, *Something]would NOT work.
We CAN get around the 3rd issue, however. Struct-based types WILL evaluate safely with == IF they contain the same data:
type Struct struct {
field Field
}
type Field struct {
value string
}
func main() {
a := Struct{field: Field{value: "test"}}
b := Struct{field: Field{value: "test2"}}
fmt.Println(a == b) // false
b.field.value = "test"
fmt.Println(a == b) // true
}
However to do this it means that NONE of the structs fields can be pointers, they MUST be whole values. Which means we would need to completely change how we manage types in https://github.com/PretendoNetwork/nex-protocols-go as well as everywhere else. This IS viable, but comes with the following caveats:
- A good amount of work to refactor the types
- No longer safe to assume we always have a pointer to the type’s data, so we need to make sure we are going to and from pointers correctly (compiler should catch this, but could make for some possibly uglier code?)
- Possibility for larger memory usage. If we stop passing pointers around, then large structs COULD take up more memory in some cases. However to be fair, none of our structs are very large and are used in pretty one-off situations with little (if any?) pointer sharing anyway. So it likely won't have a HUGE impact on memory usage
Making the List and Map types would potentially address/fix the issues mentioned in #50, but would need more of a rework in other libraries.
To get around the missing List and Map element types, we can use reflect for this. In the past we have avoided reflection when possible as it can be slow and error prone. However I ran several benchmarks against using reflect with a test implementation of List and found no noticeable difference in performance. The code for this would look something like:
func (l List[T]) newType() T {
var t T
tType := reflect.TypeOf(t).Elem()
return reflect.New(tType).Interface().(T)
}
func (l List[T]) test() {
t := l.newType()
t.Extract()
}
- [x] I have read and agreed to the Code of Conduct.
- [x] I have read and complied with the contributing guidelines.
- [ ] What I'm implementing was an approved issue.
- [x] I have tested all of my changes.
you can always cast to the primitive type and do a normal
==comparison
Not in types which take in an RVType, like List and Map. These will always use the Equals method, we don't know if it's a basic type alias or not
Also it would allow for code like u32.Equals(5), removing the need for a type cast and possible pointer dereference. Mostly a convenience thing, though yes not strictly necessary for type aliases
However it WOULD allow for us to use .Equals on non-primitive types like the structs in the protocols lib whether or not it's a pointer or not (right now it requires a pointer to the object so we have to make sure we manage that)
IIRC using reflect will not initialize the internal pointers of the struct
In my testing it did, but this can be done. I'll play around with it tomorrow unless you beat me to it
IIRC using reflect will not initialize the internal pointers of the struct, so I believe using type aliases for List and Map would not be feasible (and it would require more rebasing anyway).
I just tested this and it does create a new pointer to the struct:
package main
import (
"fmt"
"reflect"
)
type RVType interface {
Extract()
Copy() RVType
}
type Struct struct {
field string
}
func (s *Struct) Extract() {}
func (s Struct) Copy() RVType {
return &Struct{field: s.field}
}
type List[T RVType] struct {
real []T
}
func (l List[T]) newType() T {
var t T
tType := reflect.TypeOf(t).Elem()
return reflect.New(tType).Interface().(T)
}
func (l List[T]) test() {
t := l.newType()
fmt.Printf("%T\n", t)
}
func main() {
list := List[*Struct]{
real: make([]*Struct, 0),
}
list.test() // *main.Struct
}
In this example t in the test method is a pointer to Struct. With this we can do a Copy or implement a new method like New to initialize anything inside the struct
I meant the fields inside the struct, which are pointers and would be assigned as nil. I believe these fields would have to be pointers so that we can use ExtractFrom?
I meant the fields inside the struct
This is why I proposed adding a new method such as New to RVType which would initialize anything like that
I believe these fields would have to be pointers so that we can use
ExtractFrom?
ExtractFrom is a pointer receiver but it does not need a pointer at the time of calling:
package main
import (
"fmt"
)
type RVType interface {
ExtractFrom()
}
type Struct struct {
field string
}
func (s *Struct) ExtractFrom() {
s.field = "test"
}
func main() {
s := Struct{}
s.ExtractFrom()
fmt.Printf("%+v\n", s) // {field:test}
}
That being said, even if it did:
- We need to make those fields non-pointers anyway to get
Mapto work in this case - Since the
ExtractFromcall on those fields happens inside the parent'sExtractFrom, we could always just call it on a pointer to those fields(&g.ID).ExtractFrom(readable)