[GHC] #10117: Change the scheme for reporting redundant imports
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Keywords: | Operating System: Unknown/Multiple Architecture: | Type of failure: None/Unknown Unknown/Multiple | Blocked By: Test Case: | Related Tickets: Blocking: | Differential Revisions: | -------------------------------------+------------------------------------- The current scheme for reporting redundant imports is given on the [wiki:Commentary/Compiler/UnusedImports unused imports page]. [wiki:Migration/7.10#GHCsaysTheimportof...isredundant The 7.10 migration page] describes a hack to avoid redundant-import warnings during migration, without using CPP. The hack works for {{{ module Foo (Int, Word, Monoid(..)) where import Data.Monoid import Data.Word import Prelude }}} (the hack is putting `import Prelude` at the bottom). To understand why, read the specification on the [wiki:Commentary/Compiler/UnusedImports unused imports page]. But it fails for {{{ module Foo (Int, Word, Monoid(..)) where import Data.Monoid (Monoid(..)) import Data.Word (Word) import Prelude }}} because the specification treats itemised imports like `import Data.Word(Word)` differently to "import all" imports. The question: '''is there alternative spec (vs the one on the [wiki:Commentary/Compiler/UnusedImports unused imports page]) that would support the behaviour desired here, without having other undesirable consequences?''' -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Resolution: | Keywords: deprecate | warning Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Changes (by thomie): * cc: hvr (added) * keywords: => deprecate warning Comment: @hvr might be interested in this ticket. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Resolution: | Keywords: deprecate | warning Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by ezyang): Hello, I wrote a proposal and put it here: https://ghc.haskell.org/trac/ghc/wiki/Commentary/Compiler/RelaxedUnusedImpor... -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Resolution: | Keywords: deprecate | warning Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by thomie): A blogpost: http://www.yesodweb.com/blog/2016/05/are-unused-import- warnings-harmful And discussion: https://www.reddit.com/r/haskell/comments/4jvtmh/are_unused_import_warnings_... -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Resolution: | Keywords: deprecate | warning Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): I'm fine with making a change like this, if there's user consensus that it's a step forward. The wiki page says things like "arguably the 'redundant' import here is not a big deal, and we shouldn't complain too much about it". **But what is the ''principle'' here?** Is is this? * **Existing principle**: an import item should be reported as redundant if it can be deleted without changing the meaning of the program. * **New principle**: add "...unless it is the last explicit by-name import of an entity." Is that the intent? (It's not quite what the wiki page says because of subsumption... but try re-stating the principle, or modify subsumption.) I'd rather not try to support two different import-warning plans, controlled by a flag. That feels like overkill. I don't think anyone is suggesting that. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Resolution: | Keywords: deprecate | warning Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by ezyang): For backwards compatibility, new versions of a module may add new exports but will not remove exports. The problem with the existing unused imports warning is that warnings can arise even when an import is upgraded in a backwards-compatible way (a minor version bump). For example: {{{ import Prelude import Control.Applicative ((<*), pure) }}} Currently, the `pure` export from `Control.Applicative` is not redundant if `Prelude` does not export `pure`, but is redundant if it does. But the addition of `pure` to `Prelude`'s exports is OK for a minor version bump, which is what makes this warning "annoying". To rule these out, here is our principle: * **Backwards-compatible upgrades do not add warnings:** if a module is warning free, if we upgrade the minor version (as per the PVP) of any of its imports, the module should continue to be warning free. Here are warnings which are OK: * `import A` is not used at all. (For a module to be warning free, it must use some item from this import; but no matter how we upgrade other imports, that item will continue to be used.) * `import A (x)` is not used at all. (Similar.) * An entity is imported twice explicitly, e.g., `import A (x); import B (x)`. (This is always redundant (assuming no references to `A.x` or `B.x`), it doesn't matter what the exports of `A` and `B` are.) * Two imports are textually redundant, e.g., `import A; import A (x)`. (Once again, always redundant no matter what the exports.) Here are some warnings which are NOT ok: * `import A; import B (x)`, where `A` brings `x` into scope. An old version of `A` may not export `x` which would make this import-list warning free. * `import A; import B`, where `A` and `B` bring `x` into scope. Similar. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117#comment:6 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Resolution: | Keywords: deprecate | warning Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): You state a principle; that's good. Can you refine it a bit? * Rather than referring indirectly to the PVP, in addition say exactly what changes should not add warnings. I think the specific issue is this: if a module M compiles without warnings, and one of the modules it imports adds an extra export, then M should still compile without warnings. * Can you give a refined form of the old principle? Something like: an import item should be reported as redundant if it can be deleted without changing the meaning of the program, EXCEPT that a specific `import B(x)` is not reported as redundant if the only other way `x` is in scope is via whole-module imports like `import A` or `import A hiding...`. I think that's not quite right, but it'd be a helpful second step. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117#comment:7 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Resolution: | Keywords: deprecate | warning Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by ezyang): Your first bullet point is exactly right. To refine the old principle, I think this is good enough: an import is unused if it can be deleted without changing the meaning of the program, EXCEPT if the import explicitly brings x into scope (e.g., `import A (x)`), and the only other ways x was brought into scope are implicit (e.g., `import B`), AND each such implicit import is of a different module (e.g., `import A (x)` is still redundant if we `import A`). I've expanded the wikipage on this point, and I also streamlined the proposed algorithm so that we don't need a textual subsumption check anymore: I'm just a bit choosier about what import-items I mark as used when I hit a RdrName. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117#comment:8 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Resolution: | Keywords: deprecate | warning Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): OK, go for it! Which wiki page have you expanded? I don't see the above principles articulated explicitly. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Resolution: | Keywords: deprecate | warning Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by crockeea): I thought I might take a crack at #12067, but I'm afraid those changes might conflict with changes from this ticket. Can someone with a better knowledge of the codebase weigh in on this? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117#comment:10 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
#10117: Change the scheme for reporting redundant imports -------------------------------------+------------------------------------- Reporter: simonpj | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.4 Resolution: | Keywords: deprecate | warning Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect | Unknown/Multiple warning at compile-time | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by ezyang): I don't think so, but it depends on how #12067 is going to be fixed. If it's just that `addUsedDataCons` is not being called, or we are running the warning code too early, the changes should not conflict (since you'll be modifying the collection code, whereas this change modifies the reporting code.) -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/10117#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
participants (1)
-
GHC