Skip to content
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

Refactor types #56

Open
wants to merge 56 commits into
base: master
Choose a base branch
from
Open

Refactor types #56

wants to merge 56 commits into from

Conversation

jonbarrow
Copy link
Member

@jonbarrow jonbarrow commented May 22, 2024

Addresses concerns mentioned in #50

Changes:

Makes large changes to how we handle types, both in terms of the API and internally. In #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 Primitive types to just their type names. Adding Primitive was 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 List and Map into type aliases
    • NOTE: Map KEYS MUST NOW IMPLEMENT THE comparable TYPE CONSTRAINT. THIS MEANS Map AND List THEMSELVES CANNOT BE KEYS, NOR CAN *ANY* TYPE WHICH HAS THOSE AS FIELDS ANYWHERE IN ITS TREE
  • Refactors StationURL to be more accurately implemented in terms of the API
  • Adds several sanity checks in StationURL to 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 New methods for types no longer return pointers by default
  • Introduces a new pattern for a private ensureFields method existing on complex types that may need to ensure some fields are initialized before use. This is required for using complex types in Map and List types

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 Equals methods to accept anything other than a pointer to the same type
  • Make the List type into a type alias It does now
  • Make the Map type 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` 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 jonbarrow requested a review from DaniElectra May 22, 2024 22:01
Copy link
Member

@DaniElectra DaniElectra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Equals method issue can be resolved, by changing the signature to Equals(o any) bool

I don't think this would be any useful imo, you can always cast to the primitive type and do a normal == comparison

To get around the missing List and Map element types, we can use reflect for this

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).

types/pid.go Outdated Show resolved Hide resolved
@jonbarrow
Copy link
Member Author

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
Copy link
Member Author

jonbarrow commented May 25, 2024

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
Copy link
Member

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
Copy link
Member Author

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)

types/station_url.go Outdated Show resolved Hide resolved
@jonbarrow jonbarrow requested a review from DaniElectra July 8, 2024 21:57
Copy link
Member

@DaniElectra DaniElectra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to have PRs on nex-protocols-go and nex-protocols-common-go ready before merging this, so that this doesn't block any new changes there

kerberos.go Outdated Show resolved Hide resolved
rmc_message.go Outdated Show resolved Hide resolved
Comment on lines 87 to 89
copied.TypeName = adh.TypeName.Copy().(String)
copied.Length1 = adh.Length1.Copy().(UInt32)
copied.Length2 = adh.Length2.Copy().(UInt32)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since structures don't hold pointers anymore, can't we use direct assignment on here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Copy method must return an RVType to conform to the interface, so we cannot make them return more specific types. Trying to remove the assertion here throws the error cannot use adh.TypeName.Copy() (value of type RVType) as String value in assignment: need type assertion

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant like:

copied.TypeName = adh.TypeName

since both copied and adh are the same type

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, that does work. We can use that in all cases where the fields aren't pointers/don't have pointer fields

types/map.go Outdated Show resolved Hide resolved
The previous method didn't work because it was making a pointer of the
RVType interface instead of the underlying data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants