Skip to content

Commit

Permalink
modfile: clean up SetRequire
Browse files Browse the repository at this point in the history 
I started this change by expanding the documentation and tests for
SetRequire. Unfortunately, the tests failed when the existing
contents included duplicates of a module path:

    --- FAIL: TestSetRequire/existing_duplicate (0.00s)
        rule_test.go:1011: after Cleanup, len(Require) = 3; want 1
    --- FAIL: TestSetRequire/existing_duplicate_multi (0.00s)
        rule_test.go:1011: after Cleanup, len(Require) = 3; want 1

So then I fixed the detected bug, by updating the Line entries
(possibly marking them for removal) in the same loop that updates the
Require entries. (We don't need to loop over f.Syntax.Stmt separately
to remove deleted entries because f.Syntax.Cleanup already does that.)

For golang/go#45965

Change-Id: I1b665c0832112de2c4273628f266dc3d966fefdd
Reviewed-on: https://go-review.googlesource.com/c/mod/+/325230
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
  • Loading branch information
Bryan C. Mills committed Jun 8, 2021
1 parent ad21a28 commit 6e4e729
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 61 deletions.
87 changes: 48 additions & 39 deletions modfile/rule.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -870,67 +870,76 @@ func (f *File) AddNewRequire(path, vers string, indirect bool) {
f.Require = append(f.Require, &Require{module.Version{Path: path, Version: vers}, indirect, line})
}

// SetRequire sets the requirements of f to contain exactly req, preserving
// any existing line comments contents (except for 'indirect' markings)
// for the module versions named in req.
// SetRequire updates the requirements of f to contain exactly req, preserving
// the existing block structure and line comment contents (except for 'indirect'
// markings) for the first requirement on each named module path.
//
// The Syntax field is ignored for the requirements in req.
//
// Any requirements not already present in the file are added to the block
// containing the last require line.
//
// The requirements in req must specify at most one distinct version for each
// module path.
//
// If any existing requirements may be removed, the caller should call Cleanup
// after all edits are complete.
func (f *File) SetRequire(req []*Require) {
need := make(map[string]string)
indirect := make(map[string]bool)
for _, r := range req {
if prev, dup := need[r.Mod.Path]; dup && prev != r.Mod.Version {
panic(fmt.Errorf("SetRequire called with conflicting versions for path %s (%s and %s)", r.Mod.Path, prev, r.Mod.Version))
}
need[r.Mod.Path] = r.Mod.Version
indirect[r.Mod.Path] = r.Indirect
}

// Update or delete the existing Require entries to preserve
// only the first for each module path in req.
for _, r := range f.Require {
if v, ok := need[r.Mod.Path]; ok {
r.Mod.Version = v
r.Indirect = indirect[r.Mod.Path]
} else {
v, ok := need[r.Mod.Path]
if !ok {
// This line is redundant or its path is no longer required at all.
// Mark the requirement for deletion in Cleanup.
r.Syntax.Token = nil
*r = Require{}
}
}

var newStmts []Expr
for _, stmt := range f.Syntax.Stmt {
switch stmt := stmt.(type) {
case *LineBlock:
if len(stmt.Token) > 0 && stmt.Token[0] == "require" {
var newLines []*Line
for _, line := range stmt.Line {
if p, err := parseString(&line.Token[0]); err == nil && need[p] != "" {
if len(line.Comments.Before) == 1 && len(line.Comments.Before[0].Token) == 0 {
line.Comments.Before = line.Comments.Before[:0]
}
line.Token[1] = need[p]
delete(need, p)
setIndirect(line, indirect[p])
newLines = append(newLines, line)
}
r.Mod.Version = v
r.Indirect = indirect[r.Mod.Path]

if line := r.Syntax; line != nil && len(line.Token) > 0 {
if line.InBlock {
// If the line is preceded by an empty line, remove it; see
// https://golang.org/issue/33779.
if len(line.Comments.Before) == 1 && len(line.Comments.Before[0].Token) == 0 {
line.Comments.Before = line.Comments.Before[:0]
}
if len(newLines) == 0 {
continue // drop stmt
if len(line.Token) >= 2 { // example.com v1.2.3
line.Token[1] = v
}
stmt.Line = newLines
}

case *Line:
if len(stmt.Token) > 0 && stmt.Token[0] == "require" {
if p, err := parseString(&stmt.Token[1]); err == nil && need[p] != "" {
stmt.Token[2] = need[p]
delete(need, p)
setIndirect(stmt, indirect[p])
} else {
continue // drop stmt
} else {
if len(line.Token) >= 3 { // require example.com v1.2.3
line.Token[2] = v
}
}

setIndirect(line, r.Indirect)
}
newStmts = append(newStmts, stmt)

delete(need, r.Mod.Path)
}
f.Syntax.Stmt = newStmts

// Add new entries in the last block of the file for any paths that weren't
// already present.
//
// This step is nondeterministic, but the final result will be deterministic
// because we will sort the block.
for path, vers := range need {
f.AddNewRequire(path, vers, indirect[path])
}

f.SortBlocks()
}

Expand Down
103 changes: 81 additions & 22 deletions modfile/rule_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,16 @@ var addRequireTests = []struct {
},
}

type require struct {
path, vers string
indirect bool
}

var setRequireTests = []struct {
desc string
in string
mods []struct {
path string
vers string
indirect bool
}
out string
mods []require
out string
}{
{
`https://golang.org/issue/45932`,
Expand All @@ -111,11 +112,7 @@ var setRequireTests = []struct {
x.y/c v1.2.3
)
`,
[]struct {
path string
vers string
indirect bool
}{
[]require{
{"x.y/a", "v1.2.3", false},
{"x.y/b", "v1.2.3", false},
{"x.y/c", "v1.2.3", false},
Expand All @@ -138,11 +135,7 @@ var setRequireTests = []struct {
x.y/d v1.2.3
)
`,
[]struct {
path string
vers string
indirect bool
}{
[]require{
{"x.y/a", "v1.2.3", false},
{"x.y/b", "v1.2.3", false},
{"x.y/c", "v1.2.3", false},
Expand All @@ -168,11 +161,7 @@ var setRequireTests = []struct {
x.y/g v1.2.3 // indirect
)
`,
[]struct {
path string
vers string
indirect bool
}{
[]require{
{"x.y/a", "v1.2.3", true},
{"x.y/b", "v1.2.3", true},
{"x.y/c", "v1.2.3", true},
Expand All @@ -193,6 +182,76 @@ var setRequireTests = []struct {
)
`,
},
{
`existing_multi`,
`module m
require x.y/a v1.2.3
require x.y/b v1.2.3
require x.y/c v1.0.0 // not v1.2.3!
require x.y/d v1.2.3 // comment kept
require x.y/e v1.2.3 // comment kept
require x.y/f v1.2.3 // indirect
require x.y/g v1.2.3 // indirect
`,
[]require{
{"x.y/h", "v1.2.3", false},
{"x.y/a", "v1.2.3", false},
{"x.y/b", "v1.2.3", false},
{"x.y/c", "v1.2.3", false},
{"x.y/d", "v1.2.3", false},
{"x.y/e", "v1.2.3", true},
{"x.y/f", "v1.2.3", false},
{"x.y/g", "v1.2.3", false},
},
`module m
require x.y/a v1.2.3
require x.y/b v1.2.3
require x.y/c v1.2.3 // not v1.2.3!
require x.y/d v1.2.3 // comment kept
require x.y/e v1.2.3 // indirect; comment kept
require x.y/f v1.2.3
require (
x.y/g v1.2.3
x.y/h v1.2.3
)
`,
},
{
`existing_duplicate`,
`module m
require (
x.y/a v1.0.0 // zero
x.y/a v1.1.0 // one
x.y/a v1.2.3 // two
)
`,
[]require{
{"x.y/a", "v1.2.3", true},
},
`module m
require x.y/a v1.2.3 // indirect; zero
`,
},
{
`existing_duplicate_multi`,
`module m
require x.y/a v1.0.0 // zero
require x.y/a v1.1.0 // one
require x.y/a v1.2.3 // two
`,
[]require{
{"x.y/a", "v1.2.3", true},
},
`module m
require x.y/a v1.2.3 // indirect; zero
`,
},
}

var addGoTests = []struct {
Expand Down Expand Up @@ -942,10 +1001,10 @@ func TestSetRequire(t *testing.T) {

f := testEdit(t, tt.in, tt.out, true, func(f *File) error {
f.SetRequire(mods)
f.Cleanup()
return nil
})

f.Cleanup()
if len(f.Require) != len(mods) {
t.Errorf("after Cleanup, len(Require) = %v; want %v", len(f.Require), len(mods))
}
Expand Down

0 comments on commit 6e4e729

Please sign in to comment.