Function definition in GHCi
ClosedPublic

Authored by roshats on Oct 2 2015, 5:28 AM.

Details

Summary

This patch allows define and re-define functions in ghci. let is not required anymore (but can be used).

Idea: If ghci input string can be parsed as statement then run it as statement else run it as declaration.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
roshats updated this revision to Diff 4420.Oct 3 2015, 4:10 PM
roshats edited edge metadata.

@roshats, this looks great but the approach of checking for a parse error and falling back seems a bit hacky. I certainly understand your motivation for doing it this way -- working in the parser is never fun -- but ultimately I think we'll want a more principled solution. We might be willing to merge this as a stop-gap measure but would you be willing to continue working down this path to try to find a cleaner solution?

@bgamari, I'm totally agree with you. I have no experience in writing compilers but I'm interested in improving GHC and GHCi and would like to contribute.

austin edited edge metadata.Oct 12 2015, 9:07 PM
austin added subscribers: mpickering, hvr.

@roshats, this looks great but the approach of checking for a parse error and falling back seems a bit hacky. I certainly understand your motivation for doing it this way -- working in the parser is never fun -- but ultimately I think we'll want a more principled solution. We might be willing to merge this as a stop-gap measure but would you be willing to continue working down this path to try to find a cleaner solution?

Yeah, I agree with this. Technically the patch mostly looks fine (and even correctly deals with the mentioned MonoLocalBinds snag I think), but this isn't the most robust solution - for example, it really pins down the need to use runInteractiveHsc and try in this sort of weird way to catch exceptions, which is what feels bad.

@bgamari, I'm totally agree with you. I have no experience in writing compilers but I'm interested in improving GHC and GHCi and would like to contribute.

Great. So, I think @mpickering may have a good lead here with a ticket he filed earlier - Trac #10961. If it is possible to run the parser over the given expression purely, it should become relatively easy (I think) to make parseStmt Do The Right Thing (that is, look at the parse result and use that to determine what to do instead.) Perhaps @hvr might also have an idea?

compiler/main/InteractiveEval.hs
301 ↗(On Diff #4420)

OTOH, I'm pretty inclined to accept this patch just for the fact it says "arse error".

Maybe if we ever have a US American localization for GHC we can somehow figure out how to make it say "ass error". ;)

austin requested changes to this revision.Oct 12 2015, 9:08 PM
austin edited edge metadata.

(Punting to 'Request Changes' just to get it out of the review queue - let the discussion continue here or on IRC or anywhere needed!)

This revision now requires changes to proceed.Oct 12 2015, 9:08 PM

I think this will be very easy to make work properly. The Hsc monad already has a getDynFlags method which can be used, as an example you can look in the hscParse' function which does this.

The return type is GhciLStmt ... which is..

type GhciLStmt  id = LStmt id (LHsExpr id)

which matches the return type of parseStmt. So this looks easy if you write your own function instead of going through hscParseThingWithLocation.

@austin, unfortunately, I'm losing the essence of the conversation.

Writing this patch I had more problems because of printing warnings in hscParseStmtWithLocSilent than because of DynFlags. Isn't it better to separate parsing from warnings and error processing (print, ignore or do something else)?

Should I add changes in parser to this patch?

