Skip to content

Conversation

@liam923
Copy link
Contributor

@liam923 liam923 commented Oct 11, 2024

This PR changes cmt and cms files to include the locations of all identifier occurrences. This is different from before, where locations with loc_ghost = true are filtered out. The onus would then fall on Merlin to filter out occurrences of identifiers that shouldn't be displayed to users.

The immediate motivation for this change is to make project-wide occurrences work with punned let expressions. Currently, since the expression side of lets are marked ghost, they are filtered out from occurrences. One solution would be to continue filtering ghost locations, but add some sort of logic that special-cases the expression of a punned let. However, the solution that @ncik-roberts and I came up with is to instead filter locations that are not Location.none. Our reasoning is that user probably do actually want to see occurrences that are marked as ghost (for example, in ppx generated code) if there is a reasonable location in the source code to point to.

Regardless, I think filtering of locations should ultimately be handled by Merlin. If we want to iterate on what locations we choose to filter, it is much easier to change only Merlin rather than both Merlin and the compiler. This comes at a slight cost - cms/cmt files may be bloated with locations that we don't really care about, and the filtering will slow Merlin down slightly since that filtering is no longer done in a preprocessing step. But I think both of these costs are fairly small. With no evidence to back up my claim, I believe the number of ghost locations is likely small relative to non-ghost locations. And that Merlin slowdown it probably negligible compared to, say, serializing the locations after filtering.

This PR necessitates a corresponding change in Merlin because Merlin currently unconditionally includes all occurrences reported by the compiler via cms/cmt files. Thus, Merlin now needs to filter locations read from cms/cmt files. This is done in: oxcaml/merlin#110

@liam923 liam923 force-pushed the liam-let-pun-occurrences branch from 1d754ba to 0568366 Compare October 16, 2024 18:10
@liam923 liam923 changed the base branch from main to 5.2.0minus-1-microbranch October 16, 2024 18:11
@liam923 liam923 force-pushed the liam-let-pun-occurrences branch from 0568366 to 02cae45 Compare October 16, 2024 21:09
@liam923 liam923 changed the base branch from 5.2.0minus-1-microbranch to main October 16, 2024 21:09
@github-actions
Copy link

github-actions bot commented Oct 16, 2024

Parser Change Checklist

This PR modifies the parser. Please check that the following tests are updated:

  • parsetree/source_jane_street.ml

This test should have examples of every new bit of syntax you are adding. Feel free to just check the box if your PR does not actually change the syntax (because it is refactoring the parser, say).

@liam923 liam923 requested a review from ncik-roberts October 16, 2024 21:15
@liam923 liam923 marked this pull request as ready for review October 16, 2024 21:15
@ncik-roberts
Copy link
Contributor

ncik-roberts commented Oct 17, 2024

I'd be roughly in favor of continuing to filter Location.none locations on the compiler side if merlin is not going to be using them. It seems like free space savings. Ppxes generate asts with that location fairly often.

@liam923
Copy link
Contributor Author

liam923 commented Oct 17, 2024

@ncik-roberts I think you're right. I've talked myself out of thinking that it is important for Merlin to do the filtering.

@goldfirere
Copy link
Collaborator

goldfirere commented Oct 17, 2024

After discussion, we agreed that @liam923 would check build artifact sizes before merging.

@liam923 liam923 force-pushed the liam-let-pun-occurrences branch from 7be216c to dc76be7 Compare October 23, 2024 13:31
@liam923
Copy link
Contributor Author

liam923 commented Oct 23, 2024

I've completed the benchmarking, and we've decided that the artifact size increase is relatively minimal.

@goldfirere goldfirere merged commit 401dac2 into main Oct 24, 2024
19 checks passed
@goldfirere goldfirere deleted the liam-let-pun-occurrences branch October 24, 2024 13:29
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.

4 participants