Copilot commented on code in PR #243:
URL: https://github.com/apache/skywalking-go/pull/243#discussion_r3063545222
##########
plugins/core/instrument/enhance.go:
##########
@@ -81,8 +81,71 @@ func generateTypeNameByExp(exp dst.Expr) string {
data = "..." + generateTypeNameByExp(n.Elt)
case *dst.ArrayType:
data = "[]" + generateTypeNameByExp(n.Elt)
+ case *dst.FuncType:
+ data = generateFuncTypeName(n)
+ case *dst.ChanType:
+ data = generateChanTypeName(n)
default:
return ""
}
return data
}
+
+func generateFuncTypeName(n *dst.FuncType) string {
+ result := "func("
+ if n.Params != nil {
+ for i, field := range n.Params.List {
+ if i > 0 {
+ result += ", "
+ }
+ if len(field.Names) > 0 {
+ for j, name := range field.Names {
+ if j > 0 {
+ result += ", "
+ }
+ result += name.Name + " " +
generateTypeNameByExp(field.Type)
+ }
+ } else {
+ result += generateTypeNameByExp(field.Type)
+ }
+ }
+ }
+ result += ")"
+ if n.Results != nil && len(n.Results.List) > 0 {
+ if len(n.Results.List) == 1 && len(n.Results.List[0].Names) ==
0 {
+ result += " " +
generateTypeNameByExp(n.Results.List[0].Type)
+ } else {
+ result += " ("
+ for i, field := range n.Results.List {
+ if i > 0 {
+ result += ", "
+ }
+ if len(field.Names) > 0 {
+ for j, name := range field.Names {
+ if j > 0 {
+ result += ", "
+ }
+ result += name.Name + " " +
generateTypeNameByExp(field.Type)
+ }
+ } else {
+ result +=
generateTypeNameByExp(field.Type)
+ }
Review Comment:
generateFuncTypeName currently includes parameter/result variable names when
present (e.g., it will produce `func(ctx context.Context) error`). This will
break `WithArgType`/`WithResultType` matching for func-typed args in real code
where func signatures commonly have named params, since matchers throughout the
repo use name-less strings like `func(context.Context, *primitive.SendResult,
error)`. Consider generating func type names using only types (ignore
`field.Names`) so matching is stable and independent of source-level naming.
```suggestion
func generateFieldTypeNames(fields []*dst.Field) []string {
var result []string
for _, field := range fields {
typeName := generateTypeNameByExp(field.Type)
if len(field.Names) > 0 {
for range field.Names {
result = append(result, typeName)
}
} else {
result = append(result, typeName)
}
}
return result
}
func generateFuncTypeName(n *dst.FuncType) string {
result := "func("
if n.Params != nil {
params := generateFieldTypeNames(n.Params.List)
for i, param := range params {
if i > 0 {
result += ", "
}
result += param
}
}
result += ")"
if n.Results != nil && len(n.Results.List) > 0 {
results := generateFieldTypeNames(n.Results.List)
if len(results) == 1 {
result += " " + results[0]
} else if len(results) > 1 {
result += " ("
for i, res := range results {
if i > 0 {
result += ", "
}
result += res
```
##########
plugins/core/instrument/enhance_test.go:
##########
@@ -0,0 +1,168 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package instrument
+
+import (
+ "testing"
+
+ "github.com/dave/dst"
+ "github.com/stretchr/testify/assert"
+)
+
+func TestGenerateTypeNameByExp_BasicTypes(t *testing.T) {
+ tests := []struct {
+ name string
+ expr dst.Expr
+ expected string
+ }{
+ {"ident", &dst.Ident{Name: "string"}, "string"},
+ {"selector", &dst.SelectorExpr{X: dst.NewIdent("context"), Sel:
dst.NewIdent("Context")}, "context.Context"},
+ {"star selector", &dst.StarExpr{X: &dst.SelectorExpr{X:
dst.NewIdent("http"), Sel: dst.NewIdent("Request")}}, "*http.Request"},
+ {"star ident", &dst.StarExpr{X: dst.NewIdent("error")},
"*error"},
+ {"ellipsis", &dst.Ellipsis{Elt: dst.NewIdent("string")},
"...string"},
+ {"array", &dst.ArrayType{Elt: dst.NewIdent("int")}, "[]int"},
+ {"interface", &dst.InterfaceType{}, "interface{}"},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ assert.Equal(t, tt.expected,
generateTypeNameByExp(tt.expr))
+ })
+ }
+}
+
+func TestGenerateTypeNameByExp_FuncType(t *testing.T) {
+ tests := []struct {
+ name string
+ expr dst.Expr
+ expected string
+ }{
+ {
+ "func with no params no results",
+ &dst.FuncType{
+ Params: &dst.FieldList{},
+ },
+ "func()",
+ },
+ {
+ "func with single param",
+ &dst.FuncType{
+ Params: &dst.FieldList{List: []*dst.Field{
+ {Type: dst.NewIdent("int")},
+ }},
+ },
+ "func(int)",
+ },
+ {
+ "func with multiple params",
+ &dst.FuncType{
+ Params: &dst.FieldList{List: []*dst.Field{
+ {Type: &dst.SelectorExpr{X:
dst.NewIdent("context"), Sel: dst.NewIdent("Context")}},
+ {Type: dst.NewIdent("string")},
+ }},
+ },
+ "func(context.Context, string)",
+ },
+ {
+ "func with single unnamed result",
+ &dst.FuncType{
+ Params: &dst.FieldList{List: []*dst.Field{
+ {Type: dst.NewIdent("int")},
+ }},
+ Results: &dst.FieldList{List: []*dst.Field{
+ {Type: dst.NewIdent("error")},
+ }},
+ },
+ "func(int) error",
+ },
+ {
+ "func with multiple results",
+ &dst.FuncType{
+ Params: &dst.FieldList{List: []*dst.Field{
+ {Type: dst.NewIdent("string")},
+ }},
+ Results: &dst.FieldList{List: []*dst.Field{
+ {Type: dst.NewIdent("int")},
+ {Type: dst.NewIdent("error")},
+ }},
+ },
+ "func(string) (int, error)",
+ },
+ {
+ "func with complex params",
+ &dst.FuncType{
+ Params: &dst.FieldList{List: []*dst.Field{
+ {Type: &dst.SelectorExpr{X:
dst.NewIdent("context"), Sel: dst.NewIdent("Context")}},
+ {Type: &dst.StarExpr{X:
&dst.SelectorExpr{X: dst.NewIdent("primitive"), Sel:
dst.NewIdent("SendResult")}}},
+ {Type: dst.NewIdent("error")},
+ }},
+ },
+ "func(context.Context, *primitive.SendResult, error)",
+ },
Review Comment:
The new func-type generation tests don’t cover func signatures with named
parameters/results (which are common in callback types). Adding at least one
case with named params (e.g., `func(ctx context.Context, err error)` expected
to stringify without names) would help prevent regressions once func type name
generation is made name-insensitive.
```suggestion
},
{
"func with named params and named result",
&dst.FuncType{
Params: &dst.FieldList{List: []*dst.Field{
{
Names:
[]*dst.Ident{dst.NewIdent("ctx")},
Type: &dst.SelectorExpr{X:
dst.NewIdent("context"), Sel: dst.NewIdent("Context")},
},
{
Names:
[]*dst.Ident{dst.NewIdent("err")},
Type: dst.NewIdent("error"),
},
}},
Results: &dst.FieldList{List: []*dst.Field{
{
Names:
[]*dst.Ident{dst.NewIdent("sendErr")},
Type: dst.NewIdent("error"),
},
}},
},
"func(context.Context, error) error",
},
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
