On 13/02/12 21:13, David Terei wrote:
Originally you'd get this behaviour:
Prelude> :set -XSafe
Prelude> import System.IO.Unsafe
<no location info>:
base:System.IO.Unsafe can't be safely imported! The module itself
isn't safe.
Prelude> import Data.Char
<no location info>:
base:System.IO.Unsafe can't be safely imported! The module itself
isn't safe.
Prelude Data.Char>
My understanding of all this code is:
* ghci maintains its own import context as GHC will drop the context
under some situations so ghci needs to do this for persistence
* checkAdd makes sure a module is OK to import (e.g exists, is byte
code form if required, [and now] is safe if required). At least that
seems the purpose of checkAdd to me but maybe I'm wrong as I find the
code structure strange.
* setGHCContext... functions take the context ghci maintains and loads
it into ghc.
* sometimes we want to load a bunch of imports as a batch operation.
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.
My problem with the code is all the entry points. It feels as if the
code has just grown more complex for each case required and is quite
spaghetti like now.
The structure does have some logic behind it. I'll try to explain:
- At the top we have moduleCmd, which handles ":module". It
calls one of 3 functions in the next layer, depending on
what form the command has.
- addModulesToContext, remModulesFromContext, setContext
correspond to the 3 forms of the :module command (note
they are together in the source). addImportToContext
is also at this layer, corresponding to the "import"
command.
Note all these functions follow the same pattern: they
- do some error checking,
- modify the context in the state
- call setGHCContextFromGHCiState
- checkAdd is a utility used by the above layer (and also
reused later); it checks whether a module can be imported,
throwing an exception if not.
- setGHCContextFromGHCiState is the lowest layer; see its
comment. Note it re-uses checkAdd, but in such a way
that the exception is (deliberately) caught and not
printed.
- setGHCContext is used by the above. It could be
inlined to avoid confusion, except for the other call
during initialisation, but as you'll see that could be
replaced trivially.
Does that help? I see now, that the design which was so clear to me
when I wrote the code is probably rather opaque to someone else. Maybe
some clearer grouping in the source, or renaming of functions would help
(obviously there are too many things called setXXXContext).
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.
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
