Context:

• Implement signature help #4626 (comment)
• Implement signature help #4626 (comment)
• Implement signature help #4626 (comment)

@michaelpj

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.

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?

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.

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

Let's add some haddock explaining what these things are, and that e.g. only functions (?) will have argument documentation.

what these things are

You mean DocMap, TyThingMap and ArgDocMap? Or just ArgDocMap?

explaining what these things are

I am not sure what to explain here. For example, it is clear that DocMap is a map from name to doc.

What about this doc: 'DocMap' stores docs for declarations: functions, data types, instances, methods etc. 'ArgDocMap' stores docs for arguments, e.g., function arguments, method arguments.

only functions (?) will have argument documentation

Probably. Here is its docstring from GHC:

Docs for arguments. E.g. function arguments, method arguments.

We have a nice explanation of the approach for coming up with the parameter strings, but I think this part is also important and worth explaining? Especially since it's something we might want to try and improve later on.

The core of this function is these two lines. I think the function name itself already explains the core pretty well. Not sure what to add.

smallestContainingSatisfying (positionToSpan hiePath position) (nodeHasAnnotation ("HsApp", "HsExpr")) hieAst
 >>= extractInfo (positionToSpan hiePath position)

Oh, I though you meant the extractInfoFromSmallestContainingFunctionApplicationAst function. But maybe you mean the whole algorithm. I am not sure.

The algorithm is briefly described here. I can expand it a bit and add it to comments if that is what you mean.

Have you tried using this from an editor? I wonder if we can actually get away with using a stale HIE AST here. Because we're likely to be looking at a bit of the AST that has just been edited, so if we get a stale one we could be missing e.g. the last argument that got typed. So maybe we actually want to insist on an up-to-date HIE AST?

I have done many tests using Emacs eglot as the client. I also have done only a few tests in vscode. It seems to work well in both cases.

When experimenting the feature of automatically showing signature help when a space char in inserted in vscode, which is a future work and not implemented in this PR, I do notice that the stale value is not enough, I have to use useE to get the up-to-date value.

It's a little unclear to me at what point we exclude things that don't look like functions?

This is by nature of our algorithm.

Input: cursorPosition

1. hsAppNode = get the smallest HsApp AST node which contains cursorPostion with the help of extractInfoFromSmallestContainingFunctionApplicationAst
2. functionNode = find the left-most node of hsAppNode
3. try to get (functionName, functionTypes) from functionNode. Nothing means we fail to get the info.
4. count parameterIndex by traversing the hsAppNode tree from root to cursorPosition. Nothing means either cursorPosition is at functionNode or we encounter some AST nodes we do not yet know how to continue our traversal.

So step 1, 3 and 4 can exclude things that don't look like functions.

I guess it's fine for us to ignore the active parameter. I don't know if this is something that the user can control? Maybe we should try it in VSCode? Perhaps the user can switch between the parameters, and if they've done that we should leave the active parameter as what they selected? Unclear.

I think what you refer to as active parameter is activateSignature or oldActivateSignature.

Perhaps the user can switch between the parameters, and if they've done that we should leave the active parameter as what they selected?

Users cannot select active parameter. The active parameter is defined by the cursor position.

Users can select active signature and the feature of "remembering the previously selected signature when the cursor moves to another parameter" is implemented by reusing oldActivateSignature.

This isn't quite what I was suggesting, and I'm not sure it will work properly if we do have renaming inside types? e.g. does this work for forall a . a -> a? I would expect that we would get something like

• functionTypeString = "forall a1. a1 -> a1
• parameterTypeStrings = ["a", "a"]

And then the parameter type strings won't appear in the full type string? But you have a test for this, so I guess it does work? 🤔

Anyway, perhaps worth explaining if there are cases where you know this won't work, as I'm a bit confused!

You mean forall a . a -> a is as part of a larger type, right? We have a test case for it. That test case is marked as mkTestExpectFail and is linked to a future work.

If forall a . a -> a is not part of a larger type, it works. Many test cases can verify this.

This isn't quite what I was suggesting, and I'm not sure it will work properly if we do have renaming inside types

Renaming currently does not work. We have a test case to track it. It is also listed in the future work.

As we discussed in the meeting, I was trying to work on fixing it this week but have not finished it. I plan to continue working on fixing it after the deadline.

Why do we need to filter here? worth explaining!

The last type of typesOfNode may (always?) be the same as typeOfName. To avoid generating two identical signature helps, we do a filtering here.

This filtering is similar to this dropEnd1, I think.

Do you think it is worth adding a comment for it? I personally think the code already says this clearly.

What it does may be clear, but the reason is less clear. For example, in which cases are we generating the same types?

Since this filtering is similar to the one in atPoint, could we perhaps extract a common function?
Otherwise, just adding what you are saying in #4691 sounds like a good thing to write down!

in which cases are we generating the same types

I think we always generate the same types but I am not sure. I need to check how HIE files are generated to figure this out if it is necessary.

the reason is less clear

You mean why typesOfNode may (always?) contain typeOfName? Currently, I do not know. I need to check how HIE files are generated to figure this out if it is necessary.

Since this filtering is similar to the one in atPoint, could we perhaps extract a common function?

Sounds good.

I did not do so because I am not sure 1) if there always are duplicate types and 2) if the duplicate type is the last one of typesOfNode. If the answers of both questions are true, then we can use a more efficient implementation, i.e., dropEnd1, than filtering.

If you think filtering is efficient enough, I can extract a common function using filtering.

Otherwise, just adding what you are saying in #4691 sounds like a good thing to write down!

I think what I said in #4691 is two things.

One thing is that only doing dropEnd1 in some cases leads to a bug.

Another thing can be summarized well using my reply here:

The last type of typesOfNode may (always?) be the same as typeOfName. To avoid generating two identical signature helps, we do a filtering here.

So I guess you mean adding this sentence as a comment? I can do that if that is what you mean.