[commit: ghc] master: Don't add a bad import to the saved context. (e5272d9)

[Simon Marlow]

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
>>> Cvs-ghc at haskell.org
>>> http://www.haskell.org/mailman/listinfo/cvs-ghc
>>
>>