Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#10890 closed bug (fixed)

Incorrect redundant import warning for type classes

Reported by: quchen Owned by:
Priority: normal Milestone: 8.0.1
Component: Compiler Version: 7.10.2
Keywords: Cc: hvr, osa1, quchen
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: warnings/should_compile/T10890, T10890_1, T10890_2
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D1257
Wiki Page:

Description (last modified by quchen)

Given the three files below, the import marked with (!!!) is reported as redundant. Upon deleting it, GHC reports that "has" is not a visible class method of BClass.

This came up in the context of the MFP (#10751), with the correspondences Base<->Control.Monad, Extends<->Control.Monad.Fail. We would like to have Control.Monad re-export Control.Monad.Fail.MonadFail without its (clashing) member "fail" for the time being. For future compatibility, some modules should implement MonadFail right now, requiring us to import Control.Monad.Fail explicitly in order to write an instance. Said procedure leads to the described import warning, which breaks the validation script for example.

-- Base.hs
module Base (AClass(..), BClass()) where

import Extends (BClass())

class AClass a where
    has :: a
-- Extends.hs
module Extends where

class BClass b where
    has :: b
-- UseSite.hs
module UseSite where

import Base
import Extends -- (!!!)

data Bar = Bar

instance AClass Bar where
    has = Bar

instance BClass Bar where
    has = Bar

We either get

UseSite.hs:12:5: error:
    ‘has’ is not a (visible) method of class ‘BClass’

or

UseSite.hs:4:1: warning:
    The import of ‘Extends’ is redundant
      except perhaps to import instances from ‘Extends’
    To import instances alone, use: import Extends()

Change History (15)

comment:1 Changed 2 years ago by quchen

Cc: hvr added

comment:2 Changed 2 years ago by quchen

Description: modified (diff)

comment:3 Changed 2 years ago by osa1

Cc: osa1 added

comment:4 Changed 2 years ago by osa1

Not sure how much this helps with debugging this, but I found out that when compiling this slightly modified program:

module Main where

import Base (AClass (has))
import Extends (BClass (has))

data Bar = Bar

instance AClass Bar where
  has = Bar

instance BClass Bar where
  has = Bar

GHC prints this:

Main.hs:3:1: warning:
    The import of ‘Base.has’ from module ‘Base’ is redundant

Main.hs:4:1: warning:
    The import of ‘Extends.has’ from module ‘Extends’ is redundant
Linking Main ...

This is a test case I generated while trying to figure out if GHC thinks both has are coming from the same module.

comment:5 Changed 2 years ago by osa1

This is awesome, just re-order imports like this:

import Extends
import Base

and GHC doesn't print the warning anymore :) I'm about to solve this case.

comment:6 Changed 2 years ago by osa1

(I removed my old message because I realized it was wrong, I was looking at wrong RdrEnv)

So here's the relevant part of the RdrEnv while generating these warnings:

   ihs :-> [has parent:AClass
              imported from ‘Base’ at Main.hs:3:1-11
              (and originally defined at Base.hs:6:3-10),
            has parent:BClass
              imported from ‘Extends’ at Main.hs:4:1-14
              (and originally defined at Extends.hs:4:5-12)],

Because we have has in this map, extendImportMap tries to figure out where it's imported from. extendImportMap calls lookupGRE_RdrName to see which modules are exporting that symbol, and of course lookupGRE_RdrName returns two modules, Extends and Base. But here that pattern is ignored, e.g. we don't handle the cases where lookupGRE_RdrName returns multiple items. So we end up ignoring has symbol and not considering it as used.

Last edited 2 years ago by osa1 (previous) (diff)

comment:7 Changed 2 years ago by osa1

Not sure how to proceed here but I have an experimental patch https://github.com/osa1/ghc/commit/d8647e1ae28be98554b4976ab1c4bc275b3c280b.

Does anyone have any ideas?

comment:8 Changed 2 years ago by quchen

Cc: quchen added

comment:9 Changed 2 years ago by osa1

I started to think like my experimental patch doesn't make things any worse, but fixes some of the cases. I'll test it a little bit more and probably open a Phab ticket.

comment:10 Changed 2 years ago by osa1

Differential Rev(s): D1257
Status: newpatch

Thanks to @quchen for realizing that my patch reduces to previous current version if there's only one imported RdrName with same name. I created a patch for this and I'll be adding tests soon.

Last edited 2 years ago by osa1 (previous) (diff)

comment:11 Changed 2 years ago by thomie

Differential Rev(s): D1257Phab:D1257

comment:12 Changed 2 years ago by Austin Seipp <austin@…>

In 1818b48/ghc:

Fix incorrect import warnings when methods with identical names are imported

Currently, GHC's warning generation code is assuming that a name (`RdrName`)
can be imported at most once. This is a correct assumption, because 1) it's OK
to import same names as long as we don't use any of them 2) when we use one of
them, GHC generates an error because it doesn't disambiguate it automatically.

