On 14/02/2012 17:44, David Terei wrote:
On 14 February 2012 01:35, Simon Marlow<[email protected]> wrote:
On 14/02/2012 02:49, David Terei wrote:
On 13 February 2012 14:41, Simon Marlow<[email protected]> wrote:
Right, but I think what you're missing is that we sometimes need to keep
modules in the context even when they can't currently be imported. There
are
a couple of comments about this - one on setGHCContextFromGHCiState and
one
in GhciMonad pointing to the relevant tickets.
OK that helps explain the complexity. I didn't realize how much GHC
will try to load a module, if I say :add E and no such module exists
ghci still puts it in the context and tries to load it.
Hmm, I think you might be mixing up :add with :module/import. The former
adds a *target*, while the latter add to the *context*. But anyway, all of
those commands reject a module if it doesn't exist:
This doesn't sound like the behaviour I'm seeing:
$ ghci
GHCi, version 7.2.2: http://www.haskell.org/ghc/ :? for help
Prelude> :add E
<command line>:
Could not find module `E'
Use -v to see a list of the files searched for.
Failed, modules loaded: none.
Prelude> :reload
<command line>:
Could not find module `E'
Use -v to see a list of the files searched for.
Failed, modules loaded: none.
Ah, my mistake. You're right, :add is sticky because it adds a target.
Maybe it shouldn't stick if the module doesn't exist - I don't think
we had considered this case before.
However, this is still separate from the problem you were having, which
was to do with :module/import, and not :add (these two are really very
different).
I think it's right now. Let me know if not.
Cheers,
Simon
Prelude> :l hello
[1 of 1] Compiling Main ( hello.hs, interpreted )
Ok, modules loaded: Main.
*Main> :add no_such_file
target `no_such_file' is not a module name or a source file
This fails as 'no_such_file' isn't even a valid module name, not
because the module doesn't exist. So yes this case is rejected
outright as it should be since it will never be satisfied.
*Main> :module +Does.Not.Exist
<no location info>:
Could not find module `Does.Not.Exist'
It is not a module in the current program, or in any known package.
The import command had a bug where it wasn't doing this check, but that was
fixed (#5836).
I'm not really
sure how to fit -XSafe into this design right now. The existing
behaviour would seem to say I should put the unsafe import in the
context and keep repeating the failure so the user can fix it...
No, not at all. You should do the error checking up front and reject the
command if there's a problem.
So I think we need to resolve what the expected behaviour of GHCi is.
Is the current behaviour expected or is this a 'bug'?
The only time we remember imports that don't current exist is after a
:reload, where there was an error and some of the previous imports aren't
valid. They might become valid later, so we remember them in the context.
Users wanted this behaviour (there's a ticket about it, pointed to by a
comment in the code I think).
So there's an easy fix for your bug BTW. See the fix I made for #5836,
you
just need to do something else there.
Yes I saw that fix but didn't think it would give me what I want as
its only for the 'import' branch. However it seems that's the
behaviour I want as :load should repeat the failure.
The real problem (and I think this might be underlying the confusion here)
is that addImportToContext does not call checkAdd, making it the odd one out
from addModuleToContext/setContext. It doesn't call checkAdd because
there's a bit of refactoring needed that I was too lazy to do - checkAdd
takes a String, but addImportToContext has a full import declaration.
My fix for #5836 is really a hack, the right fix would be to refactor
checkAdd so that it is called by all the code paths. That would fix your
Safe Haskell bug too. I can do that if you like, or do you want to do it?
OK I still don't
like the code but given the complexity of the ghci user interface its
implementing I don't see a clear way to do it better. I think the main
issue I have is this trick of calling checkAdd multiple times,
handling exceptions differently (that should be documented please).
Will do.
Having multiple data structures and moving between them seems a better
choice but thats just an instinct, I haven't thought through what it
would look like.
Thanks for the explanations though, sorry for bad mouthing your code :)
No problem :) It clearly needs some commentary/refactoring.
Cheers,
Simon
Cheers,
David
Cheers,
Simon
e.g the 'setContext' function takes two lists of imports, maps
'checkAdd' over them and saves the new context. It then calls
'setGHCContextFromGHCiState' which does basically (map over the input
with checkAdd) the same thing but calls into GHC at the end to set the
actual context.
What I want is one interface to this all. So a setContext method that
handles calling 'checkAdd' and 'addNotSubsumed' and can handle batch
or individual additions. Then for all the :module, :import commands I
just call this 'setContext'. Based on how I've seen the code used this
seems quite doable.
Cheers,
David.
+checkAdd :: Bool -> String -> GHCi (InteractiveImport)
checkAdd star mstr = do
dflags<- getDynFlags
case safeLanguageOn dflags of
@@ -1664,8 +1661,8 @@ checkAdd star mstr = do
s<- GHC.isModuleTrusted m
case s of
True -> return $ IIDecl (simpleImportDecl $
moduleName
m)
- False -> ghcError $ CmdLineError $ "can't
import "
++
mstr
- ++ " as it isn't
trusted."
+ False -> ghcError $ CmdLineError $
+ "can't import " ++ mstr ++ " as it isn't
trusted."
False | star -> do m<- wantInterpretedModule mstr
return $ IIModule m
@@ -1687,12 +1684,26 @@ checkAdd star mstr = do
--
setGHCContextFromGHCiState :: GHCi ()
setGHCContextFromGHCiState = do
- let ok (IIModule m) = checkAdd True (moduleNameString (moduleName
m))
- ok (IIDecl d) = checkAdd False (moduleNameString (unLoc
(ideclName d)))
st<- getGHCiState
- iidecls<- filterM (tryBool . ok) (transient_ctx st ++
remembered_ctx
st)
- setGHCContext iidecls
+ goodTran<- filterM (tryBool . ok) $ transient_ctx st
+ goodRemb<- filterM (tryBool . ok) $ remembered_ctx st
+ -- drop bad imports so we don't keep replaying it to the user!
+ modifyGHCiState $ \s -> s { transient_ctx = goodTran }
+ modifyGHCiState $ \s -> s { remembered_ctx = goodRemb }
+ setGHCContext (goodTran ++ goodRemb)
+ where
+ ok (IIModule m) = checkAdd True (moduleNameString (moduleName
m))
+ ok (IIDecl d) = checkAdd False (moduleNameString (unLoc
(ideclName
d)))
+
+setContext :: [String] -> [String] -> GHCi ()
+setContext starred not_starred = do
+ is1<- mapM (checkAdd True) starred
+ is2<- mapM (checkAdd False) not_starred
+ let iss = foldr addNotSubsumed [] (is1++is2)
+ modifyGHCiState $ \st -> st { remembered_ctx = iss,
transient_ctx
= []
}
+ -- delete the transient context
+ setGHCContextFromGHCiState
-- | Sets the GHC contexts to the given set of imports, adding a
Prelude
-- import if there isn't an explicit one already.
@@ -2745,7 +2756,7 @@ tryBool :: GHCi a -> GHCi Bool
tryBool m = do
r<- ghciTry m
case r of
- Left _ -> return False
+ Left e -> showException e>> return False
Right _ -> return True
--
----------------------------------------------------------------------------
_______________________________________________
Cvs-ghc mailing list
[email protected]
http://www.haskell.org/mailman/listinfo/cvs-ghc
_______________________________________________
Cvs-ghc mailing list
[email protected]
http://www.haskell.org/mailman/listinfo/cvs-ghc
