Extended Eval Plugin #438

tittoassini · 2020-09-27T20:47:03Z

A superset of the current Eval plugin.

It adds the following features:

Tests in both plain comments and Haddock comments
For Haddock comments: shows differences between latest and previous result
Setup section, executed before every test
Execution of a section/group of tests at the time
Property testing
Setup of GHC extensions

Does not yet support CPP or LHS modules.

pepeiborra · 2020-09-27T21:38:42Z

This looks awesome, but I won't be able to review it until later this week (or possibly next week)

tittoassini · 2020-09-28T09:37:31Z

Take your time.

I was agonisingly slow in writing it, I won't complain if you are a bit slow in reviewing it :-)

test/functional/Eval.hs

plugins/default/src/Ide/Plugin/Eval/GHC.hs

plugins/default/src/Ide/Plugin/Eval/CodeLens.hs

tittoassini · 2020-10-08T21:27:27Z

The failed test on ghc-8.10.2 is unrelated to this PR.

jneira · 2020-10-08T21:44:29Z

@tittoassini yeah, could you rebase master to include the fix (c1d47b9)?

plugins/default/src/Ide/Plugin/Eval/Types.hs

src/Ide/Version.hs

pepeiborra · 2020-10-08T22:49:58Z

plugins/default/src/Ide/Plugin/Eval/GHC.hs

+  gcatch
+    -- gStrictTry op = MC.catch
+    (op >>= \v -> return $! Right $! v)
+    (\(err :: SomeException) -> return $! Left $! show $! err)


What are you trying to accomplish here?
Without evaluate I'm not sure all the bangs make much difference

I have redefined it as:

gStrictTry :: ExceptionMonad m => m b -> m (Either String b)
gStrictTry op =
gcatch
(op >>= fmap Right . gevaluate)
showErr

gevaluate :: MonadIO m => a -> m a
gevaluate = liftIO . evaluate

showErr :: Monad m => SomeException -> m (Either String b)
showErr = return . Left . show

pepeiborra · 2020-10-08T22:51:08Z

plugins/default/src/Ide/Plugin/Eval/Parse/Token.hs

+[]
+-}
+tokens :: [String] -> [Loc TokenS]
+tokens = concat . map (\(l, vs) -> map (Located l) vs) . zip [0 ..] . reverse . snd . foldl' next (InCode, [])


without a strict accumulator, parsing errors will be concealed by laziness

tokens should never actually return any parsing error.

it is just a way of classifying input lines by type and at least one of the types (codeLine) always succeeds.

plugins/default/src/Ide/Plugin/Eval/CodeLens.hs

pepeiborra · 2020-10-08T23:05:39Z

plugins/default/src/Ide/Plugin/Eval/CodeLens.hs

+  -- Use the preprocessor to normalise CPP/LHS files, but we still get an error.
+  -- fp <- handleMaybe "uri" $ uriToFilePath' uri
+  -- let nfp = toNormalizedFilePath' fp
+  -- session :: HscEnvEq <-
+  --   liftIO $
+  --     runAction "runEvalCmd.ghcSession" state $
+  --       use_ GhcSession nfp
+  -- Right (ppContent, dflags) <- liftIO $ evalGhcEnv (hscEnv session) $ runExceptT $ preprocessor fp (Just $ textToStringBuffer content)
+  -- let text = decodeUtf8 $ stringBufferToByteString ppContent
+  -- liftIO $ dbg "PREPROCESSED CONTENT" text


This should work. What error do you get?

The trivial problem was the preprocessor was not even accessible as it is not an exposed module in ghcide.

The main one was that even using the preprocessor I would still get an invalid session (or a similar error) when accessing a CPP file.

Make it an exposed module and/or reexport the function in Development.IDE, and upstream the change.
Please share the error, I don't think that we should merge this change until CPP works

I have submitted a PR to ghcide to export the Preprocessor module.

The latest commit here, adds support for CPP and LHS (Bird-style) for GHC >= 8.8.

plugins/default/src/Ide/Plugin/Eval/CodeLens.hs

tittoassini · 2020-10-30T12:27:06Z

As this PR is getting rather messy, I have created a clean branch that includes all the changes made here plus a few additional fixes and I was planning to push it as a PR to simplify the reviewers' work. Would that be ok?

pepeiborra · 2020-10-31T08:03:53Z

Personally, a new PR would mean more review work rather than less.

tittoassini · 2020-10-31T10:08:25Z

OK, then I will keep pushing updates on this PR. Il giorno sab 31 ott 2020 alle ore 09:04 Pepe Iborra < notifications@github.com> ha scritto:

…

Personally, a new PR would mean more review work rather than less. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#438 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFRQM5WSKGM6NM5URI2CLSNPAHLANCNFSM4R3YTQEQ> .

-- Pasqualino "Titto" Assini http://networkpolitics.svbtle.com http://quid2.org/

jneira · 2020-11-25T06:34:48Z

@tittoassini it would need a rebase

pepeiborra · 2020-11-30T21:00:49Z

@tittoassini Is this ready for re-review?

tittoassini · 2020-12-01T19:52:42Z

Hi @pepeiborra,

yes, I think that is ready for review.

The main problem that appears in the test is the completely random appearance of this error:

ByteCodeLink.lookupCE\nDuring interactive linking ...

The problem seems to affect also the current implementation: #610

Any idea of what might cause it?

The good news is that when using the plugin manually, it rarely appears.

I have squashed the commits into two commits:

Extended Eval Plugin (seen changes)
Contains all the changes that I had already pushed and that you had already seen, including all suggested modifications
Extended Eval Plugin (additional changes)
A bunch of other changes that I have made in the last month (no new feature, just fixes)

I apologise if this is not the correct procedure but I am no git jedi and the 30+ commit were becoming hard to manage when rebasing.

This is a one-off rewriting of the commit history. Any further change will come as a separate commit.

Thanks for your patience!

pepeiborra · 2020-12-02T23:05:57Z

plugins/default/src/Ide/Plugin/Eval/CodeLens.hs

+                                    -- Desperately looking for an import path
+                                    let impPaths0 = envImportPaths session
+                                        impPaths1 = if null (importPaths df) then Nothing else Just (importPaths df)
+                                        impPaths2 = maybeToList $ moduleImportPath nfp modName
+                                        impPaths = fromMaybe impPaths2 $ impPaths0 <> impPaths1
+                                    dbg "importPaths0" impPaths0
+                                    dbg "importPaths1" impPaths1
+                                    dbg "importPaths2" impPaths2
+                                    dbg "importPaths" impPaths
+                                    -- Restore the cradle import paths
+                                    df <- return df{importPaths = impPaths}


Suggested change

-- Desperately looking for an import path

let impPaths0 = envImportPaths session

impPaths1 = if null (importPaths df) then Nothing else Just (importPaths df)

impPaths2 = maybeToList $ moduleImportPath nfp modName

impPaths = fromMaybe impPaths2 $ impPaths0 <> impPaths1

dbg "importPaths0" impPaths0

dbg "importPaths1" impPaths1

dbg "importPaths2" impPaths2

dbg "importPaths" impPaths

-- Restore the cradle import paths

df <- return df{importPaths = impPaths}

df <- return df{importPaths = fromMaybe (importPaths df) (envImportPaths session)}

OK, there were some cases where envImportPaths and importPaths did not return anything, so I added moduleImportPath but I have not been able to reproduce the problem so I simplified this section down to your one-liner

pepeiborra · 2020-12-02T23:10:00Z

plugins/default/src/Ide/Plugin/Eval/CodeLens.hs

+                            . gStrictTry
+                            $ evalGhcEnv (hscEnvWithImportPaths session) $
+                                do
+                                    env <- getSession


Curious choice of indentation. Not only is this PR 60 files long, it's also 200 columns wide!

Treat your reviewers as you wish to be treated ...

beauty is in the eye of the beholder :-)

I have squashed those 'header' lines 350-356 in a single line (that is now 182 columns wide!) so that we can fit the code under it into less than 100 columns.

pepeiborra · 2020-12-02T23:18:10Z

plugins/default/src/Ide/Plugin/Eval/Tutorial.hs

@@ -0,0 +1,231 @@
+module Ide.Plugin.Eval.Tutorial where


