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

[Simon Marlow]

On 14/02/2012 09:35, Simon Marlow wrote:

> 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?

Never mind, I've done this now, and fixed your SafeHaskell-related 
problems at the same time (I think).  The tests are still failing 
because the output needs to be updated, but I'll leave those for you.

Cheers,
	Simon




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