Skip to content
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

Fix filepath identity in cradle dependencies when using reactive change tracking #2106

Merged
merged 4 commits into from Aug 18, 2021

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Aug 17, 2021

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.

NOTE: reactive change tracking is not enabled or even implemented anywhere, so this change is a no-op for everyone who is not using #2060

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
@pepeiborra pepeiborra changed the title Fix filepath identity in cradle dependencies Fix filepath identity in cradle dependencies when using reactive change tracking Aug 17, 2021
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for split it from the big pr
could be it related with the fact hls does not reload when config files from hie-bios changes? (see #1068)

EDIT: probably, as @fendor noted the solution could be make hie-bios return absolute paths (#775). It would be nice if this would fix both issues.

@fendor
Copy link
Collaborator

fendor commented Aug 17, 2021

One of the issues I encountered was that there is no reliable way with stack to find package.yaml (or .cabal) files, e.g. when a package is in a sub-directory, then the component directory is equal to the root directory, but package.yaml (or .cabal) is actually located in the sub-directory.

I don't think that has changed, for stack canonicalising will probably not always fix the issue.

@pepeiborra
Copy link
Collaborator Author

I think it might be causing #1068 yes. Initially I assumed that this was a complete no-op because reactive builds are not enabled yet, but on second thoughts we already play some tricks with GetFileExists and GetFileModification relying on LSP FileWatched notifications to remove the alwaysRerun dependency, and file path identity will be relevant there too.

So, to summarise, I believe this could well fix #1068 which must have regressed in #1880

@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Aug 17, 2021
@ratherforky
Copy link

I can confirm that making the hie-bios paths absolute should fix this problem (assuming the changes will also affect ghcide/session-loader/Development/IDE/Session.hs). I just discovered this independently and came to the repo to tell people, but it looks like I've been beaten to the punch!

I was suspicious of this line:

let deps = componentDependencies opts ++ maybeToList hieYaml

...since it should include the hie.yaml file (which is correctly tracked) and the cradle dependencies (which are not correctly tracked), so I threw a trace on it and found for my test project: deps = ["depsReloading.cabal","package.yaml","stack.yaml","~/Playground/depsReloading/hie.yaml"]

So the tracked file is absolute and the non-tracked files are relative. I then hard-coded the cradle dependencies to be absolute as well, and sure enough that completely fixed the problem for me (using a default stack setup)

@mergify mergify bot merged commit 8a471bb into master Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants