Skip to content

Reload .cabal files when they are modified #4630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+55 −5
Open

Reload .cabal files when they are modified #4630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ghcide/session-loader/Development/IDE/Session.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@
unless (null new_deps || not checkProject) $ do
cfps' <- liftIO $ filterM (IO.doesFileExist . fromNormalizedFilePath) (concatMap targetLocations all_targets)
void $ shakeEnqueue extras $ mkDelayedAction "InitialLoad" Debug $ void $ do
mmt <- uses GetModificationTime cfps'
mmt <- uses GetPhysicalModificationTime cfps'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how ignoring version in the VFS help us here🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because cabal only works with the physical file system. If the VFS changes, then cabal simply can't perform a reload, as cabal repl will still observe the same .cabal file. That's why we only want to invalidate the shake session, if the .cabal file on disk has been modified.

Further, let's have a look at getModificationTimeImpl:

getModificationTimeImpl
  :: Bool
  -> NormalizedFilePath
  -> Action (Maybe BS.ByteString, ([FileDiagnostic], Maybe FileVersion))
getModificationTimeImpl missingFileDiags file = do
    let file' = fromNormalizedFilePath file
    let wrap time = (Just $ LBS.toStrict $ B.encode $ toRational time, ([], Just $ ModificationTime time))
    mbVf <- getVirtualFile file
    case mbVf of
        Just (virtualFileVersion -> ver) -> do
            alwaysRerun
            pure (Just $ LBS.toStrict $ B.encode ver, ([], Just $ VFSVersion ver))
        Nothing -> do
            isWF <- use_ AddWatchedFile file
            if isWF
                then -- the file is watched so we can rely on FileWatched notifications,
                        -- but also need a dependency on IsFileOfInterest to reinstall
                        -- alwaysRerun when the file becomes VFS
                    void (use_ IsFileOfInterest file)
                else if isInterface file
                    then -- interface files are tracked specially using the closed world assumption
                        pure ()
                    else -- in all other cases we will need to freshly check the file system
                        alwaysRerun
                        
           ...

First, we don't care about the isInterface file check. Then, if the file isn't open in the VFS, we will never rerun this rule, unless the IsFileOfInterest rule is marked dirty, which it never will for .cabal files.

In my understanding, isWF will be True for .cabal files.

Copy link
Collaborator

@soulomoon soulomoon Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation, much clear to me now.

