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 renaming of labelled arguments #872

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

zth
Copy link
Collaborator

@zth zth commented Dec 18, 2023

This is an attempt to fix renaming of labelled arguments breaking because it doesn't account for ~ (so ~ is either removed from the argument definition, or added to local bindings that are renamed).

This seems to have broken a few other things though that I'm a bit unsure of how to fix. Maybe this isn't the optimal approach to this problem anyway.

@zth zth requested a review from cristianoc December 18, 2023 16:09
@@ -9,3 +9,6 @@ let c = x

let foo = (~xx) => xx + 1
// ^ren yy

let foo2 = (~xx) => xx + 1
// ^ren yy
Copy link
Collaborator

Choose a reason for hiding this comment

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

So foo was working and foo2 wasn't?
Not sure I get what's going on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They both work, but when for foo the tilde is dropped from ~xx so it becomes (yy) => ..., and for foo2 the tilde was added to the body (~yy) => ~yy + 1.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps hacking renaming itself to special-case tilde does not break other things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was my first idea but it might be a little bit of extra work, because we'll need to read the text from the loc and have a look at that. There's no information carried on that can help us when producing the rename instructions. Might be the best idea anyway though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was my first idea but it might be a little bit of extra work, because we'll need to read the text from the loc and have a look at that. There's no information carried on that can help us when producing the rename instructions. Might be the best idea anyway though.

Not sure it makes sense, but perhaps when the length does not match. E.g. if the loc says 3 and it's supposed to be 2. Though that's risky.

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.

2 participants