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

Save and load REPL context via :save and :load commands #807

Merged
merged 5 commits into from
Feb 12, 2019

Conversation

basile-henry
Copy link
Collaborator

@basile-henry basile-henry commented Jan 31, 2019

This PR closes #706

The format for the save file is a plain Dhall file where we expect the normal form to be a record literal. This will hopefully mean that the save file can be edited manually if need be without too much effort.

The save files are always kept by default (their index is incrementing) and only the last one is loaded by default.

It is possible to manually specify where to save and load the context to.

@basile-henry
Copy link
Collaborator Author

I am not sure why hydra is failing, I can build this with nix-build just fine.
It seems to be complaining that listDirectory is not being exported by System.Directory, but it has been there since version 1.2.5.0 according to the haddock comment.
Any idea what might be happening?

@Gabriella439
Copy link
Collaborator

@basile-henry: It's because CI builds dhall against GHC 7.10.3 which requires a specific (old) version of the directory library

Copy link
Collaborator

@Gabriella439 Gabriella439 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! Just one comment


saveBinding [file]

-- Save all the bindings into a big record into `file`
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably should use something other than a record to store the bindings because we need to preserve the original order. The context permits duplicate entries and if their order is switched then you change which one shadows which

Copy link
Collaborator Author

@basile-henry basile-henry Feb 1, 2019

Choose a reason for hiding this comment

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

I saw that the context keeps the shadowed bindings, but I'm not too sure why.

Maybe if we weren't normalizing the bindings before adding them to the context then we would need to have access to the shadowed bindings. But I believe that when we normalise while adding a new binding, it gets rid of any dependencies on previous bindings, or is that not the case?

Or maybe we need access to the shadowed bindings for something else?

The current solution I have is to only serialise the version of a binding accessible to the user when they type :save.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@basile-henry: It's still possible to refer to shadowed bindings using the @n suffix, like this:

⊢ :let x = 1

x : Natural

⊢ :let x = True

x : Bool

⊢ x@1

1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I didn't know about this feature.

I liked the idea of using dhall to serialise the REPL state. What would you recommend as a format?

I could use [ { name : Text, expr : Text } ] but then the save file can't really be used for anything else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This isn't just a feature of the REPL! It's a Dhall thing, where can I find the documentation about it?

➜ dhall <<< "let a = 1 let a = 2 in a@1"   
1

Ok, about the serialization format, I thought about it a bit more. Maybe I shouldn't use Dhall to serialize the context and instead serialize the context like entries a user could input into the REPL.

Something like:

:let x = 2
:let x = 3
:let y = "foo"
:let z = x + 5

My reasoning for this is that it makes the :load command more general than just loading a saved state of the context. This format is similar to the format used by a .ghci file. We could imagine in the future automatically loading such a file (maybe named .dhall-repl) on startup, but that's a separate issue.

What do you think of this idea? In my previous suggestion of [ { name : Text, expr : Text } ] I really disliked serializing Dhall expressions as Text.

I'll get started with this idea, but do tell me if you think it's not the right way to solve this issue! 😄

Also, I'm not too sure what to do with stdout when replaying these :let commands, it could get noisy very fast...

@jneira
Copy link
Collaborator

jneira commented Feb 1, 2019

Strangely the appveyor build with ghc-7.10.3 succeded 🤔
https://ci.appveyor.com/project/Gabriel439/dhall-haskell/builds/22043719/job/kkc0tg9w4d46pv4c

The format for the save file is a plain Dhall file where we expect the normal form to be a record literal. This will hopefully mean that the save file can be edited manually if need be without too much effort.

Signed-off-by: Basile Henry <bjm.henry@gmail.com>
The format used is the set of commands needed to recreate the saved context.

When loading a saved file, the output emitted by replaying the commands is discarded.

This commit also adds file autocomplete to the load command.

Signed-off-by: Basile Henry <bjm.henry@gmail.com>
(env { outputHandle = nullHandle })

-- Use a null handle (to discard output) to run all the commands
newEnv <- liftIO $ System.IO.withFile "/dev/null" System.IO.WriteMode handler
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea how platform independent this solution is. Any pointers to improve this? Or do you think this works on Windows somehow? 🤞

Copy link
Collaborator

Choose a reason for hiding this comment

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

Appveyor is happy so it works in windows 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately I don't think Appveyor runs the REPL, and this will likely be a runtime error 😅

Copy link
Collaborator

@jneira jneira Feb 2, 2019

Choose a reason for hiding this comment

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

oh right, i forgot that sometimes it compiles but fails at runtime. It seems that it will fail in windows, yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've tested the pr in my windows env and i've not been able to trigger any error. The steps were:

  • run the dhall repl
  • type some binding: :let a = 1
  • :save the session
  • :load the session
  • quit and rerun repl and load the session again

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because of the withFile pattern, if there's an error in the handle. It probably just skips the loading bit

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for testing, I'm still surprised it doesn't crash. Could you try to exit in between save and load? And the check if a is in scope.

My last test was just that, including the console output to be more precise:

D:\dhall-haskell>stack run dhall -- repl

⊢ :let a = 1

a : Natural

⊢ a

1

⊢ :save

Context saved to `.dhall-repl-0`

⊢ :quit
Goodbye.

D:\dhall-haskell>stack run dhall -- repl

⊢ :load

Loaded `.dhall-repl-0`

⊢ a

1  

Copy link
Collaborator

@jneira jneira Feb 2, 2019

Choose a reason for hiding this comment

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

I am on windows 10, not sure if it would work in older systems.
Maybe the solution ib this so answer could be applicable here: https://stackoverflow.com/a/41527205/49554

Copy link
Collaborator Author

@basile-henry basile-henry Feb 2, 2019

Choose a reason for hiding this comment

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

Ah thank you! If hSilence is properly cross platform I could use that. Otherwise I could use a Maybe Handle as part of the state instead and just not print anything when I don't have a handle.
That might be cleaner! 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I would configure the REPL so it can optionally not print output when replaying commands

@@ -75,6 +79,7 @@ data Env = Env
, explain :: Bool
, characterSet :: CharacterSet
, _standardVersion :: StandardVersion
, outputHandle :: Maybe System.IO.Handle
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps instead of storing a Maybe System.IO.Handle you can store a Text -> IO (). Then when you want to write something you set it to Data.Text.IO.putStrLn and when you want to output nothing you set it to mempty

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We also use renderIO to pretty print Dhall, which uses the handle.
I could add an extra rendering function similar to what you suggest, as part of the state, but the Maybe Handle seems lighter. What do you prefer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to still use renderIO if you change the Env to store a function of type Doc -> IO () instead?

@Gabriella439
Copy link
Collaborator

I'll go ahead and merge this. If we need any additional changes I can make them in a subsequent pull request

@Gabriella439 Gabriella439 merged commit d20b1ca into master Feb 12, 2019
@Gabriella439 Gabriella439 deleted the basile/repl-save branch February 12, 2019 06:24
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.

:save with no arguments saves REPL state
3 participants