Add sig inlay hints for where clause #4368
Conversation
Co-authored-by: fendor <[email protected]>
Yes! The latest |
|
Thanks, I was able to try it out and it works as advertised, very handy! Personally, I kind of dislike the inlay hints and would prefer them as lenses as inlay hints may make the code even less aligned for definitions with multiple function heads. See this example where at least I was briefly confused.
This is obviously unrelated to your PR and something we need to hash out in the future, I am mainly wondering what the default behaviour should be. |
Yes, that is indeed a problem. BTW, the main problem with code lens is that its position is not predictable and is determined by the editor. |
There was a problem hiding this comment.
This is looking pretty good, I think!
I wonder if we should roll in some other cases. I think the top-level code lenses should also be inlay hints (if that's supported), and we could maybe do let-bindings too, since I'm sure that's very similar to where-bindings?
|
|
||
| whereBindingType :: Maybe TcGblEnv -> Maybe HscEnv -> IO (Maybe WhereBindingTypeSigsResult) | ||
| whereBindingType (Just gblEnv) (Just hsc) = do | ||
| let wheres = findWhereQ (tcg_binds gblEnv) |
There was a problem hiding this comment.
I wonder if we should just go ahead and do lets at the same time? I would imagine it's very similar code?
| where | ||
| col = srcSpanStartCol . realSrcSpan | ||
|
|
||
| -- | Example: Find `a` and `b` from @(a,b) = (1,True)@ |
There was a problem hiding this comment.
This doesn't seem quite right, it's only finding one thing, not multiple things?
| nfp <- getNormalizedFilePathE uri | ||
| (WhereBindingTypeSigsResult (localBindings, sigMap), pm) | ||
| <- runActionE "InlayHint.GetWhereBindingTypeSigs" state $ useWithStaleE GetWhereBindingTypeSigs nfp | ||
| let bindingToInlayHints id sig = generateWhereInlayHints (T.pack $ printName (idName id)) (maybe "_" T.pack sig) |
There was a problem hiding this comment.
I think we should probably just omit things where we don't have a signature, rather than putting in _?
| , WhereBinding{..} <- bindings | ||
| , let bindingSpan = getSrcSpan (idName bindingId) | ||
| , let bindingSig = Map.lookup bindingId sigMap | ||
| , bindingSpan `notElem` sigSpans |
There was a problem hiding this comment.
This seems a little complicated? I wonder if we could just track the Ids that have signatures already?
There was a problem hiding this comment.
It seems that Sig GhcTc does not exists (not sure), that is to say, Id (IdP GhcTc) does not exist in HsLocalBinds, and currently only Name (IdP GhcRn) can be obtained from Sig GhcRn.
ref: https://hackage.haskell.org/package/ghc-9.10.1/docs/GHC-Hs-Binds.html#v:NValBinds
On the other hand, the pass of LHsBinds is GhcTc, and the Name obtained from bindingId is different from theName in Sig because their Unique is different.
ref: https://hackage.haskell.org/package/ghc-9.10.1/docs/GHC-Types-Unique.html#v:nonDetCmpUnique
If the Ids that have signatures is not easily available, I've tried comparing via OccName, but maybe SrcSpan is still better?
| , let bindingSig = Map.lookup bindingId sigMap | ||
| , bindingSpan `notElem` sigSpans | ||
| , Just bindingRange <- maybeToList $ toCurrentRange pm <$> srcSpanToRange bindingLoc | ||
| -- , Just bindingRange <- [srcSpanToRange bindingLoc] |
ghcide/test/exe/InlayHintTests.hs
Outdated
|
|
||
| editTest :: String -> TestTree | ||
| editTest file = | ||
| testWithDummyPlugin (file <> " (InlayHint EditText)") (mkIdeTestFs [copyDir "local-sig-lens"]) $ do |
There was a problem hiding this comment.
I guess the directory should be "local-sig-hint", perhaps?
| , testGroup "apply EditText" | ||
| [ hintTest "Simple" $ (@=?) | ||
| [defInlayHint { _position = Position 5 9 | ||
| , _label = InL ":: Bool" |
There was a problem hiding this comment.
I wonder if we should display the hints on the previous line rather than right after the binding. i.e. make the hint look like the edit currently does.
Advantages:
- It makes the effect of applying the hint align with how the hint looks, which is I think what we're supposed to do.
- It makes the situation less bad if the type is very long. That might already be annoying, but it could be even more annoying if it's right in the middle of something you're trying to read. Maybe we should look at what other language servers do?
There was a problem hiding this comment.
I don't think inlay hints can do this.
I found this issue: microsoft/language-server-protocol#1821.
AFAIK, standard inlay hints cannot be in virtual line.
It's more like a fixed position code lens?
Now let and where clause will show binding type in inlay hints
| findLetStmt :: ExprStmt GhcTc -> [HsLocalBinds GhcTc] | ||
| findLetStmt (LetStmt _ binds) = [binds] | ||
| -- TODO(jinser): why `foo <- expr` does not exist | ||
| -- findLetStmt (BindStmt _ _ expr) = findLetExpr (unLoc expr) |
There was a problem hiding this comment.
I'm wondering why this doesn't work, expr in BindStmt contains Var but not Bind.
ref #2966.