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 error message with mutually recursive interfaces #33656

Closed
griesemer opened this issue Aug 14, 2019 · 7 comments
Closed

go/types: incorrect error message with mutually recursive interfaces #33656

griesemer opened this issue Aug 14, 2019 · 7 comments
Assignees
Milestone

Comments

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 14, 2019

This is just one example of where the current interface type-checking still goes wrong (this assumes https://golang.org/cl/190258):

package p

type A interface {
	a() interface {
		AB
	}
}

type B interface {
	a() interface {
		AB
	}
}

type AB interface {
	a() interface {
		A
		B
	}
	b() interface {
		A
		B
	}
}

go/types testing reports:

$ go test -run Check$ -files=$HOME/tmp/x.go -errlist
--- FAIL: TestCheck (0.00s)
    check_test.go:271: /Users/gri/tmp/x.go:18:3: duplicate method a
    check_test.go:271: /Users/gri/tmp/x.go:17:3:        other declaration of a
    check_test.go:271: /Users/gri/tmp/x.go:22:3: duplicate method a
    check_test.go:271: /Users/gri/tmp/x.go:21:3:        other declaration of a
FAIL
exit status 1
FAIL    go/types        0.079s

Yet, with the spec change for #6977 (and CL 190258) this should not result in any errors.

(Note that cmd/compile also has trouble with this code if changes https://golang.org/cl/187519 et. al.) are in. The corresponding compiler issue is #6589).

Not urgent as this is a rather esoteric case. But at some point we should rethink type-checking of such mutually recursive data structures.

@dmitshur dmitshur added this to the Go1.14 milestone Aug 15, 2019
@dmitshur dmitshur removed this from the Go1.14 milestone Aug 15, 2019
@dmitshur dmitshur added this to the Unplanned milestone Aug 15, 2019
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 22, 2019

Ughhh.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 23, 2019

Change https://golang.org/cl/191418 mentions this issue: go/types: postpone interface method type comparison to the end

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 23, 2019

Change https://golang.org/cl/191458 mentions this issue: go/types: process each segment of delayed actions in FIFO order

Loading

@gopherbot gopherbot closed this in 29d3c56 Aug 26, 2019
gopherbot pushed a commit that referenced this issue Aug 26, 2019
The stack of delayed actions is grown by pushing a new action
on top with Checker.later. Checker.processDelayed processes
all actions above a top watermark and then resets the stack
to top.

Until now, pushed actions above the watermark were processed
in LIFO order. This change processes them in FIFO order, which
seems more natural (if an action A was delayed before an action
B, A should be processed before B for that stack segment).

(With this change, Checker.later could be used instead of
Checker.atEnd to postpone interface method type comparison
and then the specific example in issue #33656 does type-check.
However, in general we want interface method type comparisons
to run after all interfaces are completed. With Checker.later
we may still end up mixing interface completions and interface
method type comparisons in ways leading to other errors for
sufficiently convoluted code.)

Also, move Checker.processDelayed from resolver.go to check.go.

Change-Id: Id31254605e6944c490eab410553fff907630cc64
Reviewed-on: https://go-review.googlesource.com/c/go/+/191458
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
tomocy added a commit to tomocy/go that referenced this issue Sep 1, 2019
Introduce a new list of final actions that is executed at the
end of type checking and use it to collect method type comparisons
and also map key checks.

Fixes golang#33656.

Change-Id: Ia77a35a45a9d7eaa7fc3e9e19f41f32dcd6ef9d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/191418
Reviewed-by: Alan Donovan <adonovan@google.com>
tomocy added a commit to tomocy/go that referenced this issue Sep 1, 2019
The stack of delayed actions is grown by pushing a new action
on top with Checker.later. Checker.processDelayed processes
all actions above a top watermark and then resets the stack
to top.

Until now, pushed actions above the watermark were processed
in LIFO order. This change processes them in FIFO order, which
seems more natural (if an action A was delayed before an action
B, A should be processed before B for that stack segment).

(With this change, Checker.later could be used instead of
Checker.atEnd to postpone interface method type comparison
and then the specific example in issue golang#33656 does type-check.
However, in general we want interface method type comparisons
to run after all interfaces are completed. With Checker.later
we may still end up mixing interface completions and interface
method type comparisons in ways leading to other errors for
sufficiently convoluted code.)

Also, move Checker.processDelayed from resolver.go to check.go.

Change-Id: Id31254605e6944c490eab410553fff907630cc64
Reviewed-on: https://go-review.googlesource.com/c/go/+/191458
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
Introduce a new list of final actions that is executed at the
end of type checking and use it to collect method type comparisons
and also map key checks.

Fixes golang#33656.

Change-Id: Ia77a35a45a9d7eaa7fc3e9e19f41f32dcd6ef9d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/191418
Reviewed-by: Alan Donovan <adonovan@google.com>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
The stack of delayed actions is grown by pushing a new action
on top with Checker.later. Checker.processDelayed processes
all actions above a top watermark and then resets the stack
to top.

Until now, pushed actions above the watermark were processed
in LIFO order. This change processes them in FIFO order, which
seems more natural (if an action A was delayed before an action
B, A should be processed before B for that stack segment).

(With this change, Checker.later could be used instead of
Checker.atEnd to postpone interface method type comparison
and then the specific example in issue golang#33656 does type-check.
However, in general we want interface method type comparisons
to run after all interfaces are completed. With Checker.later
we may still end up mixing interface completions and interface
method type comparisons in ways leading to other errors for
sufficiently convoluted code.)

Also, move Checker.processDelayed from resolver.go to check.go.

Change-Id: Id31254605e6944c490eab410553fff907630cc64
Reviewed-on: https://go-review.googlesource.com/c/go/+/191458
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 17, 2019

Change https://golang.org/cl/195837 mentions this issue: go/types: make sure interfaces are complete before comparing them

Loading

gopherbot pushed a commit that referenced this issue Sep 17, 2019
Complete interfaces before comparing them with Checker.identical.
This requires passing through a *Checker to various functions that
didn't need this before.

Verified that none of the exported API entry points for interfaces
that rely on completed interfaces are used internally except for
Interface.Empty. Verified that interfaces are complete before
calling Empty on them, and added a dynamic check in the exported
functions.

Unfortunately, this fix exposed another problem with an esoteric
test case (#33656) which we need to reopen.

Fixes #34151.
Updates #33656.

Change-Id: I4e14bae3df74a2c21b565c24fdd07135f22e11c0
Reviewed-on: https://go-review.googlesource.com/c/go/+/195837
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Sep 18, 2019

Loading

@griesemer griesemer reopened this Sep 18, 2019
@griesemer griesemer self-assigned this Sep 18, 2019
@griesemer griesemer removed this from the Unplanned milestone Sep 18, 2019
@griesemer griesemer added this to the Go1.14 milestone Sep 18, 2019
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 18, 2019

Ughhh.

Loading

@rsc rsc removed this from the Go1.14 milestone Oct 9, 2019
@rsc rsc added this to the Backlog milestone Oct 9, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 7, 2020

Change https://golang.org/cl/241263 mentions this issue: go/types: update test case to exercise mutually recursive interfaces

Loading

@gopherbot gopherbot closed this in 494ec85 Aug 24, 2020
@golang golang locked and limited conversation to collaborators Aug 24, 2021
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
5 participants