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

[BUG]: Cannot use new type of string as z.Key param #400

Open
jschaf opened this issue Oct 1, 2024 · 3 comments
Open

[BUG]: Cannot use new type of string as z.Key param #400

jschaf opened this issue Oct 1, 2024 · 3 comments
Assignees
Labels
kind/bug Something is broken.

Comments

@jschaf
Copy link

jschaf commented Oct 1, 2024

What version of Ristretto are you using?

v1.0.0

What version of Go are you using?

go version go1.23.1 darwin/arm64

Have you tried reproducing the issue with the latest release?

Yes

What is the hardware spec (RAM, CPU, OS)?

N/A

What steps will reproduce the bug?

package main

import (
	"github.com/dgraph-io/ristretto"
	"github.com/dgraph-io/ristretto/z"
)

type MyKey string

func foo() {
	_, _ = ristretto.NewCache[MyKey, any](&ristretto.Config[MyKey, any]{
		KeyToHash: func(key MyKey) (uint64, uint64) { return z.KeyToHash(string(key)) },
	})
}

Expected behavior and actual result.

Expected: Go should compile this code.

Actual:

MyKey does not satisfy ristretto.Key (possibly missing ~ for string in ristretto.Key)

Additional information

The type definition of z.Key only allows direct primitive types.

package z

type Key interface {
	uint64 | string | []byte | byte | int | int32 | uint32 | int64
}

Ideally, the generic constraint would allow new type wrappers over those already supported in the Key interface.

The rationale is to allow application code to use stronger types than string. This helps distinguish between string types like UserID and InvoiceID.

I suspect the problem with supporting new types is there's no way to detect the underlying type efficiently. You can't do it with a type-switch. However, when the user provides a KeyToHash function, we don't need the generic constraint.

https://go.dev/play/p/fxOWPUemRz0

package main

import "fmt"

type WrappedInt int

func do(i interface{}) {
	switch v := i.(type) {
	case int:
		fmt.Printf("direct int: %v\n", v)
	default:
		switch v2 := v.(type) {
		case int:
			fmt.Printf("wrapped int: %v\n", v2)
		default:
			fmt.Printf("unknown type: %T\n", v2)
		}
	}
}

func main() {
	do(21)              // direct int: 23
	do(WrappedInt(123)) // unknown type: main.WrappedInt
}

Proposal

  1. When calling NewCache, check that either KeyToHash is provided or that the type is an implicitly supported key type. Then relax the Key constraint back to any.
@jschaf jschaf added the kind/bug Something is broken. label Oct 1, 2024
@mangalaman93 mangalaman93 self-assigned this Oct 8, 2024
@Pietroski
Copy link

Pietroski commented Oct 13, 2024

I'm getting a similar if not he same issue

go 1.23

require (
    github.com/dgraph-io/badger/v3 v3.2103.5
    github.com/dgraph-io/badger/v4 v4.3.1
)
# github.com/dgraph-io/badger/v3/table
../../../../../../vendor/github.com/dgraph-io/badger/v3/table/table.go:79:14: cannot use generic type ristretto.Cache[K z.Key, V any] without instantiation
../../../../../../vendor/github.com/dgraph-io/badger/v3/table/table.go:80:14: cannot use generic type ristretto.Cache[K z.Key, V any] without instantiation

@mangalaman93
Copy link
Contributor

Thank you for filling the issue. I am thinking that we can introduce another function such as NewCacheAnyK[K any, V any] that requires that keyToHash function is non nil and we take care of the rest. This will require some thought and some changes to the code. If you can create a PR, that'd be great. One of us can look into it as well when we can.

@trim21
Copy link

trim21 commented Oct 28, 2024

you can convert MyStr back to string when using as key I guess, it doesn't have overhead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something is broken.
Development

No branches or pull requests

4 participants