astutil.CopyExpr does not support *ast.FuncType #1134
Comments
|
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. |
|
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 The best solution is probably to have each type in Possible alternative solutions are therefore:
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? |
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.
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
The only way to get cycles is by manually constructing a malformed tree. The result of
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. |
|
FWIW I also came across this today. |
|
I took a quick look around and the only thing I've found so far is from google/wire: |
|
Heh, and the other solution from |
|
So, I think there are a few options:
What are your thoughts? :) |
|
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. |
|
Ah, was sad to see this miss the latest release. We can't build with Go 11.8 because of this. |
Time pressure has indeed made me a liar. But there'll be a 2022.1.1 soon, anyway, for Go 1.18.1. |
|
Awesome! Thanks :) |
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)
Thanks for fixing #1133, this issue is some kind of follow-up.
astutil.CopyExprpanics if it encounters a *ast.FuncType.Reproducer:
The original code is some transpiled code in
gitlab.com/cznic/sqlite/lib/sqlite_linux_amd64.go:26740and is also triggered byquickfix.CheckDeMorgan:Building upon your most recent commit 030b696, i guess it is fine if we just return false for now.
The text was updated successfully, but these errors were encountered: