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

Binding to a type in the same package as generated code produces invalid output #283

Closed
mathewbyrne opened this issue Jul 17, 2023 · 4 comments · Fixed by #316
Closed

Binding to a type in the same package as generated code produces invalid output #283

mathewbyrne opened this issue Jul 17, 2023 · 4 comments · Fixed by #316
Labels
bug Something isn't working
Milestone

Comments

@mathewbyrne
Copy link

Describe the bug
Binding to a type in the same package as the generated file is not detected and results in an import loop as the package is import itself.

To Reproduce
Output the generated file in the same location as a bound type.

generated: db/generated.go
bindings:
  FilterType:
    type: github.com/mathewbyrne/project/db.Filter

Expected behavior
The generated output should not have any import statement, as the type is in the same package. The type should be used directly without reference to it's package.

genqlient version
0.6

Additional context
Working around this is fairly simple by placing the generated output in a sub-package, but this still seems like a bug that could be resolved.

@mathewbyrne mathewbyrne added the bug Something isn't working label Jul 17, 2023
@costela
Copy link

costela commented Jul 17, 2023

This is such a weird coincidence! I just started working on a PR for this 😅

However, my PR currently deals with it from the package_bindings side, using a "magic" . package. The more general solution of matching the package path more intelligently requires a bit more refactoring - I think.

Will link it here as soon as the tests are done and we can see if it's enough.

@benjaminjkraft
Copy link
Collaborator

Ah, good find! I think the ideal place to update this would be in generator.ref/generator.addImportFor? Those should be able to see whatever we know about the current package and just have a special case. (But somewhere you may have to figure out -- or ask Go -- what the full package path for the current package even is.)

@costela
Copy link

costela commented Jul 18, 2023

Got a first attempt in #285.

Reworked my approach to make it work with bindings + package_bindings equally well.

Getting the fully qualified package is a bit fiddly and I'm not entirely sure it's "safe" (it relies on relative imports, which AFAICT have a rocky support history). But is seems to work.

PTAL

@mwajeeh
Copy link

mwajeeh commented Sep 4, 2023

Any updates on this?

@benjaminjkraft benjaminjkraft added this to the v1.0 milestone Jan 26, 2024
benjaminjkraft added a commit that referenced this issue Feb 18, 2024
The original motivation here is to fix #283, which is that if you try to
use `bindings` to bind to a type in the same package as the generated
code, we generate a self-import, which Go doesn't allow. Fixing that is
easy -- the three lines in `imports.go` -- once you know the
package-path of the generated code. (The test that that all fits
together is in integration-tests because that was the easiest place to
set up the right situation.)

Determining the package-path is not too much harder: you ask
`go/packages`, which we already use for `package_bindings`. But once
we're doing that, and handling errors, it's kinda silly that we ask you
to specify the package your generated code will use, because we have
that too, usually. So I rewrote that handling too, making `package` now
rarely necessary (see for example the `example` config), and warning if
it looks wrong. This is the changes in `config.go`, and is the more
substantial non-test change. (I also renamed some of the testdata dirs
to be valid package-names, to exercise more of that code, or in the case
of `find-config`, just for consistency.)
benjaminjkraft added a commit that referenced this issue Feb 19, 2024
…age (#316)

The original motivation here is to fix #283, which is that if you try to
use `bindings` to bind to a type in the same package as the generated
code, we generate a self-import, which Go doesn't allow. Fixing that is
easy -- the three lines in `imports.go` -- once you know the
package-path of the generated code. (The test that that all fits
together is in integration-tests because that was the easiest place to
set up the right situation.)

Determining the package-path is not too much harder: you ask
`go/packages`, which we already use for `package_bindings`. But once
we're doing that, and handling errors, it's kinda silly that we ask you
to specify the package your generated code will use, because we have
that too, usually. So I rewrote that handling too, making `package` now
rarely necessary (see for example the `example` config), and warning if
it looks wrong. This is the changes in `config.go`, and is the more
substantial non-test change. (I also renamed some of the testdata dirs
to be valid package-names, to exercise more of that code, or in the case
of `find-config`, just for consistency.)

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable (and checked that
the test for the bugfix fails without the rest of the changes)
- [x] Included a link to the issue fixed, if applicable
- [x] Included documentation, for new features
- [x] Added ~an entry~ two entries to the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants