helix-editor / helix

A post-modern modal text editor.
https://helix-editor.com
Mozilla Public License 2.0
33.63k stars 2.5k forks source link

Bug in sqlx query highlighting for Rust #8608

Open rmehri01 opened 1 year ago

rmehri01 commented 1 year ago

Summary

Based on a bisect from the 23.05 release, it seems like #7621 caused the highlighting of the sqlx query macros to stop working:

Before:

image

After:

image

I think this is because the injection query for normal macros overwrites the whole thing, when really we just want the nested query to be SQL and the rest should be Rust:

https://github.com/helix-editor/helix/blob/68c7537de522cf6b38705a7af51eeaac260fdc98/runtime/queries/rust/injections.scm#L4-L7

There's also a smaller regression that #6793 removed the highlighting for query_unchecked that #6256 introduced but that is fairly easy to fix :)

Reproduction Steps

I tried this:

  1. hx on a Rust file with any sqlx query, such as sqlx::query!("SELECT id FROM posts")

I expected this to happen:

The SQL string should be highlighted properly.

Instead, this happened:

It is just highlighted as a normal Rust string.

Helix log

N/A

Platform

Linux

Terminal Emulator

Kitty

Helix Version

23.05-498-g68c7537d

pascalkuthe commented 1 year ago

that is not really a regression but rather an existing bug (that could have lead to crashes before that fix) in the queries. The normal macro injection needs to ignore the sqlx macro (similar for other injections)

rmehri01 commented 1 year ago

Ah okay, sorry about that.

Is there an example of where something nested like this is done in another injection query? I see something kind of similar in vue where by default scripts are javascript and if there is lang= then it is that lang, but I don't really see one with one injection inside another?

To clarify, I think I can do something that gets the normal macro injection to not trigger so that the SQL works but then some other parts don't get properly highlighted like the comma since the macro body isn't Rust:

image

pascalkuthe commented 1 year ago

I would need to look qt the specifics but I think you should be able to only inject sql into the first string literal/argument and then injection rust into the rest of them.

It might be possible that the order of injections matters and you could get it to work just by changing the order of the existing injections now that I think about jt (potentially the first injection is preffered) but I am not sure if that works reliably ( a little fuzzy on how query order works there and I didn't intentionally implement any specific precendence)

the-mikedavis commented 1 year ago

I think swapping the order doesn't fix the precedence here since the sqlx queries are a little more 'specific' (they need to match more of the tree than the general macro ones). I'm not sure if that's something we should change so that the more specific query gets precedence rather than the first one that matches successfully or if we should use negative matches in this case (so the general macro pattern would explicitly not match on sqlx macros)

CedarMatt commented 1 month ago

Is this still active? I found an interesting addition to the "not highlighting" part of this bug recently. It turns out if you remove the comma between the macro arguments in query_as! the sql highlighting works again.

I know its a syntax error but idk maybe it will help resolve the bug.