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

astutil.CopyExpr does not support *ast.FuncType #1134

Closed
tux21b opened this issue Nov 26, 2021 · 11 comments
Closed

astutil.CopyExpr does not support *ast.FuncType #1134

tux21b opened this issue Nov 26, 2021 · 11 comments
Labels
bug started Issues we've started working on

Comments

@tux21b
Copy link

@tux21b tux21b commented Nov 26, 2021

Thanks for fixing #1133, this issue is some kind of follow-up. astutil.CopyExpr panics if it encounters a *ast.FuncType.

panic: unreachable: *ast.FuncType

goroutine 152 [running]:
honnef.co/go/tools/go/ast/astutil.CopyExpr({0x986b58, 0xc00028e048})
        /home/christoph/go/pkg/mod/honnef.co/go/tools@v0.3.0-0.dev.0.20211125235944-030b6968cbcd/go/ast/astutil/util.go:215 +0x123a

Reproducer:

var fn interface{} = func() bool {
	return true
}
if !(true && fn.(func() bool)()) {
	fmt.Println("hello")
}

The original code is some transpiled code in gitlab.com/cznic/sqlite/lib/sqlite_linux_amd64.go:26740 and is also triggered by quickfix.CheckDeMorgan:

if !((*(*func(*libc.TLS, int32, uintptr) int32)(unsafe.Pointer((uintptr(unsafe.Pointer(&aSyscall)) + 5*24 + 8 /* &.pCurrent */))))(tls, (*UnixFile)(unsafe.Pointer(pDbFd)).Fh, bp+8 /* &sStat */) != 0) {
	goto __3
}

Building upon your most recent commit 030b696, i guess it is fine if we just return false for now.

@tux21b tux21b added bug needs-triage Newly filed issue that needs triage labels Nov 26, 2021
@dominikh
Copy link
Owner

@dominikh dominikh commented Nov 26, 2021

I feel like at this point we should just make CopyExpr work for all types, probably using reflection instead of hand-written code. Otherwise you'll just keep throwing more weird code at me that I failed to anticipate (the type switch in CopyExpr was meant to be exhaustive, but I clearly didn't consider type assertions to function types, who knows what else I've missed…)

Whether or not DeMorgan should skip these is a different question, but the limitations of CopyExpr have turned into a footgun.

@dominikh dominikh removed the needs-triage Newly filed issue that needs triage label Nov 26, 2021
@tux21b
Copy link
Author

@tux21b tux21b commented Nov 26, 2021

First I wanted to let you know that I already feel slightly bad for throwing lots of weird code at you. The real code above does not even use type assertions, but castings of unsafe function pointers.

Having a CopyExpr that works all the time would be great. Reflection would fail for go/ast types that have private fields or pointers that must not be copied. The Fprint function within go/ast is reflection based as well, so I guess it is somewhat safe to assume that it is safe to use the types with reflection. Some of the types are defined differently for Go >= 1.18 (typeparams), but reflection would take care of that as well. The go/ast.Fprint function also contains some code to deal with some cyclic references inside the AST which might cause problems for us as well.

The best solution is probably to have each type in go/ast implement a Clone function explicitly. Alternatively a strongly typed Visitor interface with methods like VisitExprStmt(expr *ast.ExprStmt) which would cause compile errors in the future would also help. Unfortunately go/ast does not offer either of these at moment.

Possible alternative solutions are therefore:

  1. try a pure reflection based deep copy, implement some special handling if necessary and hope that it works / keeps working in the future
  2. Implement the copy function for each type in go/ast in this repo (similar to the current approach). Add all missing types now (including a build switch for Go 1.18) and keep the file in sync manually with each Go release.
  3. Implement a strongly typed visitor in this repo and keep it in sync manually with each Go release. Implement the CopyExpr as a visitor. CopyExpr will break at compile time whenever the visitor interface changes.

I am in favor of solution 1 or 3 (they both have different tradeoffs). If you want, I could prepare a PR for it. Which solution do you prefer?

@dominikh
Copy link
Owner

@dominikh dominikh commented Nov 26, 2021

First I wanted to let you know that I already feel slightly bad for throwing lots of weird code at you.

Please don't. My previous comment was tongue-in-cheek. Staticcheck should obviously support all valid Go programs, no matter how weird. In fact, I've added libc and sqlite to my test corpus.

Reflection would fail for go/ast types that have private fields or pointers that must not be copied

None of the types in go/ast have unexported fields, and I don't expect that to change in the future. They form a very simple tree representation. The only pointer that shouldn't be copied is *Object in Ident, and we can blacklist that in our reflect-based implementation.

The go/ast.Fprint function also contains some code to deal with some cyclic references inside the AST which might cause problems for us as well

