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

Implement "Rethinking REPLs" #248

Merged
merged 11 commits into from Jun 24, 2019
Merged

Conversation

@isovector
Copy link
Contributor

@isovector isovector commented May 31, 2019

This PR implements the Unison notebook idea --- essentially adding support for interactive REPL commands directly in source code.

The idea is that I can write the following:

magic :: String -> String
magic = reverse

-- $> magic "hello world"

to which, ghcid will now reply:

All good (25 modules, at 17:42:54)

/home/sandy/prj/polysemy/src/Polysemy.hs:160:1
$> magic "hello world"
"dlrow olleh"

In essence, this lets you run little repl commands directly in your source file.

Link to the original idea in Unison: unisonweb/unison#252

@isovector
Copy link
Contributor Author

@isovector isovector commented May 31, 2019

I'm happy to clean up the lint if you like the idea. Also, I just realized that maybe -- > isn't a good choice, since I think it has meaning to haddock.

@isovector
Copy link
Contributor Author

@isovector isovector commented May 31, 2019

@jkeuhlen made the good point that this can let people run arbitrary code. It should probably be hidden behind a flag :)

Copy link
Owner

@ndmitchell ndmitchell left a comment

Thanks, a bunch of comments, but fundamentally seems like a good feature to include, so happy to see it go in.

src/Session.hs Outdated Show resolved Hide resolved
src/Session.hs Outdated Show resolved Hide resolved
src/Session.hs Outdated Show resolved Hide resolved
src/Session.hs Outdated Show resolved Hide resolved
src/Session.hs Outdated Show resolved Hide resolved
src/Ghcid.hs Outdated Show resolved Hide resolved
src/Ghcid.hs Outdated Show resolved Hide resolved
src/Ghcid.hs Outdated Show resolved Hide resolved
src/Ghcid.hs Outdated Show resolved Hide resolved
src/Ghcid.hs Outdated
@@ -256,11 +258,11 @@ runGhcid :: Session -> Waiter -> IO TermSize -> ([String] -> IO ()) -> Options -
runGhcid session waiter termSize termOutput opts@Options{..} = do
let limitMessages = maybe id (take . max 1) max_messages

let outputFill :: String -> Maybe (Int, [Load]) -> [String] -> IO ()
outputFill currTime load msg = do
let outputFill :: String -> Maybe (Int, [Load]) -> [((FilePath, (Int, Int)), (String, String))] -> [String] -> IO ()
Copy link
Owner

@ndmitchell ndmitchell Jun 1, 2019

Choose a reason for hiding this comment

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

If we had a separate EvalResult type (which seems like a good idea anyway) then this bit type argument would have that type, which would be good.

@isovector
Copy link
Contributor Author

@isovector isovector commented Jun 1, 2019

Excited to hear that you like the idea! In addition to (or instead of) the --allow-eval flag, I was thinking we should just never run any commands that are there before you start ghcid. That way nobody can drop malicious snippets into a repo somewhere. What do you think?

I'll clean up the lint today.

@isovector
Copy link
Contributor Author

@isovector isovector commented Jun 3, 2019

@ndmitchell waiting for some guidance on this before going further --- in particular around whether Load should contain EvalResults, and if you're OK with a ReaderT around the session stuff.

Copy link
Owner

@ndmitchell ndmitchell left a comment

  • I don't think there's any need for a ReaderT - I think doing what you've already done, but slightly hiding some details so we are free to change them in future, is probably sufficient.
  • I do think EvalResult should be split off as a separate type, to keep the library version coherent and self-contained.
  • My inclination is to just run all eval snippets. If someone wants to maliciously take over your computer then TemplateHaskell is a much easier and more reliable vector that works whether you cabal install, ghci or ghcid. I can't imagine anyone is going to go for a more complex method that only works with newer versions of ghcid.

src/Session.hs Outdated
@@ -3,11 +3,12 @@
-- | A persistent version of the Ghci session, encoding lots of semantics on top.
-- Not suitable for calling multithreaded.
module Session(
Session, withSession,
Session(allowEval), withSession,
Copy link
Owner

@ndmitchell ndmitchell Jun 5, 2019

Choose a reason for hiding this comment

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

I was thinking of keeping allowEval in Session, but not exposing the allowEval (which gives you a lot of power), and instead just enableEval s = s{allowEval=True}. That way Session remains entirely abstract, but you can keep the rest of the code the same, without any ReaderT or similar.

@isovector
Copy link
Contributor Author

@isovector isovector commented Jun 7, 2019

Sounds great! I'll try to get this polished up tomorrow.

@isovector
Copy link
Contributor Author

@isovector isovector commented Jun 8, 2019

@ndmitchell I think I've responded to all of your suggestions. CI is broken, but it appears to be for unrelated reasons:

Using compiler: ghc-8.6.5
Asking cabal to calculate a build plan...
Trying with packages from nightly-2019-06-06 as hard constraints...
The following lines from cabal-install output could not be parsed: 
ansi-terminal-0.9.1 (via: ghcid-0.7.4 ghcid-0.7.4 ghcid-0.7.4 tasty-1.2.2
ansi-wl-pprint-0.6.9) (new package)
CallStack (from HasCallStack):
  error, called at src/Stack\Solver.hs:174:16 in stack-1.9.3-3S9JDRpC5h6C7YOiQDGqyN:Stack.Solver

@ethercrow
Copy link

@ethercrow ethercrow commented Jun 13, 2019

@isovector thank you for this feature! Just tried it and had only one minor inconvenience with language pragmas not affecting the ghcid repl.

{-# LANGUAGE TypeApplications #-}

-- $> map (succ @Int) [1, 2]

results in

$> map (succ @Int) [1, 2]
<interactive>:59:6-14: error:    Pattern syntax in expression context: succ@Int    Did you mean to
enable TypeApplications?

but is fine if I add

-- $> :set -XTypeApplications

I guess it could be fixed by not only collecting lines starting with -- $> but also converting {-# language blah #-} to :set -Xblah.

@isovector
Copy link
Contributor Author

@isovector isovector commented Jun 13, 2019

@ethercrow yeah, I've been running into that too. There's also an issue with the imports being in the wrong scope. Perhaps @ndmitchell has some ideas about doing this more correctly.

@isovector
Copy link
Contributor Author

@isovector isovector commented Jun 20, 2019

Gentle ping on this.

@ndmitchell
Copy link
Owner

@ndmitchell ndmitchell commented Jun 20, 2019

Sorry, Zurihac burnt out all my brain cycles for a few days and I'm still in catch up. I'll do a real review tomorrow (when my brain has returned)

@ndmitchell ndmitchell merged commit 3a7500b into ndmitchell:master Jun 24, 2019
0 of 2 checks passed
@ndmitchell
Copy link
Owner

@ndmitchell ndmitchell commented Jun 24, 2019

Thanks, took a few more days for my brain to recover, but it looks all good. Could I trouble you to write a paragraph or so for the README, explaining what the feature is and roughly how to use it? Otherwise its unlikely to be discoverable for users.

@fosskers
Copy link
Contributor

@fosskers fosskers commented Jul 17, 2019

ghcid --allow-eval works like a charm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants