From ed6e249a6c47a859c9b535f3069abb81493a2ba6 Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Sat, 24 Aug 2024 12:50:41 +0100 Subject: [PATCH] fix: AddNewRequire block ordering Try to maintain the expected block ordering with direct requires in the first block and indirect requires in the second block. Also clarify expected use of SetRequireSeparateIndirect to avoid panic. Fixes #69050 --- modfile/read.go | 105 +++++++++++++++++ modfile/rule.go | 18 ++- modfile/rule_test.go | 265 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 377 insertions(+), 11 deletions(-) diff --git a/modfile/read.go b/modfile/read.go index de1b982..e117e0f 100644 --- a/modfile/read.go +++ b/modfile/read.go @@ -15,6 +15,9 @@ import ( "unicode/utf8" ) +// requireToken is the token used for require statements. +const requireToken = "require" + // A Position describes an arbitrary source position in a file, including the // file, line, column, and byte offset. type Position struct { @@ -187,6 +190,108 @@ func (x *FileSyntax) addLine(hint Expr, tokens ...string) *Line { return new } +// requireHint returns a hint for adding a new require line. +// It tries to maintain the standard order of require blocks, with +// the first block being the direct requires and the second block +// being the indirect requires. +func (x *FileSyntax) requireHint(indirect bool) Expr { + if indirect { + // Indirect block requested return the first require + // block with indirect dependencies. + for _, stmt := range x.Stmt { + switch stmt := stmt.(type) { + case *Line: + if stmt.Token != nil && stmt.Token[0] == requireToken && isIndirect(stmt) { + return stmt + } + case *LineBlock: + if stmt.Token[0] == requireToken { + for _, line := range stmt.Line { + if isIndirect(line) { + return line + } + } + } + } + } + + // No indirect require found, append a new block and return + // is as the hint. This prevents adding the indirect to an + // existing direct block. + block := &LineBlock{ + Token: []string{requireToken}, + } + x.Stmt = append(x.Stmt, block) + + return block + } + + // Direct block requested. + var last Expr + for _, stmt := range x.Stmt { + switch stmt := stmt.(type) { + case *Line: + if stmt.Token != nil && stmt.Token[0] == requireToken { + if !isIndirect(stmt) { + // Direct require line found, return it as hint to + // combine with it. + return stmt + } + + // Indirect line first, which is unexpected. We return + // the last stmt as hint to create a new require block + // before it, to try to maintain the standard order. + if last != nil { + return last + } + + // Indirect line at the beginning of the file, prepend + // a new require block and return it as hint, to try to + // maintain the standard order. + block := &LineBlock{ + Token: []string{requireToken}, + } + x.Stmt = append([]Expr{block}, x.Stmt...) + + return block + } + case *LineBlock: + if stmt.Token[0] == requireToken { + for _, line := range stmt.Line { + if !isIndirect(line) { + // Direct require block found, return it as hint to + // combine with it. + return line + } + } + + // Indirect block before first, which is unexpected. + // Return the last stmt as hint to create a new require + // block before it to try to maintain the standard order. + if last != nil { + return last + } + + // Indirect block at the beginning of the file, which is + // unexpected. Prepend a new require block and return it + // as hint to try to maintain the standard order. + block := &LineBlock{ + Token: []string{requireToken}, + } + x.Stmt = append([]Expr{block}, x.Stmt...) + + return block + } + } + + last = stmt + continue + } + + // No requires found, addLine will create one. + return nil +} + func (x *FileSyntax) updateLine(line *Line, tokens ...string) { if line.InBlock { tokens = tokens[1:] diff --git a/modfile/rule.go b/modfile/rule.go index 3e4a1d0..6a0a8ee 100644 --- a/modfile/rule.go +++ b/modfile/rule.go @@ -1144,7 +1144,7 @@ func (f *File) addNewGodebug(key, value string) { // other lines for path. // // If no line currently exists for path, AddRequire adds a new line -// at the end of the last require block. +// using AddNewRequire. func (f *File) AddRequire(path, vers string) error { need := true for _, r := range f.Require { @@ -1166,10 +1166,14 @@ func (f *File) AddRequire(path, vers string) error { return nil } -// AddNewRequire adds a new require line for path at version vers at the end of -// the last require block, regardless of any existing require lines for path. +// AddNewRequire adds a new require line for path at version vers, regardless +// of any existing require lines for path. +// Similar to SetRequireSeparateIndirect it attempts to maintain the standard +// of having direct requires in the first block and indirect requires in the +// second block. func (f *File) AddNewRequire(path, vers string, indirect bool) { - line := f.Syntax.addLine(nil, "require", AutoQuote(path), vers) + hint := f.Syntax.requireHint(indirect) + line := f.Syntax.addLine(hint, "require", AutoQuote(path), vers) r := &Require{ Mod: module.Version{Path: path, Version: vers}, Syntax: line, @@ -1248,6 +1252,12 @@ func (f *File) SetRequire(req []*Require) { // If the file initially has one uncommented block of requirements, // SetRequireSeparateIndirect will split it into a direct-only and indirect-only // block. This aids in the transition to separate blocks. +// +// New entries in req must not be added to the file.Require slice before calling +// otherwise this function will panic. So the calling convention should be along +// the lines of: +// +// File.SetRequireSeparateIndirect(append(File.Require, newReqs...)) func (f *File) SetRequireSeparateIndirect(req []*Require) { // hasComments returns whether a line or block has comments // other than "indirect". diff --git a/modfile/rule_test.go b/modfile/rule_test.go index c75a77a..13653e9 100644 --- a/modfile/rule_test.go +++ b/modfile/rule_test.go @@ -13,6 +13,240 @@ import ( "golang.org/x/mod/module" ) +var addNewRequireTests = []struct { + desc string + in string + mods []require + out string +}{ + { + `golden`, + `module m + require ( + x.y/a v1.2.3 + x.y/b v1.2.3 + x.y/c v1.2.3 + ) + + require ( + x.y/d v1.2.3 // indirect + x.y/e v1.2.3 // indirect + x.y/f v1.2.3 // indirect + ) + `, + []require{ + {"x.y/g", "v1.2.3", true}, + {"x.y/h", "v1.2.3", false}, + {"x.y/i", "v1.2.3", true}, + {"x.y/k", "v1.2.3", false}, + }, + `module m + require ( + x.y/a v1.2.3 + x.y/b v1.2.3 + x.y/c v1.2.3 + x.y/h v1.2.3 + x.y/k v1.2.3 + ) + + require ( + x.y/d v1.2.3 // indirect + x.y/e v1.2.3 // indirect + x.y/f v1.2.3 // indirect + x.y/g v1.2.3 // indirect + x.y/i v1.2.3 // indirect + ) + `, + }, + { + `empty`, + `module m + `, + []require{ + {"x.y/g", "v1.2.3", true}, + {"x.y/h", "v1.2.3", false}, + {"x.y/i", "v1.2.3", true}, + {"x.y/k", "v1.2.3", false}, + }, + `module m + require ( + x.y/h v1.2.3 + x.y/k v1.2.3 + ) + + require ( + x.y/g v1.2.3 // indirect + x.y/i v1.2.3 // indirect + ) + `, + }, + { + `empty_direct`, + `module m + `, + []require{ + {"x.y/h", "v1.2.3", false}, + {"x.y/k", "v1.2.3", false}, + }, + `module m + require ( + x.y/h v1.2.3 + x.y/k v1.2.3 + ) + `, + }, + { + `direct_line`, + `module m + require x.y/a v1.2.3 + `, + []require{ + {"x.y/g", "v1.2.3", true}, + {"x.y/h", "v1.2.3", false}, + {"x.y/i", "v1.2.3", true}, + {"x.y/k", "v1.2.3", false}, + }, + `module m + require ( + x.y/a v1.2.3 + x.y/h v1.2.3 + x.y/k v1.2.3 + ) + + require ( + x.y/g v1.2.3 // indirect + x.y/i v1.2.3 // indirect + ) + `, + }, + { + `direct_block`, + `module m + require ( + x.y/a v1.2.3 + x.y/b v1.2.3 + ) + `, + []require{ + {"x.y/g", "v1.2.3", true}, + {"x.y/h", "v1.2.3", false}, + {"x.y/i", "v1.2.3", true}, + {"x.y/k", "v1.2.3", false}, + }, + `module m + require ( + x.y/a v1.2.3 + x.y/b v1.2.3 + x.y/h v1.2.3 + x.y/k v1.2.3 + ) + + require ( + x.y/g v1.2.3 // indirect + x.y/i v1.2.3 // indirect + ) + `, + }, + { + `indirect_line`, + `module m + require x.y/d v1.2.3 // indirect + `, + []require{ + {"x.y/g", "v1.2.3", true}, + {"x.y/h", "v1.2.3", false}, + {"x.y/i", "v1.2.3", true}, + {"x.y/k", "v1.2.3", false}, + }, + `module m + require ( + x.y/h v1.2.3 + x.y/k v1.2.3 + ) + + require ( + x.y/d v1.2.3 // indirect + x.y/g v1.2.3 // indirect + x.y/i v1.2.3 // indirect + ) + `, + }, + { + `indirect_block`, + `module m + require ( + x.y/d v1.2.3 // indirect + x.y/e v1.2.3 // indirect + ) + `, + []require{ + {"x.y/g", "v1.2.3", true}, + {"x.y/h", "v1.2.3", false}, + {"x.y/i", "v1.2.3", true}, + {"x.y/k", "v1.2.3", false}, + }, + `module m + require ( + x.y/h v1.2.3 + x.y/k v1.2.3 + ) + + require ( + x.y/d v1.2.3 // indirect + x.y/e v1.2.3 // indirect + x.y/g v1.2.3 // indirect + x.y/i v1.2.3 // indirect + ) + `, + }, + { + `indirect_block_no_module`, + `require ( + x.y/d v1.2.3 // indirect + x.y/e v1.2.3 // indirect + ) + `, + []require{ + {"x.y/g", "v1.2.3", true}, + {"x.y/h", "v1.2.3", false}, + {"x.y/i", "v1.2.3", true}, + {"x.y/k", "v1.2.3", false}, + }, + `require ( + x.y/h v1.2.3 + x.y/k v1.2.3 + ) + + require ( + x.y/d v1.2.3 // indirect + x.y/e v1.2.3 // indirect + x.y/g v1.2.3 // indirect + x.y/i v1.2.3 // indirect + ) + `, + }, + { + `no_module`, + ``, + []require{ + {"x.y/g", "v1.2.3", true}, + {"x.y/h", "v1.2.3", false}, + {"x.y/i", "v1.2.3", true}, + {"x.y/k", "v1.2.3", false}, + }, + `require ( + x.y/h v1.2.3 + x.y/k v1.2.3 + ) + + require ( + x.y/g v1.2.3 // indirect + x.y/i v1.2.3 // indirect + ) + `, + }, +} + var addRequireTests = []struct { desc string in string @@ -83,11 +317,13 @@ var addRequireTests = []struct { "x.y/w", "v1.5.6", ` module m - require x.y/z v1.2.3 + require ( - x.y/q/v2 v2.3.4 + x.y/z v1.2.3 x.y/w v1.5.6 ) + + require x.y/q/v2 v2.3.4 `, }, { @@ -222,7 +458,10 @@ var setRequireTests = []struct { {"x.y/g", "v1.2.3", false}, }, `module m - require x.y/a v1.2.3 + require ( + x.y/a v1.2.3 + x.y/h v1.2.3 + ) require x.y/b v1.2.3 @@ -234,10 +473,7 @@ var setRequireTests = []struct { require x.y/f v1.2.3 - require ( - x.y/g v1.2.3 - x.y/h v1.2.3 - ) + require x.y/g v1.2.3 `, }, { @@ -1799,6 +2035,21 @@ func TestAddRequire(t *testing.T) { } } +func TestAddNewRequire(t *testing.T) { + for _, tt := range addNewRequireTests { + t.Run(tt.desc, func(t *testing.T) { + testEdit(t, tt.in, tt.out, true, func(f *File) error { + for _, mod := range tt.mods { + f.AddNewRequire(mod.path, mod.vers, mod.indirect) + f.Cleanup() + f.SortBlocks() + } + return nil + }) + }) + } +} + func TestAddGodebug(t *testing.T) { for _, tt := range addGodebugTests { t.Run(tt.desc, func(t *testing.T) {