Fix Signature Help inside do Notation in GHC >= 9.10#4904
Conversation
Some nodes have there type info stored in nodes generated by ghc. In case no type info found in source node, fall back to traversing ghc generated nodes for types Signed-off-by: vidit-od <vidit894@gmail.com>
vidit-od
changed the title
do Notation
jian-lin
left a comment
jian-lin
left a comment
There was a problem hiding this comment.
Looks good. I'll do a more careful review later.
In the meantime, it would be good to add a test case for the issue.
Signed-off-by: vidit-od <vidit894@gmail.com>
|
@jian-lin I think this is ready for a review now |
jian-lin
changed the title
do Notation do Notation in GHC >= 9.10
Signed-off-by: vidit-od <vidit894@gmail.com>
vidit-od
force-pushed
the
Do-Signature-Traversal
branch
from
c891a41 to
9344a71
Compare
vidit-od
force-pushed
the
Do-Signature-Traversal
branch
from
6c39a93 to
20e9d05
Compare
If source -> identifier info -> details == Nothing, then we fallback to generated by ghc node and explore generated by ghc -> identifier info -> details Signed-off-by: vidit-od <vidit894@gmail.com>
|
@jian-lin I think this is waiting on a re-review from you |
| let mTypeOfName = identType identifierDetails <|> do | ||
| nodeInfo <- generatedNodeInfo hieAst | ||
| details <- M.lookup identifier (nodeIdentifiers nodeInfo) | ||
| identType details |
There was a problem hiding this comment.
@vidit-od Can we extract this do block into a function with some documentation?
|
Thanks for the reminder. I can have a look later today or tomorrow. |
jian-lin
left a comment
jian-lin
left a comment
There was a problem hiding this comment.
I need more time for further investigation, but I think I should post what I have now.
|
|
||
| module Ide.Plugin.SignatureHelp (descriptor) where | ||
|
|
||
| import Control.Applicative |
There was a problem hiding this comment.
nitpicking:
The existing import style of this file is explicit. I think it's a good idea to follow that.
| import Control.Applicative | |
| import Control.Applicative ((<|>)) |
| Nothing -> typesOfNode | ||
| Nothing -> | ||
| case typesOfNode of | ||
| [] -> typesOfGeneratedNode | ||
| ts -> ts |
There was a problem hiding this comment.
blocking:
This handles the fallback of typesOfNode. The fallback should be done in all cases. However, currently, it is only done in the Nothing case, but not the Just case.
I would handle the fallback in one place before allTypes and keep the allTypes code unchanged.
| -- types from the source NodeInfo | ||
| typesOfNode = case sourceNodeInfo hieAst of | ||
| Nothing -> [] | ||
| Just nodeInfo -> nodeType nodeInfo | ||
| -- fall back to generated NodeInfo when source has no types |
There was a problem hiding this comment.
The comment is kind of confusing: it comes too late because mTypeOfName also has the fallback logic.
I think we should move the comment to the top of the let block.
| let mTypeOfName = identType identifierDetails | ||
| let mTypeOfName = identType identifierDetails <|> do | ||
| nodeInfo <- generatedNodeInfo hieAst | ||
| details <- M.lookup identifier (nodeIdentifiers nodeInfo) |
There was a problem hiding this comment.
For some identifier, are Identifiers of SourceInfo and GeneratedInfo the same? In other words, does this M.lookup always return Nothing?
Needs to verify.
| details <- M.lookup identifier (nodeIdentifiers nodeInfo) | ||
| identType details | ||
| -- types from the source NodeInfo | ||
| typesOfNode = case sourceNodeInfo hieAst of |
There was a problem hiding this comment.
The fallback logic is:
- get
mTypeOfNamefromSourceInfoorGeneratedInfo - get
typesOfNodefromSourceInfoorGeneratedInfo
The current implementation uses two separated checks for mTypeOfName and typesOfNode. Alternatively, we can use only one check for both mTypeOfName and typesOfNode. Which way is better needs further investigation.
3 participants
Closes : #4769
As recommended, Type information of Some nodes were stored in GHC generated nodes.
This PR keeps the old pipeline and add a Fall back to traverse these generated nodes in case source node lack type information.
Attached image of signature help in cases where we did not get them before;
Prerequisite :
Major Finding credits : @jian-lin