Wouldn't it be useful to use Haddock markup consistently in this module, so that it is rendered nicely?

The Tutorial module was actually meant to be read as source code (so that one can play with it while reading and can see exactly how tests are written down in different kinds of comments).

But you are right, it also makes sense to make it into a proper Haddock-ready module so that it can work as a static README. I modified it accordingly.

Are haddocks built and accessible on the project GitHub site?

plugins/default/src/Ide/Plugin/Eval/GHC.hs

pepeiborra

Thanks for splitting in two parts. Unfortunately the second commit has tons of formatting changes so it didn't really help that much, there's tons of changes.

I don't have time to do a deeper review. It looks reasonable and I imagine that you have been using it extensively and hopefully it works.

My main concern is future maintainability. With so much new code, it would be great if you would commit to supporting this plugin, starting with a commitment to split it out to its own package.

plugins/default/src/Ide/Plugin/Eval/Util.hs

tittoassini · 2020-12-03T21:40:32Z

I do understand that it is going to take a lot of additional effort to polish this plugin and to fully realise its potential (I have a number of ideas for improvements and extensions) and I am willing to invest the time necessary.

I use it heavily every day, so it's a worthwhile investment.

I will however need help with GHC API and ghcide issues.

As for moving it into its only subpackage, would you like me to do it as part of this PR or at a later time?

pepeiborra · 2020-12-04T17:37:33Z

I do understand that it is going to take a lot of additional effort to polish this plugin and to fully realise its potential (I have a number of ideas for improvements and extensions) and I am willing to invest the time necessary.

I use it heavily every day, so it's a worthwhile investment.

I will however need help with GHC API and ghcide issues.

As for moving it into its only subpackage, would you like me to do it as part of this PR or at a later time?

Definitely as a follow up, this PR is already long enough

tittoassini · 2020-12-04T22:58:37Z

Should I rebase and squash everything in a single commit, in preparation for merging?

pepeiborra · 2020-12-05T07:29:02Z

I don't think so

pepeiborra · 2020-12-06T11:59:11Z

I haven't looked into this issue. Given that the current incarnation of the Eval plugin completely ignores the contents of the GHC session maintained by ghcide and instead reloads everything from source, there are two alternative paths forward:

Embrace the current design, and then the logical step forward is turning on the -fexternal-interpreter flag to enforce isolation of the evaluated code w.r.t the ghcide process. This would also solve Codelens eval should not interact with IO #461 and potentially other issues
Eschew the current design, and rewrite the GHC guts of the eval plugin to rely on the byte codes produced by ghcide.

wz1000 · 2020-12-06T12:07:04Z

Eschew the current design, and rewrite the GHC guts of the eval plugin to rely on the byte codes produced by ghcide.

This would require some hooks in ghcide, so that it will compile bytecode for modules that use the eval plugin (and all their dependencies). In particular, the NeedsCompilation rule will have to be modified so that it can evaluate arbitrary conditions (probably specified as an NormalizedFilePath -> Action Bool), and HLS would then have to expose this hook in the plugin API.

pepeiborra · 2020-12-06T12:13:51Z

This issue has been there for a while but is more evident now because the extended plugin tests evaluate more than one test at the time and so increases the chance of a failure.

I see that failed tests are now rerun and this tends to weed out random failures, maybe a third run should be added to reduce them even further.

Maybe the simpler solution would be to ensure that the eval tests are run sequentially

tittoassini · 2020-12-06T13:46:40Z

I haven't looked into this issue. Given that the current incarnation of the Eval plugin completely ignores the contents of the GHC session maintained by ghcide and instead reloads everything from source, there are two alternative paths forward:

Embrace the current design, and then the logical step forward is turning on the -fexternal-interpreter flag to enforce isolation of the evaluated code w.r.t the ghcide process. This would also solve Codelens eval should not interact with IO #461 and potentially other issues

That sounds great.

Do you mean simply setting -fexternal-interpreter in the haskell-language-server section of the cabal file?

That does not seem to fix the problem.

jneira · 2020-12-06T14:03:59Z

