From c521ab05095594844dc3852e5ea808c513b142bb Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Fri, 22 Dec 2017 17:01:00 -0500 Subject: [PATCH] resolve: detect when a rule imports itself 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 #45 --- resolve/index.go | 67 ++++++++++++++++++++++++----------------- resolve/label.go | 15 +++++++++ resolve/resolve.go | 30 ++++++++++-------- resolve/resolve_test.go | 65 ++++++++++++++++++++++----------------- 4 files changed, 108 insertions(+), 69 deletions(-) diff --git a/resolve/index.go b/resolve/index.go index 85178db9c..6fcb9079d 100644 --- a/resolve/index.go +++ b/resolve/index.go @@ -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 } @@ -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 } @@ -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 @@ -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 } @@ -303,12 +311,15 @@ 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 } } @@ -316,8 +327,8 @@ func (ix *RuleIndex) findRuleByImport(imp importSpec, lang config.Language, from 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 } @@ -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 } diff --git a/resolve/label.go b/resolve/label.go index 5aab26ed5..a5526f0c4 100644 --- a/resolve/label.go +++ b/resolve/label.go @@ -17,6 +17,7 @@ package resolve import ( "fmt" + "log" "path" "regexp" "strings" @@ -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+"/")) +} diff --git a/resolve/resolve.go b/resolve/resolve.go index 169e18a22..e360ef99a 100644 --- a/resolve/resolve.go +++ b/resolve/resolve.go @@ -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 @@ -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() @@ -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) } @@ -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 } @@ -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) } @@ -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 } @@ -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) } @@ -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 } @@ -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 diff --git a/resolve/resolve_test.go b/resolve/resolve_test.go index 953bf8011..a646dfc40 100644 --- a/resolve/resolve_test.go +++ b/resolve/resolve_test.go @@ -37,11 +37,12 @@ func TestResolveGoIndex(t *testing.T) { rel, content string } type testCase struct { - desc string - buildFiles []fileSpec - imp, fromRel string - wantErr string - want Label + desc string + buildFiles []fileSpec + imp string + from Label + wantErr string + want Label } for _, tc := range []testCase{ { @@ -119,9 +120,9 @@ go_library( `, }, }, - imp: "example.com/foo", - fromRel: "b", - want: Label{Name: "root"}, + imp: "example.com/foo", + from: Label{Pkg: "b", Name: "b"}, + want: Label{Name: "root"}, }, { desc: "vendor_supercedes_nonvendor", buildFiles: []fileSpec{ @@ -166,9 +167,9 @@ go_library( `, }, }, - imp: "example.com/foo", - fromRel: "shallow/deep", - want: Label{Pkg: "shallow/deep/vendor", Name: "deep"}, + imp: "example.com/foo", + from: Label{Pkg: "shallow/deep", Name: "deep"}, + want: Label{Pkg: "shallow/deep/vendor", Name: "deep"}, }, } { t.Run(tc.desc, func(t *testing.T) { @@ -184,7 +185,7 @@ go_library( ix.Finish() r := NewResolver(c, l, ix) - got, err := r.resolveGo(tc.imp, tc.fromRel) + got, err := r.resolveGo(tc.imp, tc.from) if err != nil { if tc.wantErr == "" { t.Fatal(err) @@ -240,18 +241,26 @@ go_library( r := NewResolver(c, l, ix) wantProto := Label{Pkg: "sub", Name: "foo_proto"} - if got, err := r.resolveProto("sub/bar.proto", "baz"); err != nil { + if got, err := r.resolveProto("sub/bar.proto", Label{Pkg: "baz", Name: "baz"}); err != nil { t.Error(err) } else if !reflect.DeepEqual(got, wantProto) { t.Errorf("resolveProto: got %s ; want %s", got, wantProto) } + _, err = r.resolveProto("sub/bar.proto", Label{Pkg: "sub", Name: "foo_proto"}) + if _, ok := err.(selfImportError); !ok { + t.Errorf("resolveProto: got %v ; want selfImportError", err) + } wantGoProto := Label{Pkg: "sub", Name: "embed"} - if got, err := r.resolveGoProto("sub/bar.proto", "baz"); err != nil { + if got, err := r.resolveGoProto("sub/bar.proto", Label{Pkg: "baz", Name: "baz"}); err != nil { t.Error(err) } else if !reflect.DeepEqual(got, wantGoProto) { t.Errorf("resolveGoProto: got %s ; want %s", got, wantGoProto) } + _, err = r.resolveGoProto("sub/bar.proto", Label{Pkg: "sub", Name: "foo_go_proto"}) + if _, ok := err.(selfImportError); !ok { + t.Errorf("resolveGoProto: got %v ; want selfImportError", err) + } } func TestIndexGenRuleReplacesOldRule(t *testing.T) { @@ -287,12 +296,12 @@ go_library( ix.Finish() - got, err := ix.findLabelByImport(importSpec{config.GoLang, "example.com/old"}, config.GoLang, "") + got, err := ix.findLabelByImport(importSpec{config.GoLang, "example.com/old"}, config.GoLang, NoLabel) if err == nil { t.Errorf("when importing example.com/old, got %s ; want error", got) } - got, err = ix.findLabelByImport(importSpec{config.GoLang, "example.com/new"}, config.GoLang, "") + got, err = ix.findLabelByImport(importSpec{config.GoLang, "example.com/new"}, config.GoLang, NoLabel) want := Label{Name: "go_default_library"} if err != nil { t.Errorf("when importing example.com/new, got error %v ; want %s", err, want) @@ -304,8 +313,7 @@ go_library( func TestResolveGoLocal(t *testing.T) { for _, spec := range []struct { importpath string - pkgRel string - want Label + from, want Label }{ { importpath: "example.com/repo", @@ -327,7 +335,7 @@ func TestResolveGoLocal(t *testing.T) { want: Label{Pkg: "another", Name: config.DefaultLibName}, }, { importpath: "../y", - pkgRel: "x", + from: Label{Pkg: "x", Name: "x"}, want: Label{Pkg: "y", Name: config.DefaultLibName}, }, } { @@ -335,7 +343,7 @@ func TestResolveGoLocal(t *testing.T) { l := NewLabeler(c) ix := NewRuleIndex() r := NewResolver(c, l, ix) - label, err := r.resolveGo(spec.importpath, spec.pkgRel) + label, err := r.resolveGo(spec.importpath, spec.from) if err != nil { t.Errorf("r.resolveGo(%q) failed with %v; want success", spec.importpath, err) continue @@ -358,12 +366,12 @@ func TestResolveGoLocalError(t *testing.T) { "example.com/another/sub", "example.com/repo_suffix", } { - if l, err := r.resolveGo(importpath, ""); err == nil { + if l, err := r.resolveGo(importpath, NoLabel); err == nil { t.Errorf("r.resolveGo(%q) = %s; want error", importpath, l) } } - if l, err := r.resolveGo("..", ""); err == nil { + if l, err := r.resolveGo("..", NoLabel); err == nil { t.Errorf("r.resolveGo(%q) = %s; want error", "..", l) } } @@ -376,14 +384,14 @@ func TestResolveGoEmptyPrefix(t *testing.T) { imp := "foo" want := Label{Pkg: "foo", Name: config.DefaultLibName} - if got, err := r.resolveGo(imp, ""); err != nil { + if got, err := r.resolveGo(imp, NoLabel); err != nil { t.Errorf("r.resolveGo(%q) failed with %v; want success", imp, err) } else if !reflect.DeepEqual(got, want) { t.Errorf("r.resolveGo(%q) = %s; want %s", imp, got, want) } imp = "fmt" - if _, err := r.resolveGo(imp, ""); err == nil { + if _, err := r.resolveGo(imp, NoLabel); err == nil { t.Errorf("r.resolveGo(%q) succeeded; want failure") } } @@ -391,7 +399,8 @@ func TestResolveGoEmptyPrefix(t *testing.T) { func TestResolveProto(t *testing.T) { prefix := "example.com/repo" for _, tc := range []struct { - desc, imp, pkgRel string + desc, imp string + from Label depMode config.DependencyMode wantProto, wantGoProto Label }{ @@ -409,7 +418,7 @@ func TestResolveProto(t *testing.T) { desc: "vendor", depMode: config.VendorMode, imp: "foo/bar/bar.proto", - pkgRel: "vendor", + from: Label{Pkg: "vendor"}, wantProto: Label{Pkg: "foo/bar", Name: "bar_proto"}, wantGoProto: Label{Pkg: "vendor/foo/bar", Name: config.DefaultLibName}, }, { @@ -445,7 +454,7 @@ func TestResolveProto(t *testing.T) { ix := NewRuleIndex() r := NewResolver(c, l, ix) - got, err := r.resolveProto(tc.imp, tc.pkgRel) + got, err := r.resolveProto(tc.imp, tc.from) if err != nil { t.Errorf("resolveProto: got error %v ; want success", err) } @@ -453,7 +462,7 @@ func TestResolveProto(t *testing.T) { t.Errorf("resolveProto: got %s ; want %s", got, tc.wantProto) } - got, err = r.resolveGoProto(tc.imp, tc.pkgRel) + got, err = r.resolveGoProto(tc.imp, tc.from) if err != nil { t.Errorf("resolveGoProto: go error %v ; want success", err) }