Adding (\\) operator to import list adds it without surrounding it in parenthesis

[jhrcek]

Your environment

Run entered for haskell-language-server-wrapper(haskell-language-server-wrapper) Version 1.5.1.0, Git revision dcd2163 (2504 commits) x86_64 ghc-8.10.7
Current directory: /home/jhrcek/Devel/github.com/Holmusk/PI/backend
Operating system: linux
Arguments: ["--lsp"]
Cradle directory: /home/jhrcek/Devel/github.com/Holmusk/PI/backend
Cradle type: Stack

Tool versions found on the $PATH
cabal: 3.6.2.0
stack: 2.7.3
ghc: 8.10.7

Steps to reproduce

See this gif

Expected behaviour

The \\ operator should be surrounded in () after it's added to import list

Actual behaviour

The \\ operator is added without ()

[jneira]

Thanks for reporting the issue, i've tried to reproduce it in windows using ghc-9.0.1 and i was not able:

My test module looks like yours 🤔

[jhrcek]

Interesting.
Seeing that the thing to add to import list is extracted from GHC error via regex,
I tried to compile the demo module with various versions of GHC and each time I got the same output, each time without ()s

~/Tmp> ghcup set ghc 8.8.4
[ Info  ] GHC 8.8.4 successfully set as default version
~/Tmp> ghc Demo.hs 
[1 of 1] Compiling Demo             ( Demo.hs, Demo.o )

Demo.hs:3:12: error:
    • Variable not in scope: (\\) :: [Bool] -> [a0] -> t
    • Perhaps you want to add ‘\\’ to the import list in the import of
      ‘Data.List’ (Demo.hs:2:1-19).
  |
3 | x = [True] \\ []
  |            ^^
~/Tmp> ghcup set ghc 8.10.7
[ Info  ] GHC 8.10.7 successfully set as default version
~/Tmp> ghc Demo.hs 
[1 of 1] Compiling Demo             ( Demo.hs, Demo.o )

Demo.hs:3:12: error:
    • Variable not in scope: (\\) :: [Bool] -> [a0] -> t
    • Perhaps you want to add ‘\\’ to the import list in the import of
      ‘Data.List’ (Demo.hs:2:1-19).
  |
3 | x = [True] \\ []
  |            ^^
~/Tmp> ghcup set ghc 9.0.1
[ Info  ] GHC 9.0.1 successfully set as default version
~/Tmp> ghc Demo.hs 
[1 of 1] Compiling Demo             ( Demo.hs, Demo.o )

Demo.hs:3:12: error:
    • Variable not in scope: (\\) :: [Bool] -> [a0] -> t
    • Perhaps you want to add ‘\\’ to the import list in the import of
      ‘Data.List’ (Demo.hs:2:1-19).
  |
3 | x = [True] \\ []
  |            ^^
~/Tmp> ghcup set ghc 9.2.1
[ Info  ] GHC 9.2.1 successfully set as default version
~/Tmp> ghc Demo.hs 
[1 of 1] Compiling Demo             ( Demo.hs, Demo.o )

Demo.hs:3:12: error:
    • Variable not in scope: (\\) :: [Bool] -> [a0] -> t
    • Perhaps you want to add ‘\\’ to the import list in the import of
      ‘Data.List’ (Demo.hs:2:1-19).
  |
3 | x = [True] \\ []
  |            ^^

Can you please compile this module on command line and share the output that ghc gives you?

module Demo where
import Data.List ()
x = [True] \\ []

[jneira]

I am just aware i am on 1.5.0 so maybe it is a regression

EDIT: Just tried with 1.5.1 and it worked fine too

[jneira]

Can you please compile this module on command line and share the output that ghc gives you?

with ghc-9.0.1

src\Demo.hs:3:12: error:
    • Variable not in scope: (\\) :: [Bool] -> [a0] -> t
    • Perhaps you want to add ‘\\’ to the import list in the import of
      ‘Data.List’ (src\Demo.hs:2:1-19).
  |
3 | x = [True] \\ []
  |         

[jhrcek]

Thank you for checking.
Originally I installed hls by running stack ./install.hs hls-8.10.7 from dcd2163
Now I tried to build from 1.5.1 tag and I can still reproduce this issue (double checked the commit hash in hls output corresponds to what I built).
I'm on Linux.
Not sure where the difference in behavior is coming from.
Is it possible that there might be different behavior of regex matching on windows?
I'll try to look into it more deeply over the weekend.

[jhrcek]

I found where the issue is coming from.
In our codebase we're using relude and hiding base's Prelude.
Since relude doesn't reexport Data.List's (\\) operator, it's not present in the imports map, so the code ends up in this fallback branch which contains the operator not surrounded in parens. Should be simple fix by adding something like wrapOperatorInParens to the rendered name.

I'll see if I can come up with reasonably simple reproducer for tests 😅

[jhrcek]

Alright, I'm closer to the root cause, but not sure if there's good way to write a reproducer for tests.
To recap, the incorrect import happens because the code ends up in this fallback branch, because the module Data.List is not present in the ExportsMap.

Why is Data.List missing from the ExportsMap?
During construction of the ExportsMap the following error is returned on this line:

Failed to load interface for ‘Data.List’
There are files missing in the ‘base-noprelude-4.14.3.0’ package,

This most likely happens because base-noprelude doesn't actually contain any modules, it only re-exports modules from the base (all except Prelude).

In light of this I'm not sure what's the best way to apply the fix:

• add a workaround to the fallback branch to wrap operator in parentesis?
• fix the code constructing the ExportsMap so it can load the re-exported modules? (not sure if/how this could be done)
• fix the GHC error message so it wraps the operator in parens (probably not worth it because structured ghc messages are coming soon?)

[pepeiborra]

I think you have tracked down the bug. Amazing work!

We probably shouldn't be ignoring the second component returned by unitExposedModules:

Modules exposed by the unit. A module can be re-exported from another package. In this case, we indicate the module origin in the second parameter.

https://hackage.haskell.org/package/ghc-9.2.1/docs/GHC-Unit-Info.html#v:unitExposedModules

[michaelpj]

I think this got fixed.