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: erroneously accepts recursive type aliases #25141

Closed
mdempsky opened this issue Apr 27, 2018 · 5 comments
Closed

go/types: erroneously accepts recursive type aliases #25141

mdempsky opened this issue Apr 27, 2018 · 5 comments
Assignees
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Apr 27, 2018

18130-type-alias explicitly states that type T = *T is invalid, but go/types accepts it:

$ cat v.go
package p
type T = *T
$ gotype v.go
$ echo $?
0

cmd/compile does not.

Notably though, I don't see any wording in the text actually added to the Go spec that precludes this.

/cc @griesemer

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Apr 27, 2018

Tangentially, I'm questioning what the fundamental difference (if any) is between type T = *T and type U interface { F() interface { U } }. Why disallow one, but allow the other?

We sorta support the latter, but it's sketchy and a recurring thorn. I think if we properly fix support for it, it would make supporting the former trivial. But maybe it's just better to disallow both.

Loading

@go101
Copy link

@go101 go101 commented Apr 28, 2018

Looks the difference is whether or not the declaration is an alias declaration.
type U = interface { F() interface { U } } is also denied by gc, but type T *T is also accepted by gc.

Loading

@andybons andybons added this to the Unplanned milestone Apr 30, 2018
@griesemer griesemer self-assigned this Apr 30, 2018
@griesemer griesemer removed this from the Unplanned milestone Apr 30, 2018
@griesemer griesemer added this to the Go1.11 milestone Apr 30, 2018
@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 30, 2018

Somewhat related: #23139 .

go/types has issues with detecting certain cycles. I hope to address this for 1.11.

Per the original type-alias design doc from @rsc, it must be possible to "expand out" type alias declarations; this wouldn't be possible for type T = *T without expanding endlessly. You are correct that the spec doesn't state that, though. I filed #25187.

I think it would be nice to accept all these, but our depth-first based type-checking approaches have troubles with them; and ad-hoc approaches tend to be incorrect. We need to either disallow these cases or have a sound way to describe what we allow and what we don't, and also a (understood and correct) way of type-checking them w/o incurring undue cost.

At the same time, these are probably not urgent.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 25, 2018

Change https://golang.org/cl/114517 mentions this issue: go/types: initial framework for marking-based cycle detection

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 31, 2018

Change https://golang.org/cl/115455 mentions this issue: go/types: use color-marking based cycle detection at package level

Loading

gopherbot pushed a commit that referenced this issue May 31, 2018
The existing code explicitly passes a (type name) path around
to determine cycles; it also restarts the path for types that
"break" a cycle (such as a pointer, function, etc.). This does
not work for alias types (whose cycles are broken in a different
way). Furthermore, because the path is not passed through all
type checker functions that need it, we can't see the path or
use it for detection of some cycles (e.g. cycles involving array
lengths), which required ad-hoc solutions in those cases.

This change introduces an explicit marking scheme for any kind
of object; an object is painted in various colors indicating
its state. It also introduces an object path (a stack) main-
tained with the Checker state, which is available in all type
checker functions that need access to it.

The change only introduces these mechanisms and exercises the
basic functionality, with no effect on the existing code for
now.

For #25141.

Change-Id: I7c28714bdafe6c8d9afedf12a8a887554237337c
Reviewed-on: https://go-review.googlesource.com/114517
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot gopherbot closed this in 462c182 May 31, 2018
@golang golang locked and limited conversation to collaborators May 31, 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
5 participants