diff --git a/CHANGELOG.md b/CHANGELOG.md index cf293ac..2d2aeca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## Unreleased -- No changes yet. +### Fixed +- ([#2]): Patches with named imports now support matching and manipulating any + import. + + [#2]: https://github.com/uber-go/gopatch/issues/2 ## 0.0.2 - 2020-11-04 ### Fixed diff --git a/docs/PatchesInDepth.md b/docs/PatchesInDepth.md index ddbe066..c0cacd4 100644 --- a/docs/PatchesInDepth.md +++ b/docs/PatchesInDepth.md @@ -413,13 +413,6 @@ can now update consumers of `foo.FooClient`. [patch above to avoid stuttering]: #avoid-stutter-patch ```diff -@@ -@@ - import "example.com/foo" - --foo.FooClient -+foo.Client - @@ var foo identifier @@ @@ -429,14 +422,6 @@ var foo identifier +foo.Client ``` -The first diff in this patch affects files that use unnamed imports, and the -second affects those that use named imports---regardless of name. - -> *Note*: In a future version of gopatch, we'll need only the second patch to -> make this transformation. See also, [#2]. - - [#2]: https://github.com/uber-go/gopatch/issues/2 - #### Changing imports In addition to matching on imports, you can also change imports with gopatch. @@ -519,42 +504,16 @@ The above will match, both, named and unnamed imports of |---------------------------------------|-------------------------------------| | `import foo "example.com/foo-go.git"` | `import foo "example.com/foo.git"` | | `import bar "example.com/foo-go.git"` | `import bar "example.com/foo.git"` | -| `import "example.com/foo-go.git"` | `import foo "example.com/foo.git"`* | - -> *This case is a known bug. See [#2] for more information. -> -> You can work around this by first explicitly matching and replacing the -> cases with unnamed imports first. For example, turn the patch above into two -> diffs, one addressing the unnamed imports, and one addressing the named. -> -> ```diff -> @@ -> var x identifier -> @@ -> -import "example.com/foo-go.git" -> +import "example.com/foo.git" -> -> foo.x -> -> @@ -> var foo, x identifier -> @@ -> -import foo "example.com/foo-go.git" -> +import foo "example.com/foo.git" -> -> foo.x -> ``` +| `import "example.com/foo-go.git"` | `import "example.com/foo.git"` | #### Best practices for imports -Given the known limitations and issues with imports highlighted above, the -best practices for matching and manipulating imports are: +Given the limitations of AST analysis, gopatch cannot always accurately guess a +package name for an import. -- Handle unnamed imports first. This will make sure that previously named - imports do not unintentionally become named. -- When matching any import, use a metavariable name that matches the name of - the imported package **exactly**. This name is used by gopatch to guess the - name of the package. +As a best practice, when manipulating imports, use a metavariable name that +matches the name of the imported package **exactly**. gopatch will use that as +the name of the package during source analysis. ``` # BAD | # GOOD diff --git a/e2e_test.go b/e2e_test.go index b030dca..4d8d581 100644 --- a/e2e_test.go +++ b/e2e_test.go @@ -142,8 +142,6 @@ var testsToSkip = map[string]struct{}{ "add_error_param/a": {}, // https://github.com/uber-go/gopatch/issues/6 "func_within_a_func/two_dots": {}, "mismatched_dots/unnamed": {}, // https://github.com/uber-go/gopatch/issues/9 - "noop_import/remove_all": {}, - "noop_import/remove_some": {}, "switch_elision/body": {}, "select_elision/example": {}, "case_elision/a": {}, diff --git a/examples/gomock-v1.5.0.patch b/examples/gomock-v1.5.0.patch index 3caec1e..0c02575 100644 --- a/examples/gomock-v1.5.0.patch +++ b/examples/gomock-v1.5.0.patch @@ -2,10 +2,10 @@ # the test finishes running. We no longer need to call mockCtrl.Finish # manually. @@ -var ctrl identifier +var ctrl, gomock identifier var t expression @@ - import "github.com/golang/mock/gomock" + import gomock "github.com/golang/mock/gomock" ctrl := gomock.NewController(t) ... diff --git a/internal/engine/file_test.go b/internal/engine/file_test.go index 4ae6c50..5a452d4 100644 --- a/internal/engine/file_test.go +++ b/internal/engine/file_test.go @@ -553,7 +553,7 @@ func TestFile(t *testing.T) { }, }, { - desc: "remove import", + desc: "remove unnamed import", // -import "foo" // bar minus: &pgo.File{ @@ -580,18 +580,6 @@ func TestFile(t *testing.T) { "func bar() { bar() }", ), }, - { - desc: "named import", - giveSrc: text.Unlines( - "package x", - `import baz "foo"`, - "func bar() { bar() }", - ), - wantSrc: text.Unlines( - "package x", - "func bar() { bar() }", - ), - }, }, }, { diff --git a/internal/engine/import.go b/internal/engine/import.go index f9d26a1..9f3f12f 100644 --- a/internal/engine/import.go +++ b/internal/engine/import.go @@ -33,19 +33,24 @@ import ( // ImportMatcher matches a single import. type ImportMatcher struct { - NameS string // TODO: better naming - Name Matcher // named import, if any - Path string // import path as a string + NameS string // TODO: better naming + Name Matcher // named import, if any + NameIsMetavar bool // records wehther the named import was a metavariable + // TODO: This is janky. + + Path string // import path as a string } func (c *matcherCompiler) compileImport(imp *ast.ImportSpec) ImportMatcher { var ( - name Matcher - nameS string + name Matcher + nameS string + nameIsMetavar bool ) if n := imp.Name; n != nil { nameS = n.Name name = c.compileIdent(reflect.ValueOf(n)) + nameIsMetavar = c.meta.LookupVar(nameS) == IdentMetavarType } // TODO: In the future, we should try to determine the package name @@ -54,9 +59,10 @@ func (c *matcherCompiler) compileImport(imp *ast.ImportSpec) ImportMatcher { // import path, so the code would have to actually exist on-disk. return ImportMatcher{ - NameS: nameS, - Name: name, - Path: goast.ImportPath(imp), + NameS: nameS, + Name: name, + Path: goast.ImportPath(imp), + NameIsMetavar: nameIsMetavar, } } @@ -69,32 +75,71 @@ func (m ImportMatcher) Match(file *ast.File, d data.Data) (_ data.Data, ok bool) return d, false } - name := spec.Name - if name != nil { - // If we matched a named import, record the name. - d = data.WithValue(d, importKey(m.Path), importData{Name: name.Name}) - } - - // Match was a success if this wasn't a named import. + // We need to account for four cases here: + // + // +--------------+-------------+-----------------------+ + // | Patch import | File import | Behavior | + // +--------------+-------------+-----------------------+ + // | unnamed | unnamed | match | + // | unnamed | named | no match | + // | named | unnamed | match if metavariable | + // | named | named | match name | + // +--------------+-------------+-----------------------+ + + // Patch import is unnamed. Match only if the file import is also + // unnamed. if m.Name == nil { - return d, true + return d, spec.Name == nil } - // if we didn't match a named import, assume that the name specified - // in the patch is the name. - if name == nil { - name = &ast.Ident{ + if spec.Name == nil { + if !m.NameIsMetavar { + // If the patch import is not a metavar, then we meant + // to match it verbatim, so this is not a match. + return d, false + } + + // If the patch import is a metavar, then record a fake name + // for the metavar so that there's a value associated with + // "foo" in "foo.X", but also record that the real name for + // this import metavar is empty. + d = data.WithValue(d, importMetavarKey(m.NameS), importMetavarData{ + Unnamed: true, + }) + + d = data.WithValue(d, importKey(m.Path), importData{ + Name: m.NameS, + MetavarKey: importMetavarKey(m.NameS), + }) + + d, ok = m.Name.Match(reflect.ValueOf(&ast.Ident{ NamePos: spec.Pos(), Name: m.NameS, - } + }), d, nodeRegion(spec)) + } else { + d = data.WithValue(d, importKey(m.Path), importData{Name: spec.Name.Name}) + + // Both are named. Match as-is and also associate the package + // name with the import path so that we can delete it later. + d, ok = m.Name.Match(reflect.ValueOf(spec.Name), d, nodeRegion(spec)) } - return m.Name.Match(reflect.ValueOf(name), d, nodeRegion(spec)) + // Both imports are named. Match the name and move on. + return d, ok } +type importMetavarKey string + +type importMetavarData struct{ Unnamed bool } + type importKey string // import path -type importData struct{ Name string } +type importData struct { + Name string // package name of the import + + // Name of the import metavar we used to match this value, if any. + MetavarKey importMetavarKey +} // ImportsMatcher matches multiple imports in a Go file. type ImportsMatcher struct { @@ -136,43 +181,76 @@ type importsData struct { // ImportReplacer replaces imports in a file. type ImportReplacer struct { - Name Replacer // named import, if any - Path string // import path as a string + Name Replacer // named import, if any + NameS string + NameIsMetavar bool + + Path string // import path as a string Fset *token.FileSet } func (c *replacerCompiler) compileImport(imp *ast.ImportSpec) ImportReplacer { - var name Replacer - if imp.Name != nil { - name = c.compileIdent(reflect.ValueOf(imp.Name)) + var ( + name Replacer + nameS string + nameIsMetavar bool + ) + if n := imp.Name; n != nil { + nameS = n.Name + name = c.compileIdent(reflect.ValueOf(n)) + nameIsMetavar = c.meta.LookupVar(nameS) == IdentMetavarType } // TODO: Same as matcher, maybe we should attempt to determine the // package name. return ImportReplacer{ - Name: name, - Path: goast.ImportPath(imp), - Fset: c.fset, + Name: name, + NameS: nameS, + Path: goast.ImportPath(imp), + Fset: c.fset, + NameIsMetavar: nameIsMetavar, } } // Replace adds a single import. Returns the name of the import that was // added. func (r ImportReplacer) Replace(d data.Data, cl Changelog, f *ast.File) (string, error) { - var name string + // name is the name we want to use for the named import, and pkgName is + // how the rest of the file references this import. + var name, pkgName string if r.Name != nil { - namev, err := r.Name.Replace(d, cl, f.Pos()) // pos is irrelevant - if err != nil { - return "", err + // The name replacer will produce the value for the named + // import specified in the patch as-is, or if it was a + // metavariable, using the matched value. + // + // This is undesirable for the case where the named import + // matched an unnamed import. For that case, we want to ignore + // the recorded name. So, if the named import is a + // metavariable that matched an unnamed import, don't look up + // its recorded value. + + var mdata importMetavarData + if r.NameIsMetavar { + data.Lookup(d, importMetavarKey(r.NameS), &mdata) + pkgName = r.NameS // default to metavar name + } + + if !mdata.Unnamed { + namev, err := r.Name.Replace(d, cl, f.Pos()) // pos is irrelevant + if err != nil { + return "", err + } + name = namev.Interface().(*ast.Ident).Name + pkgName = name } - name = namev.Interface().(*ast.Ident).Name + } if !astutil.AddNamedImport(r.Fset, f, name, r.Path) { return "", nil } - return name, nil + return pkgName, nil } // ImportsReplacer replaces a block of imports. @@ -223,17 +301,29 @@ func (r ImportsReplacer) Cleanup(d data.Data, f *ast.File, newNames []string) er // Delete matched imports that are no longer used. for _, imp := range impData.MatchedImports { - var idata importData - data.Lookup(d, importKey(imp), &idata) - impName := idata.Name - if len(impName) == 0 { - impName = filepath.Base(imp) + var importName, pkgName string + + if idata := new(importData); data.Lookup(d, importKey(imp), idata) { + pkgName = idata.Name + importName = idata.Name + + // If we used a metavariable to match this import, + // record if its name was actually empty. + if mdata := new(importMetavarData); data.Lookup(d, idata.MetavarKey, mdata) { + if mdata.Unnamed { + importName = "" + } + } + } + + if len(pkgName) == 0 { + pkgName = filepath.Base(imp) } // If this import was replaced by an added import, kill it. - _, replaced := taken[impName] - if replaced || !usesNameAsTopLevel(f, impName) { - astutil.DeleteNamedImport(r.Fset, f, idata.Name, imp) + _, replaced := taken[pkgName] + if replaced || !usesNameAsTopLevel(f, pkgName) { + astutil.DeleteNamedImport(r.Fset, f, importName, imp) } } diff --git a/testdata/delete_any_import b/testdata/delete_any_import new file mode 100644 index 0000000..fc36fb1 --- /dev/null +++ b/testdata/delete_any_import @@ -0,0 +1,39 @@ +-- in.patch -- +@@ +var foo identifier +@@ +-import foo "foo" + + bar + +-- unnamed.in.go -- +package x + +import "foo" + +func x() { + bar() +} + +-- unnamed.out.go -- +package x + +func x() { + bar() +} + +-- named.in.go -- +package x + +import baz "foo" + +func x() { + bar() +} + +-- named.out.go -- +package x + +func x() { + bar() +} diff --git a/testdata/delete_field b/testdata/delete_field index a650bf6..3c3ee6a 100644 --- a/testdata/delete_field +++ b/testdata/delete_field @@ -53,7 +53,7 @@ func bar() { -- unnamed.out.go -- package a -import foo "example.com/foo.git" +import "example.com/foo.git" func bar() { foo.Initialize(foo.Params{ diff --git a/testdata/delete_unnamed_import b/testdata/delete_unnamed_import index f128891..baf6d55 100644 --- a/testdata/delete_unnamed_import +++ b/testdata/delete_unnamed_import @@ -1,3 +1,5 @@ +Delete an unnamed import verbatim. + -- in.patch -- @@ @@ @@ -5,7 +7,23 @@ bar --- a.in.go -- +-- unnamed.in.go -- +package x + +import "foo" + +func x() { + bar() +} + +-- unnamed.out.go -- +package x + +func x() { + bar() +} + +-- named.in.go -- package x import foo "foo" @@ -14,9 +32,11 @@ func x() { bar() } --- a.out.go -- +-- named.out.go -- package x +import foo "foo" + func x() { bar() } diff --git a/testdata/destutter b/testdata/destutter index ad0fa14..3705c43 100644 --- a/testdata/destutter +++ b/testdata/destutter @@ -1,8 +1,5 @@ Removes stuttering in the name of a type. -TODO: `var foo identifier` for named imports should not add a name when there -wasn't there one before. fix_consumers* should be merged into one patch. - -- fix.patch -- => examples/destutter.patch diff --git a/testdata/gomock b/testdata/gomock index 70144bf..4c4d899 100644 --- a/testdata/gomock +++ b/testdata/gomock @@ -64,3 +64,34 @@ func TestBar(t *testing.T) { run(NewBarMock(ctrl)) } + +-- named_import.in.go -- +package baz + +import ( + "testing" + + mock "github.com/golang/mock/gomock" +) + +func TestBaz(t *testing.T) { + mockCtrl := mock.NewController(t) + defer mockCtrl.Finish() + + run(NewBazMock(mockCtrl)) +} + +-- named_import.out.go -- +package baz + +import ( + "testing" + + mock "github.com/golang/mock/gomock" +) + +func TestBaz(t *testing.T) { + mockCtrl := mock.NewController(t) + + run(NewBazMock(mockCtrl)) +} diff --git a/testdata/match_named_import b/testdata/match_named_import new file mode 100644 index 0000000..18caf69 --- /dev/null +++ b/testdata/match_named_import @@ -0,0 +1,50 @@ +Match a named import verbatim. + +-- in.patch -- +@@ +var X identifier +@@ +-import foo "example.com/bar" ++import bar "example.com/bar" + +-foo.X ++bar.X + +-- unnamed.in.go -- +package whatever + +import "example.com/bar" + +// Should remain untouched. +func stuff() { + foo.stuff() +} + +-- unnamed.out.go -- +package whatever + +import "example.com/bar" + +// Should remain untouched. +func stuff() { + foo.stuff() +} + + +-- named.in.go -- +package whatever + +import foo "example.com/bar" + +func stuff() { + foo.stuff() +} + +-- named.out.go -- +package whatever + +import bar "example.com/bar" + +func stuff() { + bar.stuff() +} diff --git a/testdata/noop_import b/testdata/noop_import index 4b39508..a758bdf 100644 --- a/testdata/noop_import +++ b/testdata/noop_import @@ -73,6 +73,7 @@ package main import ( "conversion/to.git" + "go.uber.org/thriftrw/ptr" ) diff --git a/testdata/recognize_all_imports b/testdata/recognize_all_imports new file mode 100644 index 0000000..4b0dc68 --- /dev/null +++ b/testdata/recognize_all_imports @@ -0,0 +1,44 @@ +-- in.patch -- +@@ +var foo, x identifier +@@ +-import foo "example.com/foo-go.git" ++import foo "example.com/foo.git" + + foo.x + +-- unnamed.in.go -- +package whatever + +import "example.com/foo-go.git" + +func foo() { + foo.X() +} + +-- unnamed.out.go -- +package whatever + +import "example.com/foo.git" + +func foo() { + foo.X() +} + +-- named.in.go -- +package whatever + +import bar "example.com/foo-go.git" + +func foo() { + bar.X() +} + +-- named.out.go -- +package whatever + +import bar "example.com/foo.git" + +func foo() { + bar.X() +} diff --git a/testdata/delete_named_import b/testdata/replace_any_import_with_unnamed similarity index 56% rename from testdata/delete_named_import rename to testdata/replace_any_import_with_unnamed index 3d1d12b..23bf1e4 100644 --- a/testdata/delete_named_import +++ b/testdata/replace_any_import_with_unnamed @@ -8,7 +8,7 @@ var fooclient identifier -fooclient.Init() +compat.Init() --- from_unnamed.in.go -- +-- unnamed.in.go -- package main import "example.com/foo/client" @@ -17,7 +17,25 @@ func main() { fooclient.Init() } --- from_unnamed.out.go -- +-- unnamed.out.go -- +package main + +import "example.com/foo-client/compat" + +func main() { + compat.Init() +} + +-- named.in.go -- +package main + +import client "example.com/foo/client" + +func main() { + client.Init() +} + +-- named.out.go -- package main import "example.com/foo-client/compat" diff --git a/testdata/replace_net_context b/testdata/replace_net_context index 0c823b1..5be504f 100644 --- a/testdata/replace_net_context +++ b/testdata/replace_net_context @@ -1,15 +1,10 @@ -# TODO(abg): This is broken because of our current "uses import" detection -# which treats the old context import as used, as it doesn't recognize that -# the new one is meant to replace it. - -- in.patch -- @@ +var context identifier @@ --import "golang.org/x/net/context" -+import "context" +-import context "golang.org/x/net/context" ++import context "context" -# TODO: Delete the requirement of having at least one AST-level -# match/replacement. context -- top_level.in.go -- @@ -24,11 +19,7 @@ func x() { -- top_level.out.go -- package foo -import ( - "context" - - "golang.org/x/net/context" -) +import "context" func x() { context.Background() @@ -53,8 +44,6 @@ package foo import ( "context" "time" - - "golang.org/x/net/context" ) func x() { diff --git a/testdata/rewrite_import_paths b/testdata/rewrite_import_paths index c5fb536..7d3c0c7 100644 --- a/testdata/rewrite_import_paths +++ b/testdata/rewrite_import_paths @@ -61,7 +61,7 @@ func z() { -- no_named_import.out.go -- package baz -import fooclient "foo-client" +import "foo-client" func z() { fooclient.NewRequest()