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: go vet fails with illegal cycle for github.com/yuin/gopher-lua #26124

Closed
griesemer opened this issue Jun 29, 2018 · 11 comments
Closed

go/types: go vet fails with illegal cycle for github.com/yuin/gopher-lua #26124

griesemer opened this issue Jun 29, 2018 · 11 comments

Comments

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 29, 2018

To reproduce:

go get github.com/yuin/gopher-lua
cd $GOPATH/src/github.com/yuin/gopher-lua/
go vet

reports

$ go vet
# github.com/yuin/gopher-lua
./alloc.go:14:7: illegal cycle in declaration of preloadLimit
./alloc.go:14:7: 	preloadLimit refers to
./config.go:14:6: 	LNumber refers to
./value.go:139:19: 	assertFunction refers to
./value.go:177:6: 	LFunction refers to
./value.go:184:6: 	LGFunction refers to
./value.go:203:6: 	LState refers to
./state.go:209:6: 	registry refers to
./alloc.go:20:6: 	allocator refers to
./alloc.go:14:7: 	preloadLimit
vet: typecheck failures
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Jun 29, 2018

Definitively a type-checker failure. The new cycle detection code is overly aggressive.

Loading

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Jun 29, 2018

Independent reproducer:

$ cat x.go
package p

const preloadLimit LNumber = 128

type LNumber float64

func (LNumber) assertFunction() *LFunction

type LFunction struct {
	GFunction LGFunction
}

type LGFunction func(*LState)

type LState struct {
	reg *registry
}

type registry struct {
	alloc *allocator
}

type allocator struct {
	_ [int(preloadLimit)]int
}

Running gotype in it yields:

$ gotype x.go 
x.go:3:7: illegal cycle in declaration of preloadLimit
x.go:3:7: 	preloadLimit refers to
x.go:5:6: 	LNumber refers to
x.go:7:16: 	assertFunction refers to
x.go:9:6: 	LFunction refers to
x.go:13:6: 	LGFunction refers to
x.go:15:6: 	LState refers to
x.go:19:6: 	registry refers to
x.go:23:6: 	allocator refers to
x.go:3:7: 	preloadLimit

This is awfully wrong. Will fix in the next few days.

Loading

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Jun 29, 2018

A trivial fix is to set

const useCycleMarking = false

in go/types/decl.go:57. This will disable the new code and the bug disappears (but it will also undo some other fixes, and the test suite will need to disable some tests).

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Jun 29, 2018

Shorter (go/types says C -> T -> M -> C):

package p
const C T = 1
type T int
func (T) M() [C]T

It seems to me that the "C -> T" link is spurious. For cycle detection it seems like a constant shouldn't care about its own type or initializer, so this should be cycle-free too:

package p
const C = T(1)
type T int
func (T) M() [C]T

But maybe I am missing some subtlety around constants. It's been a while.

Loading

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Jun 29, 2018

Also the cycle via the method (assertFunction) is odd. I think it shouldn't be very hard to fix, probably tomorrow. Thanks for the shorter reproducer. (I kept the longer one so I have a test case for the original problem.)

Loading

@cznic
Copy link
Contributor

@cznic cznic commented Jun 29, 2018

@rsc I'm using elsewhere a slightly different algorithm that breaks this cycle after C->T. The difference is in that methods are typechecked separately, after non-methods.

Snippet of actual code

        cs := &cstack{p: p}
	for _, sf := range p.SourceFiles {
		for _, d := range sf.TopLevelDecls {
			if x, ok := d.(*MethodDecl); ok {
				cs.reset().check(x, nil)
			}
		}
	}
	for _, sf := range p.SourceFiles {
		for _, d := range sf.TopLevelDecls {
			if _, ok := d.(*MethodDecl); !ok {
				cs.reset().check(d, nil)
			}
		}
	}
	p.checkEnqueued(cs)
	for _, sf := range p.SourceFiles {
		for _, d := range sf.InitFunctions {
			d.(*FuncDecl).checkBody(cs)
		}
	}
	p.checkEnqueued(cs)
	for _, sf := range p.SourceFiles {
		for _, d := range sf.TopLevelDecls {
			switch x := d.(type) {
			case *FuncDecl:
				x.checkBody(cs)
			case *MethodDecl:
				x.checkBody(cs)
			}
		}
	}
	p.checkEnqueued(cs)

For cycle detection it seems like a constant shouldn't care about its own type or initializer,

I think that would not be correct.

Loading

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Jun 29, 2018

@cznic Thanks for this. We can't completely change the algorithm at this point but I agree that methods should be handled separately. There's an independent/related issue open for that (#23203).

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 29, 2018

Change https://golang.org/cl/121755 mentions this issue: go/types: correctly compute cycle length

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 29, 2018

Change https://golang.org/cl/121757 mentions this issue: go/types: ignore artificial cycles introduced via method declarations

Loading

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Jun 29, 2018

The CL https://go-review.googlesource.com/c/go/+/121757 fixes this issue. Running go/vet on the repo now produces:

$ go vet
# github.com/yuin/gopher-lua
./channellib.go:49: reflect.SelectCase composite literal uses unkeyed fields
./compile.go:1051: unreachable code
./compile.go:1052: unreachable code
./compile.go:1111: unreachable code
./state.go:1142: unreachable code
./utils.go:260: reflect.SliceHeader composite literal uses unkeyed fields
./vm.go:777: self-assignment of cf.ReturnBase to cf.ReturnBase
./vm.go:779: self-assignment of cf.NRet to cf.NRet
./vm.go:1229: unreachable code
./script_test.go:57: result of (*lua.FunctionProto).String call not used
./state_test.go:244: unreachable code
./state_test.go:262: unreachable code

all of which look correct to me.

Loading

@ianlancetaylor ianlancetaylor changed the title go/types (go vet): go vet fails with illegal cycle for github.com/yuin/gopher-lua go/types: go vet fails with illegal cycle for github.com/yuin/gopher-lua Jun 29, 2018
gopherbot pushed a commit that referenced this issue Jul 10, 2018
The existing algorithm assumed that the length of a cycle was simply
the number of elements in the cycle slice starting at the start object.
However, we use a special "indir" indirection object to indicate
pointer and other indirections that break the inline placement of
types in composite types. These indirection objects don't exist as
true named type objects, so don't count them anymore.

This removes an unnecessary cycle error in one of the existing tests
(testdata/issues.src:100).

Also:
- added more tracing support (only active if tracing is enabled)
- better documentation in parts
- r/check.typ/check.typExpr/ in a few of places where we don't
  need to record a type indirection

Found while investigating #26124.

Change-Id: I45341743225d979a72af3fbecfa05012b32fab67
Reviewed-on: https://go-review.googlesource.com/121755
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot gopherbot closed this in deefcb2 Jul 10, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 5, 2018

Change https://golang.org/cl/139899 mentions this issue: go/types: remove work-around for issue #26124

Loading

gopherbot pushed a commit that referenced this issue Oct 5, 2018
This work-around is not needed anymore now that method
signatures are type-checked separately from their receiver
base types: no artificial cycles are introduced anymore
and so there is no need to artificially cut them.

Updates #26124.

Change-Id: I9d50171f12dd8977116a5d3f63ac39a06b1cd492
Reviewed-on: https://go-review.googlesource.com/c/139899
Reviewed-by: Alan Donovan <adonovan@google.com>
@golang golang locked and limited conversation to collaborators Oct 5, 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