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

Implement renaming for Functions, Constants and Type Variants #4282

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

GearsDatapacks
Copy link
Member

This PR closes #3259
This is pretty much ready, except for two things:

  • It's missing tests
  • There's an annoying bug to do with module shadowing which I still have to figure out how to resolve

@GearsDatapacks GearsDatapacks force-pushed the cross-module-value-renaming branch 3 times, most recently from d4c87d0 to 1f931b9 Compare February 27, 2025 17:16
@GearsDatapacks
Copy link
Member Author

GearsDatapacks commented Feb 28, 2025

Ok, this is ready for review now. I'm not super happy with the way I fixed the module access shadowing bug, but I couldn't think of another way to do it. Let me know if you have any thoughts.

@GearsDatapacks GearsDatapacks marked this pull request as ready for review February 28, 2025 20:30
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Super nice!! I'm really excited about this one.

I've not finished reviewing totally but I've gotta go so I wanted to drop my thoughts so far. See inline notes.

@GearsDatapacks GearsDatapacks marked this pull request as draft March 1, 2025 21:53
@GearsDatapacks
Copy link
Member Author

Ok, I have now added the reference information to the cache so we can rename values in cached modules.
It feels a bit wasteful, it's quite a lot of information which is only needed for the LSP, but I don't think there's any other way around it.
I have responded to a couple of your comments, let me know your thoughts.

@GearsDatapacks GearsDatapacks marked this pull request as ready for review March 2, 2025 12:52
@GearsDatapacks GearsDatapacks marked this pull request as draft March 3, 2025 22:35
@GearsDatapacks GearsDatapacks force-pushed the cross-module-value-renaming branch 7 times, most recently from aa0d57e to 5e00fad Compare March 10, 2025 13:30
@GearsDatapacks
Copy link
Member Author

This should be ready for review again.
As discussed on Discord, I couldn't find a way to easily represent the information needed for renaming in a call-graph structure. Instead, I've added a ReferenceTracker struct, which keeps track of all the reference locations, but also builds a call graph which can later be used (in a future PR) to do usage tracking.
I have also changed the functionality so that renaming a value that is imported in an unqualified fashion now aliases it instead of globally renaming, and prevented users from renaming values from external packages.

@GearsDatapacks GearsDatapacks marked this pull request as ready for review March 10, 2025 20:21
@GearsDatapacks GearsDatapacks force-pushed the cross-module-value-renaming branch from 6476e42 to 0e230df Compare March 11, 2025 23:45
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.

LSP: rename custom type variant
2 participants