Skip to content

Commit

Permalink
Fix filepath identity in cradle dependencies (#2106)
Browse files Browse the repository at this point in the history
When doing reactive change tracking we track the set of dirty keys as opposed to conservative change tracking where we check every key on every build.

Filepath identity becomes even more important in this setting, opening a new type of bug where the filepath used in the dirty key is equivalent but not structurally equal to the filepath used in the dependency graph.

This can happen e.g. with cradle dependencies where hie-bios gives us a set of relative paths and we create GetFileModification keys for them as dependencies of the GhcSession, but the LSP client raises FileWatched notifications using absolute paths that we use to create dirty keys.

To prevent this class of bugs, we would need to strengthen the notion of NormalizedFilePath to make it always absolute/relative. In practice, since 99% of the paths are introduced by the LSP client, we just need to make sure that the other 1% (introduced by e.g. hie-bios) follow the same convention.

This commit fixes the hie-bios paths to make them absolute before creating rule keys, following the LSP convention

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
pepeiborra and mergify[bot] committed Aug 18, 2021
1 parent 57ffd74 commit 8a471bb
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 9 deletions.
12 changes: 6 additions & 6 deletions ghcide/bench/lib/Experiments.hs
Expand Up @@ -156,10 +156,10 @@ experiments =
forM_ identifierP $ \p -> changeDoc doc [charEdit p]
)
( \docs -> do
Just hieYaml <- uriToFilePath <$> getDocUri "hie.yaml"
liftIO $ appendFile hieYaml "##\n"
hieYamlUri <- getDocUri "hie.yaml"
liftIO $ appendFile (fromJust $ uriToFilePath hieYamlUri) "##\n"
sendNotification SWorkspaceDidChangeWatchedFiles $ DidChangeWatchedFilesParams $
List [ FileEvent (filePathToUri "hie.yaml") FcChanged ]
List [ FileEvent hieYamlUri FcChanged ]
forM_ docs $ \DocumentPositions{..} ->
changeDoc doc [charEdit stringLiteralP]
waitForProgressDone
Expand All @@ -171,10 +171,10 @@ experiments =
bench
"hover after cradle edit"
(\docs -> do
Just hieYaml <- uriToFilePath <$> getDocUri "hie.yaml"
liftIO $ appendFile hieYaml "##\n"
hieYamlUri <- getDocUri "hie.yaml"
liftIO $ appendFile (fromJust $ uriToFilePath hieYamlUri) "##\n"
sendNotification SWorkspaceDidChangeWatchedFiles $ DidChangeWatchedFilesParams $
List [ FileEvent (filePathToUri "hie.yaml") FcChanged ]
List [ FileEvent hieYamlUri FcChanged ]
flip allWithIdentifierPos docs $ \DocumentPositions{..} -> isJust <$> getHover doc (fromJust identifierP)
),
---------------------------------------------------------------------------------------
Expand Down
9 changes: 6 additions & 3 deletions ghcide/src/Development/IDE/Core/Rules.hs
Expand Up @@ -138,7 +138,7 @@ import qualified Language.LSP.Server as LSP
import Language.LSP.Types (SMethod (SCustomMethod))
import Language.LSP.VFS
import Module
import System.Directory (canonicalizePath)
import System.Directory (canonicalizePath, makeAbsolute)
import TcRnMonad (tcg_dependent_files)

import Control.Applicative
Expand Down Expand Up @@ -674,9 +674,12 @@ loadGhcSession = do

-- add the deps to the Shake graph
let addDependency fp = do
let nfp = toNormalizedFilePath' fp
-- VSCode uses absolute paths in its filewatch notifications
afp <- liftIO $ makeAbsolute fp
let nfp = toNormalizedFilePath' afp
itExists <- getFileExists nfp
when itExists $ void $ use_ GetModificationTime nfp
when itExists $ void $ do
use_ GetModificationTime nfp
mapM_ addDependency deps

opts <- getIdeOptions
Expand Down

0 comments on commit 8a471bb

Please sign in to comment.