@tittoassini if you are fine me too 😄
But i personally try to have commits describing what it does (ideally also why if they are complex) and avoid generic messages like Update Module.hs. But not always i follow that rule so i will not force nobody to do it neither.

pepeiborra · 2020-12-06T14:18:06Z

I haven't looked into this issue. Given that the current incarnation of the Eval plugin completely ignores the contents of the GHC session maintained by ghcide and instead reloads everything from source, there are two alternative paths forward:

Embrace the current design, and then the logical step forward is turning on the -fexternal-interpreter flag to enforce isolation of the evaluated code w.r.t the ghcide process. This would also solve Codelens eval should not interact with IO #461 and potentially other issues

That sounds great.

Do you mean simply setting -fexternal-interpreter in the haskell-language-server section of the cabal file?

That does not seem to fix the problem.

No, I mean setting enabling the external interpreter in the DynFlags used by the Eval interpreter

tittoassini · 2020-12-06T14:39:38Z

No, I mean setting enabling the external interpreter in the DynFlags used by the Eval interpreter

setting the flag in setupDynFlagsForGHCiLike causes all evaluations to fail with "haskell-language-server: ghc-iserv terminated (-11)

tittoassini · 2020-12-06T15:02:08Z

@jneira
But i personally try to have commits describing what it does (ideally also why if they are complex) and avoid generic messages like Update Module.hs. But not always i follow that rule so i will not force nobody to do it neither.

You are right, I can edit the descriptions of those and/or squash them together, but given that there are also other commits that are not particularly meaningful (such as Extended Eval Plugin (seen changes)) I thought that they could all be merged in a single "Extended Eval Plugin" commit.

Really, I have no preferences whatsoever. I am just trying to figure out how you prefer things to be done :-)

jneira · 2020-12-06T15:10:44Z

I thought that they could all be merged in a single "Extended Eval Plugin" commit.

Fair enough, will use the "squash and merge" option then.

tittoassini · 2020-12-07T09:34:34Z

I have followed @pepeiborra advice and I have modified the Eval test suite to run the codelens commands in each test sequentially (previously they would be started in parallel, my bad).

If in addition I also run the test suite sequentially passing '--num-threads 1' to tasty, I cannot locally reproduce the linking issue.

Looking at the automated test run on the PR:

the circleci tests, that by a quick look seem to run sequentially, now pass (with some small exception that disapper when the test is automatically rerun).
the nix tests pass
the "Testing / test" ... tests fails (with both the linking problem and a QuickCheck library missing) I guess that these are run in parallel and that's what is causing it?

tittoassini · 2020-12-07T11:24:25Z

I thought that they could all be merged in a single "Extended Eval Plugin" commit.

Fair enough, will use the "squash and merge" option then.

Done.

tittoassini · 2020-12-16T12:11:24Z

@jneira, might it be a good idea to merge this now?

This is a rather complex PR and it would be good to have a full month till next release to iron out any kinks.

jneira · 2020-12-16T13:56:44Z

@tittoassini yeah, but maybe @pepeiborra could take a final look, cause there are changes after his review (and make ci green)

tittoassini · 2020-12-25T11:43:30Z

@tittoassini yeah, but maybe @pepeiborra could take a final look, cause there are changes after his review (and make ci green)

Green tests, @jneira, green tests as far as the eye can see :-)

Merry Christmas!

pepeiborra · 2020-12-25T11:59:56Z

I'm fine with merging this, I've already given it two reviews in the 3 months it's been open and am not going to review it again

jneira · 2020-12-25T12:04:59Z

@tittoassini thanks for the Christmas present

jneira requested a review from pepeiborra September 27, 2020 21:28

konn reviewed Sep 28, 2020

View reviewed changes

test/functional/Eval.hs Show resolved Hide resolved

pepeiborra reviewed Sep 30, 2020

View reviewed changes

plugins/default/src/Ide/Plugin/Eval/GHC.hs Outdated Show resolved Hide resolved

plugins/default/src/Ide/Plugin/Eval/CodeLens.hs Outdated Show resolved Hide resolved

alanz mentioned this pull request Oct 7, 2020

REPL Support #477

Closed

pepeiborra reviewed Oct 8, 2020

View reviewed changes

tittoassini force-pushed the master branch from fa7dc35 to 9be46df Compare November 28, 2020 23:41

tittoassini force-pushed the master branch 2 times, most recently from 1086040 to 9f822c6 Compare December 1, 2020 19:49

pepeiborra reviewed Dec 2, 2020

View reviewed changes

plugins/default/src/Ide/Plugin/Eval/GHC.hs Outdated Show resolved Hide resolved

pepeiborra reviewed Dec 2, 2020

View reviewed changes

plugins/default/src/Ide/Plugin/Eval/GHC.hs Outdated Show resolved Hide resolved

pepeiborra reviewed Dec 2, 2020

View reviewed changes

plugins/default/src/Ide/Plugin/Eval/GHC.hs Outdated Show resolved Hide resolved

pepeiborra approved these changes Dec 2, 2020

View reviewed changes

plugins/default/src/Ide/Plugin/Eval/Util.hs Outdated Show resolved Hide resolved

pepeiborra mentioned this pull request Dec 3, 2020

Split the Imports and Retrie plugins #647

Merged

tittoassini force-pushed the master branch from 2157ff5 to 0e81997 Compare December 7, 2020 10:31

xsebek mentioned this pull request Dec 10, 2020

eval action crashes server when optics are involved #663

Closed

tittoassini force-pushed the master branch from 0e81997 to 746c848 Compare December 16, 2020 12:09

jneira requested a review from pepeiborra December 16, 2020 13:56

tittoassini added 4 commits December 25, 2020 10:11

Extended Eval Plugin (rebased to 0.7.1)

a48a8f7

Eval tests as proper subpackage (to add QuickCheck dependency)

887cde9

Made addPackages idempotent

75bbc7d

added with-utf8 on Main (to avoid character encoding errors in Windows)

ff7aaca

tittoassini force-pushed the master branch from 56d4a92 to ff7aaca Compare December 25, 2020 09:20

added Eval other-modules

2584a4d

jneira merged commit a43933a into haskell:master Dec 25, 2020

pepeiborra mentioned this pull request Jan 30, 2021

Consider restoring the old Eval plugin #1276

Closed

berberman mentioned this pull request Mar 21, 2021

Eval plugin: evaluate expressions as statements #1603

Merged

pepeiborra mentioned this pull request Jan 21, 2022

Add initial CODEOWNERS file #2619

Merged

Extended Eval Plugin #438

Extended Eval Plugin #438

Conversation

tittoassini commented Sep 27, 2020

pepeiborra commented Sep 27, 2020

tittoassini commented Sep 28, 2020

tittoassini commented Oct 8, 2020

jneira commented Oct 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tittoassini commented Oct 30, 2020

pepeiborra commented Oct 31, 2020

tittoassini commented Oct 31, 2020 via email

jneira commented Nov 25, 2020

pepeiborra commented Nov 30, 2020

tittoassini commented Dec 1, 2020

Choose a reason for hiding this comment

tittoassini Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

pepeiborra Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepeiborra left a comment • edited Loading

Choose a reason for hiding this comment

tittoassini commented Dec 3, 2020

pepeiborra commented Dec 4, 2020

tittoassini commented Dec 4, 2020

pepeiborra commented Dec 5, 2020

pepeiborra commented Dec 6, 2020

wz1000 commented Dec 6, 2020

pepeiborra commented Dec 6, 2020

tittoassini commented Dec 6, 2020

jneira commented Dec 6, 2020

pepeiborra commented Dec 6, 2020

tittoassini commented Dec 6, 2020

tittoassini commented Dec 6, 2020

jneira commented Dec 6, 2020

tittoassini commented Dec 7, 2020

tittoassini commented Dec 7, 2020

tittoassini commented Dec 16, 2020

jneira commented Dec 16, 2020 • edited Loading

tittoassini commented Dec 25, 2020

pepeiborra commented Dec 25, 2020

jneira commented Dec 25, 2020

tittoassini Dec 3, 2020 •

edited

Loading

pepeiborra Dec 2, 2020 •

edited

Loading

pepeiborra left a comment •

edited

Loading

jneira commented Dec 16, 2020 •

edited

Loading