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

go/types: incorrect "no such field/method" error in cyclic interface #18395

Closed
alandonovan opened this issue Dec 21, 2016 · 10 comments
Closed

go/types: incorrect "no such field/method" error in cyclic interface #18395

alandonovan opened this issue Dec 21, 2016 · 10 comments

Comments

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Dec 21, 2016

The following excerpt minimized from a real program causes go/types to emit a spurious error:

$ cat a.go
package base

type A interface {
        B
}

type B interface {
        C
}

type C interface {
        D
        F() A
}

type D interface {
        G() B
}

var _ = A(nil).G // error: no such field or method
$ ssadump a.go
a.go:20:9: invalid operation: A(nil) (value of type A) has no field or method G
ssadump: couldn't load packages due to errors: base
@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 23, 2017

Nice. It appears that the existing algorithm for type-checking interfaces is incorrect, and this subtle example exposes the problem. Type-checking proceeds as follows:

$ gotype y.go
y.go:16:6:	-- declaring D
y.go:16:8:	.  interface{G() B}
y.go:17:10:	.  .  func() B
y.go:17:13:	.  .  .  B
y.go:7:6:	.  .  .  .  -- declaring B
y.go:7:8:	.  .  .  .  .  interface{C}
y.go:8:9:	.  .  .  .  .  .  C
y.go:11:6:	.  .  .  .  .  .  .  -- declaring C
y.go:11:8:	.  .  .  .  .  .  .  .  interface{D; F() A}
y.go:12:9:	.  .  .  .  .  .  .  .  .  D
y.go:12:9:	.  .  .  .  .  .  .  .  .  => D
y.go:13:10:	.  .  .  .  .  .  .  .  .  func() A
y.go:13:13:	.  .  .  .  .  .  .  .  .  .  A
y.go:3:6:	.  .  .  .  .  .  .  .  .  .  .  -- declaring A
y.go:3:8:	.  .  .  .  .  .  .  .  .  .  .  .  interface{B}
y.go:4:9:	.  .  .  .  .  .  .  .  .  .  .  .  .  B
y.go:4:9:	.  .  .  .  .  .  .  .  .  .  .  .  .  => B
y.go:3:8:	.  .  .  .  .  .  .  .  .  .  .  .  => interface{B}
y.go:3:6:	.  .  .  .  .  .  .  .  .  .  .  => type A interface{B}
y.go:13:13:	.  .  .  .  .  .  .  .  .  .  => A
y.go:13:10:	.  .  .  .  .  .  .  .  .  => func() A
y.go:11:8:	.  .  .  .  .  .  .  .  => interface{F() A; D}
y.go:11:6:	.  .  .  .  .  .  .  => type C interface{F() A; D}
y.go:8:9:	.  .  .  .  .  .  => C
y.go:7:8:	.  .  .  .  .  => interface{C}
y.go:7:6:	.  .  .  .  => type B interface{C}
y.go:17:13:	.  .  .  => B
y.go:17:10:	.  .  => func() B
y.go:16:8:	.  => interface{G() B}
y.go:16:6:	=> type D interface{G() B}
y.go:20:5:	-- declaring _
y.go:20:9:	.  A(nil).F
y.go:20:9:	.  .  A(nil)
y.go:20:9:	.  .  .  A
y.go:20:9:	.  .  .  => A (type)
y.go:20:11:	.  .  .  nil
y.go:20:11:	.  .  .  => nil (untyped nil value)
y.go:20:9:	.  .  => A(nil) (value of type A)
y.go:20:9: invalid operation: A(nil) (value of type A) has no field or method F
y.go:20:9:	.  => A(nil).F (invalid operand)
y.go:20:5:	=> var _ invalid type

The problem is that the collection of methods for method A depends on B at a time when interface B is not yet finished collecting its methods.

A correct (?) approach may require the collection of all methods first, for all interfaces, before proceeding with type-checking their signatures. This may also eliminate the need for a topological sort of the interfaces before type-checking them (see ordering.go, Checker.resolveOrder). Either way, a correct solution requires some serious thinking.

This is too invasive a change for Go1.9. Postponing to 1.10.

Loading

@griesemer griesemer added this to the Go1.10 milestone Jun 23, 2017
@griesemer griesemer removed this from the Go1.9 milestone Jun 23, 2017
@griesemer griesemer changed the title go/types: spurious "no such field/method" error in cyclic interface go/types: incorrect "no such field/method" error in cyclic interface Jun 23, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 21, 2017

Change https://golang.org/cl/78955 mentions this issue: go/types: add debugging code to detect use of incomplete interfaces

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 21, 2017

Status update: Started working on this. Have a basic solution based on proposed solution above (collecting methods first, before creating the respective types). Working on cleaning up the approach (it's a lot of code).

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Nov 22, 2017

If it's a lot of code maybe this should wait for Go 1.11?

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 22, 2017

@rsc: Maybe. The issue is that not having this will prevent vet from working in some cases. They are rare but so far three have been filed. I want to work on this just a tad longer to see what the final outcome is.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 23, 2017

Change https://golang.org/cl/79575 mentions this issue: go/types: correctly compute method set of some recusive interfaces

Loading

gopherbot pushed a commit that referenced this issue Nov 27, 2017
The comment for phase 2 of checker.interfaceType (typexpr.go:517)
requires that embedded interfaces be complete for correctness of
the algorithm.

Yet, the very next comment (typexpr.go:530) states that underlying
embedded interfaces may in fact be incomplete.

This is in fact the case and the underlying bug in issue #18395.

This change makes sure that new interface types are marked complete
when finished (per the implicit definition in Interface.Complete,
type.go:302). It also adds a check, enabled in debug mode only, to
detect the use of incomplete embedded interfaces during construction
of a new interface. In debug mode, this check fails for the testcase
in the issue (and several others).

This change has no noticeable impact with debug mode disabled.

For #18395.

Change-Id: Ibb81e47257651282fb3755a80a36ab5d392e636d
Reviewed-on: https://go-review.googlesource.com/78955
Reviewed-by: Alan Donovan <adonovan@google.com>
@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 27, 2017

Decided to postpone submit of fix to 1.11 in discussion with @rsc, @ianlancetaylor . See also related #22890.

Loading

@griesemer griesemer removed this from the Go1.10 milestone Nov 29, 2017
@griesemer griesemer added this to the Go1.11 milestone Nov 29, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 29, 2017

Change https://golang.org/cl/80455 mentions this issue: go/types: panic when recognizing issue #18395.

Loading

gopherbot pushed a commit that referenced this issue Nov 29, 2017
The fix (CL 79575) for #18395 is too risky at this stage of the Go 1.10
release process.

Since issue #18395 is easily recognized (but not easily fixed), report
an error instead of silently continuing. This avoids inscrutable follow
on errors.

Also, make sure all empty interfaces are "completed", and adjust
printing code to report incomplete interfaces.

For #18395.

Change-Id: I7fa5f97ff31ac9775c9a6d318fce9f526b0350cd
Reviewed-on: https://go-review.googlesource.com/80455
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 19, 2017

Change https://golang.org/cl/84898 mentions this issue: go/types: perform delayed tests even for types.Eval

Loading

@gopherbot gopherbot closed this in dd44895 Feb 12, 2018
gopherbot pushed a commit that referenced this issue Feb 12, 2018
R=go1.11

types.Eval historically never evaluated any delayed tests, which
included verification of validity of map keys, but also function
literal bodies.

Now, embedded interfaces are also type-checked in a delayed fashion,
so it becomes imperative to do all delayed checks for eval (otherwise
obviously incorrect type expressions are silently accepted).

Enabling the delayed tests also removes the restriction that function
literals were not type-checked.

Also fixed a bug where eval wouldn't return a type-checking error
because check.handleBailout was using the wrong err variable.

Added tests that verify that method set computation is using the
right types when evaluating interfaces with embedded types.

For #18395.
For #22992.

Change-Id: I574fa84568b5158bca4b4ccd4ef5abb616fbf896
Reviewed-on: https://go-review.googlesource.com/84898
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 20, 2018

Change https://golang.org/cl/108555 mentions this issue: cmd/go: respect vet typecheck failures again

Loading

gopherbot pushed a commit that referenced this issue Apr 26, 2018
Now that #18395 is fixed, let's see if we can insist
on vet during go test being able to type-check
packages again.

Change-Id: Iaa55a4d9c582ba743df2347d28c24f130e16e406
Reviewed-on: https://go-review.googlesource.com/108555
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@golang golang locked and limited conversation to collaborators Apr 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants