ghostiam / protogetter Goto Github PK

View Code? Open in Web Editor NEW

17.0 17.0 0.0 78 KB

Protobuf golang linter - use getters instead of fields.

License: MIT License

Makefile 1.08% Go 98.92%

golang golang-grpc golang-linter grpc-go linter protobuf

protogetter's People

Contributors

Stargazers

Watchers

protogetter's Issues

Fix for direct access to proto fields does not remove dereference

Consider the following offending snippet:

func GetField(proto SomeProtoType) int32 {
	return *proto.SomeInt32Field
}

Upon running golangci-lint run --fix, this is corrected to:

func GetField(proto SomeProtoType) int32 {
	return *proto.SomeInt32Field()
}

The dereference is not removed, leading to the compiler throwing an error saying cannot indirect proto.SomeInt32Field() (value of type int32). The indirection must also be removed as a part of the fix for this case.

false positive(?): flags proto fields which are being written, not read

I have some code here which sets fields in a proto.

	metadata := data.ShardMetadata{
		NumShard:  new(int32),
		ShardLoc:  new(string),
		CommitSha: new(string),
	}
	*metadata.NumShard = shardNum + 1
	*metadata.ShardLoc = bucket + "/" + data.GetBlobFilename("", t)
	*metadata.CommitSha = version.GetVersionInfo().GitCommit
	metadataJSON, err := protojson.Marshal(&metadata)

The linter flags this:

cron/internal/controller/main.go:159:3: avoid direct access to proto field metadata.NumShard, use metadata.GetNumShard() instead (protogetter)
        *metadata.NumShard = shardNum + 1

The provided fix (when I use the --fix flag) doesn't compile.

	*metadata.GetNumShard() = shardNum + 1
	*metadata.GetShardLoc() = bucket + "/" + data.GetBlobFilename("", t)
	*metadata.GetCommitSha() = version.GetVersionInfo().GitCommit

invalid operation: cannot indirect metadata.GetShardLoc() (value of type string)compiler (InvalidIndirection)

Should the linter be flagging non-get operations?

False positive: Optional proto-fields being flagged and fixed if checking if the field is nil

I have a multiple proto messages with optional fields defined, for example:

message Result {
  string data = 1;
  optional int32 parent = 2;
}

This generates a getter for the optional field like this:

func (x *Result) GetParent() int32 {
    if x != nil && x.Parent != nil {
        return *x.Parent
    }
    return 0
}

The linter flags and fixes accessing this field to use this getter method as follows:

if result.GetParent() != nil {
    ....
}

This obviously doesn't work. Checking for zero here doesn't work, as zero is a valid value for this field and is for that reason marked as optional so that I can check if the field is nil instead.

Should the linter only fix cases where we are directly de-referencing the value from the pointer?

So it could fix this:

val := *result.Parent

But not this:

val := result.Parent

Disallow Get on fields that are a part of a `oneof`

Is it possible to instruct the linter to error on cases where you're getting one of the fields inside of a oneof?

For example:

message Foo {
  oneof bar {
    string baz = 1;
    string quix = 2;
  }
}

I would like to make it a lint error to call GetBaz and GetQuix directly in favor of always doing a type switch on the output of GetBar(). I don't see a documented way to do that but was curious if such a thing was possible.

Thanks so much!

Review in context of golangci-lint integration

Feel free to split in separate issues if needed.

@ghostiam, hi!

Thank you for linter.
I deeply reviewed it.

Naming proposal – protogolint is too general and conflicts with protolint and similar. If in the future will appear the new proto-related linter when we going to question – why is it not part of your linter? IMHO, better to narrow naming to specific functionality. In your case – protoaccess, protoderefchecker, protogetter, protodeferlint, protousegetter, protoshield, protonodirectread, protoreadguard (help me 🙂). Inspiration can be drawn from https://golangci-lint.run/usage/linters/

// want "proto field read without getter:"

What is purpose of : in the end?
Based on implementation looks like a bug / typo.

// want "proto field read without getter:" "proto field read without getter:" "proto field read without getter:" "proto field read without getter:"

IMHO, it is cool to merge the similar messages (example) or make it more informative (see below).

It is cool to make message more informative and without tautology, e.g.

