PretendoNetwork / nex-go

Barebones PRUDP/NEX server library written in Go
GNU Affero General Public License v3.0
74 stars 16 forks source link

Refactor types #56

Open jonbarrow opened 6 months ago

jonbarrow commented 6 months ago

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:

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:

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 and Map types not being type aliases also means that the original issues described in #50 are still relevant. However these CAN viably be made into type aliases, but come with some caveats which are going to require more changes across the other libraries.

The List type can somewhat easily be turned into a type alias like so:

type List[T RVType] []T

However this has 2 caveats:

  1. We lose access to the Type field. Which means we can no longer use it to create new instances of the List element type. However there is a way around this, which is mentioned later
  2. 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:

  1. We lose access to the key and value type fields. Which means we can no longer use it to create new instances of the Map element types. However there is a way around this, which is mentioned later
  2. The Map values MUST be a pointer, not the raw value. We already do this, but it's important to keep in mind
  3. The Map key 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 as UInt32 can safely be used, but ONLY when NOT used as pointers. So Map[UInt32, *Something] would work, but Map[*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:

  1. A good amount of work to refactor the types
  2. 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?)
  3. 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()
}

jonbarrow commented 6 months ago

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

jonbarrow commented 6 months ago

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

DaniElectra commented 6 months ago

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?

imagen

jonbarrow commented 6 months ago

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:

  1. We need to make those fields non-pointers anyway to get Map to work in this case
  2. Since the ExtractFrom call on those fields happens inside the parent's ExtractFrom, we could always just call it on a pointer to those fields (&g.ID).ExtractFrom(readable)