Process import comments properly

[sdboyer]

Go 1.4 introduced support for declaring canonical import paths via import comments. Now, TIL that these import comments are ignored by the compiler when within a vendor/ directory, so we could disregard them. But that seems unwise. Given that we view vendor/ as an implementation detail, there's a time in the foreseeable future where the compiler may start enforcing these comments again, and we should enforce them in order for our generated lock files to be maximally forwards-compatible.

Beyond this practical reason, import comments are a useful bit of information that establish internal identity and canonicality for code locations in a world where identity is otherwise entirely determined by the importer/addresser. That makes them a very useful signal, and we should take advantage of it.

The place where this information needs to enter the system is in pkgtree.ListPackages(); the processing work itself is done in pkgtree.fillPackage(). We'll need:

1. Basic error coverage, in the event that a single package has multiple, incompatible import comments. We'll need to make our own type for this, as we can't reuse whatever go/build does (it's not exported, may not even exist as a type).
2. Validation that the import comment aligns as expected with the importRoot argument to pkgtree.ListPackages(). This'll definitely require a new error type.
3. New tests that cover all possibilities, happy and not, codified via new fixtures under internal/gps/_testdata/src and utilized in TestListPackages().

Note that changing this will probably entail bumping the solver version (the number in Gopkg.lock). It's our first time doing that, so we'll need to plan a little bit around it. In a follow-up issue, we can modify the solver to incorporate a new satisfiability check that the import comment data is respected.

/cc @ChimeraCoder

[rtfb]

I gave it some time yesterday and the beginnings didn't seem too difficult indeed. Got import comment extraction working here (sans tests and some error checking).

Tried looking at solver to find a place where it would use the new data, but that bit looks a bit overwhelming at first.

[sdboyer]

oh, awesome!! 🎉 🎉 🎉 🎊 🎊 that looks like a great start.

we should probably do this in two stages - first, getting ListPackages() fixed up (as it looks like you've largely done), and ensure that's tested. your commit message notes validation against importRoot, which will also be a very important integrity check...though we'll have to discuss a bit about whether or not it should actually return an error if violated. (can do that in a PR)

in a subsequent PR, then, the place to integrate into the solver will be right around here in satisfy.go. the methods in that func are split off from solver.go specifically so that we have all of our domain-specific satisfiability requirements grouped together in one place.

at the linked line, we're visiting an atom (a project@a particular version), and there is a set of inbound imports on the atom that've already been established, retrieved by this call. the question to answer is whether the shape of those import statements complies with the package comments metadata on the atom's package tree. the check itself, which i think will be fairly straightforward (though you will have to decipher the data in the dependency type) can be added to that checkRequiredPackagesExist() method. while you're in there, though, let's s/checkRequiredPackagesExist()/checkImportRequirementsAreValid()/.

we'll probably also need a new solver-specific error, as checkeeHasProblemPackagesFailure is already a bit overloaded, and i think the way its errors are worded are more geared towards the importee being the problem, rather than the importer.

[rtfb]

Okay, I tried following your suggestions and here's what I ended up with. Didn't submit a PR yet, since I wasn't able to introduce a new error while maintaining what checkeeHasProblemPackagesFailure does, only a sub-error.

Also, no tests for the new code path, couldn't find where can I stick it in. basicFixture gives me some idea how could such a test be implemented, but it seems like I'd need a separate type implementing a specfix, which seems kinda baroque.

[sdboyer]

oh, that seems like it's heading in a good direction! honestly, idk if this seems like a small thing to you, but having other people touch the solver logic is a big deal for me, as it's scarcely happened yet 😄

so, i haven't looked at the logic in solve_failures.go in more than a year. it'll take me a bit to refamiliarize myself with all that and think about what design makes the most sense. so, tests first: yes, basicFixture and `bimodalFixture are designed for a pretty narrow use case of testing, and is optimized for testing only a subset of the conditions that the solver checks. being that this is adding a new one, it's awkward.

basically, we have two options. ideally, i would like to see the fixture types + the associated DSL expand to fully cover these conditions as we add them. that might be a bit tricky, though, so i could accept a fallback: basically, you could craft a test similar to TestRootLockNoVersionPairMatching, where it uses the pseudo-DSL to create a base set of solving inputs fixtures, then manually munges them as needed afterwards.

if we go the latter route, then i'd want to make sure we have an issue opened for follow-up. if you're willing to do the former...well, actually, it might not be that bad. really, we'll want to work from the bimodalFixture, and probably make a new helper, quite similar to the pkg() func, that takes comment path info, extend the tpkg type to accept the comment path, and then update the handling of it appropriately corresponding handling of them (maybe newDepspecSM()?, OTTOMH?), that turns the tpkg into properly useful data.