NoUnused.Exports does not work when exposing all

[tfausak]

Describe the bug I'm not sure if this is a bug. Perhaps I don't understand how this rule is supposed to work. At any rate, if you use NoUnused.Exports.rule and also expose everything using exposing (..), the rule won't fire.

SSCCE (Short Self-Contained Correct Example) Define a module like this:

module Example exposing (..)

unused : ()
unused =
    ()

Expected behavior A warning that unused is in fact unused. Changing exposing (..) to exposing (unused) does give the warning:

Exposed function or value unused is never used outside this module.

Additional context I noticed this while developing an application where we've decided to always use exposing (..) in order to avoid the busywork of managing exports. After a while, I noticed that things I knew were unused weren't being reported as such by elm-review. It took me a while to discover that exposing (..) was the culprit.

[jfmengels]

Hi @tfausak 👋

This is done on purpose, because there is nothing actionable that the user can do when you're exposing (..). You can't get rid of the function (as it's used inside this module) and you can't touch the exports (unless by removing exposing (..)).

I will document it in the rule's documentation to make this clearer, as it's not the first time I'm seeing this confusion 🙂

[tfausak]

I think there is something actionable though: You can delete the declaration.

If I wasn't using exposing (..), then elm-review would warn about the exposed declaration being unused. So I could remove it from the exposing list. After doing that, elm-review would again warn me about an unused and unexposed top-level declaration.

I think the behavior I'm looking for is for elm-review to treat exposing (..) the same as explicitly exposing every identifier in the module.

[jfmengels]

I think there is something actionable though: You can delete the declaration.

That is interesting. This would cause the rule to behave a bit like NoUnused.Variables in practice. The main thing I'm worried about is whether this is over-stepping the bounds of the rule, mostly in a way that will cause problems to people. For instance, some people may have enabled NoUnused.Exports but disabled NoUnused.Variables for some reason (eg they want to keep some variables), and now they will get errors.

If this turns out to be a problem, I'm thinking this could be a configuration option, but I don't know if it should be to turn on reporting these situations, or to turn them off. We should probably avoid a configuration option for this to start with and see how this goes.

It's also possible that some people explicitly used exposing (..) to avoid reports for NoUnused.Exports. But that's fine because they could/should configure the rule to not report issues for that file.

I think I'm in favor of this change :+1: