Opened 4 years ago

Closed 4 years ago

#4977 closed feature request (fixed)

Warning about unqualified implicit imports

Reported by: Lemming Owned by: igloo
Priority: highest Milestone: 7.2.1
Component: Compiler Version: 7.0.1
Keywords: Cc: ghc@…, johan.tibell@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

I would like to have a GHC option like -fwarn-unqualified-imports that warns about those kinds of imports that require to have A.B.C.* constraints for PVP-compliant packages. Namely, GHC should warn about implicit unqualified imports like

import Very.Special.Module
import Another.Important.Module hiding (open, close, )

http://www.haskell.org/haskellwiki/Package_versioning_policy http://www.haskell.org/haskellwiki/Import_modules_properly http://www.reddit.com/r/haskell_proposals/comments/edu3f/discourage_unqualified_open_imports/

Attachments (1)

T4977.patch (95.7 KB) - added by simonpj 4 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 4 years ago by tibbe

  • Cc johan.tibell@… added

comment:2 follow-up: Changed 4 years ago by simonpj

I'm afraid I don't understand the specification. Could you write a specification of the feature you want?

Simon

comment:3 in reply to: ↑ 2 Changed 4 years ago by Lemming

My attempt for a specification:

Add the option -fwarn-unqualified-imports that make GHC do the following: Whenever GHC encounters import statements of one of the following types

module Main where

import Top.A
import Top.A hiding (x, y, z)
import Top.A as A
import Top.A as A hiding (x, y, z)

it shall emit a warning like:

Main.hs:3:0:
    Warning: Unqualified implicit import of module Top.A
             which risks name collisions if identifiers are added to Top.A in the future.
             Better write
                import qualified Top.A as A
             or
                import Top.A (x, y, z, ...)

Ideally the second suggested import statement (import Top.A (...)) contains all identifiers from Top.A that are actually used in Main. Then the suggested import statement can be copied into Main and Main can be compiled without further modifications.

The option -Wall may not include this warning, since some people prefer those kinds of imports.

comment:4 follow-up: Changed 4 years ago by simonpj

Hmm. So what triggers the warning? All your examples are for a module Top.A; would the warning happen if I say import M? It seems odd if the module name is influential... and these days almost all module names have dots in them.

Maybe you meant to warn of any import that brings into scope a bunch of unqualified names, unless they are explicitly enumerated. What about

import Top.A( T(..) )

Is that warned about too? What is the general rule?

Simon

comment:5 in reply to: ↑ 4 Changed 4 years ago by Lemming

Replying to simonpj:

Hmm. So what triggers the warning? All your examples are for a module Top.A; would the warning happen if I say import M?

The module name Top.A is irrelevant. 'import M' shall also trigger the warning.

Maybe you meant to warn of any import that brings into scope a bunch of unqualified names, unless they are explicitly enumerated. What about

import Top.A( T(..) )

Is that warned about too?

Ah, I missed that. Yes, importing constructors and class methods unqualified and without explicit enumeration should also cause a warning.

What is the general rule?

My intention is to warn about all import statements, that can cause the module to break, if there is something _added_ to the imported module. Removals and modifications of the API of imported modules are out of scope for any warning, since we cannot protect ourselves against such changes via the choice of import statements.

I brought the PVP into the discussion, since if you import module M with qualification or with explicit identifier list and M is part of the package p and p follows the Package Versioning Policy, then you can declare

Build-Depends: p == A.B.*

in your Cabal file. If you use imports that bring a bunch of identifiers into scope without qualification and explicit enumeration, then you have to restrict the version range to

Build-Depends: p == A.B.C.*

Thus this warning could promote the PVP and could encourage people to import in a way that allows larger import version ranges and thus reduce the number of updates to packages.

comment:6 follow-up: Changed 4 years ago by simonmar

Doesn't -fwarn-missing-import-lists do what you want?

comment:7 in reply to: ↑ 6 Changed 4 years ago by Lemming

Replying to simonmar:

Doesn't -fwarn-missing-import-lists do what you want?

I did not know about this option, since it is not listed in:

http://www.haskell.org/ghc/docs/7.0-latest/html/users_guide/options-sanity.html

Now that you mention it, I could find it in http://www.haskell.org/ghc/docs/7.0-latest/html/users_guide/flag-reference.html#id3106416 and its short description sounds, like it could be useful for me, but I am not entirely sure.

Thus at least I'd like to read a more detailed description of that flag.

comment:8 Changed 4 years ago by simonmar

I believe it does exactly what you want - that is, it warns about

import M
import M hiding (x,y)

but not

import M (x,y)

The documentation is missing though, so we should fix that before closing this ticket.

comment:9 Changed 4 years ago by igloo

  • Milestone set to 7.2.1
  • Priority changed from normal to highest

comment:10 Changed 4 years ago by simonpj

  • Owner set to igloo

In an odd moment I wrote the documentation. I've also changed the behaviour slightly to warn only for unqualified imports. I agree with Lemming that this is the case we really want to warn about.

I attach the patch because I can't make the documentation on my laptop and I don't want to break the built by a formatting error. Ian can you check and apply?

Simon

Changed 4 years ago by simonpj

comment:11 Changed 4 years ago by igloo

  • Resolution set to fixed
  • Status changed from new to closed

Applied, thanks!

Note: See TracTickets for help on using tickets.