Skip to content

Enhancement: parenthesise compound completion snippets #1700

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

Closed
konn opened this issue Apr 10, 2021 · 4 comments · Fixed by #1709
Closed

Enhancement: parenthesise compound completion snippets #1700

konn opened this issue Apr 10, 2021 · 4 comments · Fixed by #1709
Labels

Comments

@konn
Copy link
Collaborator

konn commented Apr 10, 2021

Currently, HLS's completion snippet inserts types or kinds as-is.
For example,

foldl

gets completed to:

foldl b -> a -> b b t a

where b -> a -> b, b and t a are three separate snippet placeholders.
This makes it harder to detect boundaries of generated "placeholders" after a snippet is completed tentatively.
It might be good to parenthesise each placeholder when it is compound.
For example, the above must be at least:

foldl (b -> a -> b) b (t a)

or:

foldl (_ :: b -> a -> b) (_ :: b) (_ :: t a)

Hole-based approach works better as it generates well-formed programs; however, holes can make GHC significantly slower, it might be good to parenthesise plain placeholders (with types or kinds alone) could be enough.

OliverMadine pushed a commit to OliverMadine/haskell-language-server that referenced this issue Apr 11, 2021
@pepeiborra
Copy link
Collaborator

pepeiborra commented Apr 11, 2021

This is a bit tricky:

  1. Typed holes will only generate well-formed programs when ScopedTypeVariables is on and all the inserted types are in scope.
  2. The type will be uninstantiated, and cannot be instantiated since we collect completions for a module, not for a call site. So the type will possibly be too general and either lead to a type error or make the hole suggestions less useful.
  3. If I want to use the hole suggestions to fill in the snippet, I will have to clean up the parenthesis and the type annotation afterwards, leading to way more typing noise than right now.

Given 2 and 3, I don't think this will work so well in practice. But I might be wrong! The only way to find out is prototype it and have a play with some real code.

Note that untyped holes don't have any of these problems, so I think that's probably an unambiguous improvement over the current situation.

Also, about the current situation, I don't think that generating invalid programs is necessarily an issue - in languages with named parameters the snippet includes the name of the parameter and not its type, which usually leads to an invalid program too since the name is not a binding in scope. But that's not an issue because the user is expected to rewrite the snippets immediately and editors optimise for this by deleting the whole snippet as soon as the user starts typing, and keybinding TAB to move to the next snippet.

@konn
Copy link
Collaborator Author

konn commented Apr 11, 2021

I agree that we dont need to generate well-formed programs and the first simply parenthesised solution would do.

the user is expected to rewrite the snippets immediately and editors optimise for this by deleting the whole snippet as soon as the user starts typing, and keybinding TAB to move to the next snippet

I'm slightly against on this point from my experience. We might ponder what to fill in the placeholders for a while, and browses to other location or modules to get insights. Or, some placeholder gets somewhat complicated during the edition. We can easilly end up with exitting placeholders in such cases.
So, I still strongly think that it really makes sense simply to parenthesise each placeholders (w/o holes) to make argument boundary clear.

@konn
Copy link
Collaborator Author

konn commented Apr 11, 2021

To make my point clearer: the intention of this proposal is to make the argument boundaries in snippets clear. Well-formedness is just a bonus.
I opened this issue because I encountered the situation where I completed placeholders and having a difficulty to detect the boundary of placeholders for several times, in the real-world production code. Filling a placeholder with relatively large code while preventing the snippets to be completed accidentally poses some additional care, at least for me.
After all, this gets already +3 voted, so I thin we can say that it makes senses for some use-cases and some people find it useful.

@pepeiborra
Copy link
Collaborator

Just adding parentheses when needed is an unambiguous improvement - +1

@mergify mergify bot closed this as completed in #1709 Apr 13, 2021
mergify bot pushed a commit that referenced this issue Apr 13, 2021
* Enhancement: #1700

* changed bracketed typed hole completions to regular bracketed completions

* removed redudant comment

* removed whitespace

* Updated test cases to expect bracketed completions, Type constructors with no arguments no longer bracketed

* Updated hls tests for bracketed completions

* added missing ghcide/example/HLS file

* moved ghcide/examoke.HLS to ghcide/bench/example/HLS

* Restored ghcide/bench/example/HLS

* restored ghcide/bench/example/HLS

* removed unnecessary 'T.pack' (OverloadedStrings)

Co-authored-by: Oliver Madine <olliemadine@gmail.com>
Co-authored-by: Junyoung/Clare Jang <jjc9310@gmail.com>
Co-authored-by: Potato Hatsue <1793913507@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants