Skip to content

Conversation

@davidlattimore
Copy link
Contributor

I originally did replacement by passing in the full file text. Then as some point I thought I could do without it. Turns out calling .text() on a node coming from a macro expansion isn't a great idea, especially when you then try and use ranges from the original source to cut that text. The test I added here actually panics without the rest of this change (sorry I didn't notice sooner).

I originally did replacement by passing in the full file text. Then as some point I thought I could do without it. Turns out calling .text() on a node coming from a macro expansion isn't a great idea, especially when you then try and use ranges from the original source to cut that text. The test I added here actually panics without the rest of this change (sorry I didn't notice sooner).
@davidlattimore
Copy link
Contributor Author

Any chance we can either get this change into today's release or revert #5007 ? I'm worried about the potential for panics or incorrect edits when matches are found within macro expansions.

@matklad
Copy link
Contributor

matklad commented Jun 29, 2020

I'm worried about the potential for panics or incorrect edits when matches are found within macro expansions.

This should be fine, generally. We are not at the stage where we can guarantee that all features are robust and never panic. Panics generally don't kill the whole of rust-analyzer, only the current request.

Turns out calling .text() on a node coming from a macro expansion isn't a great idea, especially when you then try and use ranges from the original source to cut that text.

Yeah, in general, applying modifications to macros is tricky. The core of the problem is that you can generate correct modification in terms of the expanded code, but mapping to the original code is somewhat lossy. I think the general promising approach here is that:

  • you compute some info in the expanded code
  • you the go back to unexpanded code and merged computed infos with original concrete syntax.

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 29, 2020

@bors bors bot merged commit e1a5bd8 into rust-lang:master Jun 29, 2020
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