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

Implements :type [+v/+d] in Eval Plugin #361

Merged
merged 14 commits into from
Aug 29, 2020
Merged

Implements :type [+v/+d] in Eval Plugin #361

merged 14 commits into from
Aug 29, 2020

Conversation

konn
Copy link
Collaborator

@konn konn commented Aug 29, 2020

This implements :type command ala GHCi.
It also restructures the handling of GHCi-Like command and adds an exception for unknown command.

Examples

{-# LANGUAGE TypeApplications #-}
foo :: Show a => a -> String
foo = show

-- >>> :type foo @Int
-- foo @Int :: Int -> String

-- >>> :type +v foo @Int
-- foo @Int :: Show Int => Int -> String

-- >>> :type 40 + 2
-- 40 + 2 :: forall a. Num a => a

-- >>> :type +d 40 + 2
-- 40 + 2 :: Integer

-- >>> default (Word)
-- >>> :type +d 40 + 2
-- 40 + 2 :: Word

-- >>> :type +nope 12
-- parse error on input ‘+’

-- >>> :nooope 42
-- unknown command 'nooope'

Known limitations

Currently, :type +d ignores default declarations specified in the module, though it can handle defaults declared in the same >>> prompt group.

I once tried to obtain the module's defaulting declarations by applying tcg_default to the tm_internals_ of TypecheckedModule obtained by use_ TypeCheck and mappend it to the HscEnv using modifySession, but this doesn't change the situation.
Is there any ideas to resolve this, or we can just let +d ignore the module's default pragma?

For example, consider the following:

default (Int)
-- >>> :type +d 42

It is expected to say that 42 :: Int. But with the current implementation, it answers Integer,ignoring the module's default declaration:

default (Int)
-- >>> :type +d 42
-- 42 :: Integer

On the other hand, if we specify default inside the >>> prompt, it works as expected:

-- >>> default (Int)
-- >>> :type +d 42
-- 42 :: Int

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes requested, but otherwise looks good to me. Thank you!

I'm not sure I understand the issue with the current module. Is it the local default declarations, or the local pragmas, that are ignored? In any case, I don't see why either would be ignored. I would leave it as a known issue.

I note that these new commands (:kind and :type) involve very lightweight computation, and perhaps it would make sense to have a hover provider to show their evaluation.

import Control.Arrow (Arrow(second))
import GHC (Ghc)
import Type.Reflection (Typeable)
import GHC (exprType)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's reorganise these imports please

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed the commit that reorganises import list.
Anyway, is there any standard formatter and their configuration file in HLS, that could take care of import lists?
It seems ormolu doesn't reorganise import list.

src/Ide/Plugin/Eval.hs Outdated Show resolved Hide resolved
@konn
Copy link
Collaborator Author

konn commented Aug 29, 2020

I'm not sure I understand the issue with the current module. Is it the local default declarations, or the local pragmas, that are ignored? In any case, I don't see why either would be ignored. I would leave it as a known issue.

Oops, sorry for the lack of explanation. It just ignores the default declaration specified on the top of the module.
I've just added additional explanation on the top.

@konn
Copy link
Collaborator Author

konn commented Aug 29, 2020

I note that these new commands (:kind and :type) involve very lightweight computation, and perhaps it would make sense to have a hover provider to show their evaluation.

Surely that also helps a lot! Since the type of the symbol under the pointer is already shown in hover, it might be helpful to provide :kind and :kind! equivalents to hover.
On the other hand, :kind! involves some normalisation and that can take some time to fully evaluate (or can loop infinitely with UndecidableInstances), so it might be favourable to make normalise as an additional action which can be triggered from hovers.
If I have some time, I will open the separate pull request(s) for hovers!

@pepeiborra
Copy link
Collaborator

I'm not sure I understand the issue with the current module. Is it the local default declarations, or the local pragmas, that are ignored? In any case, I don't see why either would be ignored. I would leave it as a known issue.

Oops, sorry for the lack of explanation. It just ignores the default declaration specified on the top of the module.
I've just added additional explanation on the top.

It might be a good idea to add a test that shows the expected behaviour and tag it as "expected failure - known issue".

@konn
Copy link
Collaborator Author

konn commented Aug 29, 2020

@pepeiborra I've just added a test, which is deliberately ignored. Thanks for your suggestion!

@pepeiborra
Copy link
Collaborator

Why not expectFailBecause? In my experience ignoring a test is never a good idea, expect fail on the other hand will run the test. That way, if the issue becomes solved indirectly (e.g. upgrading to a newer ghcide version), we will find out upfront.

@konn
Copy link
Collaborator Author

konn commented Aug 29, 2020

It's just because I am not so familiar with tasty-expected-failure and just grepping files gave me plenty of use of ignoreTestBecause. I agree that the behaviour of expectFailBecause is more desirable, so I've just pushed the change to use it. Thanks again!

@pepeiborra pepeiborra merged commit ee0768d into haskell:master Aug 29, 2020
@konn konn deleted the eval-plugin-type-cmd branch August 29, 2020 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants