-
-
Notifications
You must be signed in to change notification settings - Fork 406
Implement signature help #4626
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
Implement signature help #4626
Conversation
TODO: - handle more cases - add successful and (currently failed) tests - show documentation
7a02359
to
9168b74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think really worth trying to start getting some tests in place!
plugins/hls-signature-help-plugin/src/Ide/Plugin/SignatureHelp.hs
Outdated
Show resolved
Hide resolved
plugins/hls-signature-help-plugin/src/Ide/Plugin/SignatureHelp.hs
Outdated
Show resolved
Hide resolved
plugins/hls-signature-help-plugin/src/Ide/Plugin/SignatureHelp.hs
Outdated
Show resolved
Hide resolved
plugins/hls-signature-help-plugin/src/Ide/Plugin/SignatureHelp.hs
Outdated
Show resolved
Hide resolved
plugins/hls-signature-help-plugin/src/Ide/Plugin/SignatureHelp.hs
Outdated
Show resolved
Hide resolved
plugins/hls-signature-help-plugin/src/Ide/Plugin/SignatureHelp.hs
Outdated
Show resolved
Hide resolved
plugins/hls-signature-help-plugin/src/Ide/Plugin/SignatureHelp.hs
Outdated
Show resolved
Hide resolved
67f5cdb
to
62fbccf
Compare
This is discussed with @fendor.
This improves performance. It also improves correctness because functionName, functionType and argumentNumber are extracted from the same AST.
Vscode and Emacs(eglot) seems to treat newline as a normal char.
See comment for a comparison with alternative methods.
This comment was marked as outdated.
This comment was marked as outdated.
findArgumentRanges :: Type -> [(UInt, UInt)] | ||
findArgumentRanges functionType = | ||
let functionTypeString = printOutputableOneLine functionType | ||
functionTypeStringLength = fromIntegral $ T.length functionTypeString | ||
splitFunctionTypes = filter notTypeConstraint $ splitFunTysIgnoringForAll functionType | ||
splitFunctionTypeStrings = printOutputableOneLine . fst <$> splitFunctionTypes | ||
-- reverse to avoid matching "a" of "forall a" in "forall a. a -> a" | ||
reversedRanges = | ||
drop 1 $ -- do not need the range of the result (last) type | ||
findArgumentStringRanges | ||
0 | ||
(T.reverse functionTypeString) | ||
(T.reverse <$> reverse splitFunctionTypeStrings) | ||
in reverse $ modifyRange functionTypeStringLength <$> reversedRanges | ||
where | ||
modifyRange functionTypeStringLength (start, end) = | ||
(functionTypeStringLength - end, functionTypeStringLength - start) | ||
|
||
{- | ||
The implemented method uses both structured type and unstructured type string. | ||
It provides good enough results and is easier to implement than alternative | ||
method 1 or 2. | ||
|
||
Alternative method 1: use only structured type | ||
This method is hard to implement because we need to duplicate some logic of 'ppr' for 'Type'. | ||
Some tricky cases are as follows: | ||
- 'Eq a => Num b -> c' is shown as '(Eq a, Numb) => c' | ||
- 'forall' can appear anywhere in a type when RankNTypes is enabled | ||
f :: forall a. Maybe a -> forall b. (a, b) -> b | ||
- '=>' can appear anywhere in a type | ||
g :: forall a b. Eq a => a -> Num b => b -> b | ||
- ppr the first argument type of '(a -> b) -> a -> b' is 'a -> b' (no parentheses) | ||
- 'forall' is not always shown | ||
|
||
Alternative method 2: use only unstructured type string | ||
This method is hard to implement because we need to parse the type string. | ||
Some tricky cases are as follows: | ||
- h :: forall a (m :: Type -> Type). Monad m => a -> m a | ||
-} | ||
findArgumentStringRanges :: UInt -> Text -> [Text] -> [(UInt, UInt)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context:
- Implement signature help #4626 (comment)
- Implement signature help #4626 (comment)
- Implement signature help #4626 (comment)
I implemented a mixed way which uses both structured type and type string. See the comment for the reason of doing it this way instead of using only structured type or only type string.
It seems to work pretty well. "3 out of 87 tests failed".
One failed test case is f :: Integer -> Num Integer => Integer -> Integer
. We matched Num Integer
as the first argument. This probably can be fixed by using regex.
Another similar failed test case is f :: forall l. l -> forall a. a -> a
. When we should highlight the argument l
, we highlight the l
for the second forall
.
Here is another failed test case.
f :: a -> forall a. a -> a
f = _
The printed type string for f
is f :: forall a. a -> forall a1. a1 -> a1
. This seems tricky to fix in the current implementation because it needs us to duplicate the renaming logic (the second forall a
becomes forall a1
) of ppr
.
This case only happens when RankNTypes
is used so I do not think it is very common.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering how bad it would be to just use the structured type and re-implement some pretty-printing logic. I think the only bit we'd need to duplicate would be the printing of ->
, =>
, and forall
? Maybe that's okay?
plugins/hls-signature-help-plugin/src/Ide/Plugin/SignatureHelp.hs
Outdated
Show resolved
Hide resolved
Another thing we should think about: type arguments.
Type arguments are kind of annoying because they might be optional (with normal foralls), or they might be required (with required type arguments). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: different hightlighting strategies for "reading code" and "writing code".
By "writing code", I mean the cursor is at the end of the function application and we intend to write more args to that function application.
Other cases are "reading code".
Currently, the highlight strategy for "reading code" works well.
For example, f x | y
will highlight y
. Note that in this case the cursor |
is in the subtree of a HsApp
AST node.
Currently, the highlight strategy for "writing code" is problematic.
Case 1: only the function itself is written f |
. We want to highlight the first arg of f
. Note that in this case there is no HsApp
node and so the cursor |
is not in the subtree of any HsApp
AST node.
Case 2: the function itself and at least 1 arg have been written f x |
(f :: a -> b -> c
). We want to highlight the second arg of f
. Note that in this case there are HsApp
nodes but the cursor |
is not in the subtree of any HsApp
AST node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds to me like your two strategies are the same? In both cases the strategy is "highlight the next argument after the cursor position (which may not exist)"?
Presumably in this situation we will highlight the second argument, though:
f one t|wo
Follows @fendor's suggestion to ignore some details in the doc string.
Newlines generated by neat-interpolation are sensitive[1] to platforms, which may make many tests of hls-cabal-plugin-tests fail on Windows. To keep this PR simple and focused, neat-interpolation usage outside of the signature help plugin is not changed. I plan to change that in a future PR. [1]: nikita-volkov/neat-interpolation#14
491eed2
to
9c97238
Compare
This basically changes indentation size from 4 to 2. Refs: haskell#4703
Follow LSP specification, we use parameter.
I think this PR is in a good shape now. All tests have passed. Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Could maybe do with a few more comments but I think we should merge this and make some issues for later improvements.
plugins/hls-signature-help-plugin/src/Ide/Plugin/SignatureHelp.hs
Outdated
Show resolved
Hide resolved
This improves readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's get this merged for the next HLS release :)
Introduction
Haskell language server (HLS) implements the server part of Language Server Protocol (LSP) to improve user experience of reading and writing Haskell. Signature help is a LSP language feature which shows function signatures and documentation when the cursor is inside a function application. It can also highlight part of the signature related to the current parameter the cursor is at. This is a useful feature and users want it.
This PR aims to implement the signature help feature for HLS.
My work
I have implemented most parts of the signature help feature for HLS in this PR. The last commit during GSoC is d0200a4.
The signature help feature works well in most cases, especially when a user is reading code.
Click for a video demo
hls-signature-help-gsoc-demo.webm
When I worked on this PR, a bug of other parts of HLS was found and I created a PR to fix it.
Besides HLS proper, I also created 2 PRs for
lsp-test
to add a needed function for tests.Click for more detailed work
2025-06-02 - 2025-06-08
commit: 7a54a1d
click for screenshot
2025-06-09 - 2025-07-13
commit: 9168b74
click for video demo
Screencast.From.2025-07-13.02-11-44.webm
2025-07-14 - 2025-07-20
2025-07-21 - 2025-07-27
maybe
with case for better readability (bf0b4d5)2025-07-28 - 2025-08-03
2025-08-04 - 2025-08-10
2025-08-11 - 2025-08-17
Show function documentation in signature help (a522e88)
Show function argument documentation in signature help (d826d06)
Do not error if doc is not available (35399e7)
click for screenshots
2025-08-18 - 2025-08-24
getSignatureHelp
tolsp-test
: Add signature help request to lsp-test lsp#621Any
becomesZonkAny 0
(c7931a2)2025-08-25 - 2025-08-31
2025-09-01 - 2025-09-02
Future work
forall
when the cursor is at a type application such as@Int
.OpApp
(operator application) ofHsApp
such asf x $ y|
.Acknowledgements
This is a Google Summer of Code (GSoC) project. Thanks to all the people involved. Special thanks to my mentors, @michaelpj and @fendor, for their help.
Closes #3598
Related to #2348 since we make mkDocMap expose the argument doc map