Skip to content

Commit

Permalink
resolve: detect when a rule imports itself
Browse files Browse the repository at this point in the history 
Add selfImportError, which is now returned by
RuleIndex.findRuleByImport and related methods when a rule imports
itself (which is common with protos).

Resolver methods treat this error the same as standardImportError: no
dependency is emitted.

Fixes bazelbuild#45
  • Loading branch information
@jayconrod
Jay Conrod authored and jayconrod committed Dec 26, 2017
1 parent 87e9a4f commit c521ab0
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 69 deletions.
67 changes: 39 additions & 28 deletions resolve/index.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ func (ix *RuleIndex) skipGoEmbds() {
}

for _, l := range embedLabels {
embed, ok := ix.findRuleByLabel(l, r.label.Pkg)
embed, ok := ix.findRuleByLabel(l, r.label)
if !ok {
continue
}
Expand Down Expand Up @@ -220,16 +220,25 @@ func (ix *RuleIndex) buildImportIndex() {
}

type ruleNotFoundError struct {
imp string
fromRel string
from Label
imp string
}

func (e ruleNotFoundError) Error() string {
return fmt.Sprintf("no rule found for import %q, needed in package %q", e.imp, e.fromRel)
return fmt.Sprintf("no rule found for import %q, needed in %s", e.imp, e.from)
}

type selfImportError struct {
from Label
imp string
}

func (ix *RuleIndex) findRuleByLabel(label Label, fromRel string) (*ruleRecord, bool) {
label = label.Abs("", fromRel)
func (e selfImportError) Error() string {
return fmt.Sprintf("rule %s imports itself with path %q", e.from, e.imp)
}

func (ix *RuleIndex) findRuleByLabel(label Label, from Label) (*ruleRecord, bool) {
label = label.Abs(from.Repo, from.Pkg)
r, ok := ix.labelMap[label]
return r, ok
}
Expand All @@ -238,14 +247,15 @@ func (ix *RuleIndex) findRuleByLabel(label Label, fromRel string) (*ruleRecord,
// imp is the import to resolve (which includes the target language). lang is
// the language of the rule with the dependency (for example, in
// go_proto_library, imp will have ProtoLang and lang will be GoLang).
// fromRel is the slash-separated path to the directory containing the import,
// relative to the repository root.
// from is the rule which is doing the dependency. This is used to check
// vendoring visibility and to check for self-imports.
//
// Any number of rules may provide the same import. If no rules provide
// the import, ruleNotFoundError is returned. If multiple rules provide the
// import, this function will attempt to choose one based on Go vendoring logic.
// In ambiguous cases, an error is returned.
func (ix *RuleIndex) findRuleByImport(imp importSpec, lang config.Language, fromRel string) (*ruleRecord, error) {
// Any number of rules may provide the same import. If no rules provide the
// import, ruleNotFoundError is returned. If a rule imports itself,
// selfImportError is returned. If multiple rules provide the import, this
// function will attempt to choose one based on Go vendoring logic. In
// ambiguous cases, an error is returned.
func (ix *RuleIndex) findRuleByImport(imp importSpec, lang config.Language, from Label) (*ruleRecord, error) {
matches := ix.importMap[imp]
var bestMatch *ruleRecord
var bestMatchIsVendored bool
Expand All @@ -260,21 +270,19 @@ func (ix *RuleIndex) findRuleByImport(imp importSpec, lang config.Language, from
case config.GoLang:
// Apply vendoring logic for Go libraries. A library in a vendor directory
// is only visible in the parent tree. Vendored libraries supercede
// non-vendored libraries, and libraries closer to fromRel supercede
// non-vendored libraries, and libraries closer to from.Pkg supercede
// those further up the tree.
isVendored := false
vendorRoot := ""
if m.label.Repo == "" {
parts := strings.Split(m.label.Pkg, "/")
for i, part := range parts {
if part == "vendor" {
isVendored = true
vendorRoot = strings.Join(parts[:i], "/")
break
}
parts := strings.Split(m.label.Pkg, "/")
for i, part := range parts {
if part == "vendor" {
isVendored = true
vendorRoot = strings.Join(parts[:i], "/")
break
}
}
if isVendored && fromRel != vendorRoot && !strings.HasPrefix(fromRel, vendorRoot+"/") {
if isVendored && !packageContains(m.label.Repo, vendorRoot, from) {
// vendor directory not visible
continue
}
Expand Down Expand Up @@ -303,21 +311,24 @@ func (ix *RuleIndex) findRuleByImport(imp importSpec, lang config.Language, from
return nil, matchError
}
if bestMatch == nil {
return nil, ruleNotFoundError{imp.imp, fromRel}
return nil, ruleNotFoundError{from, imp.imp}
}
if bestMatch.label.Equal(from) {
return nil, selfImportError{from, imp.imp}
}

if imp.lang == config.ProtoLang && lang == config.GoLang {
importpath := bestMatch.rule.AttrString("importpath")
if betterMatch, err := ix.findRuleByImport(importSpec{config.GoLang, importpath}, config.GoLang, fromRel); err == nil {
if betterMatch, err := ix.findRuleByImport(importSpec{config.GoLang, importpath}, config.GoLang, from); err == nil {
return betterMatch, nil
}
}

return bestMatch, nil
}

func (ix *RuleIndex) findLabelByImport(imp importSpec, lang config.Language, fromRel string) (Label, error) {
r, err := ix.findRuleByImport(imp, lang, fromRel)
func (ix *RuleIndex) findLabelByImport(imp importSpec, lang config.Language, from Label) (Label, error) {
r, err := ix.findRuleByImport(imp, lang, from)
if err != nil {
return NoLabel, err
}
Expand All @@ -329,7 +340,7 @@ func findGoProtoSources(ix *RuleIndex, r *ruleRecord) []importSpec {
if err != nil {
return nil
}
proto, ok := ix.findRuleByLabel(protoLabel, r.label.Pkg)
proto, ok := ix.findRuleByLabel(protoLabel, r.label)
if !ok {
return nil
}
Expand Down
15 changes: 15 additions & 0 deletions resolve/label.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package resolve

import (
"fmt"
"log"
"path"
"regexp"
"strings"
Expand Down Expand Up @@ -119,3 +120,17 @@ func (l Label) Abs(repo, pkg string) Label {
}
return Label{Repo: repo, Pkg: pkg, Name: l.Name}
}

func (l Label) Equal(other Label) bool {
return l.Repo == other.Repo &&
l.Pkg == other.Pkg &&
l.Name == other.Name &&
l.Relative == other.Relative
}

func packageContains(repo, pkg string, label Label) bool {
if label.Relative {
log.Panicf("label must not be relative: %s", label)
}
return repo == label.Repo && (pkg == label.Pkg || strings.HasPrefix(label.Pkg, pkg+"/"))
}
30 changes: 17 additions & 13 deletions resolve/resolve.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,9 @@ func (r *Resolver) ResolveRule(e bf.Expr, pkgRel string) bf.Expr {
return e
}
rule := bf.Rule{Call: call}
from := Label{Pkg: pkgRel, Name: rule.Name()}

var resolve func(imp, pkgRel string) (Label, error)
var resolve func(imp string, from Label) (Label, error)
switch rule.Kind() {
case "go_library", "go_binary", "go_test":
resolve = r.resolveGo
Expand All @@ -91,12 +92,15 @@ func (r *Resolver) ResolveRule(e bf.Expr, pkgRel string) bf.Expr {
rule.DelAttr(config.GazelleImportsKey)
rule.DelAttr("deps")
deps := mapExprStrings(imports, func(imp string) string {
label, err := resolve(imp, pkgRel)
label, err := resolve(imp, from)
if err != nil {
if _, ok := err.(standardImportError); !ok {
switch err.(type) {
case standardImportError, selfImportError:
return ""
default:
log.Print(err)
return ""
}
return ""
}
label.Relative = label.Repo == "" && label.Pkg == pkgRel
return label.String()
Expand Down Expand Up @@ -206,11 +210,11 @@ func mapExprStrings(e bf.Expr, f func(string) string) bf.Expr {
// resolveGo resolves an import path from a Go source file to a label.
// pkgRel is the path to the Go package relative to the repository root; it
// is used to resolve relative imports.
func (r *Resolver) resolveGo(imp, pkgRel string) (Label, error) {
func (r *Resolver) resolveGo(imp string, from Label) (Label, error) {
if build.IsLocalImport(imp) {
cleanRel := path.Clean(path.Join(pkgRel, imp))
cleanRel := path.Clean(path.Join(from.Pkg, imp))
if build.IsLocalImport(cleanRel) {
return Label{}, fmt.Errorf("relative import path %q from %q points outside of repository", imp, pkgRel)
return Label{}, fmt.Errorf("relative import path %q from %q points outside of repository", imp, from.Pkg)
}
imp = path.Join(r.c.GoPrefix, cleanRel)
}
Expand All @@ -219,7 +223,7 @@ func (r *Resolver) resolveGo(imp, pkgRel string) (Label, error) {
return Label{}, standardImportError{imp}
}

if label, err := r.ix.findLabelByImport(importSpec{config.GoLang, imp}, config.GoLang, pkgRel); err != nil {
if label, err := r.ix.findLabelByImport(importSpec{config.GoLang, imp}, config.GoLang, from); err != nil {
if _, ok := err.(ruleNotFoundError); !ok {
return NoLabel, err
}
Expand All @@ -245,7 +249,7 @@ const (

// resolveProto resolves an import statement in a .proto file to a label
// for a proto_library rule.
func (r *Resolver) resolveProto(imp, pkgRel string) (Label, error) {
func (r *Resolver) resolveProto(imp string, from Label) (Label, error) {
if !strings.HasSuffix(imp, ".proto") {
return Label{}, fmt.Errorf("can't import non-proto: %q", imp)
}
Expand All @@ -254,7 +258,7 @@ func (r *Resolver) resolveProto(imp, pkgRel string) (Label, error) {
return Label{Repo: config.WellKnownTypesProtoRepo, Name: name}, nil
}

if label, err := r.ix.findLabelByImport(importSpec{config.ProtoLang, imp}, config.ProtoLang, pkgRel); err != nil {
if label, err := r.ix.findLabelByImport(importSpec{config.ProtoLang, imp}, config.ProtoLang, from); err != nil {
if _, ok := err.(ruleNotFoundError); !ok {
return NoLabel, err
}
Expand All @@ -272,7 +276,7 @@ func (r *Resolver) resolveProto(imp, pkgRel string) (Label, error) {

// resolveGoProto resolves an import statement in a .proto file to a
// label for a go_library rule that embeds the corresponding go_proto_library.
func (r *Resolver) resolveGoProto(imp, pkgRel string) (Label, error) {
func (r *Resolver) resolveGoProto(imp string, from Label) (Label, error) {
if !strings.HasSuffix(imp, ".proto") {
return Label{}, fmt.Errorf("can't import non-proto: %q", imp)
}
Expand Down Expand Up @@ -314,7 +318,7 @@ func (r *Resolver) resolveGoProto(imp, pkgRel string) (Label, error) {
}
}

if label, err := r.ix.findLabelByImport(importSpec{config.ProtoLang, imp}, config.GoLang, pkgRel); err != nil {
if label, err := r.ix.findLabelByImport(importSpec{config.ProtoLang, imp}, config.GoLang, from); err != nil {
if _, ok := err.(ruleNotFoundError); !ok {
return NoLabel, err
}
Expand All @@ -330,7 +334,7 @@ func (r *Resolver) resolveGoProto(imp, pkgRel string) (Label, error) {
if rel == "." {
rel = ""
}
if pkgRel == "vendor" || strings.HasPrefix(pkgRel, "vendor/") {
if from.Pkg == "vendor" || strings.HasPrefix(from.Pkg, "vendor/") {
rel = path.Join("vendor", rel)
}
return r.l.LibraryLabel(rel), nil
Expand Down
Loading

0 comments on commit c521ab0

Please sign in to comment.