Automatic import of pattern synonyms does not import the correct name

[expipiplus1]

With files Foo.hs and Bar.hs

module Foo where

pattern P = ()

module Bar where

bar = P

in Bar.hs observe that the code actions available include import Foo(P), not import Foo(pattern P)

[expipiplus1]

associated pattern synonyms do work however they too suffer from https://github.com/haskell/haskell-language-server/issues/609#issue-746334129:

-- Foo.hs
module Foo
  ( D(P)
  ) where

data D = D
pattern P = D

[peterwicksstringfield]

Difficulty: that code action is based on this diagnostic from GHC:

app/Bar.hs:5:7: error:
    • Data constructor not in scope: P
    • Perhaps you want to add ‘P’ to the import list in the import of
      ‘Foo’ (app/Bar.hs:3:1-12).
  |
5 | bar = P
  |       ^

There isn't enough information in that diagnostic to deduce that P is a pattern synonym instead of a normal data constructor.

C.f. https://gitlab.haskell.org/ghc/ghc/-/issues/18466 which is about the usefulness of the underlying error message.

The code action currently uses only the diagnostic and the parsed source of the file you are importing into (module Bar in this case).

I think in order to distinguish data constructors from pattern synonyms we would need to get the parsed source for the module we are importing from (module Foo in this case), to get access to the HsDecl of the data constructor / pattern synonym (pattern P = ()) in this case). This information is not easily available in ghcide/Development/IDE/Plugin/CodeAction.

Maybe I'm missing something? But I think to fix this bug the code action would need access to IdeState?

[runeksvendsen]

Note that this is also an issue for the "Remove all redundant imports" action, where HLS removes the wrong imports when a pattern and type of the same name are in scope:

Before action is applied (pattern Ann is redundant):

import Nix.Expr.Types.Annotated (Ann, SrcSpan, pattern Ann)

type MyAnn f = Ann SrcSpan f

after action is applied:

import Nix.Expr.Types.Annotated (SrcSpan, pattern Ann)

type MyAnn f = Ann SrcSpan f

causing a "Not in scope"-error for Ann because it removes the Ann type from the import list rather than the Ann pattern.

[michaelpj]

Note that this makes me suspect that we probably also don't add the type namespace specifier when that's needed (which I think is only when the imported type is an operator?).

[dschrempf]

@fendor and I have hacked around on this issue a bit during MuniHac. We can reproduce both problems (missing qualifiers for pattern synonym and type operators). Files to reproduce the bug:

{-# LANGUAGE PatternSynonyms #-}
module Foo where

pattern P = ()

data a +: b = Plus a b

(+:) :: Int -> Int -> Int
(+:) = (+)

{-# LANGUAGE ExplicitNamespaces #-}
module Bar where

bar = P

foo :: Int +: Int
foo = undefined

For pattern synonyms, the bug is not easy to solve because the information is not readily available when the code action is constructed. The relevant function is called suggestNewImport in CodeAction.hs.

Possible solutions:

• We could look up the GHC name cache, which may be expensive.
• CodeActionArgs provides an ExportsMap, which could serve as a source of information but it only contains the OccName which does not contain information about whether the import is a pattern synonym, or not. Also this may be expensive in terms of memory consumption.

[kozak]

Maybe we could add getModulePatternSynonyms :: !(ModuleNameEnv (HashSet IdentInfo)) to ExportsMap. The pattern synonyms could be extracted:

1. from ModIface using

    getPatSynNames :: ModIface -> [OccName]
    getPatSynNames iface =
        [ occName ifName
          | (_, decl)  <- mi_decls iface
          , IfacePatSyn { ifName } <- [decl]
        ]

2. from ModGuts using mg_patsyns
3. HieDB. I am not sure if HieDB contains this info, but worst case we can leave it empty.

The extracted synonyms would neet be intersected with exports.

In the CodeAction we could check if the identifier we are trying to import refers to the pattern synonym and change the import suggestion to include pattern .

Note. We could also add a field IdentInfo instead of the map, but it seems wasteful.

[fendor]

That sounds like a good plan! Approach (1) sounds good to me.

[michaelpj]

Could we instead record the namespace? AFAIK the direction of travel is ExplicitNamespaces with data and type heralds, with the pattern herald possibly being deprecated. Could we try and add the namespace to IdentInfo?

[kozak]

Thanks for pointing the direction out. If my understanding is correct: ExplicitNamespaces adds https://hackage.haskell.org/package/ghc-9.12.1/docs/src/GHC.Hs.Binds.html#NamespaceSpecifier to the Parser, the specifier is then used in the Renamer to disambiguate from which scope an identifier is looked up. This results in a OccName which has a NameSpace: https://hackage.haskell.org/package/ghc-9.12.1/docs/src/GHC.Types.Name.Occurrence.html#NameSpace.

In such a case we wouldn't need to add anything to IdentInfo, we could extract the NameSpace viaoccNameSpace and check what specifier to add to the import.

We already have:

isDatacon :: IdentInfo -> Bool
isDatacon = isDataOcc . name

[michaelpj]

That sounds good. I think the tricky part then is just working out in what situation the namespace qualifier is needed. I don't know if we have the information to do that - hopefully we do! I'm actually not 100% sure what the rule is. I guess it's something like "if you are importing an item that 'looks like' a different kind of item by the normal rules, then you need a namespace qualifier". But probably there is a list of cases where this comes up in practice. I think it's mainly:

1. type operators need type
2. data constructors or patterns imported at the top-level (i.e. not as a child of a data type) need data