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 compiler injection patches #537

Merged
merged 2 commits into from
Dec 7, 2021

Conversation

doorgan
Copy link
Contributor

@doorgan doorgan commented Dec 4, 2021

This addresses #532

I left a comment in the code, but the reason for this bug is that when we have a qualified call, like Mix.compilers(), the AST will look like this:

{{:., inner_metadata, [[:__aliases__, _, [:Mix]}, :compilers]}, outer_metadata, []}

Note that :compilers(ie the identifier) is a "naked" atom, ie it's not wrapped in a block, so it doesn't have metadata. The line and column information for the identifier lives in the outer_metadata, while the dot position information lives in the inner_metadata. This means that we can't know the range of the inner :. call if we don't have the parent too.

When navigating down such tree, the first child will be the :. call, then the arguments follow. If we use Z.rightmost to get the last child, but the call doesn't have any arguments like in this case, then the only child we'll get is the :. call.

The code generating this patch was trying to generate a patch on this :. call, but since that call alone doesn't have enough information to generate a range to patch, it crashed. The solution is basically to check if we landed in such call and if so then work with the parent node.

@msaraiva msaraiva merged commit c5f3fcb into surface-ui:master Dec 7, 2021
msaraiva added a commit that referenced this pull request Dec 7, 2021
@msaraiva
Copy link
Member

msaraiva commented Dec 7, 2021

Hey @doorgan!

Thank you for the PR. As you could probably see, I tried to implement a higher level API that allows applying groups of patches to elixir projects. I'm not fully satisfied with the current API but it works as expected and it has all I need to create precise patches for mix surface.init and eventually for mix surface.convert.

There were a few issues while designing it, especially due to problems that forced me to update the parent node, which should be avoided as it may format more code than it should. Anyway, I think a similar API would be great to have in Sourceror itself. I remember we've discussed this a while ago and I told I'd give it a try so here's my initial attempt :)

Let me know if you're interested in something similar. I'd be glad to contribute back.

@doorgan
Copy link
Contributor Author

doorgan commented Dec 8, 2021

@msaraiva Yup, I'll spend more time checking out ExPatcher
What I saw so far is quite promising already, something similar would be definitely a welcome addition to Sourceror :)

So far most of my focus on my similar focus was on studying the ruby https://github.com/whitequark/parser's Rewriter, but I didn't do much progress. Marc-André had some insights on this here, but again, I dind't do much progress as to come with a good api for Sourceror yet, and I'm yet not sure about how applicable these ideas are

Zurga pushed a commit to Zurga/surface that referenced this pull request Apr 15, 2022
* Update Sourceror to v0.9.0

* Fix compiler injection in mix.exs, fixes surface-ui#532
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