// want "proto field `Embedded` read without getter"
// want "direct access to proto `Embedded` field" (golangci-lint appends linter name as prefix)
// want "direct access to `Embedded` field of proto type `proto.Test`"

(help me 🙂)

Great idea with different suggested fixes modes:

const (
	StandaloneMode Mode = iota
	GolangciLintMode
)

My respect!

You wrote

Requires: []*analysis.Analyzer{inspect.Analyzer},

but dont use this dependency.

You ignore linting for linter project (see check list).
E.g. I see preallocation cases.

IMHO, try to avoid strings comparison like

return sct.Field(0).Type().String() == messageState

(but in this case I cannot suggest better approach)

Also, old projects uses github.com/golang/protobuf.

// If getter exists, use it.

In such cases it doesn't exist?

IMHO

func (c *Checker) Check

is not the best naming, it's more about suggested fixing, not checking itself.
or Checker type itself just does not match the letter S from SOLID.

In addition check looks over-engineered a lot.

I expected simple logic like:

find SelectorExpr
if not Getter then print warning and add suggest fix with simple ("Get"+value), or with help of go/format.Node

Reporting linter errors via diagnostic doesn't look as common and nice practice

if err != nil {
	pass.Report(analysis.Diagnostic{
		Pos:     n.Pos(),
		End:     n.End(),
		Message: fmt.Sprintf("error: %v", err),
	})
	return nil
}

(@ldez knows better)

🙂

P.S. Run linter in k8s and look nice results and messages 👍
No panics.

Expand me

▶ protogolint ./...
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:125:25: proto field read without getter: "mf.Metric" should be "mf.GetMetric()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:126:25: proto field read without getter: "m.Label" should be "m.GetLabel()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:283:20: proto field read without getter: "mf.Metric" should be "mf.GetMetric()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:284:23: proto field read without getter: "m.Label" should be "m.GetLabel()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:285:33: proto field read without getter: "lbl.Name" should be "lbl.GetName()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:293:9: proto field read without getter: "mf.Metric" should be "mf.GetMetric()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:294:20: proto field read without getter: "mf.Metric" should be "mf.GetMetric()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:296:23: proto field read without getter: "m.Label" should be "m.GetLabel()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:297:31: proto field read without getter: "lbl.Name" should be "lbl.GetName()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:297:56: proto field read without getter: "lbl.Value" should be "lbl.GetValue()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:310:9: proto field read without getter: "mf.Metric" should be "mf.GetMetric()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:313:20: proto field read without getter: "mf.Metric" should be "mf.GetMetric()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:314:39: proto field read without getter: "m.Label" should be "m.GetLabel()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:315:42: proto field read without getter: "metric.Counter.Value" should be "metric.GetCounter().GetValue()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:315:66: proto field read without getter: "m.Counter.Value" should be "m.GetCounter().GetValue()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:317:23: proto field read without getter: "m.Label" should be "m.GetLabel()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:336:20: proto field read without getter: "mf.Metric" should be "mf.GetMetric()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:337:10: proto field read without getter: "m.Label" should be "m.GetLabel()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:338:23: proto field read without getter: "m.Label" should be "m.GetLabel()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:339:27: proto field read without getter: "lbl.Name" should be "lbl.GetName()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:354:11: proto field read without getter: "mf.Name" should be "mf.GetName()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:355:11: proto field read without getter: "mf.Help" should be "mf.GetHelp()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:356:11: proto field read without getter: "mf.Type" should be "mf.GetType()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:357:37: proto field read without getter: "mf.Metric" should be "mf.GetMetric()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:358:20: proto field read without getter: "mf.Metric" should be "mf.GetMetric()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:366:39: proto field read without getter: "m.Label" should be "m.GetLabel()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:367:21: proto field read without getter: "m.Label" should be "m.GetLabel()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:370:12: proto field read without getter: "m.Gauge" should be "m.GetGauge()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:371:14: proto field read without getter: "m.Counter" should be "m.GetCounter()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:372:14: proto field read without getter: "m.Summary" should be "m.GetSummary()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:373:14: proto field read without getter: "m.Untyped" should be "m.GetUntyped()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:374:16: proto field read without getter: "m.Histogram" should be "m.GetHistogram()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:375:18: proto field read without getter: "m.TimestampMs" should be "m.GetTimestampMs()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:381:11: proto field read without getter: "lp.Name" should be "lp.GetName()"
/tmp/kubernetes/cluster/images/etcd-version-monitor/etcd-version-monitor.go:382:12: proto field read without getter: "lp.Value" should be "lp.GetValue()"
/tmp/kubernetes/test/e2e/kubectl/kubectl.go:1080:18: proto field read without getter: "d.Definitions" should be "d.GetDefinitions()"
/tmp/kubernetes/test/e2e/kubectl/kubectl.go:1083:21: proto field read without getter: "d.Definitions.AdditionalProperties" should be "d.GetDefinitions().GetAdditionalProperties()"
/tmp/kubernetes/test/e2e/kubectl/kubectl.go:1084:19: proto field read without getter: "p.Value" should be "p.GetValue()"
/tmp/kubernetes/test/e2e/kubectl/kubectl.go:1087:29: proto field read without getter: "p.Value.VendorExtension" should be "p.GetValue().GetVendorExtension()"
/tmp/kubernetes/test/e2e/kubectl/kubectl.go:1090:11: proto field read without getter: "p.Value" should be "p.GetValue()"
/tmp/kubernetes/test/images/agnhost/grpc-health-checking/grpc-health-checking.go:69:5: proto field read without getter: "req.Service" should be "req.GetService()"
/tmp/kubernetes/test/integration/apiserver/tracing/tracing_test.go:943:6: proto field read without getter: "span.Name" should be "span.GetName()"
/tmp/kubernetes/test/integration/apiserver/tracing/tracing_test.go:952:77: proto field read without getter: "span.TraceId" should be "span.GetTraceId()"
/tmp/kubernetes/test/integration/apiserver/tracing/tracing_test.go:984:12: proto field read without getter: "event.Name" should be "event.GetName()"
/tmp/kubernetes/test/integration/controlplane/transformation/transformation_test.go:540:25: proto field read without getter: "mf.Name" should be "mf.GetName()"
/tmp/kubernetes/test/integration/controlplane/transformation/transformation_test.go:541:25: proto field read without getter: "mf.Name" should be "mf.GetName()"