The only way to get cycles is by manually constructing a malformed tree. The result of go/parser should never contain cycles, and it's fairly straightforward to not introduce any cycles when manually constructing trees. Note that the current implementation of CopyExpr doesn't take cycles into consideration, either.

If you want, I could prepare a PR for it

Thank you for the offer, but I much prefer working on this myself.

I am fairly certain that the reflect-based version already exists somewhere in x/tools, albeit unexported, so I'm probably just going to copy that.

@dominikh dominikh added the started Issues we've started working on label Dec 31, 2021
@uhthomas
Copy link

@uhthomas uhthomas commented Mar 19, 2022

FWIW I also came across this today.

(21:20:38) ERROR: /home/thomas/.cache/bazel/_bazel_thomas/b3860946b0ed29b82afe181b73c30609/external/org_modernc_sqlite/lib/BUILD.bazel:3:11: GoCompilePkg external/org_modernc_sqlite/lib/lib.a failed: (Exit 1): builder failed: error executing command bazel-out/host/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src external/org_modernc_sqlite/lib/capi_darwin_amd64.go -src ... (remaining 73 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
compilepkg: panic: unreachable: *ast.FuncType

goroutine 170 [running]:
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe3878?, 0xc0012d4900?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:216 +0x1234
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe3c38?, 0xc0012ce8b8?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:169 +0xc39
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe3ae8?, 0xc0012d4920?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:142 +0xcf5
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe3608?, 0xc0012d2a00?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:97 +0x53c
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe3c38?, 0xc0012ce900?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:169 +0xc39
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe3ae8?, 0xc0012d4a80?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:142 +0xcf5
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe3608?, 0xc0012d2b00?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:97 +0x53c
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe3578?, 0xc0316c18f0?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:91 +0x96b
honnef.co/go/tools/quickfix.CheckDeMorgan.func2({0xbe2070?, 0xc0012d4c20}, {0xc0171c0400, 0x5, 0xc0171be200?})
        external/co_honnef_go_tools/quickfix/lint.go:167 +0x174
honnef.co/go/tools/analysis/code.PreorderStack.func1({0xbe2070?, 0xc0012d4c20?}, 0x8?, {0xc0171c0400?, 0x12?, 0x0?})
        external/co_honnef_go_tools/analysis/code/visit.go:22 +0x3a
golang.org/x/tools/go/ast/inspector.(*Inspector).WithStack(0xc015200000, {0xc002f2fd18?, 0x1?, 0x5ceec5?}, 0xc021197cc0)
        external/org_golang_x_tools/go/ast/inspector/inspector.go:126 +0x1b9
honnef.co/go/tools/analysis/code.PreorderStack(0x18?, 0x7f5484e64d28?, {0xc002f2fd18, 0x1, 0x1})
        external/co_honnef_go_tools/analysis/code/visit.go:20 +0x8a
honnef.co/go/tools/quickfix.CheckDeMorgan(0xa88c60?)
        external/co_honnef_go_tools/quickfix/lint.go:216 +0x8b
main.(*action).execOnce(0xc00048bb00)
        external/io_bazel_rules_go/go/tools/builders/nogo_main.go:286 +0x817
sync.(*Once).doSlow(0x0?, 0x0?)
        GOROOT/src/sync/once.go:68 +0xc2
sync.(*Once).Do(...)
        GOROOT/src/sync/once.go:59
main.(*action).exec(0x0?)
        external/io_bazel_rules_go/go/tools/builders/nogo_main.go:230 +0x3d
main.execAll.func1(0x0?)
        external/io_bazel_rules_go/go/tools/builders/nogo_main.go:224 +0x54
created by main.execAll
        external/io_bazel_rules_go/go/tools/builders/nogo_main.go:222 +0x47

@uhthomas
Copy link

@uhthomas uhthomas commented Mar 19, 2022

I took a quick look around and the only thing I've found so far is from google/wire:

https://github.com/google/wire/blob/9d78e0ad8cbeb6716fa6bdbca689bdb9ce0e8fe8/internal/wire/copyast.go#L30-L33

@uhthomas
Copy link

@uhthomas uhthomas commented Mar 19, 2022

Heh, and the other solution from golang/go is to encode the ast as bytes and parse it again...

https://github.com/golang/go/blob/49f16625c82483ab26929a2761031c93dd5d2c83/src/go/format/format.go#L70-L71

@uhthomas
Copy link

@uhthomas uhthomas commented Mar 19, 2022

So, I think there are a few options:

  1. Add missing types to CopyExpr.
  2. Copy the code from google/wire.
  3. Use reflect.
  4. Use google/go-cpy (reflect).

What are your thoughts? :)

