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.

Sign up for GitHub

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

Jump to bottom

desc/protoparse: report unused imports #403

Merged
merged 1 commit into from Jun 16, 2021
Merged

desc/protoparse: report unused imports #403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 19 additions & 0 deletions desc/protoparse/errors.go
View file
Open in desktop
Expand Up @@ -154,3 +154,22 @@ func (e errorWithFilename) Error() string {
func (e errorWithFilename) Unwrap() error {
return e.underlying
}

// ErrorUnusedImport may be passed to a warning reporter when an unused
// import is detected. The error the reporter receives will be wrapped
// with source position that indicates the file and line where the import
// statement appeared.
type ErrorUnusedImport interface {
error
UnusedImport() string
}

type errUnusedImport string

func (e errUnusedImport) Error() string {
return fmt.Sprintf("import %q not used", string(e))
}

func (e errUnusedImport) UnusedImport() string {
return string(e)
}
67 changes: 61 additions & 6 deletions desc/protoparse/linker.go
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ type linker struct {
descriptorPool map[*dpb.FileDescriptorProto]map[string]proto.Message
packageNamespaces map[*dpb.FileDescriptorProto]map[string]struct{}
extensions map[string]map[int32]string
usedImports map[*dpb.FileDescriptorProto]map[string]struct{}
}

func newLinker(files *parseResults, errs *errorHandler) *linker {
Expand Down Expand Up @@ -65,7 +66,7 @@ func (l *linker) linkFiles() (map[string]*desc.FileDescriptor, error) {
// options that remain.
for _, r := range l.files {
fd := linked[r.fd.GetName()]
if err := interpretFileOptions(r, richFileDescriptorish{FileDescriptor: fd}); err != nil {
if err := interpretFileOptions(l, r, richFileDescriptorish{FileDescriptor: fd}); err != nil {
return nil, err
}
// we should now have any message_set_wire_format options parsed
Expand Down Expand Up @@ -288,6 +289,7 @@ func descriptorType(m proto.Message) string {

func (l *linker) resolveReferences() error {
l.extensions = map[string]map[int32]string{}
l.usedImports = map[*dpb.FileDescriptorProto]map[string]struct{}{}
for _, filename := range l.filenames {
r := l.files[filename]
fd := r.fd
Expand Down Expand Up @@ -577,7 +579,7 @@ opts:
func (l *linker) resolve(fd *dpb.FileDescriptorProto, name string, onlyTypes bool, scopes []scope) (fqn string, element proto.Message, proto3 bool) {
if strings.HasPrefix(name, ".") {
// already fully-qualified
d, proto3 := l.findSymbol(fd, name[1:], false, map[*dpb.FileDescriptorProto]struct{}{})
d, proto3 := l.findSymbol(fd, name[1:])
if d != nil {
return name[1:], d, proto3
}
Expand Down Expand Up @@ -629,7 +631,7 @@ func fileScope(fd *dpb.FileDescriptorProto, l *linker) scope {
// packages are a hierarchy like C++ namespaces)
prefixes := internal.CreatePrefixList(fd.GetPackage())
querySymbol := func(n string) (d proto.Message, isProto3 bool) {
return l.findSymbol(fd, n, false, map[*dpb.FileDescriptorProto]struct{}{})
return l.findSymbol(fd, n)
}
return func(firstName, fullName string) (string, proto.Message, bool) {
for _, prefix := range prefixes {
Expand Down Expand Up @@ -699,6 +701,15 @@ func (l *linker) findSymbolInFile(name string, fd *dpb.FileDescriptorProto) prot
return nil
}

func (l *linker) markUsed(entryPoint, used *dpb.FileDescriptorProto) {
importsForFile := l.usedImports[entryPoint]
if importsForFile == nil {
importsForFile = map[string]struct{}{}
l.usedImports[entryPoint] = importsForFile
}
importsForFile[used.GetName()] = struct{}{}
}

func isAggregateDescriptor(m proto.Message) bool {
if m == sentinelMissingSymbol {
// this indicates the name matched a package, not a
Expand All @@ -720,7 +731,11 @@ func isAggregateDescriptor(m proto.Message) bool {
// definitively does not exist".
var sentinelMissingSymbol = (*dpb.DescriptorProto)(nil)

func (l *linker) findSymbol(fd *dpb.FileDescriptorProto, name string, public bool, checked map[*dpb.FileDescriptorProto]struct{}) (element proto.Message, proto3 bool) {
func (l *linker) findSymbol(fd *dpb.FileDescriptorProto, name string) (element proto.Message, proto3 bool) {
return l.findSymbolRecursive(fd, fd, name, false, map[*dpb.FileDescriptorProto]struct{}{})
}

func (l *linker) findSymbolRecursive(entryPoint, fd *dpb.FileDescriptorProto, name string, public bool, checked map[*dpb.FileDescriptorProto]struct{}) (element proto.Message, proto3 bool) {
if _, ok := checked[fd]; ok {
// already checked this one
return nil, false
Expand All @@ -741,7 +756,8 @@ func (l *linker) findSymbol(fd *dpb.FileDescriptorProto, name string, public boo
// we'll catch this error later
continue
}
if d, proto3 := l.findSymbol(depres.fd, name, true, checked); d != nil {
if d, proto3 := l.findSymbolRecursive(entryPoint, depres.fd, name, true, checked); d != nil {
l.markUsed(entryPoint, depres.fd)
return d, proto3
}
}
Expand All @@ -752,7 +768,8 @@ func (l *linker) findSymbol(fd *dpb.FileDescriptorProto, name string, public boo
// we'll catch this error later
continue
}
if d, proto3 := l.findSymbol(depres.fd, name, true, checked); d != nil {
if d, proto3 := l.findSymbolRecursive(entryPoint, depres.fd, name, true, checked); d != nil {
l.markUsed(entryPoint, depres.fd)
return d, proto3
}
}
Expand Down Expand Up @@ -855,3 +872,41 @@ func (l *linker) linkFile(name string, rootImportLoc *SourcePos, seen []string,
linked[name] = lfd
return lfd, nil
}

func (l *linker) checkForUnusedImports(filename string) {
r := l.files[filename]
usedImports := l.usedImports[r.fd]
node := r.nodes[r.fd]
fileNode, _ := node.(*ast.FileNode)
for i, dep := range r.fd.Dependency {
if _, ok := usedImports[dep]; !ok {
isPublic := false
// it's fine if it's a public import
for _, j := range r.fd.PublicDependency {
if i == int(j) {
isPublic = true
break
}
}
if isPublic {
break
}
var pos *SourcePos
if fileNode != nil {
for _, decl := range fileNode.Decls {
imp, ok := decl.(*ast.ImportNode)
if !ok {
continue
}
if imp.Name.AsString() == dep {
pos = imp.Start()
}
}
}
if pos == nil {
pos = ast.UnknownPos(r.fd.GetName())
}
r.errs.warn(pos, errUnusedImport(dep))
}
}
}
79 changes: 56 additions & 23 deletions desc/protoparse/options.go
View file
Open in desktop
Expand Up @@ -679,36 +679,36 @@ func (er extRangeDescriptorish) GetExtensionRangeOptions() *dpb.ExtensionRangeOp
return er.er.GetOptions()
}

func interpretFileOptions(r *parseResult, fd fileDescriptorish) error {
func interpretFileOptions(l *linker, r *parseResult, fd fileDescriptorish) error {
opts := fd.GetFileOptions()
if opts != nil {
if len(opts.UninterpretedOption) > 0 {
if remain, err := interpretOptions(r, fd, opts, opts.UninterpretedOption); err != nil {
if remain, err := interpretOptions(l, r, fd, opts, opts.UninterpretedOption); err != nil {
return err
} else {
opts.UninterpretedOption = remain
}
}
}
for _, md := range fd.GetMessageTypes() {
if err := interpretMessageOptions(r, md); err != nil {
if err := interpretMessageOptions(l, r, md); err != nil {
return err
}
}
for _, fld := range fd.GetExtensions() {
if err := interpretFieldOptions(r, fld); err != nil {
if err := interpretFieldOptions(l, r, fld); err != nil {
return err
}
}
for _, ed := range fd.GetEnumTypes() {
if err := interpretEnumOptions(r, ed); err != nil {
if err := interpretEnumOptions(l, r, ed); err != nil {
return err
}
}
for _, sd := range fd.GetServices() {
opts := sd.GetServiceOptions()
if len(opts.GetUninterpretedOption()) > 0 {
if remain, err := interpretOptions(r, sd, opts, opts.UninterpretedOption); err != nil {
if remain, err := interpretOptions(l, r, sd, opts, opts.UninterpretedOption); err != nil {
return err
} else {
opts.UninterpretedOption = remain
Expand All @@ -717,7 +717,7 @@ func interpretFileOptions(r *parseResult, fd fileDescriptorish) error {
for _, mtd := range sd.GetMethods() {
opts := mtd.GetMethodOptions()
if len(opts.GetUninterpretedOption()) > 0 {
if remain, err := interpretOptions(r, mtd, opts, opts.UninterpretedOption); err != nil {
if remain, err := interpretOptions(l, r, mtd, opts, opts.UninterpretedOption); err != nil {
return err
} else {
opts.UninterpretedOption = remain
Expand All @@ -728,61 +728,61 @@ func interpretFileOptions(r *parseResult, fd fileDescriptorish) error {
return nil
}

func interpretMessageOptions(r *parseResult, md msgDescriptorish) error {
func interpretMessageOptions(l *linker, r *parseResult, md msgDescriptorish) error {
opts := md.GetMessageOptions()
if opts != nil {
if len(opts.UninterpretedOption) > 0 {
if remain, err := interpretOptions(r, md, opts, opts.UninterpretedOption); err != nil {
if remain, err := interpretOptions(l, r, md, opts, opts.UninterpretedOption); err != nil {
return err
} else {
opts.UninterpretedOption = remain
}
}
}
for _, fld := range md.GetFields() {
if err := interpretFieldOptions(r, fld); err != nil {
if err := interpretFieldOptions(l, r, fld); err != nil {
return err
}
}
for _, ood := range md.GetOneOfs() {
opts := ood.GetOneOfOptions()
if len(opts.GetUninterpretedOption()) > 0 {
if remain, err := interpretOptions(r, ood, opts, opts.UninterpretedOption); err != nil {
if remain, err := interpretOptions(l, r, ood, opts, opts.UninterpretedOption); err != nil {
return err
} else {
opts.UninterpretedOption = remain
}
}
}
for _, fld := range md.GetNestedExtensions() {
if err := interpretFieldOptions(r, fld); err != nil {
if err := interpretFieldOptions(l, r, fld); err != nil {
return err
}
}
for _, er := range md.GetExtensionRanges() {
opts := er.GetExtensionRangeOptions()
if len(opts.GetUninterpretedOption()) > 0 {
if remain, err := interpretOptions(r, er, opts, opts.UninterpretedOption); err != nil {
if remain, err := interpretOptions(l, r, er, opts, opts.UninterpretedOption); err != nil {
return err
} else {
opts.UninterpretedOption = remain
}
}
}
for _, nmd := range md.GetNestedMessageTypes() {
if err := interpretMessageOptions(r, nmd); err != nil {
if err := interpretMessageOptions(l, r, nmd); err != nil {
return err
}
}
for _, ed := range md.GetNestedEnumTypes() {
if err := interpretEnumOptions(r, ed); err != nil {
if err := interpretEnumOptions(l, r, ed); err != nil {
return err
}
}
return nil
}

func interpretFieldOptions(r *parseResult, fld fldDescriptorish) error {
func interpretFieldOptions(l *linker, r *parseResult, fld fldDescriptorish) error {
opts := fld.GetFieldOptions()
if len(opts.GetUninterpretedOption()) > 0 {
uo := opts.UninterpretedOption
Expand Down Expand Up @@ -824,7 +824,7 @@ func interpretFieldOptions(r *parseResult, fld fldDescriptorish) error {
if len(uo) == 0 {
// no real options, only pseudo-options above? clear out options
fld.AsFieldDescriptorProto().Options = nil
} else if remain, err := interpretOptions(r, fld, opts, uo); err != nil {
} else if remain, err := interpretOptions(l, r, fld, opts, uo); err != nil {
return err
} else {
opts.UninterpretedOption = remain
Expand Down Expand Up @@ -898,11 +898,11 @@ func encodeDefaultBytes(b []byte) string {
return buf.String()
}

func interpretEnumOptions(r *parseResult, ed enumDescriptorish) error {
func interpretEnumOptions(l *linker, r *parseResult, ed enumDescriptorish) error {
opts := ed.GetEnumOptions()
if opts != nil {
if len(opts.UninterpretedOption) > 0 {
if remain, err := interpretOptions(r, ed, opts, opts.UninterpretedOption); err != nil {
if remain, err := interpretOptions(l, r, ed, opts, opts.UninterpretedOption); err != nil {
return err
} else {
opts.UninterpretedOption = remain
Expand All @@ -912,7 +912,7 @@ func interpretEnumOptions(r *parseResult, ed enumDescriptorish) error {
for _, evd := range ed.GetValues() {
opts := evd.GetEnumValueOptions()
if len(opts.GetUninterpretedOption()) > 0 {
if remain, err := interpretOptions(r, evd, opts, opts.UninterpretedOption); err != nil {
if remain, err := interpretOptions(l, r, evd, opts, opts.UninterpretedOption); err != nil {
return err
} else {
opts.UninterpretedOption = remain
Expand All @@ -922,8 +922,8 @@ func interpretEnumOptions(r *parseResult, ed enumDescriptorish) error {
return nil
}

func interpretOptions(res *parseResult, element descriptorish, opts proto.Message, uninterpreted []*dpb.UninterpretedOption) ([]*dpb.UninterpretedOption, error) {
optsd, err := desc.LoadMessageDescriptorForMessage(opts)
func interpretOptions(l *linker, res *parseResult, element descriptorish, opts proto.Message, uninterpreted []*dpb.UninterpretedOption) ([]*dpb.UninterpretedOption, error) {
optsd, err := loadMessageDescriptorForOptions(l, element.GetFile(), opts)
if err != nil {
if res.lenient {
return uninterpreted, nil
Expand Down Expand Up @@ -993,7 +993,7 @@ func interpretOptions(res *parseResult, element descriptorish, opts proto.Messag
}
}

// nw try to convert into the passed in message and fail if not successful
// now try to convert into the passed in message and fail if not successful
if err := dm.ConvertToDeterministic(opts); err != nil {
node := res.nodes[element.AsProto()]
return nil, res.errs.handleError(ErrorWithSourcePos{Pos: node.Start(), Underlying: err})
Expand All @@ -1002,6 +1002,39 @@ func interpretOptions(res *parseResult, element descriptorish, opts proto.Messag
return nil, nil
}

func loadMessageDescriptorForOptions(l *linker, fd fileDescriptorish, opts proto.Message) (*desc.MessageDescriptor, error) {
// see if the file imports a custom version of descriptor.proto
fqn := proto.MessageName(opts)
d := findMessageDescriptorForOptions(l, fd, fqn)
if d != nil {
return d, nil
}
// fall back to built-in options descriptors
return desc.LoadMessageDescriptorForMessage(opts)
}

func findMessageDescriptorForOptions(l *linker, fd fileDescriptorish, messageName string) *desc.MessageDescriptor {
d := fd.FindSymbol(messageName)
if d != nil {
md, _ := d.(*desc.MessageDescriptor)
return md
}

// TODO: should this support public imports and be recursive?
Copy link
Contributor

Choose a reason for hiding this comment

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

just making sure nothing to do here

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think this will involve a little more toying around with protoc to understand any nuances of its behavior and then mimic it exactly.

Also, FWIW, this is only for addressing #360 (using custom descriptor.proto), not for implementing the warning on unused imports. I addressed that issue at the same time just since my previous PR happened to already have test cases that cover the custom descriptor.proto case. This is a very rarely used feature of protoc (only useful if you are working on protoc or forking it and making changes to descriptor.proto). So this PR will at least get protoparse closer even if not yet perfect; so I don't think we need to do anything here right now.

for _, dep := range fd.GetDependencies() {
d := dep.FindSymbol(messageName)
if d != nil {
if l != nil {
l.markUsed(fd.AsProto().(*dpb.FileDescriptorProto), d.GetFile().AsFileDescriptorProto())
}
md, _ := d.(*desc.MessageDescriptor)
return md
}
}

return nil
}

func interpretField(res *parseResult, mc *messageContext, element descriptorish, dm *dynamic.Message, opt *dpb.UninterpretedOption, nameIndex int, pathPrefix []int32) (path []int32, err error) {
var fld *desc.FieldDescriptor
nm := opt.GetName()[nameIndex]
Expand Down
9 changes: 7 additions & 2 deletions desc/protoparse/parser.go
View file
Open in desktop
Expand Up @@ -228,10 +228,15 @@ func (p Parser) ParseFiles(filenames ...string) ([]*desc.FileDescriptor, error)
// TODO: if this re-writes one of the names in filenames, lookups below will break
protos = fixupFilenames(protos)
}
linkedProtos, err := newLinker(results, errs).linkFiles()
l := newLinker(results, errs)
linkedProtos, err := l.linkFiles()
if err != nil {
return nil, err
}
// Now we're done linking, so we can check to see if any imports were unused
for _, file := range filenames {
l.checkForUnusedImports(file)
}
if p.IncludeSourceCodeInfo {
for name, fd := range linkedProtos {
pr := protos[name]
Expand Down Expand Up @@ -316,7 +321,7 @@ func (p Parser) ParseFilesButDoNotLink(filenames ...string) ([]*dpb.FileDescript
pr.errs.errReporter = func(err ErrorWithPos) error {
return err
}
_ = interpretFileOptions(pr, poorFileDescriptorish{FileDescriptorProto: fd})
_ = interpretFileOptions(nil, pr, poorFileDescriptorish{FileDescriptorProto: fd})
}
if p.IncludeSourceCodeInfo {
fd.SourceCodeInfo = pr.generateSourceCodeInfo()
Expand Down