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

cmd/compile: inconsistent error messages based on type constraints #53692

Closed
sethvargo opened this issue Jul 5, 2022 · 11 comments
Closed

cmd/compile: inconsistent error messages based on type constraints #53692

sethvargo opened this issue Jul 5, 2022 · 11 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. TypeInference Issue is related to generic type inference
Milestone

Comments

@sethvargo
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
go version go1.18.3 darwin/arm64

Does this issue reproduce with the latest release?

Yes, including tip as of today

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

GO111MODULE="on"
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/sethvargo/Library/Caches/go-build"
GOENV="/Users/sethvargo/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/sethvargo/Development/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/sethvargo/Development/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.18.3/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.18.3/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.18.3"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/cp/qb9vbbkx4w36f6dclng481br00gy5b/T/go-build4094322222=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I suspect the root cause of my actual issue is #41176, but it would be great if @ianlancetaylor can confirm for me 😄.

Playground link for the code: https://go.dev/play/p/RqzqQxNapkw. Note this code does not compile.

Code:

package main

// Cache is a generic cache implementation.
type Cache[K comparable, V any] interface {
	Get(K)
	Set(K, V)
}

// LRU is a cache.
type LRU[K comparable, V any] struct{}

func (l *LRU[K, V]) Get(key K)        {}
func (l *LRU[K, V]) Set(key K, val V) {}

// WithLocking1 returns a cache that wraps operations in a mutex.
func WithLocking1[K comparable, V any, C Cache[K, V]](cache C) {}

// WithLocking2 returns a cache that wraps operations in a mutex.
func WithLocking2[K comparable, V any](cache Cache[K, V]) {}

// WithLocking3 returns a cache that wraps operations in a mutex.
func WithLocking3[K comparable](cache Cache[K, any]) {}

func main() {
	var lru LRU[string, int]

	WithLocking1[string, int](&lru) // ok
	WithLocking1[string](&lru)      // nok: cannot infer V
	WithLocking1(&lru)              // nok: cannot infer K

	WithLocking2[string, int](&lru) // ok
	WithLocking2[string](&lru)      // nok: does not match Cache[string, V]
	WithLocking2(&lru)              // nok: cannot infer K and V

	WithLocking3[string](&lru) // nok: does not implement Cache[string, any] (wrong type for method Set)
	                           //   have Set(key string, val int)
	                           //   want Set(string, any)
	WithLocking3(&lru) // nok: cannot infer K
}

What did you expect to see?

As shown in the comments, the error messages are all subtly different depending on the type declarations on the function. I would expect the same error messages here.

What did you see instead?

Different error messages.

Aside

As an aside, it's unclear to me what the "correct" authorship is here. I really appreciate Go's "do one thing", but there seems to be multiple ways to define generics, all of which are subtly different with unknown consequences.

@sethvargo
Copy link
Contributor Author

Here's another example, which seems to have the opposite behavior. When a function is given as a type constraint, it's not inferred, but when it's declared as a parameter, it is inferred properly: https://go.dev/play/p/qOpMvvHt8On

package main

// FetchFunc is a function that is invoked when a cached value is not found.
type FetchFunc[V any] func() (V, error)

// Cache is a generic cache implementation.
type Cache[K comparable, V any] interface {
	Get(K)
	Set(K, V)
}

// LRU is a cache.
type LRU[K comparable, V any] struct{}

func (l *LRU[K, V]) Get(key K)        {}
func (l *LRU[K, V]) Set(key K, val V) {}

func Fetch1[K comparable, V any, C Cache[K, V]](cache C, key K, fn FetchFunc[V])      {}
func Fetch2[K comparable, V any](cache Cache[K, V], key K, fn FetchFunc[V])           {}
func Fetch3[K comparable, V any, C Cache[K, V], F FetchFunc[V]](cache C, key K, fn F) {}

