On 14 February 2012 01:35, Simon Marlow <[email protected]> wrote: > On 14/02/2012 02:49, David Terei wrote: >> >> On 13 February 2012 14:41, Simon Marlow<[email protected]> wrote: >>> >>> 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. > > > Hmm, I think you might be mixing up :add with :module/import. The former > adds a *target*, while the latter add to the *context*. But anyway, all of > those commands reject a module if it doesn't exist:
This doesn't sound like the behaviour I'm seeing: $ ghci GHCi, version 7.2.2: http://www.haskell.org/ghc/ :? for help Prelude> :add E <command line>: Could not find module `E' Use -v to see a list of the files searched for. Failed, modules loaded: none. Prelude> :reload <command line>: Could not find module `E' Use -v to see a list of the files searched for. Failed, modules loaded: none. > > Prelude> :l hello > [1 of 1] Compiling Main ( hello.hs, interpreted ) > Ok, modules loaded: Main. > *Main> :add no_such_file > target `no_such_file' is not a module name or a source file This fails as 'no_such_file' isn't even a valid module name, not because the module doesn't exist. So yes this case is rejected outright as it should be since it will never be satisfied. > *Main> :module +Does.Not.Exist > > <no location info>: > Could not find module `Does.Not.Exist' > It is not a module in the current program, or in any known package. > > The import command had a bug where it wasn't doing this check, but that was > fixed (#5836). > > >> 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... > > > No, not at all. You should do the error checking up front and reject the > command if there's a problem. So I think we need to resolve what the expected behaviour of GHCi is. Is the current behaviour expected or is this a 'bug'? > > The only time we remember imports that don't current exist is after a > :reload, where there was an error and some of the previous imports aren't > valid. They might become valid later, so we remember them in the context. > Users wanted this behaviour (there's a ticket about it, pointed to by a > comment in the code I think). > > >>> 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. > > > 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? > > >> 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 >>>>>> [email protected] >>>>>> http://www.haskell.org/mailman/listinfo/cvs-ghc >>>>> >>>>> >>>>> >>>>> >>> > _______________________________________________ Cvs-ghc mailing list [email protected] http://www.haskell.org/mailman/listinfo/cvs-ghc
