Repository: zeromicro/go-zero
Pull request #3988 (2024-03-08 07:19:02)

fix(goctl): multi imports the api cause redeclared error in types.go
Now experimental features are turned on by default in Goctl, is this correct?
https://github.com/zeromicro/go-zero/blob/c7dacb01468bad3a21dc34b18eb8635f875fd29c/tools/goctl/pkg/env/env.go#L79-L81

kesonan: Plz describe which issue scenario should be fix
wjiec: Hi @kesonan, you can check the added test cases to understand that the specific scenario is in a gateway that has the following referenced relationship:
```plain
// main.api
import "xxx.api"
import "yyy.api"

// xxx.api
import "common.api"

// yyy.api
import "common.api"
```
Use `goctl api go -api main.api ... `generated `types.go` has redeclared  structs (from common.api).
wjiec: hi @kesonan @kevwan do you have time to look at this pr?
kesonan: ![image](https://github.com/zeromicro/go-zero/assets/10302073/7c5a9862-6fcb-4518-9c7b-3d1bcf9ee1bf)
The `master` branch has already fix this issue, pls try with latest version.
wjiec: Hi @kesonan , I still have this problem with the latest code in master branch, please check the contents of the `internal/types/types.go` file. Screenshots:

<img width="1728" alt="image" src="https://github.com/zeromicro/go-zero/assets/9394086/8e575ad7-c127-4a1a-992a-e47f3d0c50a2">

wjiec: Sorry, it should be that the title of the PR is not expressed clearly enough, and the redeclared error appears in the `types.go` file.
kevwan: Is the problem fixed already?
wjiec: @kevwan Unfortunately the code on master still has this problem now, and this pr is designed to fix it.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/zeromicro/go-zero/pull/3988?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 95.14%. Comparing base [(`2629636`)](https://app.codecov.io/gh/zeromicro/go-zero/commit/2629636f648bd829f5643c63b5359292be565eac?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro) to head [(`bfc3018`)](https://app.codecov.io/gh/zeromicro/go-zero/pull/3988?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro).
> Report is 40 commits behind head on master.

<details><summary>Additional details and impacted files</summary>



[see 253 files with indirect coverage changes](https://app.codecov.io/gh/zeromicro/go-zero/pull/3988/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zeromicro)

</details>
kesonan: pls execute the following cmd `goctl env` and post the output here, now it is already fixed on master.
wjiec: @kesonan Okay, this is the output of the `goctl env` command
```
GOCTL_OS=darwin
GOCTL_ARCH=arm64
GOCTL_HOME=/Users/jayson/.goctl
GOCTL_DEBUG=False
GOCTL_CACHE=/Users/jayson/.goctl/cache
GOCTL_EXPERIMENTAL=on
GOCTL_VERSION=1.6.3
PROTOC_VERSION=
PROTOC_GEN_GO_VERSION=v1.33.0
PROTO_GEN_GO_GRPC_VERSION=1.3.0
```

btw, something that could potentially have an impact on this is the value of the `GOCTL_EXPERIMENTAL` env, by default, the value of this environment variable is set to On, I'm guessing this is set to Off by default in your environment?
wjiec: I retested with the latest code on master, as well as executing `goctl api go ...` on the following envs. and the result is still wrong.
```shell
$ env | grep GOCTL
# nothing
$ ./goctl env
GOCTL_OS=darwin
GOCTL_ARCH=arm64
GOCTL_HOME=/Users/jayson/.goctl
GOCTL_DEBUG=False
GOCTL_CACHE=/Users/jayson/.goctl/cache
GOCTL_EXPERIMENTAL=on
GOCTL_VERSION=1.6.3
PROTOC_VERSION=
PROTOC_GEN_GO_VERSION=v1.33.0
PROTO_GEN_GO_GRPC_VERSION=1.3.0
```
wjiec: Yes, after I executed `. /goctl env -w GOCTL_EXPERIMENTAL=off`, the generated `types.go` is now correct!
kevwan: @kesonan PTAL.
Issues-translate-bot:  
> Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿         
----    
@kesonan ASK.  
      
kesonan: @wjiec Please add my WeChat account and we can communicate on WeChat. The efficiency here is a bit low. WeChat ID is `KesonAnn`.

Patch:
diff --git a/tools/goctl/pkg/parser/api/parser/api.go b/tools/goctl/pkg/parser/api/parser/api.go
index 69d33b40..aee27727 100644
--- a/tools/goctl/pkg/parser/api/parser/api.go
+++ b/tools/goctl/pkg/parser/api/parser/api.go
@@ -32,11 +32,8 @@ type API struct {
 func convert2API(a *ast.AST, importSet map[string]lang.PlaceholderType, is *importstack.ImportStack) (*API, error) {
 	var api = new(API)
 	api.importManager = is
-	api.importSet = make(map[string]lang.PlaceholderType)
+	api.importSet = importSet
 	api.Filename = a.Filename
-	for k, v := range importSet {
-		api.importSet[k] = v
-	}
 	one := a.Stmts[0]
 	syntax, ok := one.(*ast.SyntaxStmt)
 	if !ok {


Test patch:
diff --git a/tools/goctl/api/gogen/gen_test.go b/tools/goctl/api/gogen/gen_test.go
index d2cb6842..349312e5 100644
--- a/tools/goctl/api/gogen/gen_test.go
+++ b/tools/goctl/api/gogen/gen_test.go
@@ -2,7 +2,12 @@ package gogen
 
 import (
 	_ "embed"
+	"go/ast"
 	goformat "go/format"
+	"go/importer"
+	goparser "go/parser"
+	"go/token"
+	"go/types"
 	"os"
 	"path/filepath"
 	"strings"
@@ -48,6 +53,10 @@ var (
 	noStructTagApi string
 	//go:embed testdata/nest_type_api.api
 	nestTypeApi string
+	//go:embed testdata/import_twice.api
+	importTwiceApi string
+	//go:embed testdata/another_import_api.api
+	anotherImportApi string
 )
 
 func TestParser(t *testing.T) {
@@ -232,6 +241,46 @@ func TestHasImportApi(t *testing.T) {
 	validate(t, filename)
 }
 
+func TestImportTwiceOnExperimental(t *testing.T) {
+	defaultExperimental := env.Get(env.GoctlExperimental)
+	env.Set(t, env.GoctlExperimental, "on")
+	defer env.Set(t, env.GoctlExperimental, defaultExperimental)
+
+	filename := "greet.api"
+	err := os.WriteFile(filename, []byte(importTwiceApi), os.ModePerm)
+	assert.Nil(t, err)
+	defer os.Remove(filename)
+
+	importApiName := "importApi.api"
+	err = os.WriteFile(importApiName, []byte(importApi), os.ModePerm)
+	assert.Nil(t, err)
+	defer os.Remove(importApiName)
+
+	hasImportApiName := "hasImportApi.api"
+	err = os.WriteFile(hasImportApiName, []byte(hasImportApi), os.ModePerm)
+	assert.Nil(t, err)
+	defer os.Remove(hasImportApiName)
+
+	anotherImportApiName := "anotherImportApi.api"
+	err = os.WriteFile(anotherImportApiName, []byte(anotherImportApi), os.ModePerm)
+	assert.Nil(t, err)
+	defer os.Remove(anotherImportApiName)
+
+	api, err := parser.Parse(filename)
+	assert.Nil(t, err)
+
+	var hasInline bool
+	for _, ty := range api.Types {
+		if ty.Name() == "ImportData" {
+			hasInline = true
+			break
+		}
+	}
+	assert.True(t, hasInline)
+
+	validate(t, filename)
+}
+
 func TestNoStructApi(t *testing.T) {
 	filename := "greet.api"
 	err := os.WriteFile(filename, []byte(noStructTagApi), os.ModePerm)
@@ -287,6 +336,9 @@ func validateWithCamel(t *testing.T, api, camel string) {
 			code, err := os.ReadFile(path)
 			assert.Nil(t, err)
 			assert.Nil(t, validateCode(string(code)))
+			if strings.HasSuffix(path, "types.go") {
+				assert.Nil(t, checkRedeclaredType(string(code)))
+			}
 		}
 		return nil
 	})
@@ -301,3 +353,20 @@ func validateCode(code string) error {
 	_, err := goformat.Source([]byte(code))
 	return err
 }
+
+func checkRedeclaredType(code string) error {
+	fset := token.NewFileSet()
+	f, err := goparser.ParseFile(fset, "", code, goparser.ParseComments)
+	if err != nil {
+		return err
+	}
+
+	conf := types.Config{
+		Error:    func(err error) {},
+		Importer: importer.Default(),
+	}
+
+	info := types.Info{Types: make(map[ast.Expr]types.TypeAndValue)}
+	_, err = conf.Check(f.Name.Name, fset, []*ast.File{f}, &info)
+	return err
+}
diff --git a/tools/goctl/api/gogen/testdata/another_import_api.api b/tools/goctl/api/gogen/testdata/another_import_api.api
new file mode 100644
index 00000000..1973b80b
--- /dev/null
+++ b/tools/goctl/api/gogen/testdata/another_import_api.api
@@ -0,0 +1,17 @@
+import "importApi.api"
+
+type AnotherRequest {
+    Name string `path:"name,options=you|me"`
+}
+
+type AnotherResponse {
+    Message string `json:"message"` // message
+}
+
+@server(
+    group: greet
+)
+service A-api {
+    @handler AnotherImportHandler
+    get /greet/from/another/:name(AnotherRequest) returns (AnotherResponse)
+}
diff --git a/tools/goctl/api/gogen/testdata/has_import_api.api b/tools/goctl/api/gogen/testdata/has_import_api.api
index 6710280e..e61fc512 100644
--- a/tools/goctl/api/gogen/testdata/has_import_api.api
+++ b/tools/goctl/api/gogen/testdata/has_import_api.api
@@ -1,16 +1,17 @@
 import "importApi.api"
 
-type Request struct {
+type Request {
     Name string `path:"name,options=you|me"`
 }
 
-type Response struct {
+type Response {
     Message string `json:"message"` // message
 }
 
+@server(
+    group: greet
+)
 service A-api {
-    @server(
-        handler: GreetHandler
-    )
+    @handler GreetHandler
     get /greet/from/:name(Request) returns (Response)
-}
\ No newline at end of file
+}
diff --git a/tools/goctl/api/gogen/testdata/import_api.api b/tools/goctl/api/gogen/testdata/import_api.api
index b663997d..594c5c57 100644
--- a/tools/goctl/api/gogen/testdata/import_api.api
+++ b/tools/goctl/api/gogen/testdata/import_api.api
@@ -1,3 +1,3 @@
-type ImportData struct {
+type ImportData {
     Name string `path:"name,options=you|me"`
-}
\ No newline at end of file
+}
diff --git a/tools/goctl/api/gogen/testdata/import_twice.api b/tools/goctl/api/gogen/testdata/import_twice.api
new file mode 100644
index 00000000..00e6c0b5
--- /dev/null
+++ b/tools/goctl/api/gogen/testdata/import_twice.api
@@ -0,0 +1,2 @@
+import "hasImportApi.api"
+import "anotherImportApi.api"