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 core file location in GetLinkable #4347

Merged

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented Jul 3, 2024

Fix #4145
The error case is demonstrated in #4145 (comment)

  1. Include ModLocation in the ModSummaryResult fingerprint.
  2. Instead of getting the core file location from GetModSummary, get it from the result of GetModIface directly since that is the actual location the core file written to. While the GetModSummary contains the future core file location that would be written only if we would have made some change cause the rewrite. See Note [Avoiding bad interface files]

@soulomoon soulomoon linked an issue Jul 3, 2024 that may be closed by this pull request
@soulomoon soulomoon marked this pull request as ready for review July 3, 2024 19:59
@soulomoon soulomoon requested a review from wz1000 as a code owner July 3, 2024 19:59
@wz1000
Copy link
Collaborator

wz1000 commented Jul 4, 2024

Can you explain why the core file in the ModSummaryResult and HiFileResult are different? If one is getting updated and not the other then it seems like this is more of a dependency tracking/invalidation issue.

@soulomoon
Copy link
Collaborator Author

soulomoon commented Jul 4, 2024

@wz1000, The cause of this different is that GetModSummary's fingerprint is irrelevant to the core file location. So some of its' reverse deps are being earlycutoff even core file location changed. Is this expected ?

It's not hard to change the fingerprint algorithm to reflect the core file location though.

computeFingerprint opts ModSummary{..} = do

@wz1000
Copy link
Collaborator

wz1000 commented Jul 4, 2024

It's not hard to change the fingerprint algorithm to reflect the core file location though.

computeFingerprint opts ModSummary{..} = do

Yes I think the correct solution is to include the fingerprint of the entire ModLocation in computeFingerprint.

This patch is a good cleanup though, so perhaps you can apply it after testing that just changing computeFingerprint to take the ModLocation into account also fixes the bug.

@soulomoon
Copy link
Collaborator Author

Yes, local testing show that fingierprinting the ModLocation fix the .hi.core problem.

@soulomoon soulomoon added the performance Issues about memory consumption, responsiveness, etc. label Jul 4, 2024
@soulomoon soulomoon merged commit fa48fda into master Jul 4, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues about memory consumption, responsiveness, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HLS often cannot find a .hi.core file in ~/.cache/ghcide/
2 participants