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

Make the eval plugin use the same default language extensions as ghci. #1596

Merged
merged 15 commits into from Jun 23, 2021
Merged

Make the eval plugin use the same default language extensions as ghci. #1596

merged 15 commits into from Jun 23, 2021

Conversation

fmehta
Copy link
Contributor

@fmehta fmehta commented Mar 19, 2021

This pull request aims to fix #1591

@jneira
Copy link
Member

jneira commented Mar 19, 2021

Many thanks!, a test case would be great to make visibly eval plugin is able to handle it and to avoid future changes breaks it

@jneira
Copy link
Member

jneira commented Mar 23, 2021

@fmehta feel free to ask for help if you need some guidance to write the test
The test suite for eval plugin is here: https://github.com/haskell/haskell-language-server/blob/master/plugins/hls-eval-plugin/test/Eval.hs

@fmehta
Copy link
Contributor Author

fmehta commented Mar 23, 2021

@jneira : Thanks a lot for following up on my progress and pointing me in the right direction. And yes, of course I need to include a test for this change (what was I thinking 🙄).

The reason for my delay: I was just wondering whether it even makes sense to enable this language extension at all. Maybe it makes sense to use exactly the same language extensions as ghc, since the generated comments will be part of the source code. Since there is no specification as to what the correct behaviour of the evals should be, I was wondering if you, or anyone else, could comment on what behaviour would be desirable.

I also notice that I cannot enable this language extension within the eval plugin for some reason (I guess only those that are listed in https://github.com/haskell/haskell-language-server/blob/master/plugins/hls-eval-plugin/README.md >>> :set -XScopedTypeVariables -XStandaloneDeriving -XDataKinds -XTypeOperators -XExplicitNamespaces ) can be set. I also see that there are other language extensions that are enabled for ghci (:showi language within ghci listsNoDatatypeContexts, ExtendedDefaultRules, NoMonomorphismRestriction, NondecreasingIndentation), whereas the following are enabled by default in the eval plugin:

evalExtensions =
    [ OverlappingInstances
    , UndecidableInstances
    , FlexibleInstances
    , IncoherentInstances
    , TupleSections
    ]

I wonder whether they should not be the same?

Do you (or anyone else) have any thoughts on all of this, or should I just move on and add the tests for this commit without getting too deep here?

Thanks, FM

@pepeiborra
Copy link
Collaborator

I don't see anything wrong with enabilng ExtendedDefaultRules by default, and all the other ones ghci enables by default.

The set of extensions that are enabled currently for the eval plugin seem arbitrary and should be removed. /cc @tittoassini

@Ailrun
Copy link
Member

Ailrun commented Mar 23, 2021

I believe some of the extensions are used in the printing code (which is executed in Eval plugin session) of Eval plugin, so I guess it's not so trivial to remove....

@pepeiborra
Copy link
Collaborator

I believe some of the extensions are used in the printing code (which is executed in Eval plugin session) of Eval plugin, so I guess it's not so trivial to remove....

@berberman removed that custom printing code recently

@Ailrun
Copy link
Member

Ailrun commented Mar 23, 2021

Oh, I missed that, then probably we are good to go.

@jneira
Copy link
Member

jneira commented Jun 2, 2021

@fmehta hi, do you still plan to continue working on this? thanks!

@jneira jneira added the status: needs info Not actionable, because there's missing information label Jun 2, 2021
@fmehta
Copy link
Contributor Author

fmehta commented Jun 21, 2021

Hi @jneira
Thanks for your help. I have committed the tests.
Sorry for the delay. I was trying to run them through the top level contribution instructions at https://github.com/haskell/haskell-language-server/blob/master/CONTRIBUTING.md which did not work for me. I only later realised that I could just run cabal test within the eval plugin.

Could you please review and merge my changes in case they are OK. I will then create and work on a second followup issue for the rest of the default ghci extensions as mentioned in #1596 (comment).

Followup issue: #1954

Thanks,
FM

@berberman
Copy link
Collaborator

Could you change the purpose of this PR, resolving #1954 together?

@fmehta
Copy link
Contributor Author

fmehta commented Jun 21, 2021

Hi @berberman

Could you change the purpose of this PR, resolving #1954 together?

#1954 needs some more work. In particular:

  • I am trying to see if it is possible to get the defaults from the ghci code programatically, instead of manually.
  • I am currently unsure if and how I can specify NoDatatypeContexts and NoMonomorphismRestriction, given that I can only find DatatypeContexts and MonomorphismRestriction within GHC.LanguageExtensions.Type.Extension.

(In case you have any pointers, please let me know.)

I therefore thought that I should start a new PR for the rest. But in case you think it is better to merge both together, I can do that as well. Just let me know.

Thanks
FM

@berberman
Copy link
Collaborator

There is no negative type of extension, so If you want to disable one, you should unset it. As far as I can tell, NoMonomorphismRestriction and ExtendedDefaultRules are configured manually in GHCi: https://github.com/ghc/ghc/blob/5abf59976c7335df760e5d8609d9488489478173/ghc/GHCi/UI.hs#L473-L483.

And evalExtensions finally comes to here:

df <- getSessionDynFlags
setInteractiveDynFlags $
(foldl xopt_set idflags evalExtensions)

@fmehta
Copy link
Contributor Author

fmehta commented Jun 21, 2021

Dear @berberman

Thanks for your input. I have added the fix for #1954 to this pull request as well.

A third thing that I noticed and did not get my head around was that it seems that ExplicitForAll is enabled by default in the eval plugin, but not in ghci.

-- >>> plus = (+)
-- >>> :t plus
-- plus :: forall a. Num a => a -> a -> a

I cannot find out where this is added. Do you have any idea?

FM

Reason: It was breaking GHC 8.6.2 tests.
@fmehta
Copy link
Contributor Author

fmehta commented Jun 22, 2021

Hi @berberman
Hi @jneira

It should work now. Maybe one of you need to kickstart the CI.

Best,
FM

@jneira jneira removed pr: needs tests status: needs info Not actionable, because there's missing information labels Jun 22, 2021
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

Looks great (love the comments in tests), many thanks

@fmehta fmehta changed the title Enable ExtendedDefaultRules for expressions in eval plugin (#1591) Make the eval plugin use the same default language extensions as ghci. Jun 22, 2021
@fmehta
Copy link
Contributor Author

fmehta commented Jun 22, 2021

Thanks @jneira!

@jneira
Copy link
Member

jneira commented Jun 22, 2021

@mergify rebase master

@mergify
Copy link
Contributor

mergify bot commented Jun 22, 2021

Command rebase master: success

Branch already up to date

Hey, I reacted but my real name is @Mergifyio

Co-authored-by: Potato Hatsue <berberman@yandex.com>
@berberman berberman added the merge me Label to trigger pull request merge label Jun 23, 2021
@mergify mergify bot merged commit 9f4220f into haskell:master Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
5 participants