deterministic import names

[eric-am]

Expected Behaviour

Generated code should be deterministic.

Even when the types specified by typemap.json are in several packages with the same name or same final dir of their import path.

Actual Behavior

gqlgen does detect this situation 👍 and reacts by giving each package with the same name a unique name when imported by suffixing a number. So if we have "foobar/types" with package name "types" and "bazfoo/types" with package name "types", gqlgen will make imports roughly like:

import (
    // other stdlib stuff

    types "foobar/types"
    types1 "bazfoo/types"

The trouble is... it seems to do this semirandomly 👎 So when using gqlgen repeatedly, the git diff of the results sometimes results in a bunch of

-   types "foobar/types"
-   types1 "bazfoo/types"
+   types "bazfoo/types"
+   types1 "foobar/types"

as well as every method on the Resolvers interface and so on also having that unhelpful diff.

If one changes the names of the packages internally, but leaves them in the same paths -- e.g. files under "foobar/types" path now declare "package foobartypes" -- gqlgen doesn't change behavior; looks like it bases all the names on the last segment of the import path. I think it should probably use the declared package name instead.

This isn't a correctness issue, exactly, because it's all internally consistent and doesn't affect anything outside of that generated file, but it definitely makes for some uggo diffs, and makes the Resolvers interface a lot harder to read.

Ideally, gqlgen would use the declared package names, and then if names still have uniqueness issues, use sort on the imports before doing the name suffixing.

[eric-am]

Looks like it's Imports.addPkg that's responsible for the name decollisioning, and unfortunately it's in part because NamedTypes is a map that the loop iterating over the addPkg function is random.

[vektah]

The code generated definitely should be stable across builds, so this is definitely a bug.

It might be better to do a decollision pass after loading the program instead of on addPkg. I think chasing randomness out of the callers is probably going to be more fragile than centralizing this constraint.

This would also mean it happens pretty late, well after the program has been loaded and the package names would be available.

There's also a bit of a data loop here:

• you need the list of imports (order doesn't matter yet) to load the program (all the user defined types etc)
• you need to load the program to get the package name, to sort the imports.

I'm pretty busy currently with the new parser and probably wont get around to this for a bit, but I'm open to a PR :).

[eric-am]

woofdah. I can give it a shot, but it might be a little complex for me.

Late decollisioning might still be reasonable, since it's valid to do so per file. But I'm just sort of devil's-advocating that; it's a pretty useless distinction given that gqlgen isn't making a ton of different files with different sets of imports.

I looked at the code a bit... I haven't used tools.go.loader.* before so I'm kind of out of my depth, but one of my first impressions was... Shouldn't Ref.Package perhaps better be called ImportPath ? (And then I'd add back in a Package string field to that same structure, which is the short internally declared name of the package.) If that makes sense to you, I'd then do that as one quick separate PR before this one.