Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Respect canonical import comments #1017

Merged
merged 8 commits into from
Oct 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions internal/gps/_testdata/src/canon_confl/a.go
Original file line number Diff line number Diff line change
@@ -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"
)
9 changes: 9 additions & 0 deletions internal/gps/_testdata/src/canon_confl/b.go
Original file line number Diff line number Diff line change
@@ -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"
)
9 changes: 9 additions & 0 deletions internal/gps/_testdata/src/canonical/main.go
Original file line number Diff line number Diff line change
@@ -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"
)
5 changes: 5 additions & 0 deletions internal/gps/_testdata/src/canonical/sub/sub.go
Original file line number Diff line number Diff line change
@@ -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"
4 changes: 2 additions & 2 deletions internal/gps/identifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down
120 changes: 109 additions & 11 deletions internal/gps/pkgtree/pkgtree.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
package pkgtree

import (
"bytes"
"fmt"
"go/ast"
"go/build"
"go/parser"
gscan "go/scanner"
Expand Down Expand Up @@ -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)

Expand All @@ -152,18 +154,26 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) {
}
}

if pkg.CommentPath != "" && !strings.HasPrefix(pkg.CommentPath, importRoot) {
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
// local/relative/dot-ness, and record an error for the package if we
// 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)
}
}
Expand Down Expand Up @@ -210,6 +220,7 @@ func fillPackage(p *build.Package) error {

var testImports []string
var imports []string
var importComments []string
Copy link
Member

Choose a reason for hiding this comment

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

ok so i was originally going to suggest that we just keep a single string, rather than a []string, and upon encountering later values, just compare against that one string and error out immediately if we find an error. i was thinking that it might escape, but i ran it through with the optimization profiler (-gcflags '-m -m'), and turns out, it doesn't.

so, this is fine. just thought you might be interested that i checked that through 😄

for _, file := range gofiles {
// Skip underscore-led or dot-led files, in keeping with the rest of the toolchain.
bPrefix := filepath.Base(file)[0]
Expand All @@ -234,6 +245,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
}
Expand Down Expand Up @@ -282,14 +297,97 @@ func fillPackage(p *build.Package) error {
}
}
}

importComments = uniq(importComments)
if len(importComments) > 1 {
return &ConflictingImportComments{
ImportPath: p.ImportPath,
ConflictingImportComments: importComments,
}
}
if len(importComments) > 0 {
p.ImportComment = importComments[0]
}
imports = uniq(imports)
testImports = uniq(testImports)
p.Imports = imports
p.TestImports = testImports
return nil
}

var (
slashSlash = []byte("//")
slashStar = []byte("/*")
starSlash = []byte("*/")
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 indicates that the package declares more than one
// different canonical path.
type ConflictingImportComments struct {
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.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 // 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("import root %q is not a prefix for the package's declared canonical path %q",
e.ImportRoot, e.Canonical)
}

func quotedPaths(ps []string) string {
quoted := make([]string, 0, len(ps))
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
// import that will prevent it from compiling.
//
Expand All @@ -308,7 +406,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))
}
}

Expand Down
58 changes: 58 additions & 0 deletions internal/gps/pkgtree/pkgtree_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1292,6 +1292,64 @@ 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",
ConflictingImportComments: []string{
"vanity1",
"vanity2",
},
},
},
"non-canonical": {
fileRoot: j("canonical"),
importRoot: "noncanonical",
out: PackageTree{
ImportRoot: "noncanonical",
Packages: map[string]PackageOrErr{
"noncanonical": PackageOrErr{
Err: &NonCanonicalImportRoot{
ImportRoot: "noncanonical",
Canonical: "canonical",
},
},
"noncanonical/sub": PackageOrErr{
Err: &NonCanonicalImportRoot{
ImportRoot: "noncanonical",
Canonical: "canonical/subpackage",
},
},
},
},
},
}

for name, fix := range table {
Expand Down