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.

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

Merged
merged 1 commit into from
Aug 9, 2025
Merged

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Jun 9, 2025

Partially addresses #4236

It doesn't work with sessionLoading multipleComponents because hie-bios doesn't give us the correct cradle dependencies.

A workaround is to declare the cradle dependencies in the hie.yaml. We need to fix this in hie-bios proper, but it is no longer an HLS bug.

@fendor fendor requested a review from wz1000 as a code owner June 9, 2025 10:24
@fendor fendor force-pushed the fix/reload-cabal-files branch 3 times, most recently from 4bd3727 to b9250a5 Compare June 9, 2025 12:14
@@ -586,7 +586,7 @@ loadSessionWithOptions recorder SessionLoadingOptions{..} rootDir que = do
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

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.

@soulomoon
Copy link
Collaborator

soulomoon commented Jun 13, 2025

Just some more interesting thoughts, do you think we can make GetModificationTime rule use GetPhysicalModificationTime rule in the end to remove some duplicated file time query?

@fendor
Copy link
Collaborator Author

fendor commented Jun 13, 2025

I am not sure, maybe. The whole file watcher architecture requires a thorough overhaul, it feels very brittle.

@soulomoon
Copy link
Collaborator

soulomoon commented Jun 16, 2025

The whole file watcher architecture requires a thorough overhaul, it feels very brittle.

Indeed, are you planning to do it in this PR or open a seperate one for it?

@fendor
Copy link
Collaborator Author

fendor commented Jun 16, 2025

I am not sure 😅

Probably in a follow-up, maybe we should merge this as is and hope for the best.

@fendor fendor force-pushed the fix/reload-cabal-files branch from b9250a5 to 07759ee Compare June 16, 2025 14:14
@@ -586,7 +586,7 @@ loadSessionWithOptions recorder SessionLoadingOptions{..} rootDir que = do
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'
Copy link
Collaborator

@soulomoon soulomoon Jun 17, 2025

Choose a reason for hiding this comment

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

If I am not mistaken, GetPhysicalModificationTime is for .cabal files?
and cfps' are mostly .hs file locations, and we are type checking those files here.

But I do not really know how we handle .cabal file in HLS and where we call GetModificationTime for .cabal files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, this is wrong, it should be GetModificationTime

@fendor fendor force-pushed the fix/reload-cabal-files branch from 07759ee to 81ee813 Compare June 20, 2025 13:14
Copy link
Collaborator

@soulomoon soulomoon left a comment

Choose a reason for hiding this comment

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

LGTM, better if we have a some test to capture this change.

@fendor fendor force-pushed the fix/reload-cabal-files branch from 81ee813 to 9806087 Compare July 9, 2025 16:42
@fendor fendor force-pushed the fix/reload-cabal-files branch 2 times, most recently from 03ffe6f to 7212959 Compare August 8, 2025 15:36
@fendor fendor requested a review from michaelpj as a code owner August 8, 2025 15:36
@fendor
Copy link
Collaborator Author

fendor commented Aug 8, 2025

Finally with a regression test! This was a pain to write and I think I discovered another bug. Should be good to merge now.

EDIT: strictly speaking, I still need to add a test that the reload-logic works even when the .cabal file is not open, and we are not using the hls-cabal-plugin... I manually tested that it works.

@fendor fendor force-pushed the fix/reload-cabal-files branch from 7212959 to 1782380 Compare August 8, 2025 18:24
@fendor fendor merged commit d18697c into master Aug 9, 2025
48 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants