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

Core skeleton #1

Merged
merged 34 commits into from
Apr 12, 2019
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
6ca6176
Created a skeleton of the core library.
Mar 28, 2019
d13d54f
Added rule in-line disabling.
Mar 29, 2019
faa542f
Changed `lint.ID` to `lint.RuleID`.
mingzhi Mar 30, 2019
ded05fe
Fixed wrong check function.
mingzhi Mar 30, 2019
fa921cc
Added `RulesConfig` and implemented rules running.
mingzhi Mar 30, 2019
5cb0a2d
Created cmd `proto-gen-api_lint` and fixed descriptor source lookups.
mingzhi Mar 31, 2019
298b7cb
Used `go mod`
mingzhi Mar 31, 2019
685e712
Rewrote `protovisit`.
Apr 1, 2019
0842f9d
Added a visitor for general descriptors.
mingzhi Apr 2, 2019
6e6c9d7
Added more tests and cleared names.
Apr 2, 2019
d510581
Moved rule disabling check to context source.
mingzhi Apr 2, 2019
5afe6da
Rewrote rule checkers.
mingzhi Apr 2, 2019
6a8bb19
Pushed reflection down to checkers.
mingzhi Apr 3, 2019
ffdc046
Added gitignore
robcapo Apr 11, 2019
9923cc7
Removed rules
robcapo Apr 11, 2019
cc20751
Removed protovisit
robcapo Apr 11, 2019
3246993
Changed StartLocation to a factory
robcapo Apr 11, 2019
bfe53b6
Merge pull request #5 from jgeewax/start_location_factory
mingzhi Apr 11, 2019
a0514f1
remove cmd, we will rewrite it later
Apr 11, 2019
8e01b24
Merge pull request #6 from jgeewax/remove-cmd
robcapo Apr 11, 2019
63b7abf
Removed context and modified tests for DescriptorSource.
Apr 11, 2019
7183941
Added getters for Request and creating a Request from file descriptor…
Apr 11, 2019
c16bc2b
Merge pull request #7 from jgeewax/update-request
robcapo Apr 12, 2019
4a0bba0
Made NewDescriptorSource a private function.
Apr 12, 2019
b88445c
Removed unused error
robcapo Apr 12, 2019
f3be115
Renamed error
robcapo Apr 12, 2019
21623cb
Added an error for invalid spans, returning it if a span is provided …
robcapo Apr 12, 2019
b0c0f95
Added more tests for source.
Apr 12, 2019
6a1a362
Made error package private
robcapo Apr 12, 2019
e4ccca5
Merge pull request #8 from jgeewax/remove_unused_error
mingzhi Apr 12, 2019
ee2280f
Improved error message
robcapo Apr 12, 2019
88b5f67
Merge pull request #9 from jgeewax/update-request
mingzhi Apr 12, 2019
90d7eb1
Moved span error inline
robcapo Apr 12, 2019
4eabf74
Merge pull request #10 from jgeewax/span_error
mingzhi Apr 12, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.idea
99 changes: 99 additions & 0 deletions cmd/protoc-gen-api_lint/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
package main

import (
"context"
"io"
"io/ioutil"
"log"
"os"

"github.com/golang/protobuf/v2/proto"
"github.com/golang/protobuf/v2/reflect/protodesc"
"github.com/golang/protobuf/v2/reflect/protoreflect"
"github.com/golang/protobuf/v2/reflect/protoregistry"
pluginpb "github.com/golang/protobuf/v2/types/plugin"
"github.com/jgeewax/api-linter/lint"
"github.com/jgeewax/api-linter/rules"
)

func main() {
rules := rules.Rules()
log.Printf("Number of rules: %d\n", len(rules.AllRules()))
linter := newLinter(*rules)
linter.run()
}

type linter struct {
reader io.Reader
writer io.Writer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is never used, but instead the problems are printed via log.Printf. Is this intentional? And if so, let's get rid of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rewrite this protoc plugin using this helper function? Thanks!

rules lint.Rules
}

func (l linter) run() {
request := readLintRequest(l.reader)
response, err := lint.Run(l.rules, request)
if err != nil {
log.Fatalf("Error when running lint: %v", err)
}
log.Printf("Total number of API-Linter findings: %d", len(response.Problems))
for _, problem := range response.Problems {
log.Printf("Finding: %s, suggestion: %s", problem.Message, problem.Suggestion)
}
}

func newLinter(rules lint.Rules) *linter {
return &linter{
rules: rules,
reader: os.Stdin,
writer: os.Stdout,
}
}

func readLintRequest(r io.Reader) lint.Request {
in, err := ioutil.ReadAll(r)
if err != nil {
log.Fatalf("Error when reading CodeGeneratorRequest: %v", err)
}

var codeGenRequest pluginpb.CodeGeneratorRequest
if err := proto.Unmarshal(in, &codeGenRequest); err != nil {
log.Fatalf("Error when unmarshaling CodeGeneratorRequest: %v", err)
}

if len(codeGenRequest.GetProtoFile()) == 0 {
log.Fatalf("Error: zero proto files in CodeGeneratorRequest")
}

fd := codeGenRequest.GetProtoFile()[0]
f, err := protodesc.NewFile(fd, protoregistry.NewFiles())
if err != nil {
log.Fatalf("Error when converting proto to descriptor: %v", err)
}

ctx := context.Background()
source, err := lint.NewDescriptorSource(fd)
if err != nil {
return lintRequest{
protoFile: f,
context: lint.NewContext(ctx),
}
}

return lintRequest{
protoFile: f,
context: lint.NewContextWithDescriptorSource(ctx, source),
}
}

type lintRequest struct {
protoFile protoreflect.FileDescriptor
context lint.Context
}

func (r lintRequest) ProtoFile() protoreflect.FileDescriptor {
return r.protoFile
}

func (r lintRequest) Context() lint.Context {
return r.context
}
9 changes: 9 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module github.com/jgeewax/api-linter

go 1.12

require (
github.com/golang/protobuf/v2 v2.0.0-20190115031900-66c365cf7239
github.com/iancoleman/strcase v0.0.0-20180726023541-3605ed457bf7
github.com/stretchr/testify v1.3.0
)
23 changes: 23 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/golang/protobuf v1.2.1-0.20181127190454-8d0c54c12466/go.mod h1:Qd/q+1AKNOZr9uGQzbzCmRO6sUih6GTPZv6a1/R87v0=
github.com/golang/protobuf v1.2.1-0.20181205191652-7e65e513332f h1:jEoef3K+ZQwZ7UB1iGu6KhX8hS9cYw1aXR7djS3Vn10=
github.com/golang/protobuf v1.2.1-0.20181205191652-7e65e513332f/go.mod h1:asK8yRb/+zxJTE0SbTESCku/4OjiDfbPwk4rEyIatUA=
github.com/golang/protobuf/v2 v2.0.0-20181127193627-d7e97bc71bcb/go.mod h1:MgUD+N3FwzDmj2CdMsT5ap7K7jx+c9cQDQ7fVhmH+Xw=
github.com/golang/protobuf/v2 v2.0.0-20190115031900-66c365cf7239 h1:gRJbl/8g6n9YmDWRAI93uRuIVnw/ESX2o/SMfLO3QOY=
github.com/golang/protobuf/v2 v2.0.0-20190115031900-66c365cf7239/go.mod h1:sjQt90Yu/7/I4QgfZq+CzFOly7327WbVZ1EDQjIlnMI=
github.com/google/go-cmp v0.2.1-0.20181101181452-745b8ec83783/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M=
github.com/iancoleman/strcase v0.0.0-20180726023541-3605ed457bf7 h1:ux/56T2xqZO/3cP1I2F86qpeoYPCOzk+KF/UH/Ar+lk=
github.com/iancoleman/strcase v0.0.0-20180726023541-3605ed457bf7/go.mod h1:SK73tn/9oHe+/Y0h39VT4UCxmurVJkR5NA7kMEAOgSE=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/stretchr/objx v0.1.0 h1:4G4v2dO3VZwixGIRoQ5Lfboy6nUhCyYzaqnIAPPhYs4=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q=
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20180926154720-4dfa2610cdf3/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/tools v0.0.0-20180904205237-0aa4b8830f48/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20180928181343-b3c0be4c978b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
google.golang.org/genproto v0.0.0-20180831171423-11092d34479b/go.mod h1:JiN7NxoALGmiZfu7CAH4rXhgtRTLTxftemlI0sWmxmc=
33 changes: 33 additions & 0 deletions lint/context.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package lint

import "context"

// Context provides additional information for a rule to perform linting.
type Context struct {
Copy link
Collaborator

@robcapo robcapo Apr 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mingzhi because of the fact that the Request struct is passed to the rules, I think Context is superfluous. Any additional "context" that we want can go directly in the Request struct, grouped into more cohesive structs if it makes sense. In other words, I think our Request struct is equivalent to ESLint's context object.

context context.Context
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here? I don't see it being used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, this should be removed.

descSource DescriptorSource
}

// NewContext creates a new `Context`.
func NewContext(ctx context.Context) Context {
return Context{
context: ctx,
}
}

// NewContextWithDescriptorSource creates a new `Context` with the source.
func NewContextWithDescriptorSource(ctx context.Context, source DescriptorSource) Context {
return Context{
context: ctx,
descSource: source,
}
}

// DescriptorSource returns a `DescriptorSource` if available; otherwise,
// returns (nil, ErrSourceInfoNotAvailable).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update it? Thanks!

//
// The returned `DescriptorSource` contains additional information
// about a protobuf descriptor, such as comments and location lookups.
func (c Context) DescriptorSource() DescriptorSource {
return c.descSource
}
3 changes: 3 additions & 0 deletions lint/lint.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
// Package lint contains interfaces and functions to check API styles in
// Google Protobuf APIs.
package lint
robcapo marked this conversation as resolved.
Show resolved Hide resolved
29 changes: 29 additions & 0 deletions lint/location.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package lint

// Location describes a location in a source code file.
type Location struct {
Start, End Position
}

// IsValid checks if the location struct is constructed properly and
// returns true if it's valid.
func (l Location) IsValid() bool {
return l.Start.IsValid() && l.End.IsValid()
}

// Position describes a one-based position in a source code file.
type Position struct {
Line, Column int
}

// IsValid checks if the position struct is constructed properly and
// returns true if it's valid.
func (p Position) IsValid() bool {
return p.Line > 0 && p.Column > 0
}

// StartLocation marks the very start position in a file.
var StartLocation = Location{
robcapo marked this conversation as resolved.
Show resolved Hide resolved
Start: Position{1, 1},
End: Position{1, 1},
}
42 changes: 42 additions & 0 deletions lint/mocks/Request.go

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

104 changes: 104 additions & 0 deletions lint/mocks/Rule.go

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

22 changes: 22 additions & 0 deletions lint/problem.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package lint

import (
"github.com/golang/protobuf/v2/reflect/protoreflect"
)

// Problem contains information about a result produced by an API Linter.
type Problem struct {
// Message provides a short description of the problem.
Message string
// Suggestion provides a suggested fix, if applicable.
Suggestion string
robcapo marked this conversation as resolved.
Show resolved Hide resolved
// Location provides the location of the problem. If both
// `Location` and `Descriptor` are specified, the location
// is then used from `Location` instead of `Descriptor`.
Location Location
// Descriptor provides the descriptor related
// to the problem. If present and `Location` is not
// specified, then the starting location of the descriptor
robcapo marked this conversation as resolved.
Show resolved Hide resolved
// is used as the location of the problem.
Descriptor protoreflect.Descriptor
}
11 changes: 11 additions & 0 deletions lint/request.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package lint

import "github.com/golang/protobuf/v2/reflect/protoreflect"

// Request defines input data for a rule to perform linting.
type Request interface {
mingzhi marked this conversation as resolved.
Show resolved Hide resolved
// ProtoFile returns a `FileDescriptor` when the rule's `FileTypes`
// contains `ProtoFile`.
ProtoFile() protoreflect.FileDescriptor
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we considering cases where a rule might need visibility into multiple files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the proto files that it depends, which can be accessed from FileDescriptor.Imports()? Or other types of files, which should have their own getter, like OnePlatformConfigFile()?

Context() Context
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in person, can we remove the Context struct, as well as the context.Context member, and move the DescriptorSource to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please. Thanks!

}
11 changes: 11 additions & 0 deletions lint/response.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package lint

// Response describes the result returned by a rule.
type Response struct {
Problems []Problem
}

// mrge merges another response.
func (resp *Response) merge(other Response) {
resp.Problems = append(resp.Problems, other.Problems...)
}