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: incorrect error message when assigning an interface to a constant #24755

Open
mundaym opened this Issue Apr 7, 2018 · 14 comments

Comments

Projects
None yet
6 participants
@mundaym
Member

mundaym commented Apr 7, 2018

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

go version devel +68c7cb25a7 Wed Apr 4 12:18:29 2018 +0100 darwin/amd64

Does this issue reproduce with the latest release?

Yes (go version go1.10.1 darwin/amd64).

What did you do?

Compiled https://play.golang.org/p/jzbFN6k7Jhk

What did you expect to see?

./const.go:10:7: const initializer I((*T)(nil)) is not a constant

What did you see instead?

./const.go:10:12: cannot convert (*T)(nil) (type *T) to type I:
	*T does not implement I (missing F method)

*T does implement I so this error message is wrong. The real issue is that the RHS is not a valid constant.

Note that the correct error message is shown if the declaration of F() is moved to be above the assertion in the file (i.e. https://play.golang.org/p/9f2SQfBfteA). If the const is changed to var then the program compiles correctly regardless of where F is declared.

@mvdan

This comment has been minimized.

Member

mvdan commented Apr 7, 2018

go/types gets this right, which further confirms your thinking:

f.go:10:11: I((*T)(nil)) (value of type I) is not constant

@mvdan mvdan added the NeedsFix label Apr 7, 2018

@odeke-em

This comment has been minimized.

Member

odeke-em commented Apr 9, 2018

Thank you for reporting this @mundaym

/cc @mdempsky @griesemer

@griesemer griesemer added this to the Go1.11 milestone Apr 9, 2018

@mvdan

This comment has been minimized.

Member

mvdan commented May 29, 2018

Smaller repro:

package p

type I interface{ F() }
type T struct{}

const _ = I(T{})

func (T) F() {}
@mvdan

This comment has been minimized.

Member

mvdan commented May 29, 2018

The issue seems to stem from how gc is designed. Reading main.go, I see how it typechecks nodes in phases:

// Phase 1: const, type, and names and types of funcs.
[...]
// Phase 2: Variable assignments.

So it seems to me like a possible fix would be to insert an extra phase before "variable assignments" called "constant declarations". That way, in our example above, func (T) F() would always be typechecked before const _ = I(T{}).

@griesemer @mdempsky thoughts? Is there a better way to fix this issue? (Edit: ended up sending this as a CL - see below)

@mvdan

This comment has been minimized.

Member

mvdan commented May 29, 2018

I guess that another potential fix would be to detect if we're in phase 1 in the typechecker, and avoid giving errors like missing F method. After all, there is no guarantee that all methods have been typechecked and added to their respective types until we're past that phase. But it seems like special-casing all these errors would be a game of whack-a-mole.

@gopherbot

This comment has been minimized.

gopherbot commented May 29, 2018

Change https://golang.org/cl/115096 mentions this issue: cmd/compile: typecheck types and funcs before consts

@griesemer griesemer modified the milestones: Go1.11, Go1.12 May 29, 2018

@griesemer

This comment has been minimized.

Contributor

griesemer commented May 29, 2018

Let's leave this alone for 1.11. It's more subtle that it seems (see my comments on the issue) and it's definitively not urgent.

@gopherbot gopherbot closed this in 9ce87a6 Oct 29, 2018

@mdempsky

This comment has been minimized.

Member

mdempsky commented Oct 29, 2018

Sorry I missed this issue and CL until now.

I'm not convinced the committed fix fully addresses this. Typechecking package-scope declarations can't really be split into phases; there's always a way to construct code that depends on any particular kind of declaration being typechecked before any other.

E.g., this test case still repros the issue:

package p

import "unsafe"

type x [unsafe.Sizeof(w)]int

type I interface{ F() }
type T struct{}

const w = I(T{})

func (T) F() {}

(And as a more superficial issue, there are now two phase "3"s in main.go.)

@mdempsky mdempsky reopened this Oct 29, 2018

@griesemer

This comment has been minimized.

Contributor

griesemer commented Oct 29, 2018

@mdempsky Agreed, I have also mentioned this in the CL (but the quick test case I tried to create didn't produce an error - see my CL feedback).

@griesemer

This comment has been minimized.

Contributor

griesemer commented Oct 29, 2018

Simpler repro (no need for "unsafe"), same error besides otherwise being incorrect:

package p

type x [w]int

type I interface{ F() }
type T struct{}

const w = I(T{})

func (T) F() {}

I'm going to roll back the change.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 29, 2018

Change https://golang.org/cl/145617 mentions this issue: cmd/compile: revert "typecheck types and funcs before consts"

@mdempsky

This comment has been minimized.

Member

mdempsky commented Oct 29, 2018

For what it's worth, while simply typechecking var/type/func declarations separately from const declarations doesn't work, I think the idea I sketched in #13890 to separate package-scope typechecking into "type resolution" and "constant evaluation" phases would work here.

gopherbot pushed a commit that referenced this issue Oct 29, 2018

cmd/compile: revert "typecheck types and funcs before consts"
This reverts commit 9ce87a6.

The fix addresses the specific test case, but not the general
problem.

Updates #24755.

Change-Id: I0ba8463b41b099b1ebf49759f88a423b40f70d58
Reviewed-on: https://go-review.googlesource.com/c/145617
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

@griesemer griesemer self-assigned this Oct 29, 2018

@mvdan

This comment has been minimized.

Member

mvdan commented Oct 31, 2018

Thanks for the follow-up - looking forward to seeing what a better fix would be like :)

@griesemer

This comment has been minimized.

Contributor

griesemer commented Dec 5, 2018

Too late for 1.12.

@griesemer griesemer modified the milestones: Go1.12, Go1.13 Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment