Re: [ghc-steering-committee] Split -Wunused-imports (recommend accept)

I'm content to accept this proposal. Like Joachim, my sense is that "relaxed redundant imports" would be worth exploring further; moreover the proposal's definitions of "unused" and "duplicate" are essentially specified only by GHC's implementation. But this proposal is a beneficial change and we can always further refine the warning flags in the future. Cheers, Adam On 27/06/2023 13:39, Arnaud Spiwack wrote:

The problem statement, as I understand it:

I'm writing a library, with dependency L-1 from where I import X and Y(f). A new version L-2 is released, where X exports (f) as well. My CI is set to reject all warnings, and start complaining on L-2. Silencing the warning on L-2 requires some rather gratuitous contortion (import X hiding (f)) just to be able to compile with L-1! (even if we don't require L-1 to compile without warning).

I'm not really fond of the idea of removing this type of duplicate import warning from -wall, to be honest. But I do agree that we shouldn't have to contort the code that way if the libraries are otherwise completely compatible. The fact that it's easy, by merely silencing a warning on imports to break compatibility with a previous version is not something to be proud of.

Therefore *I'm in favour* of this proposal (albeit reluctantly).

Oleg Grengrus's comment on the discussion thread makes me think that maybe there should be two different recommended sets of warnings: one for libraries (where compatibility with several versions of a dependency is desirable), and one for applications (where you are typically bound to a single version of your dependencies). Though it's probably too much overhead for a single warning difference between the two.

On Fri, 16 Jun 2023 at 16:05, Chris Dornan mailto:chris@chrisdornan.com> wrote:

Just for the record, I am also open to refinement of the import-warning toolbox in future but I think this proposal is the best next step.

Given how straightforward the proposition is, how about we set a week for objections to be raised and move to accept the proposal if none are forthcoming (by Saturday, 2023-06-24).

Chris

> On 16 Jun 2023, at 13:33, Joachim Breitner mailto:mail@joachim-breitner.de> wrote: > > Hi, > > this proposal highlights a fundamental tension with compiler warnings. > > If you assume a never changing background of dependencies, then the > current behavior isn’t too bad: GHC correctly tell you that you can > remove a line of code, and that is a good thing. I don’t think that > this is directly what is bothering developers. > > But dependencies do change, and exports get moved around, and code that > wasn’t warning before suddenly warns. And that is what’s annoying! > > So there is a tension between “given the current assumptions, your code > can be improved” vs. “assumptions change”. > > I agree that at least in this case (and probably in general) we want > the -Wall warnings to be reasonably insensitive to dependency changes, > so that good code stays “good”. Of course this is not a hard rule – > think of deprecation warnings, but a decent guideline. > > Therefore in favor. > > > The proposal mentions as alternative “Relaxed redundant imports”, which > refines the warnings a bit more. In that alternative, an _duplicate > explicit import_ is cause for a warning; in Chris’ example that would > be > >   import X (f) -- exports some function @f@ >   import y (f) -- exports the same function @f@ >   g = f > > Under the current proposal, this would no longer warn with -Wall, but I > think it’s silly to not warn here, as even with changing dependencies, > this is a code smell. I sympathize with the author’s wish to not make > this more complicated than necessary to achieve the goal (warnings > about duplicate _implicit_ imports causing headcaches), but would like > to keep the door open for someone who wants to implement the refined > version in the future. That would probably entail a >  -Wduplicate-explicit-import > flag which can then live in -Wall, but is still compatible with this > proposal, so all izz well. > > > > Adam points out (as quoted in the proposal) that users of >   -Weverything -Wno-unused-imports > might see new breakage. I don’t think someone using -Weverything can > expect any kind of stability, else we paint ourselves into a corner (or > start introducing -Wreally-everything, -Wreally-really-everything, …) > > > Cheers, > Joachim > > > > > Am Donnerstag, dem 15.06.2023 um 16:50 +0100 schrieb Chris Dornan: >> Dear Committee, >> >> Georgi Lyubenov has a simple proposal to improve the quality of life for many package maintainers that I recommend that we accept. >> >> ## Split -Wunused-imports >> >> Rendered proposal: <https://github.com/googleson78/ghc-proposals/blob/split-unused-imports/propo... https://github.com/googleson78/ghc-proposals/blob/split-unused-imports/propo...> >> Discussion of proposal: <https://github.com/ghc-proposals/ghc-proposals/pull/586 https://github.com/ghc-proposals/ghc-proposals/pull/586> >> >> ### Summary >> >> (Straight from the proposal.) >> >> We propose to split the -Wunused-imports warning into two parts: >> >> - warn on actual unused imports >> - warn on duplicate imports >> >> while also not including the second in -Wall. This will greatly reduce "churn" for library authors. >> >> >> ## In more Detail >> >> As we know, many Haskell developers, including significant commercial projects, incorporate -Wall >> -Werror into their workflow at some stage. To this end it is important that we minimise 'false >> negatives', whereby the compiler emits 'frivolous' warnings. >> >> This proposal identifies an important class of -Wall warnings that many package maintainers >> appear to regard quite bothersome, namely when the compiler warns that import statement is >> superflous even when it is importing an entity that is being used by the module. The issue is >> the way `-Wunused-imports` works, which will issue a warning for the following program. >> >> ```haskell >> import X -- exports some function @f@ >> import y -- exports the same function @f@ >> >> g = f >> ``` >> >> The compiler will warn that the second Y import is redundant. >> >> Many developers do not want to be warned about this. As far as they are concerned each import is >> yielding enties that are in use and the fact that one of them can be removed is just not >> interesting. >> >> In contrast, developers that use -Wall do want to be warned about the following. >> >> ```haskell >> import X -- exports some function @f@ >> import Z -- does not export f >> >> g = f >> ``` >> >> Here our module has a completely false dependency on Z which should be cleaned up at the point that >> warnings are cleaned up. >> >> This proposal proposes modifying -Wunused-imports so that it will warn about the second example but >> stay silent for the first, and adds a -Wduplicate-imports that will issue a warning for the the >> first example. In effect the enabling of both warnings in the new proposal will restore the current >> -Wunused-imports behaviour. -Wall should *not* enable -Wduplicate-imports but is avilable to those >> developers that rely on the current behaviour of -Wunused-imports (which some clearly do). >> >> >> ## Recommendation >> >> I recommend that we accept this proposal.

-- Adam Gundry, Haskell Consultant Well-Typed LLP, https://www.well-typed.com/ Registered in England & Wales, OC335890 27 Old Gloucester Street, London WC1N 3AX, England