[WIP] update go/loader to go/packages for module support #52
Conversation
| cfg := cfg.FromFunc(f.Decls[0].(*ast.FuncDecl)) | ||
| DefUse(cfg, prog.Created[0]) | ||
| LiveVars(cfg, prog.Created[0]) | ||
| cfg := cfg.FromFunc(c.prog.Syntax[0].Decls[0].(*ast.FuncDecl)) |
There was a problem hiding this comment.
note: everything that does c.prog.Syntax[0] is basically incorrect, many files are loaded, even if this is only used in benchmarking (I think?) so maybe doesn't matter
|
Is this branch up-to-date? I'm curious to see the progress. :) |
|
It appears there's some work going into gopls to also support extraction to variable and function. https://github.com/golang/tools/blob/master/internal/lsp/source/extract.go I can't tell how much overlap there is, but wanted to point it out nonetheless. |
|
hey @sorenh - hit a bit of a snag, but have a plan and just need to find the time to do it, so I have a badly broken branch laying around for now. they changed the interaction with the filesystem quite a bit and I'm hoping to get it workable without having to drastically change the filesystem api we have (for now), we use this quite a bit for testing, modifying stdin, etc so it's relatively messy. been a busy couple of weeks for me. interesting, that looks new. well, could cede to the overlords (that's the official package now), they appear to have the analysis needed to do it accurately but I haven't picked through it with a fine tooth comb which I guess could be useful, I hope they stole some stuff so that this was useful ;) |
|
Not sure if tagging like this works on Github, but @stamblerre was very responsive on some other work I did on gopls. Maybe reach out to make there's no duplication of effort. |
|
Extracting functions and variables is now supported in |
|
@stamblerre The vscode plugin still relies on godocter, doesn't it? |
|
It does, but it will likely switch to |
|
it is relatively close, there are 4 tests failing (out of hundreds) so getting there. cgo and some extract func tests that are supposed to fail but don't. i know, it has been a year (and what a year)... but need to test actual support with modules after getting the tests all working, the tests were all before modules existed so likely need to make some modules tests and then (not in this patch) migrate the tests to all be modules compliant (our little GOPATH hackery has been taken from us) |
currently stuck at getting back ast idents that aren't filled in so we get an NPE trying to find where they're from. this seems odd!
had to add some more missing functionality from loader to put all the packages in a bucket like we want. need to test this with deeper dependency graphs. also had to move the testdata around again, but I think it's more resembling what we want now in go mod world, where previously we had just been setting GOPATH to the testdata directory, now in practice each go module acts this way at its root. TODO need to fix up other packages that still have loader functionality, namely refactoring package. should be easy now, though, the way things are set up. then test test test
this smells a little funky, but for exported it works and the positions are still correct so debugging should still work ok, we may just want to hide this output if it puts "command-line-arguments" always but maybe a product of testing facilities?
the new package loader actually loads in packages that don't exist even if they are result of an error, previously the loader didn't seem to have these in the set of all packages returned though I couldn't figure out logically how it happened from reading the code. in any case, slight change to printing to make this more clear and update the test slightly. tests still failing at refactorings that should error but do not
the GO111MODULES env var is going away in 1.17 so we don't have this luxury really for very long, but it would be nice to get tests working prior to migrating things so this is gonna be how it is. TODO I don't think we can set the GO111MODULE flag where it is now, need to do it only for tests so that users can actually use modules TODO cgo is broken TODO a few extract func tests seem broken, the nature of them seems that the behavior of the type checking has changed or the extract func refactoring itself was looking for some specific behaviors that have now broken, but in any case, the failing tests themselves the refactorings produce edit sets that would make the changes that should fail so it's not a loading issue or a testing issue, there's a behavior thing to look into and debug. ^ only remaining issues afaik
we needed to write the contents out to the loaders overlay config which is the mechanism they allow now to modify the files, we were using clever filesystem trick to show the edited contents previously but we have to do this more clumsily now to feed into the loader as it only reads from the filesystem or the overlay
|
fixed the issue with build errors not flipping errors now, those tests work again (yay). snoozed and lost though and now actually have to migrate the tests to use modules if they import any packages, as the GOPATH hackery is no longer supported in 1.18 it seems like (I guess I could get them working on an older version but that seems like doubling my effort). still think cgo is broke too. other thing i thought of is even if this does get back to working golang has made lots of changes to the lang we don't really have any test coverage for like generics, so we should maybe make a note of last working go version and use at your own peril warning - not sure this will get any love with gopls superceding, figured I'd try to get it working at least though... |
we wrote all the tests without gomodules and using gopath. this is the fast way. the tests can all be updated to go modules but that should be done with a script, we probably need to test both.
|
there is only one broken test now, for refactoring an inline C file (an imported cgo package seems fine). I managed to figure out that we can still do the GOPATH dance with GO111MODULES=off, which for some reason I had in my head was deprecated. so woo. from what I gather, the packages package (heh) actually does support loading cgo now as of 1.15 and the type checker or someone deep down seems to be creating a file in the cache with the contents of the file itself, but not the name we expect (some gobbledy gook name in the cache) so refactoring throws a fit since the file we want to refactor appears to not exist in the fileset. I'm down the rabbit hole trying to figure out how to get the file handle we want, it turns into the cache file in the token.FileSet and idk how to dig it back out. it's unclear how to simply disallow cgo too, there's a maze of configs it seems and nothing exposed from packages loader but maybe this is easy. so close! |
|
I feel kind of inclined to ship it since this will probably work with the majority of go programs now (vs master, which does not), and this is only a fringe case (inline cgo). but i'll sleep on it and think if there's a "nice" way to error at least. |
|
Considering master is broken against packages with modules, think this is a large improvement. yells into void :) I did some testing on cross package refactors with and without modules and this seems to work now. I didn't spend too too much time on it but they seem to work out of the vim plugin, which is setting the scope, which is maybe something to update. re: the broken cgo case, the current broken test seems like it might be kinda broken functionality wise anyway per #22 - I didn't test this either. |
| } | ||
|
|
||
| // Load loads a package, calling packages.Load | ||
| func Load(conf *packages.Config, errorH func(error), args ...string) (*Program, error) { |
There was a problem hiding this comment.
All of the tests that were touched here now pass in conf, but none of them were updated to pass errorH.
There was a problem hiding this comment.
yea, good eye. the errorH function was basically to handle cgo somewhat more gracefully by catching it and spitting out a legible error. the new modules handles cgo better than the loader program so I believe it's dead code, though the broken test seems to be a bug maybe in modules handling (I gave up at the time). thanks for catching, should prob remove if that's the case
working on #50
posting in case of bus incident to show mercy to poor other soul who may choose to spend their time attempting to accomplish this (there aren't really any docs for migrating, but managed to figure it out)... this hasn't begun to try to update refactoring package which is possibly a mine field, but I'm hoping not. the names analysis is working and passing tests, shouldn't take too long to plug in elsewhere to start testing. hopefully get some more time this week...