On 13 February 2012 14:41, Simon Marlow <[email protected]> wrote:
> 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.
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. 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...
>
>
>> 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.
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. 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).
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 :)
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
