-
Notifications
You must be signed in to change notification settings - Fork 8
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
Ignore none
locations instead of ghost locations
#110
base: main
Are you sure you want to change the base?
Conversation
@@ -25,11 +25,13 @@ let decl_of_path_or_lid env namespace path lid = | |||
end | |||
| _ -> Env_lookup.by_path path namespace env | |||
|
|||
let should_ignore_lid (lid : Longident.t Location.loc) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to have a comment here saying why this is the right choice for what to ignore.
I'm converting this to a draft PR for now because I realized some other changes are also necessary. |
This PR is ready for review again. |
Holding off on further review pending ocaml-flambda/flambda-backend#3137 |
Currently, the occurrences query ignores identifiers if their location is "ghost". This causes issues with finding occurrences in the presence of let punning. Also, this probably produces some undesirable behavior in the presence of ppxs (occurrences within ppx-generated code will be ignored). This PR changes occurrences to instead ignore identifiers whose location is
Location.none
.This PR also causes the occurrences query to begin filtering occurrences read from an index file. This goes in hand with this flambda PR, which causes occurrences in cmt/cms files to not be pre-filtered. Before, flambda also filtered "ghost" locations when producing cmt/cms files. Now, it will do no filtering, leaving the onus on Merlin.
Because of this, one of the tests introduced by this PR is left unfixed - it will be corrected once the flambda PR is merged. I've made a separate draft PR (#109) that contains the changes from this PR, but it is based on the flambda version that contains the relevant patch. In that draft PR, the test is corrected and passing.