But I am not sure about false positives.

error: processInner: not implemented for type: *ast.ArrayType (protogetter)

Hello 👋 thanks for creating and maintaining this great project!

We just hit an internal protgetter issue, it seems, which I figured I'd quickly report here.

Example code:

import (
	"cloud.google.com/go/bigquery"
	"google.golang.org/protobuf/types/known/structpb"
)

func unmarshalStruct(bqValue bigquery.Value) (*structpb.Struct, error) {
	s, ok := bqValue.(string)
	if !ok {
		return nil, fmt.Errorf("unsupported BigQuery value: %#v", bqValue)
	}
	var structValue structpb.Struct
	if err := structValue.UnmarshalJSON([]byte(s)); err != nil {
		return nil, fmt.Errorf("invalid BigQuery value: %#v: %w", bqValue, err)
	}
	return &structValue, nil
}

The error:

[golangci-lint] unmarshal.go:480:12: error: processInner: not implemented for type: *ast.ArrayType (protogetter)
if err := structValue.UnmarshalJSON([]byte(s)); err != nil {

Recommend Projects

React

A declarative, efficient, and flexible JavaScript library for building user interfaces.
Vue.js

🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
Typescript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
TensorFlow

An Open Source Machine Learning Framework for Everyone
Django

The Web framework for perfectionists with deadlines.
Laravel

A PHP framework for web artisans
D3

Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

javascript

JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
web

Some thing interesting about web. New door for the world.
server

A server is a program made to process requests and deliver data to clients.
Machine learning

Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Visualization

Some thing interesting about visualization, use data art
Game

Some thing interesting about game, make everyone happy.

Recommend Org

Facebook

We are working to build community through open source technology. NB: members must have two-factor auth.
Microsoft

Open source projects and samples from Microsoft.
Google

Google ❤️ Open Source for everyone.
Alibaba

Alibaba Open Source for everyone
D3

Data-Driven Documents codes.
Tencent

China tencent open source team.