dgraph-io / ristretto

A high performance memory-bound Go cache
https://dgraph.io/blog/post/introducing-ristretto-high-perf-go-cache/
Apache License 2.0
5.62k stars 373 forks source link

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

Open jschaf opened 3 weeks ago

jschaf commented 3 weeks ago

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.
Pietroski commented 2 weeks ago

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 commented 6 days ago

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.