let cs_exist = catMaybes (zipWith (<$) cfps' mmt)
modIfaces <- uses GetModIface cs_exist
-- update exports map
Expand Down Expand Up @@ -627,7 +627,7 @@
[] -> error $ "GHC version could not be parsed: " <> version
((runTime, _):_)
| compileTime == runTime -> do
atomicModifyIORef' cradle_files (\xs -> (cfp:xs,()))

Check warning on line 630 in ghcide/session-loader/Development/IDE/Session.hs

View workflow job for this annotation

GitHub Actions / Hlint check run 

Warning in loadSessionWithOptions in module Development.IDE.Session: Use atomicModifyIORef'_ ▫︎ Found: "atomicModifyIORef' cradle_files (\\ xs -> (cfp : xs, ()))" ▫︎ Perhaps: "atomicModifyIORef'_ cradle_files ((:) cfp)"
session (hieYaml, toNormalizedFilePath' cfp, opts, libDir)
| otherwise -> return (([renderPackageSetupException cfp GhcVersionMismatch{..}], Nothing),[])
-- Failure case, either a cradle error or the none cradle
Expand Down Expand Up @@ -894,7 +894,7 @@
x <- map errMsgDiagnostic closure_errs
DriverHomePackagesNotClosed us <- pure x
pure us
isBad ci = (homeUnitId_ (componentDynFlags ci)) `OS.member` bad_units

Check warning on line 897 in ghcide/session-loader/Development/IDE/Session.hs

View workflow job for this annotation

GitHub Actions / Hlint check run 

Suggestion in newComponentCache in module Development.IDE.Session: Redundant bracket ▫︎ Found: "(homeUnitId_ (componentDynFlags ci)) `OS.member` bad_units" ▫︎ Perhaps: "homeUnitId_ (componentDynFlags ci) `OS.member` bad_units"
-- Whenever we spin up a session on Linux, dynamically load libm.so.6
-- in. We need this in case the binary is statically linked, in which
-- case the interactive session will fail when trying to load
Expand Down
31 changes: 29 additions & 2 deletions ghcide/src/Development/IDE/Core/FileStore.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ import System.FilePath
import System.IO.Error
import System.IO.Unsafe


data Log
= LogCouldNotIdentifyReverseDeps !NormalizedFilePath
| LogTypeCheckingReverseDeps !NormalizedFilePath !(Maybe [NormalizedFilePath])
Expand Down Expand Up @@ -147,6 +146,29 @@ getModificationTimeImpl missingFileDiags file = do
then return (Nothing, ([], Nothing))
else return (Nothing, ([diag], Nothing))


getPhysicalModificationTimeRule :: Recorder (WithPriority Log) -> Rules ()
getPhysicalModificationTimeRule recorder = defineEarlyCutoff (cmapWithPrio LogShake recorder) $ Rule $ \GetPhysicalModificationTime file ->
getPhysicalModificationTimeImpl file

getPhysicalModificationTimeImpl
:: NormalizedFilePath
-> Action (Maybe BS.ByteString, ([FileDiagnostic], Maybe FileVersion))
getPhysicalModificationTimeImpl file = do
let file' = fromNormalizedFilePath file
let wrap time = (Just $ LBS.toStrict $ B.encode $ toRational time, ([], Just $ ModificationTime time))

alwaysRerun

liftIO $ fmap wrap (getModTime file')
`catch` \(e :: IOException) -> do
let err | isDoesNotExistError e = "File does not exist: " ++ file'
| otherwise = "IO error while reading " ++ file' ++ ", " ++ displayException e
diag = ideErrorText file (T.pack err)
if isDoesNotExistError e
then return (Nothing, ([], Nothing))
else return (Nothing, ([diag], Nothing))

-- | Interface files cannot be watched, since they live outside the workspace.
-- But interface files are private, in that only HLS writes them.
-- So we implement watching ourselves, and bypass the need for alwaysRerun.
Expand All @@ -170,7 +192,11 @@ resetFileStore ideState changes = mask $ \_ -> do
case c of
LSP.FileChangeType_Changed
-- already checked elsewhere | not $ HM.member nfp fois
-> atomically $ deleteValue (shakeExtras ideState) GetModificationTime nfp
->
atomically $ do
ks <- deleteValue (shakeExtras ideState) GetModificationTime nfp
vs <- deleteValue (shakeExtras ideState) GetPhysicalModificationTime nfp
pure $ ks ++ vs
_ -> pure []


Expand Down Expand Up @@ -233,6 +259,7 @@ getVersionedTextDoc doc = do
fileStoreRules :: Recorder (WithPriority Log) -> (NormalizedFilePath -> Action Bool) -> Rules ()
fileStoreRules recorder isWatched = do
getModificationTimeRule recorder
getPhysicalModificationTimeRule recorder
getFileContentsRule recorder
addWatchedFileRule recorder isWatched

Expand Down
8 changes: 8 additions & 0 deletions ghcide/src/Development/IDE/Core/RuleTypes.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
-- Copyright (c) 2019 The DAML Authors. All rights reserved.
-- SPDX-License-Identifier: Apache-2.0

{-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE DerivingStrategies #-}
{-# LANGUAGE GADTs #-}
{-# LANGUAGE PatternSynonyms #-}
Expand Down Expand Up @@ -316,6 +317,13 @@ instance Hashable GetModificationTime where

instance NFData GetModificationTime

data GetPhysicalModificationTime = GetPhysicalModificationTime
deriving (Generic, Show, Eq)
deriving anyclass (Hashable, NFData)

-- | Get the modification time of a file on disk, ignoring any version in the VFS.
type instance RuleResult GetPhysicalModificationTime = FileVersion

pattern GetModificationTime :: GetModificationTime
pattern GetModificationTime = GetModificationTime_ {missingFileDiagnostics=True}

Expand Down
9 changes: 8 additions & 1 deletion ghcide/src/Development/IDE/Core/Rules.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@
| LogLoadingHieFileFail !FilePath !SomeException
| LogLoadingHieFileSuccess !FilePath
| LogTypecheckedFOI !NormalizedFilePath
| LogDependencies !NormalizedFilePath [FilePath]
deriving Show

instance Pretty Log where
Expand All @@ -205,6 +206,11 @@
<+> "the HLS version being used, the plugins enabled, and if possible the codebase and file which"
<+> "triggered this warning."
]
LogDependencies nfp deps ->
vcat
[ "Add dependency" <+> pretty (fromNormalizedFilePath nfp)
, nest 2 $ pretty deps
]

templateHaskellInstructions :: T.Text
templateHaskellInstructions = "https://haskell-language-server.readthedocs.io/en/latest/troubleshooting.html#static-binaries"
Expand Down Expand Up @@ -715,7 +721,8 @@
let nfp = toNormalizedFilePath' fp
itExists <- getFileExists nfp
when itExists $ void $ do
use_ GetModificationTime nfp
use_ GetPhysicalModificationTime nfp
logWith recorder Logger.Info $ LogDependencies file deps
mapM_ addDependency deps

let cutoffHash = LBS.toStrict $ B.encode (hash (snd val))
Expand Down Expand Up @@ -802,7 +809,7 @@
{ source_version = ver
, old_value = m_old
, get_file_version = use GetModificationTime_{missingFileDiagnostics = False}
, get_linkable_hashes = \fs -> map (snd . fromJust . hirCoreFp) <$> uses_ GetModIface fs

Check warning on line 812 in ghcide/src/Development/IDE/Core/Rules.hs

View workflow job for this annotation

GitHub Actions / Hlint check run 

Suggestion in getModIfaceFromDiskRule in module Development.IDE.Core.Rules: Use fmap ▫︎ Found: "\\ fs -> map (snd . fromJust . hirCoreFp) <$> uses_ GetModIface fs" ▫︎ Perhaps: "fmap (map (snd . fromJust . hirCoreFp)) . uses_ GetModIface"
, get_module_graph = useWithSeparateFingerprintRule_ GetModuleGraphTransDepsFingerprints GetModuleGraph f
, regenerate = regenerateHiFile session f ms
}
Expand Down
10 changes: 9 additions & 1 deletion plugins/hls-cabal-plugin/src/Ide/Plugin/Cabal.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
{-# LANGUAGE DuplicateRecordFields #-}
{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE PatternSynonyms #-}
{-# LANGUAGE TypeFamilies #-}

module Ide.Plugin.Cabal (descriptor, haskellInteractionDescriptor, Log (..)) where
Expand Down Expand Up @@ -154,7 +155,7 @@ descriptor recorder plId =
\ide vfs _ (DidSaveTextDocumentParams TextDocumentIdentifier{_uri} _) -> liftIO $ do
whenUriFile _uri $ \file -> do
log' Debug $ LogDocSaved _uri
restartCabalShakeSession (shakeExtras ide) vfs file "(saved)" $
restartCabalShakeSessionPhysical (shakeExtras ide) vfs file "(saved)" $
addFileOfInterest recorder ide file OnDisk
, mkPluginNotificationHandler LSP.SMethod_TextDocumentDidClose $
\ide vfs _ (DidCloseTextDocumentParams TextDocumentIdentifier{_uri}) -> liftIO $ do
Expand Down Expand Up @@ -188,6 +189,13 @@ restartCabalShakeSession shakeExtras vfs file actionMsg actionBetweenSession = d
keys <- actionBetweenSession
return (toKey GetModificationTime file:keys)


restartCabalShakeSessionPhysical :: ShakeExtras -> VFS.VFS -> NormalizedFilePath -> String -> IO [Key] -> IO ()
restartCabalShakeSessionPhysical shakeExtras vfs file actionMsg actionBetweenSession = do
restartShakeSession shakeExtras (VFSModified vfs) (fromNormalizedFilePath file ++ " " ++ actionMsg) [] $ do
keys <- actionBetweenSession
return (toKey GetModificationTime file:toKey GetPhysicalModificationTime file:keys)

-- ----------------------------------------------------------------
-- Plugin Rules
-- ----------------------------------------------------------------
Expand Down
Loading