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/go2go: improve error messages when types don't satisfy constraints #41125

Closed
urandom opened this issue Aug 29, 2020 · 7 comments
Closed

cmd/go2go: improve error messages when types don't satisfy constraints #41125

urandom opened this issue Aug 29, 2020 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@urandom
Copy link

urandom commented Aug 29, 2020

The following program fails to compile: https://go2goplay.golang.org/p/-2Qmhtax9VX
The following error is observed:
prog.go2:238:9: *mapIt(string, int, reverse(string, ReverseIterator(string))) does not satisfy Iterator(T) (missing method Next)

Note that on line 224, the same parametric type is being instantiated, and used in 228 without a problem. Also note line 238, invoking the method the type checker complains about.

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 29, 2020
@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Aug 29, 2020
@ianlancetaylor
Copy link
Contributor

If I'm reading the code correctly, there should be an error here. But it would be nice if the error were better.

The line is

	err := ForEach(m2, func(i string) {

Here m2 has type mapIt[string, int, reverse[string, ReverseIterator[string]]].

The ForEach function is

func ForEach[T any, I Iterator[T]](it I, consumer func(T)) error {

So I must be the type of m2, shown above. And T must be string. So we require that the type of m2 must satisfy the constraint Iterator[string].

We have

type Iterator[T any] interface {
	Next() (T, bool)
}

So the type of m2 must have a method Next() (string, bool). The mapIt Next method is

func (i *mapIt[T, U, I]) Next() (U, bool) {

so in this case the type of the Next method is actually Next() (int, bool). But we need Next() (string, bool).

So I believe the type checker is correct to report an error. But the error shouldn't be missing method Next. It should be something more like "method Next with type Next() (int, bool) does not satisfy constraint which requires Next() (string, bool).

That would be a start, anyhow. It's still pretty hard to understand what is going on here. It would be good to see if there is something we can do to make it clearer.

@urandom
Copy link
Author

urandom commented Aug 30, 2020

Yes, that is indeed the issue, though I also didn't realize it at first. I've seen the type checker be able to tell the difference between methods with different instantiated parameters, so it's definitely able to report the error correctly in such cases

@gopherbot
Copy link

Change https://golang.org/cl/251717 mentions this issue: [dev.go2go] go/types: improve error when a type argument doesn't satisfy constraint

@griesemer
Copy link
Contributor

The new error message is now:

tmp.go2:245:9: *mapIt[string, int, reverse[string, ReverseIterator[string]]] does not satisfy Iterator[T]: wrong method signature
                found func (*mapIt[T, U, I]).Next() (U, bool)
                want  func (Iterator[T any]).Next() (string, bool)

This can be improved further (the T, U, I parameters should be substituted), but this is already better than what we had.

Leaving this issue open for now for future improvements.

@urandom, please simplify test cases when reporting an issue; it makes it significantly easier to figure out what's going on.

gopherbot pushed a commit that referenced this issue Aug 31, 2020
…sfy constraint

If the cause is a method with incorrect signature, report the
expected and presented signature.

Updates #41125.

Change-Id: Ibd0fa23547b77df924ec5421cc8e657d41c5a5a6
Reviewed-on: https://go-review.googlesource.com/c/go/+/251717
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@griesemer griesemer changed the title cmd/go2go: type stops satisfying constraint cmd/go2go: improve error messages when types don't satisfy constraints Sep 3, 2020
@urandom
Copy link
Author

urandom commented Sep 10, 2020

Perhaps the following example illustrates a similar message problem:
https://go2goplay.golang.org/p/O1Un2Vm9Dh0

The compiler outputs:
prog.go2:33:30: interface contains type constraints (T)

I initially through that parametric interfaces weren't allowed in type assertion expressions.

As pointed out in golang-nuts however, it appears that the problem is the fact that the interface has a type list. Perhaps the message should say so instead.

@griesemer
Copy link
Contributor

@urandom Well, a type list is a type constraint. But agreed, this should probably say "type list" instead (or whatever we use as the official terminology once we have a spec). Not urgent.

@griesemer
Copy link
Contributor

The Go 1.18 error message is now:

./prog.go:239:16: *mapIt[string, int, reverse[string, ReverseIterator[string]]] does not implement Iterator[string] (wrong type for Next method)
		have Next() (int, bool)
		want Next() (string, bool)

which seems pretty decent. Closing.

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants