From 6e4e7296867b5c082b3a1b4c3aa03051b7d11953 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Fri, 4 Jun 2021 12:10:41 -0400 Subject: [PATCH] modfile: clean up SetRequire 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 Run-TryBot: Bryan C. Mills TryBot-Result: Go Bot Reviewed-by: Michael Matloob --- modfile/rule.go | 87 ++++++++++++++++++++---------------- modfile/rule_test.go | 103 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 129 insertions(+), 61 deletions(-) diff --git a/modfile/rule.go b/modfile/rule.go index 5be5715..71cda84 100644 --- a/modfile/rule.go +++ b/modfile/rule.go @@ -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() } diff --git a/modfile/rule_test.go b/modfile/rule_test.go index 2ec24ea..7c6f0e8 100644 --- a/modfile/rule_test.go +++ b/modfile/rule_test.go @@ -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`, @@ -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}, @@ -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}, @@ -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}, @@ -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 { @@ -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)) }