Filter all autogen modules

[fmaste]

This fixes #3656. From #719 this was already a problem and the idea of creating a new field was also discussed on #1046, so I decided to give it a try

I created a field 'autogen-modules' to list modules that are autogenerated, like Paths_*. This should contain modules that are either on 'exposed-modules' or 'other-modules' so we can filter those when needed

[23Skidoo]

Can you please rebase this against master to get rid of the spurious merge commits? Also needs tests, docs, and a line in the changelog.

[23Skidoo]

s/autoge/autogen/

[23Skidoo]

Otherwise LGTM.

[ezyang]

This patch looks good, thank you! I think you can reduce the duplication in Check by using allBuildInfo instead of duplicating the logic for each type of component. See for example mentionedExtensions in Check for an example that does this. Then you can get rid of the libModuleAutogen and variants (which IMO don't add much: just call buildInfo if you need it!)

Other notes: in a lot of places, the message reads autogen-module when it should read autogen-modules.

[ezyang]

I don't think there's a need to include this field in the default cabal init; it's definitely a power-user case.

[ezyang]

OK, so let me summarize what I think are the blocking changes for this patchset are:

• As @23Skidoo mentions, there needs to be documentation. As part of documenting this, you need to be quite clear about what the intended semantics of autogen-modules are. Without introducing an exposed autogen modules field as well as an other autogen modules field, the only way to specify if the autogenerated module is exported or not is by duplicating it in exposed-modules/other-modules. So I think the behavior you want is this: for ANY version of Cabal, we should ERROR if an autogenerated module is not included in exposed-modules/other-modules/main-is/etc (I think the coverage here in the code is incomplete; the end-all-be-all of what modules may show up is in Distribution.Simple.SrcDist and you may have to refactor that code to get at this source of modules.) We should NOT error if a person specifies autogen-modules for an insufficiently new cabal version: this will help BC.
• The allModuleNames logic is just incorrect, because, a module name can be duplicated among a library and a test suite (doesn't happen often, but it's allowed!) and one might be autogenerated, while the other not. The way to fix this is by refactoring SrcDist so that you can get out the list of modules that it attempts to find (basically, any invocation of allSourcesBuildInfo) and then you can just compare directly. Ask the question: is a module name in autogen-modules among one of the modules that allSourcesBuildInfo looks for an hs file for?
• Relatedly, it is a little confusing that you have to specify autogenerated modules twice. No good way around it, as far as I can tell. (You could suggest that if Cabal >= 1.25, an autogen-modules that is not mentioned anywhere implies an other-modules entry, but this might be too much magic. No need to implement.)
• I would like it if you reduced the error message duplication in Check, but I understand if that's not a priority. Put that in the nice to have bin.
• Renaming autogenModuleName would be nice, but not essential for the patch
• Most important: a test showing the whole shebang works!

[ezyang]

I gave you commit bits! So when you think the PR is ready and doesn't break anything, you can take pushing it to master into your own hands ;)

[fmaste]

Some remarks and questions:

1. Now I return PackageBuildWarning without any version check when a module is on autogen-modules but not on other-modules or exposed-modules! Should it be an error? A PackageBuildImpossible?
2. When Paths_PACKAGENAME is mentioned on other-modules or exposed-modules but not on any autogen-modules it's a PackageBuildWarning for spec >= 1.25 and PackageDistSuspiciousWarn for lower versions with a recommendation to use 1.25. I think the former is OK but don't know how you usually treat the latter case. Maybe just an error when the spec version is lower than 1.25 and there's something on autogen-modules.
3. Removed allModuleNames as recommended.
4. I added function libModulesAutogen next to libModules, exeModulesAutogen next to exeModules and so on for Benchmark and TestSuite. It was mentioned that those are just a recordfield name away and not necessary, but so do benchmarkModules and exeModules. I think that all these help to be more explicit about the meaning of this new field, really helpful when reading the docs and using Cabal as a library for a custom build.
5. Adding to number 3, I tried to get all this in or from Distribution.Simple.SrcDist but the refactor was huge. Started making little changes on this direction but I decided to leave it for a future PR, also most of the functions there reside on IO monad when only with a PackageDescription -> PackageDescription suffices.
6. I renamed autogenModuleName to autogenPathsModuleName and deprecate it.
7. Just the docs missing.

[ezyang]

1. It should be an error, unless you plan to implement a feature along the lines of, "If a module is in autogen-modules but not anywhere else, it is implicitly in other-modules." Because if the user commits the error, the build is just not going to work (the autogen module won't get linked in.)
2. So, if I understand your question correctly, you are wondering if we should give a warning at all if someone pre-1.25 mentions Paths_* without putting it in autogen-modules. If you asked me, I wouldn't warn at all: just keep Paths_* as a special-case. But I'm pretty conservative. I think it would also be pretty reasonable to make it a HARD ERROR to say >= 1.25 and not put paths in autogen-modules. I'm less hot about warning in both cases, because you still have to stick the special case in anyway, so it's not like it's really helping users (besides letting them know about autogen-modules)
3. (n/a)
4. OK
5. OK, I don't blame you. I guess libModules is probably a reasonable approximation; it won't catch if you're autogenerating the main-is field but I seriously doubt anyone is actually doing that

Awesome work! The patch is really shaping up.

A thought: maybe if you can't find a file in sdist, the error message should mention that if the file is autogenerated, you should add it to autogen-modules?