Skip to content
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

Missing Inferred Type in Tooltip when using Visible Type Applications #4017

Open
julmb opened this issue Jan 25, 2024 · 16 comments
Open

Missing Inferred Type in Tooltip when using Visible Type Applications #4017

julmb opened this issue Jan 25, 2024 · 16 comments
Labels
Hackathon This issue is suitable for hackathon sessions type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..

Comments

@julmb
Copy link

julmb commented Jan 25, 2024

Your environment

Which OS do you use?
ArchLinux

Which version of GHC do you use and how did you install it?
9.6.4 from ghcup

How is your project built (alternative: link to the project)?
cabal

Which LSP client (editor/plugin) do you use?
Code - OSS + vscode-haskell

Which version of HLS do you use and how did you install it?
2.6.0.0 from ghcup

Have you configured HLS in any way (especially: a hie.yaml file)?

"haskell.manageHLS": "PATH",
"haskell.plugin.importLens.codeActionsOn": false,
"haskell.plugin.class.codeActionsOn": false,
"haskell.plugin.semanticTokens.globalOn": true,

Steps to reproduce

Hovering over print in this example:

main = (print :: Int -> IO ()) 0

image

Hovering over print in this example:

main = print @Int 0

image

Expected behaviour

The inferred type for print is shown in the tooltip in both examples.

Actual behaviour

The inferred type for print is only shown with type annotations via ::, but not with visible type applications via @. The inferred type is shown correctly without any type annotations (together with a warning about defaulting the type variable to Integer).

Debug information

I could not find anything relevant in the HLS output log either when typing the expression or when hovering to produce the tooltip.

@julmb julmb added status: needs triage type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc.. labels Jan 25, 2024
@jhrcek
Copy link
Collaborator

jhrcek commented Jan 25, 2024

I find this curious and managed to reproduce the issue (with latest hls + ghc 9.4.8 on fedora)

I narrowed down somewhat that the issue is observable in the vicinity of the following code:
The types info is generated from the list of types retrieved from HieAST here:

When type application is not used, this usually contains 2 values (most general type and currently inferred type(?))
But when type application is present, this list of types is empty.
But I have no idea why. If only there was an easy way to pretty-print hie ast around the point of interest to see what's going on. I might look into this over the weekend just to learn more deeply how this stuff works..

@soulomoon
Copy link
Collaborator

soulomoon commented Jan 25, 2024

They are pretty dependent on the hieAst. Since we are hover only on the print.
The print @Int as a whole has types: [5]. Actually, the type follows the evidence veriable $dShow_xxx in the hieAst

we can see it from the HIeAst.

main = (print :: Int -> IO ()) 0 we have