@dominikh
Copy link
Owner

@dominikh dominikh commented Mar 19, 2022

At this point this is just blocked on me doing the work, which I will do in time for next week's release. CopyExpr needs to be adjusted to support generics, anyway.

@uhthomas
Copy link

@uhthomas uhthomas commented Mar 30, 2022

Ah, was sad to see this miss the latest release.

We can't build with Go 11.8 because of this.

(21:39:44) ERROR: /home/thomas/.cache/bazel/_bazel_thomas/b3860946b0ed29b82afe181b73c30609/external/org_modernc_sqlite/lib/BUILD.bazel:3:11: GoCompilePkg external/org_modernc_sqlite/lib/lib.a failed: (Exit 1): builder failed: error executing command bazel-out/host/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src external/org_modernc_sqlite/lib/capi_darwin_amd64.go -src ... (remaining 73 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
compilepkg: panic: unreachable: *ast.FuncType

goroutine 156 [running]:
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe7480?, 0xc0000fc740?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:216 +0x1234
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe7840?, 0xc0000fa318?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:169 +0xc39
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe76f0?, 0xc0000fc760?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:142 +0xcf5
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe7210?, 0xc0000fe640?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:97 +0x53c
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe7840?, 0xc0000fa360?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:169 +0xc39
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe76f0?, 0xc0000fc8c0?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:142 +0xcf5
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe7210?, 0xc0000fe740?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:97 +0x53c
honnef.co/go/tools/go/ast/astutil.CopyExpr({0xbe7180?, 0xc017e27290?})
        external/co_honnef_go_tools/go/ast/astutil/util.go:91 +0x96b
honnef.co/go/tools/quickfix.CheckDeMorgan.func2({0xbe5c50?, 0xc0000fca60}, {0xc026cea000, 0x5, 0xc026ce6000?})
        external/co_honnef_go_tools/quickfix/lint.go:167 +0x174
honnef.co/go/tools/analysis/code.PreorderStack.func1({0xbe5c50?, 0xc0000fca60?}, 0x8?, {0xc026cea000?, 0x12?, 0x0?})
        external/co_honnef_go_tools/analysis/code/visit.go:22 +0x3a
golang.org/x/tools/go/ast/inspector.(*Inspector).WithStack(0xc023db6000, {0xc008790d18?, 0x1?, 0x5ceec5?}, 0xc017127cc0)
        external/org_golang_x_tools/go/ast/inspector/inspector.go:126 +0x1b9
honnef.co/go/tools/analysis/code.PreorderStack(0x18?, 0x7f007c8621d8?, {0xc008790d18, 0x1, 0x1})
        external/co_honnef_go_tools/analysis/code/visit.go:20 +0x8a
honnef.co/go/tools/quickfix.CheckDeMorgan(0xa8bdc0?)
        external/co_honnef_go_tools/quickfix/lint.go:216 +0x8b
main.(*action).execOnce(0xc0004e8000)
        external/io_bazel_rules_go/go/tools/builders/nogo_main.go:288 +0x817
sync.(*Once).doSlow(0x0?, 0x0?)
        GOROOT/src/sync/once.go:68 +0xc2
sync.(*Once).Do(...)
        GOROOT/src/sync/once.go:59
main.(*action).exec(0x0?)
        external/io_bazel_rules_go/go/tools/builders/nogo_main.go:232 +0x3d
main.execAll.func1(0x0?)
        external/io_bazel_rules_go/go/tools/builders/nogo_main.go:226 +0x54
created by main.execAll
        external/io_bazel_rules_go/go/tools/builders/nogo_main.go:224 +0x47

@dominikh
Copy link
Owner

@dominikh dominikh commented Mar 31, 2022

which I will do in time for next week's release

Time pressure has indeed made me a liar. But there'll be a 2022.1.1 soon, anyway, for Go 1.18.1.

@uhthomas
Copy link

@uhthomas uhthomas commented Mar 31, 2022

Awesome! Thanks :)

dominikh added a commit that referenced this issue Apr 3, 2022
In gh-1134, I argued in favour of extending CopyExpr to use reflection
to handle all types, under the assumption that we missed many types.
I have now decided against that, for two reasons:

1. There weren't many unhandled types left
2. We cannot copy comments because of limitations in go/ast, and it
seems better to refuse generating suggested fixes than to swallow
comments. This is a general problem, but function literals are
particularly likely to contain comments.

Thus, we just add FuncType to the list of types we cannot copy. The
effect is that several checks in the quickfix package will not generate
suggested fixes in some cases.

Closes gh-1134

(cherry picked from commit 8af8898)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug started Issues we've started working on
Projects
None yet
Development

No branches or pull requests

3 participants