Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infix backticked holes get their backticks deleted once filled #1690

Closed
masaeedu opened this issue Apr 8, 2021 · 10 comments · Fixed by #1708
Closed

Infix backticked holes get their backticks deleted once filled #1690

masaeedu opened this issue Apr 8, 2021 · 10 comments · Fixed by #1708
Labels
component: ghcide type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..

Comments

@masaeedu
Copy link

masaeedu commented Apr 8, 2021

Steps to reproduce

Write this:

data A = A

foo :: A -> A -> A
foo A A = A

test :: A -> A -> A
test a1 a2 = a1 `_` a2

Open up the context menu on the hole and select "replace with foo".

Expected behaviour

The implementation should now look like:

test :: A -> A -> A
test a1 a2 = a1 `foo` a2

Actual behaviour

The implementation now looks like:

test :: A -> A -> A
test a1 a2 = a1 foo a2
@Ailrun Ailrun added component: wingman type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. labels Apr 8, 2021
@isovector
Copy link
Collaborator

My guess is that this is a bug in the exactprint machinery, that it's dropping the backtick annotations.

@isovector
Copy link
Collaborator

But this isn't Wingman; I don't provide any replace with _ code actions.

@Ailrun
Copy link
Member

Ailrun commented Apr 8, 2021

Oh, sorry. I somehow thought this is related with Wingman.

@isovector
Copy link
Collaborator

No worries, I did too!

@isovector
Copy link
Collaborator

I think this (and all analogous pieces of code) are the offender

(anns, val') <- annotate dflags needs_space val
modifyAnnsT $ mappend anns

IIUC, exactprint attaches annotations to source spans, which get overwritten when we replace a node. I think if we read the annotations off the node that used to be there, and attach them to the newly replaced node, this should fix a bunch of issues --- including that big ugly needsParensSpace thing (or at least, the space part of it.)

@isovector
Copy link
Collaborator

(all of this assumes that replace with _ uses ExactPrint)

@isovector
Copy link
Collaborator

Which it doesn't seem to, but I think that will fix some problems in WIngman anyway.

@jneira
Copy link
Member

jneira commented Apr 8, 2021

Maybe the cause is the code is using the ParseModule version without annotations?

-- | WARNING:
-- We currently parse the module both with and without Opt_Haddock, and
-- return the one with Haddocks if it -- succeeds. However, this may not work
-- for hlint or any client code that might need the parsed source with all
-- annotations, including comments.
-- For that use case you might want to use `getParsedModuleWithCommentsRule`
-- See https://github.com/haskell/ghcide/pull/350#discussion_r370878197
-- and https://github.com/mpickering/ghcide/pull/22#issuecomment-625070490
-- GHC wiki about: https://gitlab.haskell.org/ghc/ghc/-/wikis/api-annotations
getParsedModuleRule :: Rules ()
getParsedModuleRule =

If that is the case replacing it with:

-- | This rule provides a ParsedModule preserving all annotations,
-- including keywords, punctuation and comments.
-- So it is suitable for use cases where you need a perfect edit.
getParsedModuleWithCommentsRule :: Rules ()

could fix it

@berberman
Copy link
Collaborator

This code action is provided by ghcide, which depends on diagnostic messages, not parsed ast.

suggestFillHole :: Diagnostic -> [(T.Text, TextEdit)]
suggestFillHole Diagnostic{_range=_range,..}
| Just holeName <- extractHoleName _message
, (holeFits, refFits) <- processHoleSuggestions (T.lines _message)
= map (proposeHoleFit holeName False) holeFits
++ map (proposeHoleFit holeName True) refFits
| otherwise = []
where
extractHoleName = fmap head . flip matchRegexUnifySpaces "Found hole: ([^ ]*)"
proposeHoleFit holeName parenthise name =
( "replace " <> holeName <> " with " <> name
, TextEdit _range $ if parenthise then parens name else name)
parens x = "(" <> x <> ")"

@jneira
Copy link
Member

jneira commented Apr 8, 2021

So updating the regexp should fix it, it seems it could be a good first issue

OliverMadine pushed a commit to OliverMadine/haskell-language-server that referenced this issue Apr 11, 2021
@mergify mergify bot closed this as completed in #1708 Apr 13, 2021
mergify bot pushed a commit that referenced this issue Apr 13, 2021
…1708)

* Fix: #1690 - Infix typed holes are now filled using infix notation

* fix: postfix hole uses postfix notation of infix operator

Co-authored-by: Oliver Madine <[email protected]>
Co-authored-by: Javier Neira <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ghcide type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..
Projects
None yet
5 participants