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

proposal: x/tools/go/analysis/passes/shadow: option to ignore generated code #61574

Open
ainar-g opened this issue Jul 25, 2023 · 3 comments
Open
Labels
Milestone

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Jul 25, 2023

protoc-gen-go-grpc, in particular, generates code like:

func (c *myServiceClient) GetProfiles(ctx context.Context, in *ProfilesRequest, opts ...grpc.CallOption) (MyService_GetProfilesClient, error) {
	stream, err := c.cc.NewStream(ctx, &MyService_ServiceDesc.Streams[0], "/MyService/getProfiles", opts...)
	if err != nil {
		return nil, err
	}
	x := &myServiceGetProfilesClient{stream}
	if err := x.ClientStream.SendMsg(in); err != nil {
		return nil, err
	}
	if err := x.ClientStream.CloseSend(); err != nil {
		return nil, err
	}
	return x, nil
}

Another, more generic option would be providing a list of ignored files.

@gopherbot gopherbot added this to the Proposal milestone Jul 25, 2023
@timothy-king
Copy link
Contributor

Another, more generic option would be providing a list of ignored files.

An already available option is post processing the json output to filter the list/regex of files to ignore.

But going more broadly, how do you want to be able to specify this option? My personal preference would be an annotation in the function or package, e.g. var _ = shadowannot.Ignore(myServiceClient.GetProfiles). Is this feasible for the generated code you are looking at? Do you have an alternative pathway for giving annotations/options in mind?

@ainar-g
Copy link
Contributor Author

ainar-g commented Jul 28, 2023

But going more broadly, how do you want to be able to specify this option?

In any way, really. The existing tools our projects use have a wild variety of ways to ignore things from the command line:

  • gocyclo has --ignore=<regexp> to specify a regular expression for filenames.

  • errcheck has three flags—--ignoregenerated, --ignorepkg=<pkg>, and --ignoretests—all of which are self-explanatory, but there is also a deprecated flag --ignore=<pkg:regexp> to once again ignore files based on a regular expression

  • gosec has --exclude-dir=<dir> and --exclude-generate, which are again self-explanatory.

I personally think that gocyclo's approach is the most flexible, especially when talking about generated files, since they often have special filenames, like foo.pb.go.

On a more finegrained level, one could specify an identifier the same way go doc expects it. I.e. my.own/pkg/path.Identifier.method.

Is this feasible for the generated code you are looking at? Do you have an alternative pathway for giving annotations/options in mind?

I think there is a way to customize the gRPC code generator by either forking it or using some kind of a plugin, but that feels like on overkill to just add an annotation.

@timothy-king
Copy link
Contributor

Ignoring based on a regexp at the file level makes sense to me. I think a combination of the package and the file's base name sounds fairly robust and helpful (*.pb.go likely covers a project well). I believe all of this information we need to make this string should be available in the analysis.Pass object. It should be possible to make this check fairly fast too. This could be done in shadow.Analyzer.Flags so no other analyzers would need to be impacted.

If someone deeply wanted fine grained filtering, they could use multiple files to contain the 'ignore shadow' functions.

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

No branches or pull requests

3 participants