Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork399
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -764,6 +770,10 @@ instance PluginRequestMethod Method_TextDocumentDocumentSymbol where | |||
| si = SymbolInformation name' (ds ^. L.kind) Nothing parent (ds ^. L.deprecated) loc | |||
| in [si] <> children' | |||
| -- TODO(@linj) is this correct? | |||
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 this is reasonable. ReallycombineResponses should have a way to return an error. There are a bunch of methods where it really only makes sense if we have one handler.
Wecould try to combine responses: we would combine the signatures, and so long as only one of them indicated an active signature it would be okay. But that's a bit sketchy and I doubt we'll have several anyway!
TODO:- handle more cases- add successful and (currently failed) tests- show documentation
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!
| case fromCurrentPosition positionMapping position of | ||
| Nothing -> pure Nothing | ||
| Just oldPosition -> do | ||
| let functionName = |
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.
you probably want to do all these in one go, this looks like it will traverse the AST three times! I think you can just do one call toextractInfoFromSmallestContainingFunctionApplicationAst and get all the pieces together though?
| if nodeHasAnnotation ("HsVar", "HsExpr") hieAst | ||
| then | ||
| case M.elems $ M.filter isUse $ getSourceNodeIds hieAst of | ||
| [identifierDetails] -> identType identifierDetails >>= (prettyType >>> Just) |
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 suggest not printing the type here and instead returning the type itself. That would mean that when you're doingmkArguments you can pattern match on the type itself, not on the string representation. Then you don't need to worry about e.g. whether it prints on multiple lines or whatever.
| (Just $ InL argumentNumber) | ||
| -- TODO(@linj) can type string be a multi-line string? | ||
| mkArguments :: UInt -> Text -> [ParameterInformation] |
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 this would be a lot simpler if you were just looking at the type itself!
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.
Although hmm, the offsets are into the string 😱 So maybe you would need to incrementally build up theParameterInformations and the overalllabel, but that would probably still be nicer?
| type Annotation = (FastStringCompat, FastStringCompat) | ||
| nodeHasAnnotation :: Annotation -> HieAST a -> Bool | ||
| nodeHasAnnotation annotation = sourceNodeInfo >>> maybe False (isAnnotationInNodeInfo annotation) |
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.
nit: I think it tends to be easier to read a simple case expression than one of the pattern-matching functions likemaybe. I usually only usemaybe if it's partially-applied.
You can still avoid naming theHieAst parameter if you want usingLambdaCase.
| nodeHasAnnotation :: Annotation -> HieAST a -> Bool | ||
| nodeHasAnnotation annotation = sourceNodeInfo >>> maybe False (isAnnotationInNodeInfo annotation) | ||
| -- TODO(@linj): the left most node may not be the function node. example: (if True then f else g) x |
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.
write a test!
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.
this is a great example btw. In this case I think we would have to give no signature help, I think, since it's only dynamically known which function we're getting.
| if nodeHasAnnotation ("HsVar", "HsExpr") hieAst | ||
| then | ||
| case mapMaybe extractName $ M.keys $ M.filter isUse $ getSourceNodeIds hieAst of | ||
| [name] -> Just name -- TODO(@linj) will there be more than one name? |
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 not 100% sure when/if this can happen. I think it might happen if we're looking at a typeclass method, in which case we could get the generic typeclass method and the specific one perhaps? That would be a good use-case for multilple signatures, e.g. if we have
foldMap @[] f ^then we might want to see both the genericFoldable signature and the one for the list instance.
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 my general suggestion would be: let's treat possibly-multiple node annotations as producing possibly-multiple signatures.
| (Just $ InL argumentNumber) | ||
| ] | ||
| (Just 0) | ||
| (Just $ InL argumentNumber) |
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.
do we know if this is 0 or 1-indexed? specification doesn't say (of course 🙄 ) so I guess we'll have to test it in vscode to see
Uh oh!
There was an error while loading.Please reload this page.
Closes#3598
I will update progress here.
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