From d6140bd92c6f72b12db5a3b7ce706cdc565c1387 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vytautas=20=C5=A0altenis?= Date: Mon, 14 Aug 2017 21:38:33 +0300 Subject: [PATCH 1/8] Cleanup: fix typos --- internal/gps/identifier.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/gps/identifier.go b/internal/gps/identifier.go index de24e88c34..d01b794755 100644 --- a/internal/gps/identifier.go +++ b/internal/gps/identifier.go @@ -41,10 +41,10 @@ import ( type ProjectRoot string // A ProjectIdentifier provides the name and source location of a dependency. It -// is related to, but differs in two keys ways from, an plain import path. +// is related to, but differs in two key ways from, a plain import path. // // First, ProjectIdentifiers do not identify a single package. Rather, they -// encompasses the whole tree of packages, including tree's root - the +// encompass the whole tree of packages, including tree's root - the // ProjectRoot. In gps' current design, this ProjectRoot almost always // corresponds to the root of a repository. // From 0e41a07120d76a70eb2520a9401c2b64eeedd8eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vytautas=20=C5=A0altenis?= Date: Mon, 14 Aug 2017 21:40:04 +0300 Subject: [PATCH 2/8] Cleanup: simplify switch statement, reuse build.IsLocalImport We already import and use build package here, so there's no harm in reuse. The code is both shorter and more readable. --- internal/gps/pkgtree/pkgtree.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index f14598c286..4fc4e710a2 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -157,13 +157,11 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) { // see any. var lim []string for _, imp := range append(pkg.Imports, pkg.TestImports...) { - switch { - // Do allow the single-dot, at least for now - case imp == "..": - lim = append(lim, imp) - case strings.HasPrefix(imp, "./"): - lim = append(lim, imp) - case strings.HasPrefix(imp, "../"): + if build.IsLocalImport(imp) { + // Do allow the single-dot, at least for now + if imp == "." { + continue + } lim = append(lim, imp) } } From 19c7f60dae622d499ee3945398ecbb080f146c39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vytautas=20=C5=A0altenis?= Date: Mon, 14 Aug 2017 21:43:52 +0300 Subject: [PATCH 3/8] Implement import comment extraction Look for import comments following the package declaration. The implementation of findImportComment() very closely follows go/build's findImportComment(). Tests and validation against importRoot to come in a later commit. --- internal/gps/pkgtree/pkgtree.go | 76 +++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 3 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 4fc4e710a2..5405b905b3 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -5,7 +5,9 @@ package pkgtree import ( + "bytes" "fmt" + "go/ast" "go/build" "go/parser" gscan "go/scanner" @@ -123,9 +125,9 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) { ip := filepath.ToSlash(filepath.Join(importRoot, strings.TrimPrefix(wp, fileRoot))) // Find all the imports, across all os/arch combos - //p, err := fullPackageInDir(wp) p := &build.Package{ - Dir: wp, + Dir: wp, + ImportPath: ip, } err = fillPackage(p) @@ -208,6 +210,7 @@ func fillPackage(p *build.Package) error { var testImports []string var imports []string + var importComments []string for _, file := range gofiles { // Skip underscore-led or dot-led files, in keeping with the rest of the toolchain. bPrefix := filepath.Base(file)[0] @@ -232,6 +235,10 @@ func fillPackage(p *build.Package) error { var ignored bool for _, c := range pf.Comments { + ic := findImportComment(pf.Name, c) + if ic != "" { + importComments = append(importComments, ic) + } if c.Pos() > pf.Package { // +build comment must come before package continue } @@ -280,7 +287,16 @@ func fillPackage(p *build.Package) error { } } } - + importComments = uniq(importComments) + if len(importComments) > 1 { + return &ConflictingImportComments{ + ImportPath: p.ImportPath, + ImportComments: importComments, + } + } + if len(importComments) > 0 { + p.ImportComment = importComments[0] + } imports = uniq(imports) testImports = uniq(testImports) p.Imports = imports @@ -288,6 +304,60 @@ func fillPackage(p *build.Package) error { return nil } +var ( + slashSlash = []byte("//") + slashStar = []byte("/*") + starSlash = []byte("*/") + newline = []byte("\n") + importKwd = []byte("import ") +) + +func findImportComment(pkgName *ast.Ident, c *ast.CommentGroup) string { + afterPkg := pkgName.NamePos + token.Pos(len(pkgName.Name)) + 1 + commentSlash := c.List[0].Slash + if afterPkg != commentSlash { + return "" + } + text := []byte(c.List[0].Text) + switch { + case bytes.HasPrefix(text, slashSlash): + eol := bytes.IndexByte(text, '\n') + if eol < 0 { + eol = len(text) + } + text = text[2:eol] + case bytes.HasPrefix(text, slashStar): + text = text[2:] + end := bytes.Index(text, starSlash) + if end < 0 { + // malformed comment + return "" + } + text = text[:end] + if bytes.IndexByte(text, '\n') > 0 { + // multiline comment, can't be an import comment + return "" + } + } + text = bytes.TrimSpace(text) + if !bytes.HasPrefix(text, importKwd) { + return "" + } + quotedPath := bytes.TrimSpace(text[len(importKwd):]) + return string(bytes.Trim(quotedPath, `"`)) +} + +// ConflictingImportComments conflict +type ConflictingImportComments struct { + ImportPath string + ImportComments []string +} + +func (e *ConflictingImportComments) Error() string { + return fmt.Sprintf("import path %s had conflicting import comments: %q", + e.ImportPath, strings.Join(e.ImportComments, "\", \"")) +} + // LocalImportsError indicates that a package contains at least one relative // import that will prevent it from compiling. // From 242e387235bf7a9e792adc970e0b61178c9ab456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vytautas=20=C5=A0altenis?= Date: Tue, 15 Aug 2017 22:14:14 +0300 Subject: [PATCH 4/8] Canonical path extraction: tests and error checking Added a check and an error type for importRoot to be a prefix of canonical import path, tests for the new code paths. Fixed one unrelated bug along the way: Sprintf("%q", strings.Join(strs, "\", \"")) doesn't work as intended, since the %q verb will escape the inner doublequotes. --- internal/gps/_testdata/src/canon_confl/a.go | 9 ++++ internal/gps/_testdata/src/canon_confl/b.go | 9 ++++ internal/gps/_testdata/src/canonical/main.go | 9 ++++ .../gps/_testdata/src/canonical/sub/sub.go | 5 ++ internal/gps/pkgtree/pkgtree.go | 36 +++++++++++++-- internal/gps/pkgtree/pkgtree_test.go | 46 +++++++++++++++++++ 6 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 internal/gps/_testdata/src/canon_confl/a.go create mode 100644 internal/gps/_testdata/src/canon_confl/b.go create mode 100644 internal/gps/_testdata/src/canonical/main.go create mode 100644 internal/gps/_testdata/src/canonical/sub/sub.go diff --git a/internal/gps/_testdata/src/canon_confl/a.go b/internal/gps/_testdata/src/canon_confl/a.go new file mode 100644 index 0000000000..9815345118 --- /dev/null +++ b/internal/gps/_testdata/src/canon_confl/a.go @@ -0,0 +1,9 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package canonical // import "vanity1" + +var ( + A = "A" +) diff --git a/internal/gps/_testdata/src/canon_confl/b.go b/internal/gps/_testdata/src/canon_confl/b.go new file mode 100644 index 0000000000..b8b706bbd4 --- /dev/null +++ b/internal/gps/_testdata/src/canon_confl/b.go @@ -0,0 +1,9 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package canonical // import "vanity2" + +var ( + B = "B" +) diff --git a/internal/gps/_testdata/src/canonical/main.go b/internal/gps/_testdata/src/canonical/main.go new file mode 100644 index 0000000000..4fe31ce3d5 --- /dev/null +++ b/internal/gps/_testdata/src/canonical/main.go @@ -0,0 +1,9 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package pkg // import "canonical" + +var ( + A = "A" +) diff --git a/internal/gps/_testdata/src/canonical/sub/sub.go b/internal/gps/_testdata/src/canonical/sub/sub.go new file mode 100644 index 0000000000..783186dfc2 --- /dev/null +++ b/internal/gps/_testdata/src/canonical/sub/sub.go @@ -0,0 +1,5 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package sub // import "canonical/subpackage" diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 5405b905b3..17322411e3 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -154,6 +154,13 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) { } } + if pkg.CommentPath != "" && !strings.HasPrefix(pkg.CommentPath, importRoot) { + return &NonCanonicalImportRoot{ + ImportRoot: importRoot, + Canonical: pkg.CommentPath, + } + } + // This area has some...fuzzy rules, but check all the imports for // local/relative/dot-ness, and record an error for the package if we // see any. @@ -347,15 +354,36 @@ func findImportComment(pkgName *ast.Ident, c *ast.CommentGroup) string { return string(bytes.Trim(quotedPath, `"`)) } -// ConflictingImportComments conflict +// ConflictingImportComments indicates that the package declares more than one +// different canonical paths. type ConflictingImportComments struct { ImportPath string ImportComments []string } func (e *ConflictingImportComments) Error() string { - return fmt.Sprintf("import path %s had conflicting import comments: %q", - e.ImportPath, strings.Join(e.ImportComments, "\", \"")) + return fmt.Sprintf("import path %s had conflicting import comments: %s", + e.ImportPath, quotedPaths(e.ImportComments)) +} + +// NonCanonicalImportRoot reports the situation when the dependee imports a +// package via something else that the package's declared canonical path. +type NonCanonicalImportRoot struct { + ImportRoot string + Canonical string +} + +func (e *NonCanonicalImportRoot) Error() string { + return fmt.Sprintf("importing via path %q, but package insists on a canonical path %q", + e.ImportRoot, e.Canonical) +} + +func quotedPaths(ps []string) string { + var quoted []string + for _, p := range ps { + quoted = append(quoted, fmt.Sprintf("%q", p)) + } + return strings.Join(quoted, ", ") } // LocalImportsError indicates that a package contains at least one relative @@ -376,7 +404,7 @@ func (e *LocalImportsError) Error() string { case 1: return fmt.Sprintf("import path %s had a local import: %q", e.ImportPath, e.LocalImports[0]) default: - return fmt.Sprintf("import path %s had local imports: %q", e.ImportPath, strings.Join(e.LocalImports, "\", \"")) + return fmt.Sprintf("import path %s had local imports: %s", e.ImportPath, quotedPaths(e.LocalImports)) } } diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index ab134f02e7..79a2a50f79 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -1292,6 +1292,52 @@ func TestListPackages(t *testing.T) { }, }, }, + "canonical": { + fileRoot: j("canonical"), + importRoot: "canonical", + out: PackageTree{ + ImportRoot: "canonical", + Packages: map[string]PackageOrErr{ + "canonical": { + P: Package{ + ImportPath: "canonical", + CommentPath: "canonical", + Name: "pkg", + Imports: []string{}, + }, + }, + "canonical/sub": { + P: Package{ + ImportPath: "canonical/sub", + CommentPath: "canonical/subpackage", + Name: "sub", + Imports: []string{}, + }, + }, + }, + }, + }, + "conflicting canonical comments": { + fileRoot: j("canon_confl"), + importRoot: "canon_confl", + out: PackageTree{}, + err: &ConflictingImportComments{ + ImportPath: "canon_confl", + ImportComments: []string{ + "vanity1", + "vanity2", + }, + }, + }, + "non-canonical": { + fileRoot: j("canonical"), + importRoot: "noncanonical", + out: PackageTree{}, + err: &NonCanonicalImportRoot{ + ImportRoot: "noncanonical", + Canonical: "canonical", + }, + }, } for name, fix := range table { From 90d2a5e92d8ae606b13f7594fe5d8b314a9b4e79 Mon Sep 17 00:00:00 2001 From: Vytautas Saltenis Date: Sat, 9 Sep 2017 20:56:17 +0300 Subject: [PATCH 5/8] Record NonCanonicalImportRoot error instead of immediate return --- internal/gps/pkgtree/pkgtree.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 17322411e3..4440e0f7a4 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -155,10 +155,13 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) { } if pkg.CommentPath != "" && !strings.HasPrefix(pkg.CommentPath, importRoot) { - return &NonCanonicalImportRoot{ - ImportRoot: importRoot, - Canonical: pkg.CommentPath, + ptree.Packages[ip] = PackageOrErr{ + Err: &NonCanonicalImportRoot{ + ImportRoot: importRoot, + Canonical: pkg.CommentPath, + }, } + return nil } // This area has some...fuzzy rules, but check all the imports for @@ -355,7 +358,7 @@ func findImportComment(pkgName *ast.Ident, c *ast.CommentGroup) string { } // ConflictingImportComments indicates that the package declares more than one -// different canonical paths. +// different canonical path. type ConflictingImportComments struct { ImportPath string ImportComments []string @@ -367,7 +370,7 @@ func (e *ConflictingImportComments) Error() string { } // NonCanonicalImportRoot reports the situation when the dependee imports a -// package via something else that the package's declared canonical path. +// package via something other than the package's declared canonical path. type NonCanonicalImportRoot struct { ImportRoot string Canonical string From 1853dc3bb14d9bca10db215ade180fdb6897ec37 Mon Sep 17 00:00:00 2001 From: Vytautas Saltenis Date: Sun, 10 Sep 2017 11:51:37 +0300 Subject: [PATCH 6/8] Fix NonCanonicalImportRoot test --- internal/gps/pkgtree/pkgtree_test.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index 79a2a50f79..03c62e438b 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -1332,10 +1332,22 @@ func TestListPackages(t *testing.T) { "non-canonical": { fileRoot: j("canonical"), importRoot: "noncanonical", - out: PackageTree{}, - err: &NonCanonicalImportRoot{ + out: PackageTree{ ImportRoot: "noncanonical", - Canonical: "canonical", + Packages: map[string]PackageOrErr{ + "noncanonical": PackageOrErr{ + Err: &NonCanonicalImportRoot{ + ImportRoot: "noncanonical", + Canonical: "canonical", + }, + }, + "noncanonical/sub": PackageOrErr{ + Err: &NonCanonicalImportRoot{ + ImportRoot: "noncanonical", + Canonical: "canonical/subpackage", + }, + }, + }, }, }, } From 47e42c7305a828028cb5612fee71e8408469b1b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vytautas=20=C5=A0altenis?= Date: Fri, 6 Oct 2017 20:19:08 +0300 Subject: [PATCH 7/8] Address feedback: naming, wording and comments --- internal/gps/pkgtree/pkgtree.go | 18 +++++++++--------- internal/gps/pkgtree/pkgtree_test.go | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 4440e0f7a4..303b3f15b9 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -300,8 +300,8 @@ func fillPackage(p *build.Package) error { importComments = uniq(importComments) if len(importComments) > 1 { return &ConflictingImportComments{ - ImportPath: p.ImportPath, - ImportComments: importComments, + ImportPath: p.ImportPath, + ConflictingImportComments: importComments, } } if len(importComments) > 0 { @@ -360,29 +360,29 @@ func findImportComment(pkgName *ast.Ident, c *ast.CommentGroup) string { // ConflictingImportComments indicates that the package declares more than one // different canonical path. type ConflictingImportComments struct { - ImportPath string - ImportComments []string + ImportPath string // An import path refering to this package + ConflictingImportComments []string // All distinct "canonical" paths encountered in the package files } func (e *ConflictingImportComments) Error() string { return fmt.Sprintf("import path %s had conflicting import comments: %s", - e.ImportPath, quotedPaths(e.ImportComments)) + e.ImportPath, quotedPaths(e.ConflictingImportComments)) } // NonCanonicalImportRoot reports the situation when the dependee imports a // package via something other than the package's declared canonical path. type NonCanonicalImportRoot struct { - ImportRoot string - Canonical string + ImportRoot string // A root path that is being used to import a package + Canonical string // A canonical path declared by the package being imported } func (e *NonCanonicalImportRoot) Error() string { - return fmt.Sprintf("importing via path %q, but package insists on a canonical path %q", + return fmt.Sprintf("import root %q is not a prefix for the package's declared canonical path %q", e.ImportRoot, e.Canonical) } func quotedPaths(ps []string) string { - var quoted []string + quoted := make([]string, 0, len(ps)) for _, p := range ps { quoted = append(quoted, fmt.Sprintf("%q", p)) } diff --git a/internal/gps/pkgtree/pkgtree_test.go b/internal/gps/pkgtree/pkgtree_test.go index 03c62e438b..4e3f27268a 100644 --- a/internal/gps/pkgtree/pkgtree_test.go +++ b/internal/gps/pkgtree/pkgtree_test.go @@ -1323,7 +1323,7 @@ func TestListPackages(t *testing.T) { out: PackageTree{}, err: &ConflictingImportComments{ ImportPath: "canon_confl", - ImportComments: []string{ + ConflictingImportComments: []string{ "vanity1", "vanity2", }, From 611e64ba451e9ca7232bbf902f5062b7329dca4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vytautas=20=C5=A0altenis?= Date: Sat, 7 Oct 2017 09:40:01 +0300 Subject: [PATCH 8/8] Fix lint --- internal/gps/pkgtree/pkgtree.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/gps/pkgtree/pkgtree.go b/internal/gps/pkgtree/pkgtree.go index 303b3f15b9..4b15ee422b 100644 --- a/internal/gps/pkgtree/pkgtree.go +++ b/internal/gps/pkgtree/pkgtree.go @@ -318,7 +318,6 @@ var ( slashSlash = []byte("//") slashStar = []byte("/*") starSlash = []byte("*/") - newline = []byte("\n") importKwd = []byte("import ") )