But apparently the story is different with typeclass methods. If I import two
methods with same names, it's OK to use them in typeclass instance
declarations, because the context specifies which one to use. For example, this
is OK (where modules A and B define typeclasses A and B, both with a function
has),

    import A
    import B

    data Blah = Blah

    instance A Blah where
      has = Blah

    instance B Blah where
      has = Blah

But GHC's warning generator is not taking this into account, and so if I change
import list of this program to:

    import A (A (has))
    import B (B (has))

GHC is printing these warnings:

    Main.hs:5:1: Warning:
        The import of ‘A.has’ from module ‘A’ is redundant

    Main.hs:6:1: Warning:
        The import of ‘B.has’ from module ‘B’ is redundant

Why? Because warning generation code is _silently_ ignoring multiple symbols
with same names.

With this patch, GHC takes this into account. If there's only one name, then
this patch reduces to the previous version, that is, it works exactly the same
as current GHC (thanks goes to @quchen for realizing this).

Reviewed By: austin

Differential Revision: https://phabricator.haskell.org/D1257

GHC Trac Issues: #10890

comment:13 Changed 2 years ago by thoughtpolice

Resolution: fixed
Status: patchclosed

Thanks Ömer!

comment:14 Changed 2 years ago by Simon Peyton Jones <simonpj@…>

In 9376249/ghc:

Fix unused-import stuff in a better way

The fix for Trac #10890 in commit 1818b48, namely
   Fix incorrect import warnings when methods with identical names are imported
was wrong, as demonstrated by the new test T10890_2.  It suppressed
far too many warnings!

This patch fixes the original problem in a different way, by making
RdrName.greUsedRdrName a bit cleverer.

But this too is not really the Right Thing.  I think the Right Thing is
to store the /GRE/ in the tcg_used_rdrnames, not the /RdrName/.  That
would be a lot simpler and more direct.

But one step at a time.

comment:15 Changed 2 years ago by simonpj

Test Case: warnings/should_compile/T10890, T10890_1, T10890_2

I followed up with this

commit 3e94842d4d1258fbd6a1202bf74d41ce1d01c753
Author: Simon Peyton Jones <simonpj@microsoft.com>
Date:   Fri Oct 30 09:41:47 2015 +0000

    Record usage information using GlobalRdrElt
    
    This patch implements an improvment that I've wanted to do for ages, but
    never gotten around to.
    
    Unused imports are computed based on how imported entities occur (qualified,
    unqualified).   This info was accumulated in tcg_used_rdrnames :: Set RdrName.
    But that was a huge pain, and it got worse when we introduced duplicate
    record fields.
    
    The Right Thing is to record tcg_used_gres :: [GlobalRdrElt], which records
    the GRE *after* filtering with pickGREs.  See Note [GRE filtering] in RdrName.
    This is much, much bette.  This patch deletes quite a bit of code, and is
    conceptually much easier to follow.
    
    Hooray.  There should be no change in functionality.
Note: See TracTickets for help on using tickets.