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/go2go: recursive constraints doesn't work correctly #39680

Closed
tdakkota opened this issue Jun 18, 2020 · 9 comments
Closed

cmd/go2go: recursive constraints doesn't work correctly #39680

tdakkota opened this issue Jun 18, 2020 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@tdakkota
Copy link

tdakkota commented Jun 18, 2020

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

n/a

Does this issue reproduce with the latest release?

n/a

What operating system and processor architecture are you using (go env)?

n/a

What did you do?

https://go2goplay.golang.org/p/jWH4GUTN-pX

What did you expect to see?

type checking failed for main
prog.go2:54:2: Ident does not satisfy Expr (struct{} not found in Expr, Stmt)
prog.go2:55:2: Ident does not satisfy Node (struct{} not found in Expr, Stmt)

or just

type checking failed for main
prog.go2:55:2: Ident does not satisfy Node (struct{} not found in Expr, Stmt)

What did you see instead?

type checking failed for main
prog.go2:54:2: Ident does not satisfy Expr (struct{} not found in ⊥)
prog.go2:55:2: Ident does not satisfy Node (struct{} not found in Expr, Stmt)
@tdakkota
Copy link
Author

tdakkota commented Jun 18, 2020

Another example

What did you do?

https://go2goplay.golang.org/p/kd9puCHjnbE

What did you expect to see?

compilation fail

What did you see instead?

runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc0203c0390 stack=[0xc0203c0000, 0xc0403c0000]
fatal error: stack overflow

runtime stack:
runtime.throw(0x69220d, 0xe)
	/usr/local/go-faketime/src/runtime/panic.go:1116 +0x72
runtime.newstack()
	/usr/local/go-faketime/src/runtime/stack.go:1059 +0x78d
runtime.morestack()
	/usr/local/go-faketime/src/runtime/asm_amd64.s:449 +0x8f
...

@beoran
Copy link

beoran commented Jun 18, 2020

In the latter example there is the constraint

type constr(type T) interface {
	type T
}

I think that such a constraint is meaningless since, if I understand correctly, it simply reduces to the type T itstelf. E.g. such a constr(int) simply would be int. What is the practical use of such a recursive constraint? If there isn't any it should probably be not be allowed.

@tdakkota
Copy link
Author

First example is example of practical use. It's ast.Node, but for Go2.
I just tried use type constraint to make a tagged union.

@andybons
Copy link
Member

@ianlancetaylor @griesemer

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 18, 2020
@andybons andybons added this to the Unplanned milestone Jun 18, 2020
@tdakkota
Copy link
Author

Change

go/src/go/types/type.go

Lines 829 to 837 in 5e75416

func optype(typ Type) Type {
if t := typ.TypeParam(); t != nil {
if u := t.Bound().allTypes; u != nil {
return u
}
return theTop
}
return typ
}

to

func optype(typ Type) Type {
	if t := typ.TypeParam(); t != nil {
		u := t.Bound().allTypes
		if u != nil && typ != u { // prevent infinite recursion
			return u
		}
		return theTop
	}
	return typ
}

resolves the problem

@griesemer griesemer self-assigned this Jun 18, 2020
@griesemer
Copy link
Contributor

Your first example is simply incorrect code: Given an interface E embedded in an interface I as in:

type I interface { E; type ... }

the resulting type list of I is the intersection of the type lists declared in I and E. This is so we preserve the general property that if I embeds E then I implements E. This is also documented in the design draft.

This code works: https://go2goplay.golang.org/p/5ddBkbO2ZJb

@bcmills
Copy link
Contributor

bcmills commented Jun 19, 2020

@beoran

I think that such a constraint is meaningless since, if I understand correctly, it simply reduces to the type T itstelf.

As currently implemented, if T is an interface, then type T itself matches all subtypes of T (that is, all interface and concrete types that implement T), whereas interface { type T } matches only (literal and defined) types whose underlying type is exactly the same interface type (with no additional methods).

@gopherbot
Copy link

Change https://golang.org/cl/238863 mentions this issue: [dev.go2go] go/types: avoid endless recursion when computing operational types

gopherbot pushed a commit that referenced this issue Jun 19, 2020
…nal types

The operational type for a type parameter T constrained by
interface { type T } is the top type, not itself. The latter
can lead to infinite recursions.

Fix suggested by @tdakkota.

Fixes #39680.

Change-Id: I8a6a6c503f09294b9276965f1d9e05c5d22ec912
Reviewed-on: https://go-review.googlesource.com/c/go/+/238863
Reviewed-by: Robert Griesemer <gri@golang.org>
@griesemer
Copy link
Contributor

@tdakkota Thanks for your 2nd example and the suggested fix. It is exactly right.

This is now fixed in dev.go2go.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants