Skip to content

Commit

Permalink
fix optional proto fields false positive
Browse files Browse the repository at this point in the history
  • Loading branch information
ghostiam committed Nov 1, 2023
1 parent 1bc8154 commit 280e547
Show file tree
Hide file tree
Showing 6 changed files with 212 additions and 29 deletions.
94 changes: 91 additions & 3 deletions processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,50 @@ func (c *processor) process(n ast.Node) (*Result, error) {
c.writeFrom("*")
c.processInner(x.X)

case *ast.BinaryExpr:
// Check if the expression is a comparison.
if x.Op != token.EQL && x.Op != token.NEQ {
return &Result{}, nil
}

// Check if one of the operands is nil.

xIdent, xOk := x.X.(*ast.Ident)
yIdent, yOk := x.Y.(*ast.Ident)

xIsNil := xOk && xIdent.Name == "nil"
yIsNil := yOk && yIdent.Name == "nil"

if !xIsNil && !yIsNil {
return &Result{}, nil
}

// Extract the non-nil operand for further checks

var expr ast.Expr
if xIsNil {
expr = x.Y
} else {
expr = x.X
}

se, ok := expr.(*ast.SelectorExpr)
if !ok {
return &Result{}, nil
}

if !isProtoMessage(c.info, se.X) {
return &Result{}, nil
}

// Check if the Getter function of the protobuf message returns a pointer.
hasPointer, ok := getterResultHasPointer(c.info, se.X, se.Sel.Name)
if !ok || hasPointer {
return &Result{}, nil
}

c.filter.AddPos(x.X.Pos())

default:
return nil, fmt.Errorf("not implemented for type: %s (%s)", reflect.TypeOf(x), formatNode(n))
}
Expand Down Expand Up @@ -225,14 +269,14 @@ func isProtoMessage(info *types.Info, expr ast.Expr) bool {
return false
}

func methodIsExists(info *types.Info, x ast.Expr, name string) bool {
func typesNamed(info *types.Info, x ast.Expr) (*types.Named, bool) {
if info == nil {
return false
return nil, false
}

t := info.TypeOf(x)
if t == nil {
return false
return nil, false
}

ptr, ok := t.Underlying().(*types.Pointer)
Expand All @@ -241,6 +285,15 @@ func methodIsExists(info *types.Info, x ast.Expr, name string) bool {
}

named, ok := t.(*types.Named)
if !ok {
return nil, false
}

return named, true
}

func methodIsExists(info *types.Info, x ast.Expr, name string) bool {
named, ok := typesNamed(info, x)
if !ok {
return false
}
Expand All @@ -253,3 +306,38 @@ func methodIsExists(info *types.Info, x ast.Expr, name string) bool {

return false
}

func getterResultHasPointer(info *types.Info, x ast.Expr, name string) (hasPointer, ok bool) {
named, ok := typesNamed(info, x)
if !ok {
return false, false
}

for i := 0; i < named.NumMethods(); i++ {
method := named.Method(i)
if method.Name() != "Get"+name {
continue
}

var sig *types.Signature
sig, ok = method.Type().(*types.Signature)
if !ok {
return false, false
}

results := sig.Results()
if results.Len() == 0 {
return false, false
}

firstType := results.At(0)
_, ok = firstType.Type().(*types.Pointer)
if !ok {
return false, true
}

return true, true
}

return false, false
}
1 change: 1 addition & 0 deletions protogetter.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ func Run(pass *analysis.Pass, cfg *Config) ([]Issue, error) {

nodeTypes := []ast.Node{
(*ast.AssignStmt)(nil),
(*ast.BinaryExpr)(nil),
(*ast.CallExpr)(nil),
(*ast.SelectorExpr)(nil),
(*ast.StarExpr)(nil),
Expand Down
117 changes: 91 additions & 26 deletions testdata/proto/test.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions testdata/proto/test.proto
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ message Test {

// issue #4
optional bool opt_bool = 12;

// Optional enum
enum OEnum {
O_ENUM1 = 0;
O_ENUM2 = 1;
}
optional OEnum opt_enum = 13;
}

message Embedded {
Expand Down
11 changes: 11 additions & 0 deletions testdata/test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,17 @@ func testInvalid(t *proto.Test) {

t.Embedded.SetS("test") // want `avoid direct access to proto field t\.Embedded\.SetS\("test"\), use t\.GetEmbedded\(\)\.SetS\("test"\) instead`
t.Embedded.SetMap(map[string]string{"test": "test"}) // want `avoid direct access to proto field t\.Embedded\.SetMap\(map\[string\]string{"test": "test"}\), use t\.GetEmbedded\(\)\.SetMap\(map\[string\]string{"test": "test"}\) instead`

// Optional enum
switch *t.OptEnum { // want `avoid direct access to proto field \*t\.OptEnum, use t\.GetOptEnum\(\) instead`
case proto.Test_O_ENUM1:
case proto.Test_O_ENUM2:
}

if t.OptEnum != nil && *t.OptEnum == proto.Test_O_ENUM1 { // want `avoid direct access to proto field \*t\.OptEnum, use t\.GetOptEnum\(\) instead`
}

_ = *t.OptEnum // want `avoid direct access to proto field \*t\.OptEnum, use t\.GetOptEnum\(\) instead`
}

func testValid(t *proto.Test) {
Expand Down
11 changes: 11 additions & 0 deletions testdata/test.go.golden
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,17 @@ func testInvalid(t *proto.Test) {

t.GetEmbedded().SetS("test") // want `avoid direct access to proto field t\.Embedded\.SetS\("test"\), use t\.GetEmbedded\(\)\.SetS\("test"\) instead`
t.GetEmbedded().SetMap(map[string]string{"test": "test"}) // want `avoid direct access to proto field t\.Embedded\.SetMap\(map\[string\]string{"test": "test"}\), use t\.GetEmbedded\(\)\.SetMap\(map\[string\]string{"test": "test"}\) instead`

// Optional enum
switch t.GetOptEnum() { // want `avoid direct access to proto field \*t\.OptEnum, use t\.GetOptEnum\(\) instead`
case proto.Test_O_ENUM1:
case proto.Test_O_ENUM2:
}

if t.OptEnum != nil && t.GetOptEnum() == proto.Test_O_ENUM1 { // want `avoid direct access to proto field \*t\.OptEnum, use t\.GetOptEnum\(\) instead`
}

_ = t.GetOptEnum() // want `avoid direct access to proto field \*t\.OptEnum, use t\.GetOptEnum\(\) instead`
}

func testValid(t *proto.Test) {
Expand Down

0 comments on commit 280e547

Please sign in to comment.