func main() {
	var lru LRU[string, int]

	Fetch1(&lru, "foo", func() (int, error) { return 0, nil }) // ok

	Fetch2[string, int](&lru, "foo", func() (int, error) { return 0, nil }) // ok
	Fetch2(&lru, "foo", func() (int, error) { return 0, nil })              // nok: cannot infer K and V

	Fetch3(&lru, "foo", FetchFunc[int](func() (int, error) { return 0, nil })) // ok
	Fetch3(&lru, "foo", func() (int, error) { return 0, nil })                 // nok: does not implement FetchFunc[int]
}

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. TypeInference Issue is related to generic type inference labels Jul 6, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.20 milestone Jul 6, 2022
@ianlancetaylor
Copy link
Contributor

These cases are in fact different. The only error message I see that looks clearly unhelpful is

	WithLocking2[string](&lru)      // nok: does not match Cache[string, V]

for which the full error is

foo.go:32:23: type *LRU[string, int] of &lru does not match Cache[string, V]

The other cases seem to be saying the right thing. It seems normal to me to get different kinds of errors based on different kinds of type inference. Maybe I'm missing something.

@ianlancetaylor
Copy link
Contributor

CC @griesemer

@sethvargo
Copy link
Contributor Author

It would also be helpful to understand why the functions in this comment are all subtly different. I've read through all the generics documentation, and I think I'm missing something.

@ianlancetaylor
Copy link
Contributor

The error cases in the second comment can only work if the compiler uses the argument to infer the type arguments of the generic type of the parameter. There is no type inference rule that will do that. The type inference is as described at https://tip.golang.org/ref/spec#Type_inference. Rules that are not listed there are not applied, even if they may be obvious to the human reader.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@griesemer
Copy link
Contributor

I agree with @ianlancetaylor here. All these cases are slightly different and result in slightly different inference mechanisms, resulting to slightly different errors.

This is not to say that we cannot do better - improving inference and related error messages was simply not the highest priority so far.

With respect to "but there seems to be multiple ways to define generics, all of which are subtly different with unknown consequences" I respectfully disagree: there's exactly one way to define generic types and functions, by declaring type parameters and constraints. We allow - as syntactic sugar - the leaving away of the interface wrapper around constraint types, which is about the only variation permitted.

But there are different ways to invoke/instantiate a generic function: with or without type inference, and when using type inference, with partial type argument lists.

Leaving this open for now, as a reference for when we work on better type inference and/or error messages.

@griesemer griesemer modified the milestones: Go1.20, Go1.21 Dec 2, 2022
@griesemer
Copy link
Contributor

Simplified code for the case that could use a better error message (playground link):

package main

type Cache[K comparable, V any] interface{}

type LRU[K comparable, V any] struct{}

func WithLocking2[K comparable, V any](Cache[K, V]) {}

func main() {
	WithLocking2[string](LRU[string, int]{})
}

At the moment the error is:

type LRU[string, int] of LRU[string, int]{} does not match Cache[string, V]

The reason it doesn't match is because LRU[string, int] and Cache[string, V] are different defined types (even after inferring V = int). Should make that clearer.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/474197 mentions this issue: go/types, types2: clean up defined type identity check/unification

@griesemer
Copy link
Contributor

With the above CL, the error message for the above example is now:

type LRU[string, int] of LRU[string, int]{} does not match inferred type Cache[string, int] for Cache[string, V]

making it clearer that V got inferred to int, so it's LRU and Cache which don't match.

@sethvargo
Copy link
Contributor Author

I like it.

gopherbot pushed a commit that referenced this issue Mar 9, 2023
Factor out check for identical origin.
Match unification code with type identity check.
Add a test case for #53692.

Change-Id: I1238b28297a5ac549e99261c8a085dd46f3dd65f
Reviewed-on: https://go-review.googlesource.com/c/go/+/474197
Run-TryBot: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Robert Griesemer <gri@google.com>
@griesemer
Copy link
Contributor

The above CL has now landed. Given the variety of calls and type parameters, a variety of error messages is expected. The one that's been most confusing is fixed with the CL.

There's more opportunity for better error messages. Ok to file issues for such cases, ideally one issue per case.

Closing this as fixed for now.

@golang golang locked and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. TypeInference Issue is related to generic type inference
Projects
None yet
Development

No branches or pull requests

4 participants