don't crash when an unused operator import ends in .

[tscholak]

closes #2118
closes #2025

• added a test for when an unused import can be removed that ends in .
• resolved a crash (Prelude.head: empty list) in hls when an unused import ends in .
• add a test for when an unused qualified export ends in .

[tscholak]

@jneira @jhrcek @pepeiborra this is an attempt of a fix for both #2025 and #2118. At the moment I'm just avoiding a crash, but it now occurs to me that a true solution would also have to exhibit the correct expected behavior. However, I'm not familiar with the code I'm changing, and I have no idea what the expected behavior is.
What is supposed to happen here? Can someone please give me some input-output examples for this function?

[pepeiborra]

What is supposed to happen here? Can someone please give me some input-output examples for this function?

The ghcide test suite enforces the expected behaviour for code actions. Ideally this PR would extend it with an example that displays the crash, and the expected behaviour should follow from other examples

[tscholak]

@pepeiborra Hm, I'm sorry, but I don't think so. Clearly there are no examples in the test suite that covered the issue with (^.). That also means that there are no examples I can use to deduce the correct behavior given the occurrence of (^.) in the imports. So, I'm asking again. What is the expected output of this function for (^.) and variations thereof?

[tscholak]

@pepeiborra Perhaps I misunderstood. Are you saying that adding an example with (^.) to the hls integration tests is enough because if I were to implement this function incorrectly, then somehow the existing logic in ghcide would pick this up and make the test fail? If so, then I understand where you're coming from. However, working like this also makes for a very slow iteration loop.

[jhrcek]

When I was looking at this issue I came up with this minimal reproducer consisting of two modules:

module A where
(@.) = 0 -- Must have an operator whose name ends with '.'
a = 1 -- .. but also something else

{-# OPTIONS_GHC -Wunused-imports #-}
module B where
import A (a,(@.)) -- Must use something from module A, but not (@.) <- generating the code action to remove this import triggers the head error
x = a

A new test case consisting of these 2 modules should be added in this list of cases:

haskell-language-server/ghcide/test/exe/Main.hs

Line 933 in 28222e9

removeImportTests = testGroup "remove import actions"

The current implementation of code "Remove redundant imports" code action is parsing stuff from GHC's warnings.
One step of processing is "unqualifying" the symbols which is the source of this issue, called from rangesForBindingImport on this line.

I think you cannot actually have something qualified in import
like this:

import Some.Module ( x, y, ... {- there can never be qualified thing like B.something in this list if I understand haskell syntax well enough.. -})

So the fix might be as simple as removing the "unqualify" call from "modifyBinding" which is called from the function linked above.
Unfortunately the function modifyBinding is also used for processing unused exports, where I think you CAN have qualified imports. So the fix would have to take that into account.

[tscholak]

Alright, I than gather that the following unit tests for modifyBinding would be appropriate, at a minimum:

modifyBinding "something" = "something"
modifyBinding "A.something" = "something"
modifyBinding "+" = "(+)" -- <<< somehow arguments are already stripped of parentheses, is that true?
modifyBinding "A.(+)" = "(+)" -- <<< or would the lhs be "A.+"?
modifyBinding "^." = "(^.)" -- <<< as opposed to "" on the rhs, my current fix
modifyBinding "A.(^.)" = "(^.)"

[tscholak]

@jhrcek I added an integration test as you suggested.
What would be a good way of testing the other usage of modifyBinding, i.e., processing unused exports?

[tscholak]

btw, here's how I run the tests:

$ nix develop
$ cabal test ghcide --test-show-details=direct --test-option=--pattern --test-option="remove import actions" 

[jhrcek]

First of all thank you for working on this!
I don't have much advice to offer, beyond what I already commented.
I only contribute once in a few months and don't know the codebase very well :)

I'd approach the fix as follows:

1. make wrapOperatorInParens top level function
2. instead of calling modifyBinding in rangesForBindingImport, I'd only call wrapOperatorInParens there (note - this effectively removes the "unqualify" call when dealing with imports).
   because I think there's no way to actually have something qualified in the imports list:

module A (Prelude.head) where
        --     ^... you can have qualified things in exports, so the unqualify makes sense here.

import B (Qualified.Think.here)
          --      ^... there can never be qualified thing like this, so no point in calling unqualify
          -- I checked haskell report which specifies the syntax: https://www.haskell.org/onlinereport/haskell2010/haskellch5.html#x11-1010005.3

3. keep the call to modifyBinding in the smallerRangesForBindingExport unless you have an example how that is currently broken

[tscholak]

I tried to write a test that triggers modifyBinding but so far to no avail.
What code would result in a pattern match with L _ (IEThingWith _ _ _ _ _)?

smallerRangesForBindingExport :: [LIE GhcPs] -> String -> [Range]
smallerRangesForBindingExport lies b =
    concatMap (mapMaybe srcSpanToRange . ranges') lies
  where
    b' = modifyBinding b
    ranges' (L _ (IEThingWith _ thing _  inners labels))
      | showSDocUnsafe (ppr thing) == b' = []
      | otherwise =
          [ l' | L l' x <- inners, showSDocUnsafe (ppr x) == b'] ++
          [ l' | L l' x <- labels, showSDocUnsafe (ppr x) == b']
    ranges' _ = []

[jhrcek]

Grepping GHC code base, I found this: https://gitlab.haskell.org/ghc/ghc/-/blob/master/testsuite/tests/parser/should_compile/T14189.hs

Also see haddock: https://hackage.haskell.org/package/ghc-8.10.2/docs/GHC-Hs-ImpExp.html#v:IEThingWith

[tscholak]

Thanks! I also found the IEThingWith Note in https://hackage.haskell.org/package/ghc-8.10.2/docs/src/GHC.Hs.ImpExp.html#IE illuminating:

Note [IEThingWith]
~~~~~~~~~~~~~~~~~~

A definition like

    module M ( T(MkT, x) ) where
      data T = MkT { x :: Int }

gives rise to

    IEThingWith T [MkT] [FieldLabel "x" False x)]           (without DuplicateRecordFields)
    IEThingWith T [MkT] [FieldLabel "x" True $sel:x:MkT)]   (with    DuplicateRecordFields)

See Note [Representing fields in AvailInfo] in Avail for more details.

[tscholak]

Hm, I think we cannot have data constructors or classes ending in . without prepending them with the type keyword. But then we wouldn't have a IEThingWith anymore. Consider:

{-# LANGUAGE TypeOperators #-}
module B where
newtype (@.) = MkT {unT :: Int}
class (^.) a where

and

{-# LANGUAGE TypeOperators #-}
module A (type (B.@.) (MkT), type (B.^.)) where
import qualified B
a :: ()
a = ()

[jhrcek]

Optional suggestion: I just realized there's (.|.) operator in Data.Bits in base, so we could potentially shrink the reproducer to just one module if you fancy minimalism 😉

{-# OPTIONS_GHC -Wunused-imports #-}
module ModuleA where
import Data.Bits ((.|.), xor)
x=xor

[konn]

I tried this PR and confirmed that this also fixes #1705 (so it seems TH is irrelevant in #1705). Thank you for a great fix!

[jneira]

really a great work, three issues closed at once, solid tests after a thoroughfull investigation, congrats!