haskell / haskell-language-server

Official haskell ide support via language server (LSP). Successor of ghcide & haskell-ide-engine.
Apache License 2.0
2.61k stars 351 forks source link

Code action not showing on multi-line imports with unused record field #4310

Open battermann opened 2 weeks ago

battermann commented 2 weeks ago

Steps to reproduce

Given ModuleB:

module ModuleB where
newtype B = B {b1 :: String}
x = 1

and ModuleA which has an unused record field import within a multi-line import statement:

module ModuleA where
import ModuleB
  ( B (b1),
    x,
  )
x' = x

Expected behaviour

I expect the code action for removing the unused record field: Remove B, B(b1) from import to be shown on selecting the single line with the unused import.

Actual behaviour

Instead, the code action does not show:

image

However, it shows when collecting the complete range of the import statement.

image

jhrcek commented 2 weeks ago

Thanks for the report. I managed to reproduce it using the following single file reproducer:

import Data.Monoid
   ( Sum(getSum)
   , getAp
   )
x = getAp

The great first step would be to add a failing test case to the test suite which would make it easy to isolate the root cause by rerunning the reproducer and adding Debug.Trace statements in various places.

I'll probably get to fixing this in the next few days but feel free to take a stab at it.

battermann commented 2 weeks ago

It looks like the range for the warning comes from GHC, so it is probably more a GHC issue than HLS:

src/ModuleA.hs:3:1: warning: [-Wunused-imports]
    The import of ‘Sum, Sum(getSum)’
    from module ‘Data.Monoid’ is redundant
  |
3 | import Data.Monoid
  | ^^^^^^^^^^^^^^^^^^...
jhrcek commented 2 weeks ago

I'm not sure if that's the issue. My theory: It might be some logic in HLS which does something like "try to calculate the range of this code action" - which because it fails to identify the correct range due to diagnostic message containing Sum(getSum) vs us looking for range of just getSum which is not found on its own in the AST, so it falls back to code range of the whole import declaration. Then since you have both the