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: interface conversion error lost method details #48471

Closed
rsc opened this issue Sep 19, 2021 · 11 comments
Closed

cmd/compile: interface conversion error lost method details #48471

rsc opened this issue Sep 19, 2021 · 11 comments
Labels
NeedsFix release-blocker
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Sep 19, 2021

The new type checker no longer explains interface satisfaction errors during assignment and type assertion failures by saying what method is missing, nor by showing when signatures differ, nor by showing when methods are available that differ only in case. The messages also omit the context - "in argument to f", "in assignment" and so on. These also make the errors much easier to understand, especially in implicit conversions such as function argument assignment. (The fine-grained position information might provide similar precision, but it requires tool support or character counting for humans to use.)

These are all critical aids when writing Go code, which we have spent years refining and making useful. They are a key part of why people say Go has understandable error messages.

Note in the transcript below that the impossible type assertion does include the information in the signature mismatch case, but formatted in a single line, making it harder to compare. Please restore the old multiline format, which is more readable for people because the two things being compared are lined up one right below the other.

These details need to be ported forward to the new type checker before the Go 1.18 release.

The specific logic for the old errors is in cmd/compile/internal/typecheck's Assignop and tcDotType.

% cat x.go
package p

type I interface{ M(int) }

type T struct{}

type T2 struct{}

func (*T2) m(int)

type T3 struct{}

func (*T3) M(string) {}

func f(I)

func g() {
	f(new(T))
	var i I
	i = new(T)
	i = I(new(T))
	i = new(T2)
	i = new(T3)
	i = new(I)
	_ = i.(*T2)
	_ = i.(*T3)
	_ = i
}
% go tool compile x.go
x.go:18:4: incompatible type: cannot use new(T) (value of type *T) as I value
x.go:20:6: incompatible type: cannot use new(T) (value of type *T) as I value
x.go:21:8: cannot convert new(T) (value of type *T) to I
x.go:22:6: incompatible type: cannot use new(T2) (value of type *T2) as I value
x.go:23:6: incompatible type: cannot use new(T3) (value of type *T3) as I value
x.go:24:6: incompatible type: cannot use new(I) (value of type *I) as I value
x.go:25:6: impossible type assertion: i (variable of type I) (missing method M)
x.go:26:6: impossible type assertion: i (variable of type I) (wrong type for method M (have func(string), want func(int)))
% go1.17 tool compile x.go
x.go:18:7: cannot use new(T) (type *T) as type I in argument to f:
	*T does not implement I (missing M method)
x.go:20:4: cannot use new(T) (type *T) as type I in assignment:
	*T does not implement I (missing M method)
x.go:21:7: cannot convert new(T) (type *T) to type I:
	*T does not implement I (missing M method)
x.go:22:4: cannot use new(T2) (type *T2) as type I in assignment:
	*T2 does not implement I (missing M method)
		have m(int)
		want M(int)
x.go:23:4: cannot use new(T3) (type *T3) as type I in assignment:
	*T3 does not implement I (wrong type for M method)
		have M(string)
		want M(int)
x.go:24:4: cannot use new(I) (type *I) as type I in assignment:
	*I is pointer to interface, not interface
x.go:25:7: impossible type assertion:
	*T2 does not implement I (missing M method)
		have m(int)
		want M(int)
x.go:26:7: impossible type assertion:
	*T3 does not implement I (wrong type for M method)
		have M(string)
		want M(int)
% 

/cc @griesemer @findleyr

@rsc rsc added this to the Go1.18 milestone Sep 19, 2021
@rsc rsc added the NeedsFix label Sep 19, 2021
@rsc
Copy link
Contributor Author

@rsc rsc commented Sep 19, 2021

See also #48472.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 20, 2021

Change https://golang.org/cl/350697 mentions this issue: cmd/compile: print method details when interface conversion error

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Sep 20, 2021

One suggestion I made in #48472 which I think is worth considering as a possibility: I would almost always like to see a list of all the missing methods when some aren't implemented. That not only gives more context for the error, but also saves repeated compile cycles when filling out several required methods. If there are really many methods, we could truncate at 5 or so - usually it's only 2 or 3 in my experience.

@rsc
Copy link
Contributor Author

@rsc rsc commented Sep 20, 2021

Printing more than one sounds fine too. Just more than zero!

@neclepsio
Copy link

@neclepsio neclepsio commented Sep 22, 2021

I tried to build with https://golang.org/cl/350697 to debug a problem I have with generics, and found that assignableTo does not assign reason in every case (including, unfortunately, the one I'm hitting). This results, maybe among others, in an error message ending in ":" for line 85 of assignments.go.

@heschi
Copy link
Contributor

@heschi heschi commented Sep 29, 2021

Ping -- how's the CL going?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 29, 2021

We'll need to spend some time fine-tuning error messages. At the moment we have higher-priority items to work on.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Oct 7, 2021

Thanks for point me here @ericlagergren from #48833! I implemented the prior diagnostics in Go1.8 or so, so looks like it is time for me to roll up my sleeves one of these days.

@danscales
Copy link
Contributor

@danscales danscales commented Nov 10, 2021

I've started working on this, to take off some load from Robert.

@danscales danscales assigned danscales and unassigned odeke-em and griesemer Nov 10, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 11, 2021

Change https://golang.org/cl/363437 mentions this issue: cmd/compile: add missing method info for method with correct name except for case

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 11, 2021

Change https://golang.org/cl/363436 mentions this issue: cmd/compile: fix up types2 error messages to be more like Go1.17

gopherbot pushed a commit that referenced this issue Nov 12, 2021
When being used by the compiler, fix up types2 error messages to be more
like Go 1.17 compiler errors. In particular:

  - add information about which method is missing when a type is not
    assignable/convertible/etc. to an interface.

  - add information about any existing method which has the same name,
    but wrong type.

  - add extra hint in the case that the source or destination type is a
    pointer to an interface, rather than an interface.

  - add extra hint "need type assertion" in the case that the source is
    an interface that is implemented by the destination.

  - the following change in the CL stack also adds information about any
    existing method with a different name that only differs in case.

Include much of the new logic in a new common function
(*Checker).missingMethodReason().

types2 still adds a little more information in some cases then the Go
1.17 compiler. For example, it typically says "(value of type T)",
rather than "(type T)", where "value" could also be "constant",
"variable", etc.

I kept the types2 error messages almost all the same when types2 is not
used by the compiler. The only change (to reduce amount of compatibility
code) was to change "M method" phrasing in one case to "method M"
phrasing in one error message (which is the phrasing it uses in all
other cases). That is the reason that there are a few small changes in
types2/testdata/check/*.src.

Added new test test/fixedbugs/issue48471.go to test that the added
information is appearing correctly.

Also adjusted the pattern matching in a bunch of other
test/fixedbugs/*.go, now that types2 is producing error messages closer
to Go 1.17. Was able to remove a couple test files from the types2
exception list in run.go.

Updated #48471

Change-Id: I8af1eae6eb8a5541d8ea20b66f494e2e795e1956
Reviewed-on: https://go-review.googlesource.com/c/go/+/363436
Trust: Dan Scales <danscales@google.com>
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix release-blocker
Projects
None yet
Development

No branches or pull requests

8 participants