Fix incorrect redundant import warnings when methods with same names imported - T10890
ClosedPublic

Authored by osa1 on Sep 20 2015, 1:34 PM.

Details

Summary

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).

Test Plan
  • Add tests.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
osa1 updated this revision to Diff 4219.Sep 20 2015, 1:34 PM
osa1 retitled this revision from to Fix incorrect redundant import warnings when methods with same names imported - T10890.
osa1 updated this object.
osa1 edited the test plan for this revision. (Show Details)
osa1 updated the Trac tickets for this revision.
osa1 added a subscriber: quchen.
osa1 updated this object.Sep 20 2015, 1:40 PM
osa1 added a reviewer: simonpj.
osa1 updated this revision to Diff 4220.Sep 20 2015, 2:15 PM
  • add first test
osa1 updated this revision to Diff 4221.Sep 20 2015, 2:21 PM
osa1 edited edge metadata.
  • add second test
osa1 updated this revision to Diff 4224.Sep 20 2015, 3:14 PM
osa1 edited edge metadata.
  • remove empty stderr file
bgamari updated this object.Sep 21 2015, 5:22 AM
bgamari edited edge metadata.
bgamari edited edge metadata.EditedSep 21 2015, 5:31 AM

Does this still warn properly if I remove the B instance; e.g.,

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

data Blah = Blah

instance A Blah where
  has = Blah
osa1 added a comment.Sep 21 2015, 10:38 AM

No. In that case current GHC gives this 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’ is redundant
      except perhaps to import instances from ‘B’
    To import instances alone, use: import B()

With this patch it doesn't give any warnings. Arguably this patch makes it better, but surely we should fix this for real.

I did some hacking to understand how we can fix this. In the GlobalRdrEnv we record has as a name that can have parents A or B. In the warning generation code we should somehow check if it's used in A's context or B's context, but as far as I understand this is not possible right now, because we don't have any scoping etc. information, all we have is a list of RdrNames and GlobalRdrName.

I have some ideas but I need to think about this more.

austin requested changes to this revision.EditedSep 21 2015, 11:49 AM
austin edited edge metadata.

Looks OK, but see below for my refactoring. Also, please rework the comments on extendImportMap. This fix happens at a distance from the actual warning code, so I had to noodle about it a bit while sitting here, but I believe the intention behind it is this:

Before

  1. GHC looked up the RdrName in the GlobalRdrEnv and wanted a singleton [GlobalRdrElt] back
  2. If it got a single name back, it recorded that name in the ImportMap, under the name of the import it came from.
  3. GHC later checks if this name came from any other import (not recorded in the ImportMap), and if it did come from an import declaration that extendImportMap did not record, it throws a warning.
  4. In the above example, GHC returns two GlobalRdrElts as the RdrName occurs 'twice', as the typeclass context means the 'textual' names are same but the actual names are different. Therefore, it does not record any information in the import map, as a bug.
  5. Therefore, GHC throws a warning because it sees that there is no declaration recorded in the ImportMap as to where these GlobalRdrElts came from.

After

  1. Lookup the RdrName, and grab all results, because there may be more than one.
  2. Fold over every returned GlobalRdrElt
  3. Add every GRE to the ImportMap under its associated import declaration that it came from.
  4. Therefore, although one RdrName gives two GREs, both GREs properly have their imports recorded.

I think this is right and had to sit for about ten minutes to make sure. Please add a short version of this to the comments if you don't mind!

compiler/rename/RnNames.hs
1463–1464

The pattern gres <- lookupGRE_RdrName will always match now so this case is now dead code; and foldr will do the right thing anyway. I'd suggest a refactoring like:

extendImportMap rdr_env rdr imp_map
  = foldr recordRdrName imp_map nonLocalGREs
  where
    recordRdrName gre m = add_imp gre (bestImport (gre_imp gre)) m
    nonLocalGREs = filter (not . gre_lcl) (lookupGRE_RdrName rdr rdr_env)
This revision now requires changes to proceed.Sep 21 2015, 11:49 AM
osa1 updated this revision to Diff 4244.Sep 21 2015, 12:45 PM
osa1 edited edge metadata.
osa1 marked an inline comment as done.Sep 21 2015, 12:46 PM

@austin, do you have any ideas about the other problem? (we don't give a warning when one of the hases is not used)

austin accepted this revision.Oct 12 2015, 9:39 PM
austin edited edge metadata.
In D1257#35530, @osa1 wrote:

@austin, do you have any ideas about the other problem? (we don't give a warning when one of the hases is not used)

I noodled about this a bit but I don't have a solution OTTOMH I'm afraid. @bgamari might, but otherwise I don't think this should hold up this patch anymore.

This revision is now accepted and ready to land.Oct 12 2015, 9:39 PM
This revision was automatically updated to reflect the committed changes.