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: type checking for generic constraints doesn't work correctly #39754

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

cmd/go2go: type checking for generic constraints doesn't work correctly #39754

tdakkota opened this issue Jun 22, 2020 · 9 comments
Assignees
Milestone

Comments

@tdakkota
Copy link

@tdakkota tdakkota commented Jun 22, 2020

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

$ go version
go version devel +0a030888da Sat Jun 20 04:46:40 2020 +0000 windows/amd64

Does this issue reproduce with the latest release?

n/a

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

  • windows/amd64 with 0a03088
  • go2go playground

What did you do?

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

What did you expect to see?

go2go's typechecker error for line 57.

What did you see instead?

Successful compilation.

@tdakkota
Copy link
Author

@tdakkota tdakkota commented Jun 22, 2020

But

	_ = F2(string, Optional(int), Result(int))(OptionalFrom(int)(0), func(o Optional(int)) (r Result(int)) {
		return
	})

fails with

type checking failed for main
prog.go2:51:17: Optional(int) does not satisfy Box(V) (missing method Val)
@tdakkota
Copy link
Author

@tdakkota tdakkota commented Jun 22, 2020

Another example
https://go2goplay.golang.org/p/Juv92RyNgwh

crashes on 0a03088 and runs on playground.

Partial stack trace:

runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc0201e1580 stack=[0xc0201e0000, 0xc0401e0000]
fatal error: stack overflow

runtime stack:
runtime.throw(0xd3743b, 0xe)
	G:/workspace/exprm/go/src/runtime/panic.go:1116 +0x79
runtime.newstack()
	G:/workspace/exprm/go/src/runtime/stack.go:1059 +0x80d
runtime.morestack()
	G:/workspace/exprm/go/src/runtime/asm_amd64.s:449 +0x97

goroutine 1 [running]:
go/types.(*Checker).completeInterface(0xc0001665a0, 0x0, 0xc00016e380)
	G:/workspace/exprm/go/src/go/types/typexpr.go:818 +0xaba fp=0xc0201e1590 sp=0xc0201e1588 pc=0xbc2a1a
go/types.(*TypeParam).Bound(0xc0001bc140, 0x0)
	G:/workspace/exprm/go/src/go/types/type.go:811 +0xe5 fp=0xc0201e15e0 sp=0xc0201e1590 pc=0xbb3185
go/types.optype(0xd6cdc0, 0xc0001bc140, 0x0, 0x0)
	G:/workspace/exprm/go/src/go/types/type.go:830 +0x66 fp=0xc0201e1648 sp=0xc0201e15e0 pc=0xbb3226
go/types.(*TypeParam).Interface(0xc0001bc140, 0x0)
	G:/workspace/exprm/go/src/go/types/type.go:848 +0x4c fp=0xc0201e1698 sp=0xc0201e1648 pc=0xbb38ac
go/types.(*TypeParam).Interface(0xc0001bc180, 0x0)
	G:/workspace/exprm/go/src/go/types/type.go:848 +0x6c fp=0xc0201e16e8 sp=0xc0201e1698 pc=0xbb38cc
go/types.(*TypeParam).Interface(0xc0001bc140, 0x0)
	G:/workspace/exprm/go/src/go/types/type.go:848 +0x6c fp=0xc0201e1738 sp=0xc0201e16e8 pc=0xbb38cc
go/types.(*TypeParam).Interface(0xc0001bc180, 0x0)
	G:/workspace/exprm/go/src/go/types/type.go:848 +0x6c fp=0xc0201e1788 sp=0xc0201e1738 pc=0xbb38cc
go/types.(*TypeParam).Interface(0xc0001bc140, 0x0)
	G:/workspace/exprm/go/src/go/types/type.go:848 +0x6c fp=0xc0201e17d8 sp=0xc0201e1788 pc=0xbb38cc
go/types.(*TypeParam).Interface(0xc0001bc180, 0x0)
...
@cagedmantis cagedmantis added this to the Unreleased milestone Jun 22, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Jun 22, 2020

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 22, 2020

Looks like the type checker fails to report an error.

Smaller test case:

package main

type Optional(type T) struct {
	p   T
	set bool
}

func (o Optional(T)) Val() (T, bool) {
	return o.p, true
}

type Box(type T) interface {
	Val() (T, bool)
}

func F1(type V interface{}, A, B Box(V))() {}

func main() {
	F1(int, Optional(int), Optional(string))()
}

This requires that both Optional(int) and Optional(string) implement Box(int). Optional(int) does implement it, but Optional(string) does not. The type checker passes this without error but probably should not.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 25, 2020

@mmaedel I'm not sure what problem you are running into, but it's a different problem. Please open a separate issue. Thanks.

@tdakkota
Copy link
Author

@tdakkota tdakkota commented Jun 27, 2020

https://go2goplay.golang.org/p/jmaTevSNW0X
Seems related. Type checker can't compare type of result of func New with I generic type

UPD.
This bug is related to optype function.
When type checker tries to assign result of func New to I, optype is called

func optype(typ Type) Type { // typ is I generic 
	if t := typ.TypeParam(); t != nil { // I is a type param
		if u := t.Bound().allTypes; u != nil && u != typ { // t bound only by method list, not by type list, so allTypes is nil
			return u.Under()
		}
		return theTop // here function returns
	}
	return typ
}

Tuple(A, B) becomes T top type, i.e. Any

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 28, 2020

Change https://golang.org/cl/240339 mentions this issue: [dev.go2go] go/types: fix type check skipping for constraints when th…

gopherbot pushed a commit that referenced this issue Jun 29, 2020
…ere’s more than one generic constraint.

Check of first constraint was breaking the loop, so other constraints was skipped

Fixes #39754.

Change-Id: I48caa380472371a438c2d2029d1383d03dbf22df
GitHub-Last-Rev: bf418db
GitHub-Pull-Request: #39902
Reviewed-on: https://go-review.googlesource.com/c/go/+/240339
Reviewed-by: Robert Griesemer <gri@golang.org>
@tdakkota
Copy link
Author

@tdakkota tdakkota commented Jun 29, 2020

Seems to be fixed. Closing.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 30, 2020

Change https://golang.org/cl/240478 mentions this issue: [dev.go2go] go/types: fix broken test case

gopherbot pushed a commit that referenced this issue Jun 30, 2020
While at it, simplified test case a bit.

Fixes #39935.
Updates #39754.

Change-Id: Ia3b51f23807d25e62113757c51a660c7e3a9b381
Reviewed-on: https://go-review.googlesource.com/c/go/+/240478
Reviewed-by: Robert Griesemer <gri@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.