[(0, ()), (1, IO), (2, Int), (3, Show), (4, 'Many),
 (5, HFunTy 4 2 1), (6, Type), (7, HTyVarTy a), (8, Show),
 (9, HFunTy 4 7 1), (10, HQualTy 8 9),
 (11, HForAllTy ((a, 6), [spec]) 10)]
 
            Node@hello.hs:5:9-13: Source: From source
                                  {(annotations: {(HsVar, HsExpr), (XExpr, HsExpr)}), 
                                   (types: [5, 11]), 
                                   (identifier info: {(name print,  Details:  Just 11 {usage}), 
                                                      (name $dShow_aRb,  Details:  Just 3 {usage of evidence variable})})}

main = print @Int 0 we have

[(0, ()), (1, IO), (2, Int), (3, Show), (4, 'Many),
 (5, HFunTy 4 2 1), (6, Type), (7, HTyVarTy a), (8, Show),
 (9, HFunTy 4 7 1), (10, HQualTy 8 9),
 (11, HForAllTy ((a, 6), [spec]) 10)]

        Node@hello.hs:5:8-17: Source: From source
                              {(annotations: {(HsAppType, HsExpr), (XExpr, HsExpr)}), 
                               (types: [5]), 
                               (identifier info: {(name $dShow_aGC,  Details:  Just 3 {usage of evidence variable})})}
                              
          Node@hello.hs:5:8-12: Source: From source
                                {(annotations: {(HsVar, HsExpr)}),  (types: [11]), 
                                 (identifier info: {(name print,  Details:  Just 11 {usage})})}
                                
          Node@hello.hs:5:15-17: Source: From source
                                 {(annotations: {(HsTyVar, HsType)}),  (types: []), 
                                  (identifier info: {(name Int,  Details:  Nothing {usage})})}

@jhrcek
Copy link
Collaborator

jhrcek commented Jan 25, 2024

Nice investigation @soulomoon I'd be interested how you got the printout of the relevant ranges.
And do you have to recompile/reinstall hls just to get the "print" statements in?

@soulomoon
Copy link
Collaborator

soulomoon commented Jan 25, 2024

You can setup ghc session and build the HieAst for a simple file @jhrcek , without the HLS.
It should speed up investigation on hieAst. I hope it helps.

    let filePath = "hello.hs"
    content <- Prelude.readFile filePath
    runGhc (Just libdir) $ do
        dflags <- getSessionDynFlags
        let dflags' = gopt_set dflags Opt_Haddock
        setSessionDynFlags dflags'
        target <- guessTarget filePath Nothing Nothing
        setTargets [target]
        load LoadAllTargets
        modSum <- getModSummary $ mkModuleName "Hello"
        parsedModule <- parseModule modSum
        typecheckedModule <- typecheckModule parsedModule
        let rms = fromJust $ renamedSource typecheckedModule
        let tcGblEnv = fst $ tm_internals_ typecheckedModule
        file <- mkHieFile modSum tcGblEnv rms
        let ast = snd $ List.head $ Map.toList $ getAsts $ hie_asts file
        let types = Arr.elems $ hie_types file
        liftIO $ Prelude.writeFile "hieAst.txt" $ showSDoc dflags' (ppr (Prelude.zip [0::Int ..] types)) ++ "\n" ++ showSDoc dflags' (ppr ast)

@jhrcek
Copy link
Collaborator

jhrcek commented Jan 26, 2024

Awesome, thanks!
I'll play around with this snippet.
I turned your snipped into runnable script for local poking around.
It required me to declare Outputable instance for HieType to make it work.
https://gist.github.com/jhrcek/9785e1e4ae909995d06a8810e1d6d98c

This seems pretty tedious. I wonder what do the hls devs do when they want to investigate "what's in the hie ast"? Isn't there a way to pretty print it for debugging purposes? You know, to debug things like "the info I was expecting is not in this ast node, where can I get it instead?"..

@soulomoon
Copy link
Collaborator

Awesome, thanks! I'll play around with this snippet. I turned your snipped into runnable script for local poking around. It required me to declare Outputable instance for HieType to make it work. https://gist.github.com/jhrcek/9785e1e4ae909995d06a8810e1d6d98c

This seems pretty tedious. I wonder what do the hls devs do when they want to investigate "what's in the hie ast"? Isn't there a way to pretty print it for debugging purposes? You know, to debug things like "the info I was expecting is not in this ast node, where can I get it instead?"..

I am not sure the others, I just going through the source code of hls or ghc. it's a bit off topic here, maybe we can discuss this in matrix.

@michaelpj
Copy link
Collaborator

So it seems like this is a bug in HIE file generation? I would expect the print node to still have a type even with the type application.

@jhrcek
Copy link
Collaborator

jhrcek commented Jan 26, 2024

I figured out a nicer way to show hie file (with full types reconstructed instead of with TypeIndexes) and it seems that in both cases the type info is associated with the print AST node. So not sure what leads to it not being rendered.
Will try to dig deeper over weekend.

Screenshot from 2024-01-26 13-15-59
Screenshot from 2024-01-26 13-15-01

@soulomoon
Copy link
Collaborator

soulomoon commented Jan 26, 2024

I figured out a nicer way to show hie file (with full types reconstructed instead of with TypeIndexes) and it seems that in both cases the type info is associated with the print AST node. So not sure what leads to it not being rendered. Will try to dig deeper over weekend.

This does look better.

Take a look at the evidence variables. They are the ones with more concrete types.

@jhrcek
Copy link
Collaborator

jhrcek commented Jan 28, 2024

Ok, I got to the bottom of this (what a fun learning experience this was 😄)

TL;DR: not showing types when TypeApplications is used seems like deliberate design - the type is not shown at the bottom of markdown block because it is already shown at the top (so having it repeated at the bottom would seem redundant)

More details:
As I commented above (refer to the screenshots) the HieAST does contain type information in both cases:

  1. when explicit type annotation is used there are 2 types in the print's AST node:
  • forall a. Show a => a -> IO ()
  • Int -> IO ()
  1. when TypeApplications is used there is only one type:
  • forall a. Show a => a -> IO ()

Then there is this dropEnd1 logic which basically drops the last type from the list in cases when there's only single identifier associated with the hovered-over symbol. That logic has been implemented back in 2020 in this PR: 5dfee4c#diff-91f773bae93bbf86035573f4dbffa3035c1689b1bb7e17bde5937bb1271c0131R104
I think the reason it's there is because that last type is actually rendered at the top of the markdown block, so it seems it just "works as designed".

I hope the following screenshots from my debugging sessions (where I just Debug.Traced the relevant expressions in the linked code) help to clarify the difference in behavior more then lengthy explanations:

Screenshot from 2024-01-28 07-42-37
Screenshot from 2024-01-28 07-42-04

@michaelpj
Copy link
Collaborator

Shame there's no comment explaining, maybe @wz1000 remembers and could write one?

It seems like the logic we want is to not reproduce the "original" type signature. It's not clear that we're really achieving that, though; in that case we should be filtering out the original type signature from the list rather than what we're doing now. I'd be somewhat in favour of just displaying everything (ideally with a title for that section of the hover explaining what it means!), at least that's consistent.

@julmb
Copy link
Author

julmb commented Jan 29, 2024

This explains how the HIE AST is shown in the user interface, but I am still wondering why the AST contains only the general type forall a. Show a => a -> IO () and not the instantiated one Int -> IO ().

I was expecting HLS to behave similarly to GHCI in this case:

ghci> :type print
print :: Show a => a -> IO ()
ghci> :type print @Int
print @Int :: Int -> IO ()

In fact, I would argue that showing the instantiated signature is more important than showing the original one (which can always be looked up in the documentation or source).

Additionally, the fact that the user is using visible type applications indicates that the type inference situation is tricky and these are exactly the situations in which the user really benefits from insight into which types their expressions ultimately got assigned.

@wz1000
Copy link
Collaborator

wz1000 commented Jan 29, 2024

This is just how the language works.

for print (1 :: Int), the typechecker has to elaborate print to print @Int. Since @Int wasn't written explicitly by the user, it is attached to the print.

If the user explicitly writes print @Int, then the typechecker doesn't need to do any elaboration, and the type of the span that print is used in is forall a. .... It would be incorrect to give that span the type Int -> ... because you can't apply a function Int -> ... to a type argument @Int.

I suppose what you want is the type of the expression print @Int, which is the feature request in #709, currently blocked on both GHC not providing us information in an easily accessible way and LSP not really supporting a reasonable way to expose this to the user.

@michaelpj
Copy link
Collaborator

I think there's maybe an issue in how people think about things indeed. I think it's quite natural to think that in print (1 :: Int) the typechecker uses the information from (1 :: Int) to infer that print "has" the type Int -> .... That's not what actually happens: print always has the type forall a . ..., and the compiler just infers the type argument to print. And indeed those come apart in the print @Int case. But I can see how it's confusing.

Perhaps one issue in the print (1 :: Int) case is that we report the type of (effectively) a non-existent expression, which is confusing. Is there a way we could say that that's what we're reporting? in an ideal world I imagine something like having this in the hover:

print @Int :: Int -> ...

@julmb
Copy link
Author

julmb commented Jan 29, 2024

Ah, I think I understand now, that actually makes a lot of sense, thank you for explaining.

So what I really want is to view the type of the expression print @Int, which is currently not possible. And the only reason it works in the annotated case is that the typechecker elaborates print to print @Int and since the @Int is implicit, by hovering over print, I get the type of the expression print @Int.

The situation in the annotated case seems a little confusing, but nonetheless I find it incredibly useful to be able to view the inferred/elaborated types that way.

Of course, ideally, we would like to be able to view the types of arbitrary expressions as proposed in #709. I have been teaching Haskell to a friend of mine and I think that this functionality would be very useful for people learning the language, as many things make so much more sense when you see the underlying types of the expressions involved.

@michaelpj
Copy link
Collaborator

Thinking about this, I think there are some things we could do. In particular, while showing people the type of an arbitrary expression is hard, we can make some guesses about what expressions people might want to see. In particular, when hovering over an identifier here are a few ideas for things we could show:

  1. The type of the identifier applied to any invisible arguments that it is supplied with. This would handle the print @Int case, and in general would mean we behave consistently whether invisible arguments are provided or not.
  2. The type of the identifier applied to each prefix of arguments that it is provided with. This is going to be overkill in a lot of situations where there are long applications but might be interesting.
  3. The type of the entire application that the identifier appears in. I think this would cover a bunch of the cases where people want the type of an expression. Perhaps more generally we could make a guess at the next "enclosing expression" and give the type of that?

@michaelpj michaelpj added the Hackathon This issue is suitable for hackathon sessions label May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hackathon This issue is suitable for hackathon sessions type: bug Something isn't right: doesn't work as intended, documentation is missing/outdated, etc..
Projects
Development

No branches or pull requests

5 participants