Fix unused-import stuff in a better way
authorSimon Peyton Jones <simonpj@microsoft.com>
Wed, 28 Oct 2015 17:16:55 +0000 (17:16 +0000)
committerSimon Peyton Jones <simonpj@microsoft.com>
Fri, 30 Oct 2015 09:45:47 +0000 (09:45 +0000)
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.

compiler/basicTypes/RdrName.hs
compiler/rename/RnNames.hs
testsuite/tests/module/mod177.stderr
testsuite/tests/warnings/should_compile/T10890/T10890_2.hs [new file with mode: 0644]
testsuite/tests/warnings/should_compile/T10890/T10890_2.stderr [new file with mode: 0644]
testsuite/tests/warnings/should_compile/T10890/T10890_2A.hs [new file with mode: 0644]
testsuite/tests/warnings/should_compile/T10890/T10890_2B.hs [new file with mode: 0644]
testsuite/tests/warnings/should_compile/T10890/all.T

index 6917fea..2f10455 100644 (file)
@@ -59,7 +59,7 @@ module RdrName (
         pprNameProvenance,
         Parent(..),
         ImportSpec(..), ImpDeclSpec(..), ImpItemSpec(..),
-        importSpecLoc, importSpecModule, isExplicitItem
+        importSpecLoc, importSpecModule, isExplicitItem, bestImport
   ) where
 
 #include "HsVersions.h"
@@ -78,6 +78,7 @@ import Util
 import StaticFlags( opt_PprStyle_Debug )
 
 import Data.Data
