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

[David Terei]

On 14 February 2012 05:26, Simon Marlow <marlowsd at gmail.com> wrote:
> 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.

Great thanks!

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