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

Tue Jun 27 12:39:57 UTC 2023

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 <chris at 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 <mail at 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/proposals/0000-split-wunused-imports.md
> >
> >> Discussion of proposal: <
> 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.
> >>
> >> _______________________________________________
> >> ghc-steering-committee mailing list
> >> ghc-steering-committee at haskell.org
> >>
> https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-steering-committee
> >
> > --
> > Joachim Breitner
> >  mail at joachim-breitner.de
> >  http://www.joachim-breitner.de/
> >
> > _______________________________________________
> > ghc-steering-committee mailing list
> > ghc-steering-committee at haskell.org
> > https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-steering-committee
>
> _______________________________________________
> ghc-steering-committee mailing list
> ghc-steering-committee at haskell.org
> https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-steering-committee
>

-- 
Arnaud Spiwack
Director, Research at https://moduscreate.com and https://tweag.io.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.haskell.org/pipermail/ghc-steering-committee/attachments/20230627/aa1b81ca/attachment.html>