Move lint rules to a FileDescriptorSet-based library

[bufdev]

This is more to start a discussion than anything concrete, not sure if you'd be interested in this at all.

So a few points:

• Protoreflect is a magical library, but using https://godoc.org/github.com/jhump/protoreflect/desc types directly has been something I've tried to avoid, as it hard-ties the linters to the compiler implementation (you can create desc.FileDescriptors from serialized FileDescriptorSets, but it gets weird).
• (jhump if you see this, correct me if I'm wrong) but once google.golang.org/protobuf comes out, there may be a desire to move away from desc entirely, as https://godoc.org/google.golang.org/protobuf/reflect/protoreflect takes care of a lot of the same concerns.
• This package isn't stable yet though , and actually for pure linting, I've found it hard to use, see APIv2: protoreflect.FileDescriptor should be able to preserve source info golang/protobuf#859

To make it easy to switch between v1 and v2 golang/protobuf, and to directly use FileDescriptorSets, I wrote https://godoc.org/github.com/bufbuild/buf/internal/pkg/protodesc which is what all of Buf's linting and breaking change detection uses. This takes in raw FileDescriptorSets (proxied through https://godoc.org/github.com/bufbuild/buf/internal/pkg/protodescpb but that can be removed) and gives you objects that are easy to use in terms of linting. It's meant to be a super-lightweight package that doesn't effectively tie the linting to a compilation library.

Would there be any interest if:

• I moved this package to a proper library, i.e. something like github.com/bufbuild/protodesc, and hardened it as a separate library (extra testing, documentation, cleanup).
• Port the lint rules inside api-linter to use this instead?

This would have a couple ancillary benefits, besides future-proofing for google.golang.org/protobuf:

• I could also semi-directly port https://github.com/bufbuild/buf/blob/master/internal/buf/bufbuild/runner.go over here, which is a hardened wrapper around protoparse, that also does parallel compilation, which results in massive speedups, for example on a 4-core MacBook Pro (the vast majority of the time is spent in protoparse if you profile this):

$ time api-linter $(find . -name '*.proto') >/dev/null

real	0m2.555s
user	0m3.809s
sys	0m0.266s

$ time buf check lint >/dev/null

real	0m0.754s
user	0m4.252s
sys	0m0.349s

• We could also have a common concept of what a lint rule is, which would make combining efforts easier in the future.

Thoughts?

[mingzhi]

Thank you for opening this discussion! I think we will revisit this topic once the golang/protoreflect is released and mature enough. We recently migrated from it to jhump's (issue #146). We are satisfied so far with what jhump's protoreflect offers:

1. Resolving dependencies completely.
2. No need of bundling our linter with protoc; but we have no issue making the two working together.

Is there any blocking issue that we have to move to protobuf V2 that we haven't realized for this linter?

[bufdev]

Sorry, maybe I wasn’t clear - I’m not proposing ditching protoreflect. You’d still use protoreflect, but instead of basing your lint rules on the types in protoreflect, you’d use a lighter library. This would not use protoc in any way. I’d recommend checking about the code I linked in Buf for details.

[bufdev]

@lukesneeringer for context...specifically what I'm approximately suggesting here:

• Use a pure interface to represent all the descriptor types, and not rely on the protoreflect structs. Buf does this with https://github.com/bufbuild/buf/blob/46b6effd99dfd443af0939a6dd106643a4e82d86/internal/pkg/protodesc/protodesc.go#L118 and I could extract this to a library (and add support for things api-linter needs, i.e. custom options)
• Change ProtoRule and Problem to be pure interfaces such as:

// ProtoRule is a rule to check Protobuf files. 
type ProtoRule interface {
  // ID is the unique ID that identifies this rule.
  ID() string
  // Description is a human-readable description of the rule.
  Description() string
  // Check checks the given files against the rule.
  //
  // File is the agreed-upon interface for FileDescriptorProtos, we reference protodesc.File
  // for this GitHub Issue.
  // Note that this takes a list of Files instead of a single rule as some ProtoRules check
  // entire packages, for example when checking file option consistency.
  //
  // Returns a list of Problems, or error on system error.
  Check(files ...protodesc.File) ([]Problem, error)
}

// Problem is an identified problem from a ProtoRule.
type Problem interface {
	// Filename is the filename to display.
	// This is typically a relative path when produced by linters.
	// If the filename is not known, this will be empty.
	Filename() string
	// StartLine is the starting line.
	// As opposed to SourceCodeInfo_Locations, this is not zero-indexed.
	// If the line is not known, this will be 0.
	StartLine() int
	// StartColumn is the starting column.
	// As opposed to SourceCodeInfo_Locations, this is not zero-indexed.
	// If the column is not known, this will be 0.
	StartColumn() int
	// EndLine is the ending line.
	// As opposed to SourceCodeInfo_Locations, this is not zero-indexed.
	// If the end line is not known, this will be explicitly set to the value
	// of StartLine() (which may be 0 if the start line is also not known).
	EndLine() int
	// EndColumn is the ending column.
	// As opposed to SourceCodeInfo_Locations, this is not zero-indexed.
	// If the end column is not known, this will be explicitly set to the value
	// of StartColumn() (which may be 0 if the start column is also not known).
	EndColumn() int
	// ID is the ID of the rule this Problem is associated with.
	// This will always be non-empty.
	ID() string
	// Message is the human-readable message.
	// This will always be non-empty.
	Message() string
	// Suggestion provides a suggested fix, if applicable.
	//
	// This integrates with certain IDEs to provide "push-button" fixes,
	// so these need to be machine-readable, not just human-readable.
	//
	// If this is set, StartLine(), EndLine(), StartColumn(), and EndColumn()
	// must have valid values.
	Suggestion() string
}

This would allow switching out protoreflect types in the future (for example, with v2 golang/protobuf), and also allow a common understanding of what a lint rule is without any necessary dependencies (so for example, there's compatibility between buf and api-linter). The problem is nice as an interface as well so there can be different backing types (Buf uses https://github.com/bufbuild/buf/blob/46b6effd99dfd443af0939a6dd106643a4e82d86/internal/pkg/analysis/analysis.go#L14 for example).

[mingzhi]

Since the interfaces have a deep hierarchy, I am wondering how flexible to switch from one protoreflect type to another. I image that we need to create some wrappers for each switch, and those wrappers are not thin or simple.

Do your interfaces support resolving dependencies/imports? Do they support options? We have many rules checking HTTP Rule in the method options.

If we would like to use proto interfaces, can we wait until v2 golang/protobuf is released?

I agree that it is nice to make problem as an interface .

[bufdev]

We created all the wrappers already at https://github.com/bufbuild/buf/tree/master/internal/pkg/protodesc that represent the full FileDescriptorSet. It was somewhat time-consuming, but the work is done. Yes, they do support dependencies/imports as necessary for linting.

It does not currently support options, as mentioned, but happy to extend to this. There's two ways to do this:

• Special-case for google.api options - this is the easiest, but least flexible.
• Expose custom options directly - this would take a bit more thinking, but is doable.

The golang/protobuf interfaces that are coming out aren't the best for lint rules, they only have minimal support for locations, so you'll want to have additional support anyways. See golang/protobuf#859

[bufdev]

I'd really recommend checking out https://godoc.org/github.com/bufbuild/buf/internal/pkg/protodesc

[mingzhi]

I took a quick look at your protodesc. It seems to be significantly different to either jhump's or golang's protoreflect library. It feels that it is another protoreflect library.

We spent about 1-month-SWE time to migrate from golang's to jhump's library and being familiar about it, when this linter had much fewer rules. If we would like to migrate to yours, which would touch almost every file, including tests (and we have not mentioned changes in Google and other places), I expect 3-month-SWE work. So, I still don't see what benefits would balance the cost. Once we migrated to your interfaces, there would be learning curve for other rule developers.

[bufdev]

It's definitely not another protoreflect library, it just exposes interfaces suitable for lint rules that are as minimal as possible. I'd be able to put up a PR that migrates between the two with about 6-8 hours of work if it would make it easier (maybe a bit more, but not much) - I'm familiar with both, and the concepts easily translate. I don't think the 3-month estimate is accurate for what it's worth on my end, but I could be wrong.

Additionally, being so much simpler than protoreflect/desc, the learning curve is pretty small - the interfaces are extremely minimal and only add what is necessary to wrap the FileDescriptorSet types with the least amount of surprise. I'd expect it would be easier to work with (that was certainly my experience, and hence why we wrote this).

[mingzhi]

We treat API users as our top priority. Currently, we are focusing on writing more and high-quality rules and documentations. We will revisit this issue in the future.