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

Open
mundaym opened this issue Apr 7, 2018 · 17 comments
Open
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mundaym
Copy link
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
Copy link
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 The path to resolution is known, but the work has not been done. label Apr 7, 2018
@odeke-em
Copy link
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
Copy link
Member

mvdan commented May 29, 2018

Smaller repro:

package p

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

const _ = I(T{})

func (T) F() {}

@mvdan
Copy link
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
Copy link
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
Copy link

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
Copy link
Contributor

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.

@mdempsky
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link

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

@mdempsky
Copy link
Member

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
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
Copy link
Member

mvdan commented Oct 31, 2018

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

@griesemer
Copy link
Contributor

Too late for 1.12.

@griesemer griesemer modified the milestones: Go1.12, Go1.13 Dec 5, 2018
@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Dec 5, 2018
@griesemer griesemer modified the milestones: Go1.13, Go1.14 May 6, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@mundaym
Copy link
Member Author

mundaym commented Nov 25, 2020

Not sure when this got fixed but the Go playground example now gives the correct error message. Closing this.

@mundaym mundaym closed this as completed Nov 25, 2020
@mdempsky
Copy link
Member

I don't think it does? I'm still seeing the wrong error message ("does not implement") at https://play.golang.org/p/jzbFN6k7Jhk.

@mdempsky mdempsky reopened this Nov 25, 2020
@mundaym
Copy link
Member Author

mundaym commented Nov 25, 2020

Sorry, yes you're right, I looked at the wrong link.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Triage Backlog
Development

No branches or pull requests

7 participants