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.

Sign up for GitHub

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

Jump to bottom

unconvert: use x/tools/go/packages #32

Merged
merged 3 commits into from
Jan 10, 2019
Merged

unconvert: use x/tools/go/packages #32

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3c46877
unconvert: use x/tools/go/packages
egonelbre Sep 11, 2018
18ebce0
Use cgoEnabled as part of build environment
nmiyake Jan 9, 2019
47d0271
Merge branch 'master' into use-go-packages
egonelbre Jan 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 46 additions & 64 deletions unconvert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@ import (
"sync"
"unicode"

"github.com/kisielk/gotool"
"golang.org/x/text/width"
"golang.org/x/tools/go/buildutil"
"golang.org/x/tools/go/loader"
"golang.org/x/tools/go/packages"
)

// Unnecessary conversions are identified by the position
Expand Down Expand Up @@ -195,10 +194,7 @@ func main() {
defer pprof.StopCPUProfile()
}

importPaths := gotool.ImportPaths(flag.Args())
if len(importPaths) == 0 {
return
}
importPaths := flag.Args()

var m fileToEditSet
if *flagAll {
Expand Down Expand Up @@ -293,29 +289,17 @@ func mergeEdits(importPaths []string) fileToEditSet {
return m
}

type noImporter struct{}

func (noImporter) Import(path string) (*types.Package, error) {
panic("golang.org/x/tools/go/loader said this wouldn't be called")
}

func computeEdits(importPaths []string, os, arch string, cgoEnabled bool) fileToEditSet {
ctxt := build.Default
ctxt.GOOS = os
ctxt.GOARCH = arch
ctxt.CgoEnabled = cgoEnabled

var conf loader.Config
conf.Build = &ctxt
conf.TypeChecker.Importer = noImporter{}
for _, importPath := range importPaths {
if *flagTests {
conf.ImportWithTests(importPath)
} else {
conf.Import(importPath)
}
func computeEdits(importPaths []string, osname, arch string, cgoEnabled bool) fileToEditSet {
cgoEnabledVal := "0"
if cgoEnabled {
cgoEnabledVal = "1"
}
prog, err := conf.Load()

pkgs, err := packages.Load(&packages.Config{
Mode: packages.LoadSyntax,
Env: append(os.Environ(), "GOOS="+osname, "GOARCH="+arch, "CGO_ENABLED="+cgoEnabledVal),
Tests: *flagTests,
}, importPaths...)
if err != nil {
log.Fatal(err)
}
Expand All @@ -324,17 +308,18 @@ func computeEdits(importPaths []string, os, arch string, cgoEnabled bool) fileTo
file string
edits editSet
}

ch := make(chan res)
var wg sync.WaitGroup
for _, pkg := range prog.InitialPackages() {
for _, file := range pkg.Files {
pkg, file := pkg, file
for _, pkg := range pkgs {
for i, file := range pkg.Syntax {
pkg, file, fileName := pkg, file, pkg.CompiledGoFiles[i]
wg.Add(1)
go func() {
defer wg.Done()
v := visitor{pkg: pkg, file: conf.Fset.File(file.Package), edits: make(editSet)}
v := visitor{pkg: pkg, file: file, edits: make(editSet)}
ast.Walk(&v, file)
ch <- res{v.file.Name(), v.edits}
ch <- res{fileName, v.edits}
}()
}
}
Expand All @@ -356,8 +341,8 @@ type step struct {
}

type visitor struct {
pkg *loader.PackageInfo
file *token.File
pkg *packages.Package
file *ast.File
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@mdempsky mdempsky Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look tomorrow to that approach.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was much nicer thanks.

edits editSet
path []step
}
Expand Down Expand Up @@ -386,35 +371,32 @@ func (v *visitor) unconvert(call *ast.CallExpr) {
if len(call.Args) != 1 || call.Ellipsis != token.NoPos {
return
}
ft, ok := v.pkg.Types[call.Fun]
if !ok {
ft := v.pkg.TypesInfo.TypeOf(call.Fun)
if ft == nil {
fmt.Println("Missing type for function")
return
}
if !ft.IsType() {
// Function call; not a conversion.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you get rid of this?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

return
}
at, ok := v.pkg.Types[call.Args[0]]
if !ok {

at := v.pkg.TypesInfo.TypeOf(call.Args[0])
if at == nil {
fmt.Println("Missing type for argument")
return
}
if !types.Identical(ft.Type, at.Type) {
if !types.Identical(ft, at) {
// A real conversion.
return
}
if !*flagFastMath && isFloatingPoint(ft.Type) && isOperation(call.Args[0]) && isOperation(v.path[len(v.path)-2].n) {
if !*flagFastMath && isFloatingPoint(ft) && isOperation(call.Args[0]) && isOperation(v.path[len(v.path)-2].n) {
// Go 1.9 gives different semantics to "T(a*b)+c" and "a*b+c".
return
}
if isUntypedValue(call.Args[0], &v.pkg.Info) {
if isUntypedValue(call.Args[0], v.pkg.TypesInfo) {
// Workaround golang.org/issue/13061.
return
}
if *flagSafe && !v.isSafeContext(at.Type) {
if *flagSafe && !v.isSafeContext(at) {
// TODO(mdempsky): Remove this message.
fmt.Println("Skipped a possible type conversion because of -safe at", v.file.Position(call.Pos()))
fmt.Println("Skipped a possible type conversion because of -safe at", v.pkg.Fset.Position(call.Pos()))
return
}
if v.isCgoCheckPointerContext() {
Expand All @@ -425,7 +407,7 @@ func (v *visitor) unconvert(call *ast.CallExpr) {
return
}

v.edits[v.file.Position(call.Lparen)] = struct{}{}
v.edits[v.pkg.Fset.Position(call.Lparen)] = struct{}{}
}

// isFloatingPointer reports whether t's underlying type is a floating
Expand Down Expand Up @@ -479,12 +461,12 @@ func (v *visitor) isSafeContext(t types.Type) bool {
}
// We're a conversion in the pos'th element of n.Rhs.
// Check that the corresponding element of n.Lhs is of type t.
lt, ok := v.pkg.Types[n.Lhs[pos]]
if !ok {
lt := v.pkg.TypesInfo.TypeOf(n.Lhs[pos])
if lt == nil {
fmt.Println("Missing type for LHS expression")
return false
}
return types.Identical(t, lt.Type)
return types.Identical(t, lt)
case *ast.BinaryExpr:
if n.Op == token.SHL || n.Op == token.SHR {
if ctxt.i == 1 {
Expand All @@ -501,24 +483,24 @@ func (v *visitor) isSafeContext(t types.Type) bool {
} else {
other = n.X
}
ot, ok := v.pkg.Types[other]
if !ok {
ot := v.pkg.TypesInfo.TypeOf(other)
if ot == nil {
fmt.Println("Missing type for other binop subexpr")
return false
}
return types.Identical(t, ot.Type)
return types.Identical(t, ot)
case *ast.CallExpr:
pos := ctxt.i - 1
if pos < 0 {
// Type conversion in the function subexpr is okay.
return true
}
ft, ok := v.pkg.Types[n.Fun]
if !ok {
ft := v.pkg.TypesInfo.TypeOf(n.Fun)
if ft == nil {
fmt.Println("Missing type for function expression")
return false
}
sig, ok := ft.Type.(*types.Signature)
sig, ok := ft.(*types.Signature)
if !ok {
// "Function" is either a type conversion (ok) or a builtin (ok?).
return true
Expand All @@ -532,7 +514,7 @@ func (v *visitor) isSafeContext(t types.Type) bool {
}
return types.Identical(t, pt)
case *ast.CompositeLit, *ast.KeyValueExpr:
fmt.Println("TODO(mdempsky): Compare against value type of composite literal type at", v.file.Position(n.Pos()))
fmt.Println("TODO(mdempsky): Compare against value type of composite literal type at", v.pkg.Fset.Position(n.Pos()))
return true
case *ast.ReturnStmt:
// TODO(mdempsky): Is there a better way to get the corresponding
Expand Down Expand Up @@ -566,12 +548,12 @@ func (v *visitor) isSafeContext(t types.Type) bool {
if typeExpr == nil {
fmt.Println(ctxt)
}
pt, ok := v.pkg.Types[typeExpr]
if !ok {
fmt.Println("Missing type for return parameter at", v.file.Position(n.Pos()))
pt := v.pkg.TypesInfo.TypeOf(typeExpr)
if pt == nil {
fmt.Println("Missing type for return parameter at", v.pkg.Fset.Position(n.Pos()))
return false
}
return types.Identical(t, pt.Type)
return types.Identical(t, pt)
case *ast.StarExpr, *ast.UnaryExpr:
// TODO(mdempsky): I think these are always safe.
return true
Expand All @@ -580,7 +562,7 @@ func (v *visitor) isSafeContext(t types.Type) bool {
return true
default:
// TODO(mdempsky): When can this happen?
fmt.Printf("... huh, %T at %v\n", n, v.file.Position(n.Pos()))
fmt.Printf("... huh, %T at %v\n", n, v.pkg.Fset.Position(n.Pos()))
return true
}
}
Expand Down