Open ahicks92 opened 5 years ago
@camlorn You're right. There are multiple bugs resulting from how imports are being handled. Basically the code currently ignores all imports and then tries to figure out imports using go tools. This results in quite a lot of problems. I'd appreciate if you could take the time to make a PR. I was also planning to fix it a while ago but didn't have the time to do so (still planning to do so :D).
Issues marked with this milestone are also relevant to this issue.
Here is a fun issue:
context
,errors
, andgithub.com/go-resty/resty
in a fairly complicated source file inside a pre-modules monorepo setup that has to do fun things to GOPATH to work.golang.org/x/tools/imports
apparently tries to work out what you need to import in order to automatically add it codecontext
andgithub.com/go-resty/resty
, droppingerrors
and causing code to not compile.My temporary fix is to fork Genny in order to remove the continue statements used to skip import statements. This can't work on multiple generated types in the same invocation, but I'm generating one type per file for a REST client for our internal microservices, so it lets me make progress.
Having the import statements does fix it.
I think that
golang.org/x/tools/imports
adding imports back in hid the issue. The first thing i tried to do before I dug in far enough to figure out what is actually going on was make a test case. I couldn't cause this to fail in an isolated setting. I think that mostly it's smart enough to magically work out what it needs to do just enough that dropping all import statements can work in most cases.I can't share the monorepo or the code, and since I can't get a standalone test case working I'm not sure how to let someone else reproduce it. If necessary, I may be able to get permission to share the specific file being used as input, since it contains no code specific to our setup. Dropping all import statements can be seen by modifying the
Generics
function in parse.go to return just before invocation ofgolang.org/x/tools/imports
.Since the way Genny works means that we can't have variation in the import statements themselves based off the type parameters (or, at least as far as I know it doesn't), I think the fix here is to refactor the code cleanup to run per typeset rather than on the entire concatenated output. Flags can then be passed to the first one telling it that it's the first one, so that it knows not to drop package and import statements, then unconditionally drop from all the others.
Furthermore, it looks like there's some other oddities that probably should be handled in the same refactor, as I think they can be solved in a similar way:
packageContainer
(i.e.packageContainer := NewPackageContainer()
). Currently I think that gets through because such code is frequently indented.import "foo"
and I don't think this code handles that either.If refactored to run code cleanup once per typeset, it's guaranteed to be package, imports, code in that order possibly with comments, which should handle the latter points and make it safe to strip whitespace. Specifically, I believe ignoring everything until a non-comment non-package non-import line is encountered and then just including everything as-is after that point will do it.
I'll try to find the time to refactor if I'm headed in the right direction, but it might be better if someone who is familiar with the internals does it, plus I haven't got a lot of time on my hands so it might be a while. If anyone can at least let me know if this all seems right before I put the time in to refactor, I'd appreciate it.