Use explicit export lists for modular solver

[BardurArantsson]

Using explicit export lists allows for trivial detection of dead code,
and this commit also removes quite a bit of said dead code.

---

This is preliminary work to separating out the solver into its own package.

[BardurArantsson]

OK, Travis now says this is good to go, so if someone would be so kind as to review...?

[phadej]

cc @kosmikus

[BardurArantsson]

Ah, yes, I can see that @kosmikus and I may be stepping a little on each other's toes :).

@kosmikus : Are you OK with merging this before your stuff? Everything done I've done is really simple (just code removal + adding explicit imports) and it should be absolutely trivial to figure out any conflicts. (Whereas I would be somewhat worried that I wouldn't be able to figure out conflicts with your code.)

[BardurArantsson]

@grayjay and @kosmikus : Any objections to merging this? It will cause a few conflicts for some of the currently open solver-related PRs, but AFAIK they should all be minor and pretty easily resolved. All I've done here is to add explicit export lists and delete dead code.

[23Skidoo]

I'd merge this PR if it just added explicit exports. Not sure how @kosmikus feels about that dead code, though.

[kosmikus]

Sorry, I definitely want to take a look before any code gets removed. And if it makes merging other stuff more difficult, I'm also reluctant.

[BardurArantsson]

I mostly just started to remove the dead code because I wanted to avoid warnings from GHC. Well, that and... it's dead code. Hopefully it's not just dead code because it's accidentally dead... But I guess that's a possibility. @kosmikus : I guess that's what you're worried about?

My reasons for wanting this in ASAP are that a) it's quite a lot of tedious make-work to redo every time something conflicting is merged, and b) causes only simple-to-resolve conflicts for any other branches (just pick "your version" of the code and remove dead code), c) it actually means less work for any other upcoming PRs since they don't have to go update dead code, and finally d) it's stalled behind more complex solver changes which may be a lot harder to review.

[kosmikus]

FWIW, I don't think there's code that's accidentally dead. But the code may still be useful, either because I've been using it in ad-hoc testing, or because it's stuff that may be used in the future and serves as a useful reminder. I haven't looked at exactly what you've removed.

[martinvlk]

Andres Löh:

I've been using it in ad-hoc testing, or because it's stuff that may be used in the future and serves as a useful reminder

Surely this would unnecessarily pollute the code base?
Can we keep such reminders and ad hoc testing code in a separate snippes
file or something, if they are really useful?

M.

[phadej]

@martinvlk Please keep in mind that solver part will be separated into own library package, so even some functions aren't used in cabal-install they may be useful for someone else. And if they aren't that useful, then they could be moved into cabal-solver package test-suite.

I have no idea about the functions themselves, but their documentation notes are something which seems to be valid and more broad - and worth keeping.

[BardurArantsson]

@phadej Could you point to the comments you'd like to have preserved? (Perhaps just put a line comment into the commit diff?)

In general I'm not really all that sympathetic to the "might be useful" argument for keeping code around. We have version control for a reason and without further qualifiers the "might be useful" argument can essentially be used to justify keeping any code around forever. One needs some kind of non-arbitrary way to judge what to keep and what not to keep. (Keeping dead code around has a non-trivial maintenance cost... and what you usually want to keep as low as possible is the maintenance cost since it gets paid every time anyone needs to change the code.) EDIT: So, I'm more of a YAGNI type person, is what I'm saying :).

I am sympathetic to the "we sometimes use this in ghci" argument, but I think we need to handle it differently if there is such "dead" code -- let's at least make it explicit. I'm not sure what the best thing to do here would be, but at the very least we could export the relevant functions with a comment in the export list saying that they're just being used for interactive testing. Ideally, we'd avoid the need for interactive testing entirely by incorporating this stuff into the test suites. Perhaps we should move the "used for ghci" bits to separate modules? I don't know how feasible that is.

[grayjay]

@BardurArantsson I don't mind resolving the conflicts with my pull requests, but I don't have enough experience with the solver to know whether the dead code should be removed.

[BardurArantsson]

@grayjay Thanks.

@kosmikus Have you had any chance to review the code to figure out what you'd like to keep and/or consider my most recent comment (before this one)? If not, do you think you might spare the time sometime during the next week[1]? If not, I'd like to go propose merging based on the YAGNI principle and the fact that any removed code can be retrieved from the Git history[2]. If any of it is important for interactive testing (by you or others), then we'll discover that pretty soonish and we can bring it back. If you prefer, I could perhaps split this into two separate commits -- one for the export lists and one for dead code removal -- but I'm not sure that adds much value.

[1] Doesn't literally have to be the next week, just something concrete so that we can have a reasonable expectation of progress.
[2] I realize that such resurrected code may need changes to work again, but such is life.

[BardurArantsson]

(Of course, any guidance on what you'd like to keep would be most welcome.)

[BardurArantsson]

@kosmikus Ping?

I really want to move forward on this as soon as possible and to start looking at merging @grayjay 's PRs (if applicable). Given that the dead code could be ressurrected if needed, I suggest that this be merged. (In accordance with the YAGNI principle.)

[grayjay]

@BardurArantsson Are you eager to merge my pull requests because #2928 is waiting on them? I would rather resolve the conflicts with #2928 than rush the merging of my pull requests. @kosmikus advised me on my solver PRs, so I would like to wait until he has time to review them.

[BardurArantsson]

@grayjay Yeah, it's mostly about avoiding excessive conflicts and #2928 is my ultimate goal[1], but I gather that there's probably going to be a release of Cabal/cabal-install soonish(?) to coincide with GHC-8.0, so the split (#2928) may be delayed anyway. I was mostly thinking of merging some of your more "obvious" fixes -- i.e. the space leak, etc. if possible.

Regardless, I think I'll try to split this commit into two (explicit lists, code removal) and merge that soon. It doesn't break any functionality, so it should be safe to merge before the GHC-8.0 branch-off point.

[1] Well, it's a stepping stone to hopefully being able to use the solver from other projects. I'm not quite sure if #2928 actually gets us far enough along that road...

[grayjay]

@BardurArantsson Okay. I think #2914 is pretty straightforward, but #2916 completely reorganizes backjumping.

[BardurArantsson]

OK, this is the split-up version.

[BardurArantsson]

Btw, I'll definitely merge the first commit (export lists) here soon(!) as it should be entirely uncontroversial, but I'd still like feedback on the dead code bits... I still think we should go with the YAGNI principle, so unless someone actively objects, I'm leaning towards merging. I'll leave it for a few days (at the very least).

[23Skidoo]

If you merge the first commit, but not the second, you'll break the Travis build because it runs with -Werror. If I were you, I'd just add the dead functions to the export lists, but add a comment marking them as such. Maybe later they could be moved to a separate module intended for interactive use.

[BardurArantsson]

@23Skidoo Already added to the export lists :). I tested the build locally and it should be -Wall clean (and so -Werror clean too). We'll see, I guess :)

[BardurArantsson]

Oh great, it appears that pushing the first commit has marked this PR as "merged" with no obvious way to undo that. I guess I'll open a new PR for the 2nd commit...