Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imports: named import with metavariable should recognize unnamed import #2

Closed
abhinav opened this issue Apr 21, 2021 · 0 comments · Fixed by #29
Closed

imports: named import with metavariable should recognize unnamed import #2

abhinav opened this issue Apr 21, 2021 · 0 comments · Fixed by #29
Labels
bug Something isn't working

Comments

@abhinav
Copy link
Contributor

abhinav commented Apr 21, 2021

When trying to match all imports of a path, a patch that specified named imports, even with metavariables will turn unnamed imports into named imports.

Input

Patch:

@@
var foo, x identifier
@@
-import foo "example.com/foo-go.git"
+import foo "example.com/foo.git"

 foo.x

Source:

package whatever

import "example.com/foo-go.git"

func foo() {
  foo.X()
}

Expectation

package whatever

import "example.com/foo-go.git"

func foo() {
  foo.X()
}

Reality

package whatever

import foo "example.com/foo-go.git"

func foo() {
  foo.X()
}

Problem

This is undesirable; the foo metavariable should remember that the import was unnamed and reproduce that. Alternatively, we should provide a different means of matching imports that allows addressing both named and unnamed cases.

Workaround

For now, a workaround is to reproduce the patch as-is twice, once without the named import, once with:

@@
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
@abhinav abhinav added the bug Something isn't working label Apr 21, 2021
abhinav added a commit that referenced this issue Jun 24, 2021
Users previously had to work around the limitation that a named import in a
patch would transform an unnamed import to a named import.

That is,

```diff
@@
var foo, X identifier
@@
-import foo "example.com/foo"
+import foo "example.com/foo/v2"

 foo.X
```

Would transform unnamed imports in matched files to named imports:

```diff
package thing

-import "example.com/foo"
+import foo "example.com/foo/v2"
```

This was undesirable.

Teach the import matcher and replacer to recognize that the matched import
is unnamed, and remember that for later, associating it with the
metavariable name, not the import path. Associating it with the import path
would make it impossible to rename import paths like the case above.

The implementaton is a bit messy, but we basically have to differentiate
between the package name of an import and the identifier we use to refer to
types defined in that import.

Resolves #2
abhinav added a commit that referenced this issue Jun 25, 2021
Users previously had to work around the limitation that a named import in a
patch would transform an unnamed import to a named import.

That is,

```diff
@@
var foo, X identifier
@@
-import foo "example.com/foo"
+import foo "example.com/foo/v2"

 foo.X
```

Would transform unnamed imports in matched files to named imports:

```diff
package thing

-import "example.com/foo"
+import foo "example.com/foo/v2"
```

This was undesirable.

Teach the import matcher and replacer to recognize that the matched import
is unnamed, and remember that for later, associating it with the
metavariable name, not the import path. Associating it with the import path
would make it impossible to rename import paths like the case above.

The implementaton is a bit messy, but we basically have to differentiate
between the package name of an import and the identifier we use to refer to
types defined in that import.

Resolves #2
abhinav added a commit that referenced this issue Jul 7, 2021
The destutter example was working around #2. This is no longer necessary
since #29. testdata/destutter tests this example patch against both,
named and unnamed imports, and the test case remains unchanged.
abhinav added a commit that referenced this issue Jul 23, 2021
The destutter example was working around #2. This is no longer necessary
since #29. testdata/destutter tests this example patch against both,
named and unnamed imports, and the test case remains unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

1 participant