Clarify the meaning of hidden_modules and fix the code

[facundominguez]

The documentation of the hidden_modules attribute of the haskell_library rules currently says:

Modules that should be unavailable for import by dependencies.

The text itself is wrong, since dependencies of the haskell_library rule cannot import any modules of it. But there are also a couple of situations that need to be explained.

Non-existent modules

The following instantiation will produce an error in the analysis phase

haskell_library(
    name = "lib",
    srcs = [
        "src/A.hs",
        "src/B.hs",
    ],
    hidden_modules = ["C"],
)

The error is:

Traceback (most recent call last):                                                                                                                                                                    
        File ".../76262e73dad9f87ef6526dd61590ea62/external/rules_haskell/haskell/private/haskell_impl.bzl", line 505, column 27, in haskell_library_impl
                set.mutable_difference(exposed_modules, set.from_list(other_modules))              
        File ".../76262e73dad9f87ef6526dd61590ea62/external/rules_haskell/haskell/private/set.bzl", line 103, column 26, in _mutable_difference
                s0._set_items.pop(item)                                                                                                                                                               
Error in pop: KeyError: "C"

This is because mutable_difference is calling pop(item) instead of pop(item, default = None). However, I would argue that it should be considered an error to list hidden modules that do not exist. If we agree on this, the documentation should say so, and the code should check it and produce an intelligible message.

Rexported modules

It turns out that hidden_modules does not only hide modules of this library but also modules in reexported_modules. And I'd feel inclined again to argue that this should rather be considered an error, and that documentation should say so, and that an appropriate error message should be produced in this situation.

If not considered an error, then at least it should be explained in the documentation that hidden_modules affects the reexported modules as well.

Environment

• Version of the rules: master

[aherrmann]

However, I would argue that it should be considered an error to list hidden modules that do not exist. If we agree on this, the documentation should say so, and the code should check it and produce an intelligible message.

Agreed, this sounds good to me.

It turns out that hidden_modules does not only hide modules of this library but also modules in reexported_modules.

To clarify, you mean a case like this?

haskell_library(
    ...
    reexported_modules = {":other-lib": "SomeMod"},
    hidden_modules = ["SomeMod"],
)

Yes, seems reasonable to guard against that.

[martyall]

I am working on this issue as a "good first issue", but I'm not sure that the second point about "reexported modules" makes sense in light of the first.

If the requirement is that hidden modules must be a subset of the modules defined in the src attribute, then how are they also going to appear as modules defined in some library in the deps attribute? In this example above:

haskell_library(
    ...
    reexported_modules = {":other-lib": "SomeMod"},
    hidden_modules = ["SomeMod"],
)

SomeMod cannot appear in the modules defined by src (at least without a qualified package name afaik), so if you're guarding against that then there's no need to check for this additional condition. Is there something I don't understand?

[martyall]

Furthermore, the following behavior of reexporting hidden modules is currently allowed, which also seems problematic (modified example from test case):

haskell_library(
    name = "lib-a",
    srcs = glob(["lib-a/*.hs"]),
    hidden_modules = ["Foo"],
    src_strip_prefix = "lib-a",
    deps = ["//tests/hackage:base"],
)

haskell_library(
    name = "lib-c",
    srcs = glob(["lib-c/*.hs"]),
    reexported_modules = {"lib-a": "Foo"},
    src_strip_prefix = "lib-c",
    deps = [
        ":lib-a",
        "//tests/hackage:base",
    ],
)

[facundominguez]

SomeMod cannot appear in the modules defined by src (at least without a qualified package name afaik), so if you're guarding against that then there's no need to check for this additional condition. Is there something I don't understand?

Hello! If the specification and the implementation are changed to require hidden_modules to be a subset of modules in src, then I think no extra checks are necessary. Now that haskell_module is merged, probably hidden_modules should also check if the modules are listed in the modules attribute as well.

[martyall]

I'm not sure I see an example in the test folder which uses the modules attribute, and I'm having trouble determining what it might be from the source code. It looks like you have the option to either declare the src files attribute or the modules attribute, but not both?

[aherrmann]

@martyall Thank you for looking into this!

I'm not sure I see an example in the test folder which uses the modules attribute

@martyall You can find examples using modules within this test folder.

It looks like you have the option to either declare the src files attribute or the modules attribute, but not both?

Correct. For more context on haskell_module take a look at https://github.com/tweag/rules_haskell/issues/1552.

[martyall]

Regarding this issue about re-exporting hidden modules from dependencies that I highlighted above, this actually results in a ghc panic

You also get this panic if you try to reexport a module that doesn't exist. I think that we should open a separate issue to validate reexported modules rather than try to solve that in this issue.

[aherrmann]

You also get this panic if you try to reexport a module that doesn't exist. I think that we should open a separate issue to validate reexported modules rather than try to solve that in this issue.

That seems fair. @martyall want to go ahead and open that issue?

[martyall]

opened as #1811