+import Data.List( sortBy )
 
 {-
 ************************************************************************
@@ -593,15 +594,14 @@ greQualModName gre@(GRE { gre_name = name, gre_lcl = lcl, gre_imp = iss })
  | otherwise                              = pprPanic "greQualModName" (ppr gre)
 
 greUsedRdrName :: GlobalRdrElt -> RdrName
--- For imported things, return a RdrName to add to the
--- used-RdrName set, which is used to generate
--- unused-import-decl warnings
--- Return an Unqual if possible, otherwise any Qual
+-- For imported things, return a RdrName to add to the used-RdrName
+-- set, which is used to generate unused-import-decl warnings.
+-- Return a Qual RdrName if poss, so that identifies the most
+-- specific ImportSpec.  See Trac #10890 for some good examples.
 greUsedRdrName gre@GRE{ gre_name = name, gre_lcl = lcl, gre_imp = iss }
-  | lcl                               = Unqual occ
-  | not (all (is_qual . is_decl) iss) = Unqual occ
-  | (is:_) <- iss                     = Qual (is_as (is_decl is)) occ
-  | otherwise                         = pprPanic "greRdrName" (ppr name)
+  | lcl, Just mod <- nameModule_maybe name = Qual (moduleName mod)     occ
+  | not (null iss), is <- bestImport iss   = Qual (is_as (is_decl is)) occ
+  | otherwise                              = pprTrace "greUsedRdrName" (ppr gre) (Unqual occ)
   where
     occ = greOccName gre
 
@@ -924,7 +924,7 @@ shadowName env name
 {-
 ************************************************************************
 *                                                                      *
-                        Provenance
+                        ImportSpec
 *                                                                      *
 ************************************************************************
 -}
@@ -981,6 +981,31 @@ instance Eq ImpItemSpec where
 instance Ord ImpItemSpec where
    compare is1 is2 = is_iloc is1 `compare` is_iloc is2
 
+
+bestImport :: [ImportSpec] -> ImportSpec
+-- Given a non-empty bunch of ImportSpecs, return the one that
+-- imported the item most specficially (e.g. by name), using
+-- textually-first as a tie breaker. This is used when reporting
+-- redundant imports
+bestImport iss
+  = case sortBy best iss of
+      (is:_) -> is
+      []     -> pprPanic "bestImport" (ppr iss)
+  where
+    best :: ImportSpec -> ImportSpec -> Ordering
+    -- Less means better
+    best (ImpSpec { is_item = item1, is_decl = d1 })
+         (ImpSpec { is_item = item2, is_decl = d2 })
+      = best_item item1 item2 `thenCmp` (is_dloc d1 `compare` is_dloc d2)
+
+    best_item :: ImpItemSpec -> ImpItemSpec -> Ordering
+    best_item ImpAll ImpAll = EQ
+    best_item ImpAll (ImpSome {}) = GT
+    best_item (ImpSome {}) ImpAll = LT
+    best_item (ImpSome { is_explicit = e1 })
+              (ImpSome { is_explicit = e2 }) = e2 `compare` e1
+     -- False < True, so if e1 is explicit and e2 is not, we get LT
+
 unQualSpecOK :: ImportSpec -> Bool
 -- ^ Is in scope unqualified?
 unQualSpecOK is = not (is_qual (is_decl is))
@@ -1000,23 +1025,6 @@ isExplicitItem :: ImpItemSpec -> Bool
 isExplicitItem ImpAll                        = False
 isExplicitItem (ImpSome {is_explicit = exp}) = exp
 
-{-
--- Note [Comparing provenance]
--- Comparison of provenance is just used for grouping
--- error messages (in RnEnv.warnUnusedBinds)
-instance Eq Provenance where
-  p1 == p2 = case p1 `compare` p2 of EQ -> True; _ -> False
-
-instance Ord Provenance where
-   compare (Prov l1 i1) (Prov l2 i2)
-     = (l1 `compare` l2) `thenCmp` (i1 `cmp_is` i2)
-     where  -- See Note [Comparing provenance]
-       []     `cmp_is` []     = EQ
-       []     `cmp_is` _      = LT
-       (_:_)  `cmp_is` []     = GT
-       (i1:_) `cmp_is` (i2:_) = i1 `compare` i2
--}
-
 pprNameProvenance :: GlobalRdrElt -> SDoc
 -- ^ Print out one place where the name was define/imported
 -- (With -dppr-debug, print them all)
index 12f9024..e0b0683 100644 (file)
@@ -1548,8 +1548,8 @@ findImportUsage imports rdr_env rdrs sel_names
     import_usage :: ImportMap
     import_usage
       = foldr (extendImportMap_Field rdr_env)
-       (foldr (extendImportMap rdr_env) Map.empty rdrs)
-       (Set.elems sel_names)
+              (foldr (extendImportMap rdr_env) Map.empty rdrs)
+              (Set.elems sel_names)
 
     unused_decl decl@(L loc (ImportDecl { ideclHiding = imps }))
       = (decl, nubAvails used_avails, nameSetElems unused_imps)
@@ -1608,11 +1608,12 @@ extendImportMap_Field rdr_env (FieldOcc rdr sel) =
 -- the RdrName in that import decl's entry in the ImportMap
 extendImportMap_GRE :: [GlobalRdrElt] -> ImportMap -> ImportMap
 extendImportMap_GRE gres imp_map
-  = foldr recordRdrName imp_map nonLocalGREs
+  | (gre:_) <- gres
+  , not (isLocalGRE gre)   -- Should always be true, because we only need record
+                           -- uses of imported things, but that's not true yet
+   = add_imp gre (bestImport (gre_imp gre)) imp_map
+  | otherwise       = imp_map
   where
-    recordRdrName gre m = add_imp gre (bestImport (gre_imp gre)) m
-    nonLocalGREs = filter (not . gre_lcl) gres
-
     add_imp :: GlobalRdrElt -> ImportSpec -> ImportMap -> ImportMap
     add_imp gre (ImpSpec { is_decl = imp_decl_spec }) imp_map
       = Map.insertWith add decl_loc [avail] imp_map
@@ -1622,21 +1623,6 @@ extendImportMap_GRE gres imp_map
                    -- For srcSpanEnd see Note [The ImportMap]
         avail    = availFromGRE gre
 
-    bestImport :: [ImportSpec] -> ImportSpec
-    bestImport iss
-      = case partition isImpAll iss of
-          ([], imp_somes) -> textuallyFirst imp_somes
-          (imp_alls, _)   -> textuallyFirst imp_alls
-
-    textuallyFirst :: [ImportSpec] -> ImportSpec
-    textuallyFirst iss = case sortWith (is_dloc . is_decl) iss of
-                           []     -> pprPanic "textuallyFirst" (ppr iss)
-                           (is:_) -> is
-
-    isImpAll :: ImportSpec -> Bool
-    isImpAll (ImpSpec { is_item = ImpAll }) = True
-    isImpAll _other                         = False
-
 warnUnusedImport :: NameEnv (FieldLabelString, Name) -> ImportDeclUsage
                  -> RnM ()
 warnUnusedImport fld_env (L loc decl, used, unused)
index 21bf46c..d695eea 100644 (file)
@@ -1,5 +1,5 @@
 
-mod177.hs:4:1: Warning:
+mod177.hs:5:1: warning:
     The import of ‘Data.Maybe’ is redundant
       except perhaps to import instances from ‘Data.Maybe’
     To import instances alone, use: import Data.Maybe()
diff --git a/testsuite/tests/warnings/should_compile/T10890/T10890_2.hs b/testsuite/tests/warnings/should_compile/T10890/T10890_2.hs
new file mode 100644 (file)
index 0000000..7469639
--- /dev/null
@@ -0,0 +1,17 @@
+module T10890_2 where
+
+-- Previously GHC was printing this warning:
+--
+--   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
+
+import T10890_2A (A (has))
+import T10890_2B (B (has))
+
+data Blah = Blah
+
+instance A Blah where
+  has = Blah
diff --git a/testsuite/tests/warnings/should_compile/T10890/T10890_2.stderr b/testsuite/tests/warnings/should_compile/T10890/T10890_2.stderr
new file mode 100644 (file)
index 0000000..a693c47
--- /dev/null
@@ -0,0 +1,5 @@
+
+T10890_2.hs:12:1: warning:
+    The import of ‘T10890_2B’ is redundant
+      except perhaps to import instances from ‘T10890_2B’
+    To import instances alone, use: import T10890_2B()
diff --git a/testsuite/tests/warnings/should_compile/T10890/T10890_2A.hs b/testsuite/tests/warnings/should_compile/T10890/T10890_2A.hs
new file mode 100644 (file)
index 0000000..82ee73c
--- /dev/null
@@ -0,0 +1,4 @@
+module T10890_2A where
+
+class A a where
+  has :: a
diff --git a/testsuite/tests/warnings/should_compile/T10890/T10890_2B.hs b/testsuite/tests/warnings/should_compile/T10890/T10890_2B.hs
new file mode 100644 (file)
index 0000000..f143e30
--- /dev/null
@@ -0,0 +1,4 @@
+module T10890_2B where
+
+class B a where
+  has :: a
index 3e323f0..d5c6894 100644 (file)
@@ -5,3 +5,7 @@ test('T10890',
 test('T10890_1',
      extra_clean(['Base.o', 'Base.hi', 'Extends.o', 'Extends.hi']),
      multimod_compile, ['T10890_1', '-v0 -fwarn-unused-imports'])
+
+test('T10890_2',
+     extra_clean(['T10890_2A.o', 'T10890_2A.hi', 'T10890_2B.o', 'T10890_2B.hi']),
+     multimod_compile, ['T10890_2', '-v0 -fwarn-unused-imports'])