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

don't rewrite ~ @ to ~@ #331

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

frenchy64
Copy link

@frenchy64 frenchy64 commented Aug 12, 2024

The basic issue is that ~ @a is being reformatted to ~@a, which is not semantically equivalent.

My guess is that most would choose to write ~(deref a) instead. This works fine, however (zprint-str '~(clojure.core/deref a)) prints ~@a, which might be the most likely manifestation of this bug in practice.

An alternative approach might be to not delete the space between the ~ @ in the first place. LMK if you'd rather do that.

@frenchy64 frenchy64 marked this pull request as ready for review August 12, 2024 22:34
@frenchy64 frenchy64 changed the title don't rewrite ~ @ to ~@ don't rewrite ~ @ to ~@ Aug 14, 2024
@kkinnear
Copy link
Owner

Thank you very much for finding this problem. I notice that you have found it in several tools... Took me a while to figure out all of the details. Fortunately rewrite-clj is parsing it correctly. Several things...

  1. I'm thinking I'm want to fix this a bit differently. In the case where this comes up, I want to make the l-str of the :unquote be "~ " instead of "~". I'm doing this by looking "ahead and down" to see if the :unquote is followed by a :deref. You, on the other hand, were looking "back and up" (in my way of thinking) to see if the :deref was preceded by :unquote. And you were then making the l-str of the :deref be " @". I have made these changes in what has become a function called prefix-tags, instead directly in fzprint*. It seems to work. I am impressed that you figured out how to fix this at all in zprint.

  2. I see you have changed from "~,@" to "~ @" today. That was indeed my thinking as well. I examined looking at whatever was before the :deref and just using that, but the complexity outweighed the utility in this rarely encountered situation.

It is moderately easier for me to go with the fix that I've put together and pick up your tests, give you credit in the "Contributors" section and not use this PR. Alternatively, if you would prefer I use the PR, I can manually merge it and make the changes after that. Either would be fine, whatever you prefer.

Thanks again for surfacing this problem and providing a solution!

@frenchy64
Copy link
Author

I'm happy if you copy the tests and put a Co-authored-by: line in the commit using the git info from my branch.

I think my first version used your approach of starting with unquote. I have a PR in cljfmt that takes the opposite approach of not allowing :unquote and :deref nodes to touch in the first place.

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