compiler/main/InteractiveEval.hs
301 ↗(On Diff #4420)

Oops, didn't know that there is such a word.

@austin, unfortunately, I'm losing the essence of the conversation.

Writing this patch I had more problems because of printing warnings in hscParseStmtWithLocSilent than because of DynFlags. Isn't it better to separate parsing from warnings and error processing (print, ignore or do something else)?

Should I add changes in parser to this patch?

Sorry, I don't think my original message was very clear.

Concretely my suggestion is to directly call parseStmt from Parser in your own function you write rather than using the existing hscParseThingWithLocation and doing some funny stuff to parse the error message. You can see an example of how to do this on line 330 in HscMain. Does this make sense? Please feel free to ask if it doesn't.

Thanks @mpickering. I think I now better understand what to do.

Why are there two sets of dynamic flags? One that can be obtained with getDynFlags and one with getInteractiveDynFlags.

Why are there two sets of dynamic flags? One that can be obtained with getDynFlags and one with getInteractiveDynFlags.

One is used during interactive evaluation (e.g. GHCi) while the other is used during "normal" compilation. To quote from compiler/main/GHC.hs

The GHC session maintains two sets of DynFlags:

  • The "interactive" DynFlags, which are used for everything related to interactive evaluation, including runStmt, runDecls, exprType, lookupName and so on (everything under "Interactive evaluation" in this module).
  • The "program" DynFlags, which are used when loading whole modules with load

Thanks, @bgamari. Wouldn't it be better to use "interactive" DynFlags in HasDynFlags instance of GHCi? I'll try it.

@roshats, I'm afraid I'm not familiar enough with the history of this code to know why that choice was made. Perhaps you know, @austin?

roshats updated this revision to Diff 4725.Oct 27 2015, 2:47 PM
roshats edited edge metadata.

I've removed exception catching the way @mpickering suggested. And I have to use "interactive" DynFlags for GHCi.

This changes don't fix Trac #10961 yet. Isn't it better to work on Trac #10961 after Trac #10143 will be done?

thomie requested changes to this revision.Nov 2 2015, 8:16 AM
thomie added a reviewer: thomie.

(I can't really comment on hscParseStmtWithLocSilent, maybe @mpickering can)

To make this a better patch:

  • rebase onto master, so we can know if it validates (please make sure it does).
  • in testsuite/tests/safeHaskell/ghci/p14.script, change the line that says
let f x = x - 1

to

let f x = x - 1; f :: Int -> Int

And check the new output of the test. Does it do the right thing? (that is RULES are not allowed when -XSafe is on).

This revision now requires changes to proceed.Nov 2 2015, 8:16 AM

@mpickering: does hscParseStmtWithLocSilent (and rest of patch) look good to you?

@roshats: can you rebase this patch onto master (git pullall --rebase, using https://ghc.haskell.org/trac/ghc/wiki/WorkingConventions/Git/Submodules#gitpullall), and then run arc diff HEAD^ (or run arc which to see what arc itself thinks about the state of your repo). I'd like to get this into ghc-8.

thomie resigned from this revision.Nov 10 2015, 5:53 AM
thomie removed a reviewer: thomie.

Small experiment: resigning as reviewer, to try to get this patch back in the review queue.

thomie accepted this revision.Nov 10 2015, 5:54 AM
thomie added a reviewer: thomie.
mpickering edited edge metadata.Nov 10 2015, 6:36 AM

Ok, so now I understand what is going on. You are parsing the Stmt to check whether it is a Stmt or a Decl before running the appropriate code path. In that case, I don't think that hscParseStmtWithLocSilent belongs in hscMain, it behaves quite differently from the other functions there.

Therefore, I suggest inlining hscParseStmt... and GHC.parseStmt into a new function GHCI.IsStmt which should make the intention much clearer. parseStmt to me suggests that it will return a Stmt, in fact, there is another function called exactly the same in Parser which does, that's confusing!

I am also confused about all the changes to DynFlags. They seem irrelevant. What was the intention here?

Does that make sense? Ask if that isn't clear.

@thomie

compiler/main/HscMain.hs
1672 ↗(On Diff #4725)

I don't think this belongs here, see my longer comment at the end.

ghc/GhciMonad.hs
203

What is this change for?

266

Rename this to something like isStmt and inline hscParseStmtWithLocSilent.

ghc/InteractiveUI.hs
473

It seems that you have changed getDynFlags to be different to getSessionDynFlags in GHCiMonad but also here. Why change both?

2095

I don't understand why any of these calls to getDynFlags have been changed.

mpickering requested changes to this revision.Nov 10 2015, 6:37 AM
mpickering edited edge metadata.
thomie resigned from this revision.Nov 10 2015, 7:00 AM
thomie removed a reviewer: thomie.

@mpickering, I'm agree with proposed changes.

parseStmt (isStmt in future) should not require to pass DynFlags. So I should use getDynFlags. T8831 requires to use interactive dynamic flags but getDynFlags for GHCi use session dynamic flags. That's why I changed getDynFlags implementation for GHCi.

T8831 requires to use interactive dynamic flags but getDynFlags for GHCi use session dynamic flags. That's why I changed getDynFlags implementation for GHCi.

I don't understand how T8831 can be involved. But I agree with @mpickering: please revert those getDynFlag changes if they are not strictly necessary, because they make this patch more difficult to read.

@thomie, T8831 sets TemplateHaskell in interactive dynamic flags set but when I make getDynFlags I get system dynamic flags. Error:

<interactive>:4:1: error:
  Couldn't match type ‘Exp’ with ‘[Dec]’
  Expected type: DecsQ
    Actual type: ExpQ
  In the expression: foo

DynFlags changes reverted.

@thomie, I'm not using arc. How can I provide required information in another way?

roshats updated this revision to Diff 5039.Nov 11 2015, 1:12 PM
roshats edited edge metadata.

@mpickering, I've inlined functions. Is it better now?

roshats updated this revision to Diff 5040.Nov 11 2015, 1:14 PM
roshats edited edge metadata.
roshats updated this revision to Diff 5042.Nov 11 2015, 2:11 PM

Looks much better! Nearly ready to merge I think.

Now that any decl is allowed, can you please add to the tests an example of each decl to make sure it works properly? I assume this choice was discussed somewhere. The fallout from this can be seen in the output for p14.

You can find a list of the different decl types here

bgamari requested changes to this revision.Nov 14 2015, 4:00 PM
bgamari edited edge metadata.

@mpickering said,

Now that any decl is allowed, can you please add to the tests an example of each decl to make sure it works properly? I assume this choice was discussed somewhere. The fallout from this can be seen in the output for p14.

Indeed, tests exercising these various cases would be great.

Otherwise, we need some release notes and some discussion in the users guide. Almost there!

This revision now requires changes to proceed.Nov 14 2015, 4:00 PM

somuchwin

This new patch looks great! Sorry I've not gotten back to it.

I agree with Matthew and Ben. This is very close, and needs a few nudges. Just to recap:

  • Add a blurb to the release notes, so everyone knows of this triumph.
  • A few tests exercising the various 'decl' types.

And with that, this will be ready to go in I think. Very happy to see this make it for 8.0. :)

And to an earlier question...

@roshats, I'm afraid I'm not familiar enough with the history of this code to know why that choice was made. Perhaps you know, @austin?

iiam

roshats updated this revision to Diff 5101.Nov 15 2015, 1:31 AM
roshats edited edge metadata.

@mpickering, I've added tests only for some HsDecl, sorry. Can you help with the others: ForD, DocD, SpliceD, and RoleAnnotD. Also I don't know how to test RuleD, VectD, and AnnD.

@austin, should I add release notes? If yes, where is the right place for them?

@roshats, the release notes are our way of telling people about all of the new features that have landed in a release. We absolutely want to make sure that users are aware of your feature; please add a short summary to docs/users_guide/8.0.1-release-notes.rst.

Also, we want to make sure the GHCi documentation in the users guide is consistent with the behavior that you have introduced. Could you have a look at docs/users_guide/ghci.rst and make the appropriate changes to reflect the fact that non-statements can now be entered?

As it turns out, you've also nearly closed an unrelated bug, Trac #9015, with this patch. All we need is isImport and isDecl functions and we'll be able to close it. @roshats, given that you've already implemented isStmt, perhaps you'd be able to handle these as well (in a future Diff)?

bgamari requested changes to this revision.Nov 15 2015, 5:14 AM
bgamari edited edge metadata.
This revision now requires changes to proceed.Nov 15 2015, 5:14 AM

@bgamari, I've made some changes for Trac #9015: https://github.com/roshats/ghc/compare/roshats:T7253...roshats:T9015. I can't find tests for GhciMonad. Are they exist?

I want to add some tests for various declarations before adding notes.

@roshats, I suspect you'll want to place your tests in testsuite/tests/ghci/should_run.

roshats updated this revision to Diff 5153.Nov 17 2015, 12:11 PM
roshats edited edge metadata.

@mpickering, I've added more declarations in test. Is it ok now? Some declarations are available even without this patch and there are tests for them.

mpickering accepted this revision.Nov 17 2015, 12:33 PM
mpickering edited edge metadata.

Yep, looks good to me.

@roshats Will you have time soon to update the documentation?

@mpickering, do you mean docs/users_guide/ghci.rst? I'm already working on it.

Good to hear. I'm just aware you started working on this patch quite a long time ago so want to make sure it doesn't keep dragging on forever!

roshats updated this revision to Diff 5182.Nov 18 2015, 11:09 AM
roshats edited edge metadata.

Updated docs/users_guide/ghci.rst. @mpickering, @bgamari, is it ready to be merged?

Final thing, can you add a small section to the release notes as Ben asked?

mpickering accepted this revision.Nov 18 2015, 11:42 AM
mpickering edited edge metadata.
bgamari accepted this revision.Nov 19 2015, 5:06 AM
bgamari edited edge metadata.

Thanks @roshats!

bgamari added a comment.EditedNov 20 2015, 3:35 AM

@roshats, can you send me (ben@well-typed.com) your email address so I can ensure you get proper attribution for your patch. Sadly Arcanist has somehow lost this information.

austin accepted this revision.Nov 20 2015, 8:14 AM
austin edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Nov 20 2015, 8:14 AM
This revision was automatically updated to reflect the committed changes.