unconvert: use x/tools/go/packages #32
Conversation
unconvert.go
Outdated
| fmt.Println("Missing type for function") | ||
| return | ||
| } | ||
| if !ft.IsType() { | ||
| // Function call; not a conversion. |
There was a problem hiding this comment.
To elaborate, I think this is necessary to prevent unconvert from mishandling edge cases like:
type T func(T)
var f T
_ = f(f)
Without checking that call.Fun is a type expression, I expect unconvert would rewrite the last line into just _ = f.
There was a problem hiding this comment.
Good question. I honestly cannot exactly remember at this point anymore. But what I can gather from context is that, TypesInfo.TypeOf should always return a type. But I'll verify tomorrow.
There was a problem hiding this comment.
TypeOf will always return a type, yes. But this check was distinguishing whether an expression was a value or a type. This is important for distinguishing functionName(x) (function call) from typeName(x) (conversion).
unconvert.go
Outdated
| pkg *loader.PackageInfo | ||
| file *token.File | ||
| pkg *packages.Package | ||
| file *ast.File |
There was a problem hiding this comment.
I think this PR can be a lot less invasive if you change pkg's type to *types.Info (and then a subsequent PR to rename pkg to info), and leave file as *token.File.
There was a problem hiding this comment.
As a side benefit: if we stick to just *types.Info, then we can potentially make computeEdits conditionally use go/loader or go/packages based on a compile-time or run-time flag in case users have problems with go/packages. (No need to proactively implement this though. We can implement that if users report issues.)
There was a problem hiding this comment.
I'll take a look tomorrow to that approach.
There was a problem hiding this comment.
I changed pkg *loader.PackageInfo to info *types.Info at master, so you should be able to get rid of these changes now and focus just on the go/loader to go/packages change in computeEdits. Cheers.
There was a problem hiding this comment.
This change was much nicer thanks.
|
Thanks! |
About 2x faster and handles go modules better.
However there are still some upstream problems with
x/tools/go/packages. So still need to wait for those fixes. Example issue caused by upstream kisielk/errcheck#150