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.

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

remove external executable dependency 'goimports' #11

Merged
merged 5 commits into from
Apr 19, 2021

Conversation

katsusan
Copy link
Member

Currently in-use goimports command shares the same internal implementation with golang.org/x/tools/imports.
But calling an external executable may be unstable and a bit inefficient, so I propose using corresponding library to replace it,
which also solves the problem of building failure in docker as go get -u golang.org/x/tools/cmd/goimports needs GOPROXY
to be set if in China mainland.

// Chinese version
英文不太好见谅,大意就是goimports和golang.org/x/tools/imports这个库底层都是调用golang.org/x/tools/internal/imports的
Process函数,因此不会有表现上的不同。此外依赖goimports的话需要在Readme里申明一下prerequisite,并且在docker构建时
如果在大陆网络条件下会go get失败,加入GOPROXY的话也要对国外用户区分,综合以上说法我觉得替换成库实现更简便高效。

Copy link
Member

@changkun changkun left a comment

Choose a reason for hiding this comment

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

Very interesting change. Thanks a lot for the information. I didn't know that goimports is a direct wrapper of the golang.org/x/tools/imports.

  • Will the imports package also fix the code formatting?

Could you also clean up a recent change to the command check of goimports, see https://github.com/golang-design/ssaplayground/pull/7/files for more details.

This change arises me question regarding getting SSA information more directly, such as:

src/route/api.go Outdated Show resolved Hide resolved
@katsusan
Copy link
Member Author

  • Will the imports package also fix the code formatting?

Sure. Process(filename string, src []byte, opt *Options) provides options
opt.FormatOnly which give you the choice to do format only or not.
In brief, opt.FormatOnly == true → do format only;
opt.FormatOnly == false → do autoimport then format.

Looking at following example:

package main

import (
	"fmt"

	"golang.org/x/tools/imports"
)

func main() {
	doimports()
}

// source code we want to do autoimport
const src string = `
package main

func main() {
	ctx := context.Background()
	println(ctx)

		r := gin.New()
	println(r)
}
`

func doimports() {
	out, err := imports.Process("", []byte(src), &imports.Options{})
	if err != nil {
		fmt.Println("imports failed,", err)
		return
	}
	fmt.Printf("%s\n", out)
}

it will give the output:

package main

import (
	"context"

	"github.com/gin-gonic/gin"
)

func main() {
	ctx := context.Background()
	println(ctx)

	r := gin.New()
	println(r)
}

Currently infeasible in my opinion. the generated ssa.html is the human-friendly visualization
of all SSA passes which go team provide, while https://golang.org/x/tools/go/ssa can show
the presentation of ssa form in a detailed but obscure way (try golang.org/x/tools/cmd/ssadump for example).

In addition, there are other reasons for not doing it :

  1. "golang.org/x/tools/go/ssa" is not stable yet.
    THIS INTERFACE IS EXPERIMENTAL AND IS LIKELY TO CHANGE. by its document.
  2. Go compiler provides options with which you can customize you SSA passes in ssa.html.
    For example, go build -gcflags=-d=ssa/short_circuit/off will skip the short_circuit pass.

Copy link
Member

@changkun changkun left a comment

Choose a reason for hiding this comment

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

It looks like there is a behavior change:

If I use the following snippet:

func main() {

	fmt.Println(time.Second)
}

Your change will result in:

cannot run autoimports for your code, err:
1:1: expected 'package', found 'func'

whereas the live server returns ssa.html properly.

Copy link
Member

@changkun changkun left a comment

Choose a reason for hiding this comment

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

Thanks a lot for another awesome fix!

@changkun changkun merged commit 0b01232 into golang-design:master Apr 19, 2021
@katsusan
Copy link
Member Author

katsusan commented Apr 19, 2021

Sorry for not testing fully. New commit has fixed this, which keeps sync with goimports options

  1. no package declaration with func main
  2. no package declaration with func dummy
  3. package main + func main
  4. package main + func dummy
  5. package dummy with func main
  6. package dummy with func dummy

Here are test results of the 6 cases above.

cannot build ssa for your code, err:
main$GOSSAPATHgo:2:1: expected 'package', found 'import'
  • case3: ok

  • case4:
    // same as goimports

cannot build ssa for your code, err:
# command-line-arguments
dumped SSA to $GOSSAPATH\ssa$GOSSAPATHhtml
# command-line-arguments
runtime$GOSSAPATHmain_main·f: function main is undeclared in the main package
  • case5: ok

  • case6: ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants