Implement basic one-off language inections #82

cottand · 2024-06-16T19:41:14Z

Implement PsiLanguageInjectionHost in strings, so that they may be injected with the one-off action "Inject Language Or Reference". Works towards closing #81.

There are several not-yet-implemented things, some more important than others. In no particular order:

NixStringLiteralEscaper.getOffsetInHost() is not implemented, because it's hard. This means the action "Edit Fragment" works but does not edit the fragment (it opens a separate window to edit the guest language). This also means guest languages cannot be formatted.
Indentation when decoding Nix indented string is not implemented. I think this might simply not be possible, because with my attempts editor just crashed. It might be related to how the IDEA API works with "ranges" (the host and the guest language's text ranges) and that might not play well with indented strings, which outright remove a good chunk of text in the middle of the range. The example I was following does not have this problem, because multiline strings are not indented
Interpolation when decoding Nix intented strings is not escaped. I think this is the least confusing thing to do, but it makes the highlighting UX not ideal when dealing with injected languages that have braces (like JSON).
Permanent injections are not implemented yet. I am happy to try to do that in a separate PR, I think one-off injections is already an improvement

Another potential issue is that I implemented much of the boilerplate in Kotlin, because I am more at ease writing in it. It's not super in line with the rest of the code, so I am happy to re-write in Java now that the actual development is done, if you prefer.

For reference, I used the Terraform HCL plugin as my guide, because it also has multiline strings with interpolation.

I am happy to keep working on this PR to implement the things above, or do separate PRs so they are smaller and easier to review. See some pictures below.

Some languages:

A warning hint of Yaml inside a Nix string:

The fragment editor to see the isolated guest language (this does not work if you edit the fragment, because getOffsetInHost() is not implemented):

One line injection:

cottand · 2024-06-16T19:48:40Z

Not sure why it is not passing in CI, gradle check passes on my machine :c

JojOatXGME · 2024-06-17T16:11:30Z

Not sure why it is not passing in CI, gradle check passes on my machine :c

EdtExtensionTest > later_exceptions_are_added_as_suppressed_exceptions() FAILED
    java.lang.AssertionError at EdtExtensionTest.java:78

Doesn't look related to your changes. I just triggered a re-build and it succeeded.

JojOatXGME

Hi, big thanks for your contribution. Unfortunatly, I am abit distracted this week. I may not always respond as fast I would like. Just that you know. Anyway, here is my initial quick review.

Indentation when decoding Nix indented string is not implemented. I think this might simply not be possible, because with my attempts editor just crashed.

I think IDEA supports that for text blocks in Java. However, that is a rather new think. If I remember correctly, I think IDEA doesn't yet support that for Kotlin. I could imagine that they use a different (maybe internal) API in case of Java. I don't know. I didn't yet really look at the API you are using.

Another potential issue is that I implemented much of the boilerplate in Kotlin, because I am more at ease writing in it. It's not super in line with the rest of the code, so I am happy to re-write in Java now that the actual development is done, if you prefer.

I don't think that is necessary.

I am happy to keep working on this PR to implement the things above, or do separate PRs so they are smaller and easier to review. See some pictures below.

Not sure. I am happy to integrate this PR as long as it doesn't break anything.

src/main/java/org/nixos/idea/psi/impl/AbstractNixPsiElement.java

JojOatXGME · 2024-06-17T15:17:08Z

src/main/java/org/nixos/idea/psi/NixStringLiteralEscaper.kt

+
+class NixStringLiteralEscaper(host: AbstractNixString) : LiteralTextEscaper<PsiLanguageInjectionHost>(host) {
+
+    override fun isOneLine(): Boolean = false


question (non-blocking): What is this used for? Some strings may only be one line, right? Maybe we should return true for NixStdString?

I took the example from HCL which also always returns false. I will try to implement proper smart logic, see if that makes any difference, but I suspected it might be best to copy my reference for now 🤷

JojOatXGME · 2024-06-17T15:18:24Z

src/main/java/org/nixos/idea/psi/NixStringLiteralEscaper.kt

+        if (myHost is NixIndString) {
+            outChars.append(NixIndStringUtil.escape(subText))
+        } else {
+            NixStringUtil.escape(outChars, subText)


issue: I suspect that you may want to unescape the string here, instead of escaping it.

Ah I misnamed NixIndStringUtil.escape(), it should be unescape. I'll fix that, good catch.

In this case, I would suggest using NixStringUtil.parse, as it already supports both types of strings. You would have to iterate over NixString.getStringParts(). Each part implements either NixStringText or NixAntiquotation. The first one can be unescaped by NixStringUtil.parse. The second one represents a string interpolation, not sure what to do with it.

JojOatXGME · 2024-06-17T15:22:57Z

src/main/java/org/nixos/idea/util/NixStringUtil.java

@@ -93,7 +94,7 @@ public static void escape(@NotNull StringBuilder builder, @NotNull CharSequence
     * @param textNode A part of a string.
     * @return The resulting string after resolving all escape sequences.
     */
-    public static @NotNull String parse(@NotNull NixStringText textNode) {
+    public static @NotNull String parse(@NotNull NixStringPart textNode) {


issue: This seems wrong, if I am not missing something. NixStringPart may also be a NixAntiquotation, which is how interpolations were called by Nix in the past. This means this method my now run into a runtime exception: “Unexpected token in string”.

You are correct, good catch. I needed to change this for
NixStringManipulator.handleContentChange() but that's not implementet yet, so I will revert this.

src/main/java/org/nixos/idea/util/NixStringUtil.java

src/main/java/org/nixos/idea/util/NixIndStringUtil.kt

src/main/java/org/nixos/idea/psi/NixStringManipulator.kt

JojOatXGME · 2024-06-17T16:25:24Z

src/main/java/org/nixos/idea/psi/NixStringManipulator.kt

+    }
+
+    override fun getRangeInElement(element: NixString): TextRange = when {
+        element.textLength == 0 -> TextRange.EMPTY_RANGE


note: This first case seems unreachable to me.

To me too (and it should be unreachable in the HCL plugin too, but they also do it there).

My guess is they just wanted to cover all the codepaths?

Or maybe it's possible to have non-compiling code that returns zero-length tokens.

JojOatXGME · 2024-06-17T16:27:27Z

src/main/java/org/nixos/idea/psi/NixStringManipulator.kt

+        element is NixIndString -> TextRange(2, element.textLength - 2)
+        // element is not IndString, so it must be StdString
+        element.textLength == 1 -> TextRange(0, 1)
+        else -> TextRange(1, element.textLength - 1)


question (non-blocking): Is it important that the returned range only contains the content of the string? Just wondering because this looks as you may return the quotes as part of the range in some cases. (Specifically when the string is not closed.)

correct, I also don't know what the consequences of this are. My reference has the same.

I can always cut out quotes if you want, I have no opinion here

change wording in comment undo change signature of NixStringUtil.parse remove remnant in NixIndStringUtilTest make NixIndStringUtil.escape JvmStatic

cottand

Thanks for your thorough review @JojOatXGME !

I may not always respond as fast I would like

No hurry at all! I will tag you for review when I think it is ready again but take as long as you need to get back to me. Your reviews are thorough so it must be time consuming. I will be offline next week myself.

I didn't yet really look at the API you are using.

I am using the Java API so I don't think the problem is there. I suspect dealing with the offsets needs special treatment when passing the decoded text back to the editor (otherwise you might be giving it different ranges it expects?). I am not fully clear on the inner workings of those yet. I will do a bit more exploration, but I suspect I need to implement decode() while manually keeping track of offsets like here

src/main/java/org/nixos/idea/psi/NixStringManipulator.kt

cottand · 2024-06-17T18:47:15Z

src/main/java/org/nixos/idea/psi/NixStringManipulator.kt

+    }
+
+    override fun getRangeInElement(element: NixString): TextRange = when {
+        element.textLength == 0 -> TextRange.EMPTY_RANGE


To me too (and it should be unreachable in the HCL plugin too, but they also do it there).

My guess is they just wanted to cover all the codepaths?

Or maybe it's possible to have non-compiling code that returns zero-length tokens.

cottand · 2024-06-17T18:50:40Z

src/main/java/org/nixos/idea/psi/NixStringManipulator.kt

+        element is NixIndString -> TextRange(2, element.textLength - 2)
+        // element is not IndString, so it must be StdString
+        element.textLength == 1 -> TextRange(0, 1)
+        else -> TextRange(1, element.textLength - 1)


correct, I also don't know what the consequences of this are. My reference has the same.

I can always cut out quotes if you want, I have no opinion here

src/main/java/org/nixos/idea/util/NixIndStringUtil.kt

cottand · 2024-06-17T18:58:03Z

src/main/java/org/nixos/idea/util/NixStringUtil.java

@@ -93,7 +94,7 @@ public static void escape(@NotNull StringBuilder builder, @NotNull CharSequence
     * @param textNode A part of a string.
     * @return The resulting string after resolving all escape sequences.
     */
-    public static @NotNull String parse(@NotNull NixStringText textNode) {
+    public static @NotNull String parse(@NotNull NixStringPart textNode) {


You are correct, good catch. I needed to change this for
NixStringManipulator.handleContentChange() but that's not implementet yet, so I will revert this.

src/test/java/org/nixos/idea/util/NixIndStringUtilTest.java

JojOatXGME · 2024-06-22T19:39:03Z

Hi, looked a bit through the source code of IntelliJ Community to understand how they are handling string interpolations for Java. I noticed the following;

They implement PsiLanguageInjectionHost in fragments, not the for whole string. In our case, this would mean we would implement PsiLanguageInjectionHost in NixStringText, instead of NixString
They implement a com.intellij.lang.injection.general.LanguageInjectionPerformer (see JavaInjectionPerformer in IntelliJ source code)
They implement a com.intellij.lang.injection.MultiHostInjector (see JavaConcatenationToInjectorAdapter in IntelliJ source code)
Their JavaConcatenationToInjectorAdapter extends ConcatenationInjectorManager.BaseConcatenation2InjectorAdapter, which is not part of IDEAs API. ConcatenationInjectorManager also provides an extension point using the interface com.intellij.lang.injection.ConcatenationAwareInjector. This interface is also again implemented for Java as ConcatenationInjector.

So, the implementation for Injections in Java seem rather complex, but I didn't yet understand for what all this complexity is for. I hope there is a simpler solution to properly support strings containing interpolations. Anyway, I wanted to share with you what I noticed, although I hoped to provide more specific guidance. I haven't really looked at your reference (Terraform HCL plugin), if they also have injections, maybe there implementation is easier to understand.

cottand · 2024-07-06T19:13:28Z

Hello! Sorry for the delay.

Thanks for taking a look at the Java source.

They implement PsiLanguageInjectionHost in fragments, not the for whole string.

I think they do this to be able to perforn injections in concatenated strings, like in "some: "+"{yaml: stuff}". I am not against implementing this for Nix too, but it does make the code more complex, so maybe we can keep it simple for now and revisit in a separate PR if necessary.

I also think it adds complexity, because before when decoding the string (ie, telling IDEA "this is what the string is supposed to be in YAML") you need to concatenate the fragments again. Let me know if this is also your understanding too.

They implement a LanguageInjectionPerformer

They implement a MultiHostInjector

From reading the IDEA docs, I believe this is necessary to add 'annotated' permanent injections, like Kotlin has too:

@Language("YAML")
val someConfig = """
hello:
  - I am
  - a language
  - injection
"""

I would like this for Nix, but it increeases the complexity a lot like you said, so if I get to it it will be in a separate PR.

They implement a com.intellij.lang.injection.MultiHostInjector

For this and the latter part of (4), I think that's for injections inside string concatenation too.

As for implementing indent-aware injections in the host, Java escapes its strings keeping track of stateful offsets here, which are populated during LiteralTextEscaper::decode here. This bit actually looks simpler in Java, maybe it's because it goes through the single fragment rather than the entire string literal (compare this with terraform's!). I will try to implement this for Nix now see if it works

cottand · 2024-07-07T11:00:08Z

hey @JojOatXGME ! I think this is ready for another review

I have implemented proper offset tracking and handleChange, so now you can launch a code fragment editor and it will get written back to the original file
I have implemented dealing with the indents inside decode(), so the fragment editor will not show the indent from the original Nix file
- this also means code formatting of the guest language works!

The trade-off is that the decode() implementation only works for indented strings for now.

I have looked at how Java and Kotlin do injections in multiline strings too. They use a different approach (like you said, they work on the fragment directly). I have yet to understand how they handle string concatenation of each fragment, and how the interpolation plays into that.

Their approach results in a slightly better UX where an interpolation in the host lang (Nix) is not highlighted as the guest lang (YAML, etc) when doing an injection. See the following secreenshots:

Yaml in Kotlin (interpolation in yaml is highlighted):

Yaml in Nix (interpolation looks like yaml text):

Overall I think the former approach, used in this PR, is 'good enough' for now at least. It's also how Terraform does it, even though they also do have string interpolations.

I am still happy to keep looking at the Java/Kotlin approach and come up with better injection in this PR or in a new one.

Feel free to review whenever in your own time. Thank you!

JojOatXGME · 2024-07-07T15:06:07Z

Thanks, I will look at it!

Overall I think the former approach, used in this PR, is 'good enough' for now at least. It's also how Terraform does it, even though they also do have string interpolations.

I am a bit concerned because this means we have two different modes while escaping the text. If a user adds ${...} in the fragment editor of a bash script, the ${...} needs to be escaped. However, when the user adds a string interpolation in the Nix file, and then opens the fragment editor, we must not escape the ${...} from the Nix level interpolation. However, there are probably easy solutions for that. I will take a look at your code with that in mind.

JojOatXGME

Unfortunately, I still found a few issues. The issue of the missing escaping in AbstractNixString.updateText is probably tricky to resolve 😟. The other remaining topics seem rather trivial (or non-blocking). If you don't mind, I may try out some changes later today, as I got some ideas while looking at your PR.

JojOatXGME · 2024-07-13T09:44:16Z

src/main/java/org/nixos/idea/psi/NixStringLiteralEscaper.kt

+        // it uses different escaping mechanisms
+        if (myHost !is NixIndString) return false
+
+        val subText: String = rangeInsideHost.substring(myHost.text)


issue (non-blocking): I think this is technically wrong.

If it's impossible to properly decode some chars from the specified range (e. g. if the range starts or ends inside escaped sequence), decode the longest acceptable prefix of the range and return false
— Javadoc of LiteralTextEscaper.decode

According to my understanding of the documentation, if the range starts between the first and last charachter of ''\n, we should immediately return false without modifying outChars. However, I can imagine that this is only relevant in edge cases, which may not be that important for now.

JojOatXGME · 2024-07-13T09:47:32Z

src/main/java/org/nixos/idea/psi/NixStringLiteralEscaper.kt

+        val subText: String = rangeInsideHost.substring(myHost.text)
+        val array = IntArray(subText.length + 1)
+        val success = unescapeAndDecode(subText, outChars, array)
+        outSourceOffsets = array


price: I like this solution of creating a lookup table.

I wish I could take credit, I borrowed the idea from terraform

src/main/java/org/nixos/idea/psi/NixStringLiteralEscaper.kt

src/main/java/org/nixos/idea/psi/impl/AbstractNixString.kt

JojOatXGME · 2024-07-13T11:24:45Z

src/main/java/org/nixos/idea/psi/impl/AbstractNixString.kt

+        // if the first line was removed in the fragment, add it back to preserve a multiline string
+        val withLeadingBlankLine = if (lines.first().isNotEmpty()) listOf("") + withIndent else withIndent
+
+        originalNode?.replaceWithText(withLeadingBlankLine.joinToString(separator = System.lineSeparator()))


issue: This line throws an exception when using line feeds which don't match the default on the system. We probably have to use the line feeds configured in IntelliJ for the current file, instead of using System.lineSeparator(). Or maybe it works to always use \n. Not sure what replaceWithText expects here.

See exception I got on Windows

java.lang.AssertionError: Wrong line separators: '\r\n first_...' at offset 0 at com.intellij.openapi.util.text.StringUtil.assertValidSeparators(StringUtil.java:2484) at com.intellij.openapi.editor.impl.DocumentImpl.assertValidSeparators(DocumentImpl.java:706) at com.intellij.openapi.editor.impl.DocumentImpl.replaceString(DocumentImpl.java:600) at com.intellij.openapi.editor.impl.DocumentImpl.replaceString(DocumentImpl.java:591) at com.intellij.psi.impl.PsiToDocumentSynchronizer.doCommitTransaction(PsiToDocumentSynchronizer.java:212) at com.intellij.psi.impl.PsiToDocumentSynchronizer.lambda$commitTransaction$1(PsiToDocumentSynchronizer.java:188) at com.intellij.psi.impl.PsiToDocumentSynchronizer.lambda$doSync$0(PsiToDocumentSynchronizer.java:106) at com.intellij.psi.impl.PsiToDocumentSynchronizer.performAtomically(PsiToDocumentSynchronizer.java:124) at com.intellij.psi.impl.PsiToDocumentSynchronizer.doSync(PsiToDocumentSynchronizer.java:106) at com.intellij.psi.impl.PsiToDocumentSynchronizer.commitTransaction(PsiToDocumentSynchronizer.java:188) at com.intellij.pom.core.impl.PomModelImpl.commitTransaction(PomModelImpl.java:195) at com.intellij.pom.core.impl.PomModelImpl.lambda$runTransaction$1(PomModelImpl.java:151) at com.intellij.psi.impl.DebugUtil.performPsiModification(DebugUtil.java:535) at com.intellij.pom.core.impl.PomModelImpl.lambda$runTransaction$2(PomModelImpl.java:103) at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$executeNonCancelableSection$2(CoreProgressManager.java:228) at com.intellij.openapi.progress.impl.CoreProgressManager.computeUnderProgress(CoreProgressManager.java:634) at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$computeInNonCancelableSection$3(CoreProgressManager.java:236) at com.intellij.openapi.progress.Cancellation.computeInNonCancelableSection(Cancellation.java:53) at com.intellij.openapi.progress.impl.CoreProgressManager.computeInNonCancelableSection(CoreProgressManager.java:236) at com.intellij.openapi.progress.impl.CoreProgressManager.executeNonCancelableSection(CoreProgressManager.java:227) at com.intellij.pom.core.impl.PomModelImpl.runTransaction(PomModelImpl.java:92) at com.intellij.psi.impl.source.tree.ChangeUtil.prepareAndRunChangeAction(ChangeUtil.java:141) at com.intellij.psi.impl.source.tree.CompositeElement.replaceChild(CompositeElement.java:632) at com.intellij.psi.impl.source.tree.LeafElement.replaceWithText(LeafElement.java:138) at org.nixos.idea.psi.impl.AbstractNixString.updateText(AbstractNixString.kt:43) at org.nixos.idea.psi.impl.AbstractNixString.updateText(AbstractNixString.kt:12) at org.nixos.idea.psi.NixStringManipulator.handleContentChange(NixStringManipulator.kt:24) at org.nixos.idea.psi.NixStringManipulator.handleContentChange(NixStringManipulator.kt:10) at com.intellij.psi.ElementManipulators.handleContentChange(ElementManipulators.java:65) at com.intellij.psi.impl.source.tree.injected.changesHandler.CommonInjectedFileChangesHandler.updateHostElement(CommonInjectedFileChangesHandler.kt:170) at com.intellij.psi.impl.source.tree.injected.changesHandler.CommonInjectedFileChangesHandler.updateHostOrFail(CommonInjectedFileChangesHandler.kt:149) at com.intellij.psi.impl.source.tree.injected.changesHandler.CommonInjectedFileChangesHandler.commitToOriginal(CommonInjectedFileChangesHandler.kt:122) at com.intellij.codeInsight.intention.impl.QuickEditHandler.lambda$commitToOriginal$10(QuickEditHandler.java:343) at com.intellij.psi.impl.source.PostprocessReformattingAspectImpl.lambda$disablePostprocessFormattingInside$1(PostprocessReformattingAspectImpl.java:119) at com.intellij.psi.impl.source.PostprocessReformattingAspectImpl.disablePostprocessFormattingInside(PostprocessReformattingAspectImpl.java:128) at com.intellij.psi.impl.source.PostprocessReformattingAspectImpl.disablePostprocessFormattingInside(PostprocessReformattingAspectImpl.java:118) at com.intellij.codeInsight.intention.impl.QuickEditHandler.commitToOriginal(QuickEditHandler.java:343) at com.intellij.codeInsight.intention.impl.QuickEditHandler.documentChanged(QuickEditHandler.java:278) at com.intellij.openapi.editor.impl.DocumentImpl.lambda$changedUpdate$1(DocumentImpl.java:913) at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$executeNonCancelableSection$2(CoreProgressManager.java:228) at com.intellij.openapi.progress.impl.CoreProgressManager.registerIndicatorAndRun(CoreProgressManager.java:685) at com.intellij.openapi.progress.impl.CoreProgressManager.computeUnderProgress(CoreProgressManager.java:641) at com.intellij.openapi.progress.impl.CoreProgressManager.lambda$computeInNonCancelableSection$3(CoreProgressManager.java:236) at com.intellij.openapi.progress.Cancellation.computeInNonCancelableSection(Cancellation.java:57) at com.intellij.openapi.progress.impl.CoreProgressManager.computeInNonCancelableSection(CoreProgressManager.java:236) at com.intellij.openapi.progress.impl.CoreProgressManager.executeNonCancelableSection(CoreProgressManager.java:227) at com.intellij.openapi.editor.impl.DocumentImpl.changedUpdate(DocumentImpl.java:910) at com.intellij.openapi.editor.impl.DocumentImpl.updateText(DocumentImpl.java:814) at com.intellij.openapi.editor.impl.DocumentImpl.deleteString(DocumentImpl.java:570) at com.intellij.openapi.editor.actions.BackspaceAction.lambda$doBackSpaceAtCaret$0(BackspaceAction.java:65) at com.intellij.openapi.editor.ex.util.EditorUtil.runWithAnimationDisabled(EditorUtil.java:1015) at com.intellij.openapi.editor.actions.BackspaceAction.doBackSpaceAtCaret(BackspaceAction.java:70) at com.intellij.openapi.editor.actions.BackspaceAction$Handler.executeWriteAction(BackspaceAction.java:32) at com.intellij.openapi.editor.actionSystem.EditorWriteActionHandler$1.run(EditorWriteActionHandler.java:42) at com.intellij.openapi.application.impl.ApplicationImpl.runWriteAction(ApplicationImpl.java:975) at com.intellij.openapi.editor.actionSystem.EditorWriteActionHandler.doExecute(EditorWriteActionHandler.java:56) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.execute(EditorActionHandler.java:202) at com.intellij.database.run.SqlNotebookDeleteHandler.doExecute(SqlNotebookDeleteHandler.java:30) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.execute(EditorActionHandler.java:202) at com.intellij.codeInsight.inline.completion.CancellationKeyInlineCompletionHandler.doExecute(InlineCompletionActions.kt:31) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.execute(EditorActionHandler.java:202) at com.intellij.codeInsight.editorActions.BackspaceHandler.handleBackspace(BackspaceHandler.java:91) at com.intellij.codeInsight.editorActions.BackspaceHandler.executeWriteAction(BackspaceHandler.java:47) at com.intellij.openapi.editor.actionSystem.EditorWriteActionHandler$1.run(EditorWriteActionHandler.java:42) at com.intellij.openapi.application.impl.ApplicationImpl.runWriteAction(ApplicationImpl.java:975) at com.intellij.openapi.editor.actionSystem.EditorWriteActionHandler.doExecute(EditorWriteActionHandler.java:56) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.lambda$execute$2(EditorActionHandler.java:192) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.doIfEnabled(EditorActionHandler.java:89) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.lambda$execute$3(EditorActionHandler.java:191) at com.intellij.openapi.editor.impl.CaretModelImpl.lambda$runForEachCaret$3(CaretModelImpl.java:302) at com.intellij.openapi.editor.impl.CaretModelImpl.doWithCaretMerging(CaretModelImpl.java:411) at com.intellij.openapi.editor.impl.CaretModelImpl.runForEachCaret(CaretModelImpl.java:311) at com.intellij.openapi.editor.impl.CaretModelImpl.runForEachCaret(CaretModelImpl.java:288) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.execute(EditorActionHandler.java:189) at com.intellij.openapi.editor.actions.DeleteSelectionHandler.executeWriteAction(DeleteSelectionHandler.java:33) at com.intellij.openapi.editor.actionSystem.EditorWriteActionHandler$1.run(EditorWriteActionHandler.java:42) at com.intellij.openapi.application.impl.ApplicationImpl.runWriteAction(ApplicationImpl.java:975) at com.intellij.openapi.editor.actionSystem.EditorWriteActionHandler.doExecute(EditorWriteActionHandler.java:56) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.lambda$execute$4(EditorActionHandler.java:199) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.doIfEnabled(EditorActionHandler.java:89) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.execute(EditorActionHandler.java:198) at com.intellij.codeInsight.lookup.impl.BackspaceHandler.doExecute(BackspaceHandler.java:25) at com.intellij.openapi.editor.actionSystem.DynamicEditorActionHandler.doExecute(DynamicEditorActionHandler.java:63) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.lambda$execute$4(EditorActionHandler.java:199) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.doIfEnabled(EditorActionHandler.java:89) at com.intellij.openapi.editor.actionSystem.EditorActionHandler.execute(EditorActionHandler.java:198) at com.intellij.openapi.editor.actionSystem.EditorAction.lambda$actionPerformed$0(EditorAction.java:92) at com.intellij.openapi.command.impl.CoreCommandProcessor.executeCommand(CoreCommandProcessor.java:225) at com.intellij.openapi.command.impl.CoreCommandProcessor.executeCommand(CoreCommandProcessor.java:177) at com.intellij.openapi.editor.actionSystem.EditorAction.actionPerformed(EditorAction.java:101) at com.intellij.openapi.editor.actionSystem.EditorAction.actionPerformed(EditorAction.java:77) at com.intellij.openapi.actionSystem.ex.ActionUtil.doPerformActionOrShowPopup(ActionUtil.java:344) at com.intellij.openapi.keymap.impl.ActionProcessor.performAction(ActionProcessor.java:32) at com.intellij.openapi.keymap.impl.IdeKeyEventDispatcher$myActionProcessor$1.performAction(IdeKeyEventDispatcher.kt:496) at com.intellij.openapi.keymap.impl.IdeKeyEventDispatcherKt.doPerformActionInner$lambda$4$lambda$3(IdeKeyEventDispatcher.kt:831) at com.intellij.openapi.application.TransactionGuardImpl.performActivity(TransactionGuardImpl.java:106) at com.intellij.openapi.application.TransactionGuardImpl.performUserActivity(TransactionGuardImpl.java:95) at com.intellij.openapi.keymap.impl.IdeKeyEventDispatcherKt.doPerformActionInner$lambda$4(IdeKeyEventDispatcher.kt:831) at com.intellij.openapi.actionSystem.ex.ActionUtil.performDumbAwareWithCallbacks(ActionUtil.java:381) at com.intellij.openapi.keymap.impl.IdeKeyEventDispatcherKt.doPerformActionInner(IdeKeyEventDispatcher.kt:829) at com.intellij.openapi.keymap.impl.IdeKeyEventDispatcherKt.access$doPerformActionInner(IdeKeyEventDispatcher.kt:1) at com.intellij.openapi.keymap.impl.IdeKeyEventDispatcher.processAction$intellij_platform_ide_impl(IdeKeyEventDispatcher.kt:559) at com.intellij.openapi.keymap.impl.IdeKeyEventDispatcher.processAction(IdeKeyEventDispatcher.kt:509) at com.intellij.openapi.keymap.impl.IdeKeyEventDispatcher.processActionOrWaitSecondStroke(IdeKeyEventDispatcher.kt:448) at com.intellij.openapi.keymap.impl.IdeKeyEventDispatcher.inInitState(IdeKeyEventDispatcher.kt:441) at com.intellij.openapi.keymap.impl.IdeKeyEventDispatcher.dispatchKeyEvent(IdeKeyEventDispatcher.kt:303) at com.intellij.ide.IdeEventQueue.dispatchKeyEvent(IdeEventQueue.kt:620) at com.intellij.ide.IdeEventQueue._dispatchEvent$lambda$11(IdeEventQueue.kt:581) at com.intellij.openapi.application.impl.RwLockHolder.runWithEnabledImplicitRead(RwLockHolder.kt:75) at com.intellij.openapi.application.impl.RwLockHolder.runWithImplicitRead(RwLockHolder.kt:67) at com.intellij.ide.IdeEventQueue._dispatchEvent(IdeEventQueue.kt:581) at com.intellij.ide.IdeEventQueue.access$_dispatchEvent(IdeEventQueue.kt:72) at com.intellij.ide.IdeEventQueue$dispatchEvent$processEventRunnable$1$1$1.compute(IdeEventQueue.kt:355) at com.intellij.ide.IdeEventQueue$dispatchEvent$processEventRunnable$1$1$1.compute(IdeEventQueue.kt:354) at com.intellij.openapi.progress.impl.CoreProgressManager.computePrioritized(CoreProgressManager.java:793) at com.intellij.ide.IdeEventQueue$dispatchEvent$processEventRunnable$1$1.invoke(IdeEventQueue.kt:354) at com.intellij.ide.IdeEventQueue$dispatchEvent$processEventRunnable$1$1.invoke(IdeEventQueue.kt:349) at com.intellij.ide.IdeEventQueueKt.performActivity$lambda$1(IdeEventQueue.kt:1014) at com.intellij.openapi.application.TransactionGuardImpl.performActivity(TransactionGuardImpl.java:114) at com.intellij.ide.IdeEventQueueKt.performActivity(IdeEventQueue.kt:1014) at com.intellij.ide.IdeEventQueue.dispatchEvent$lambda$7(IdeEventQueue.kt:349) at com.intellij.openapi.application.impl.ApplicationImpl.runIntendedWriteActionOnCurrentThread(ApplicationImpl.java:848) at com.intellij.ide.IdeEventQueue.dispatchEvent(IdeEventQueue.kt:391) at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:207) at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:128) at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:117) at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:113) at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:105) at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:92)

hmmm I am not sure how to get a line separator from the IDEA API. I cannot use VirtualFile::getDetectedLineSeparator() so I was guessing from system.

I will replace this with \n and hope it resolves your exception :/

JojOatXGME · 2024-07-13T11:32:07Z

src/main/java/org/nixos/idea/psi/impl/AbstractNixString.kt

+            LOG.info("not a nix ind string")
+            return this
+        }
+        val originalNode = astNode.firstChildNode.treeNext.firstChildNode as? LeafPsiElement


issue: This method doesn't seem to escape the input, and may therefore also remove existing escape sequences. For some reason, some lines were also duplicated for me after trying it with the following code:

pkgs.writeShellScript "my-script.sh" '' first_of_array=''${ARRAY[0]} from_nix=${lib.escapeShellArg someVar} ''

this is a good catch. I was aware this method did not escape the input and would remove existing escape sequences. This is because at the stage where we get the bash text, I have no way to know if the coming ${ is a bash interpolation or a Nix one, so I don't know whether it should be escaped. Likewise for newlines: If I get a \n char, should it get converted to an escaped ''\n or into an actual \n?

I think this is where the method of not injecting in the text fragment may have reached its limits :/

JojOatXGME · 2024-07-13T11:34:47Z

src/main/java/org/nixos/idea/psi/NixStringLiteralEscaper.kt

+         *
+         * @returns the minIndent of the string if successful, or null if unsuccessful.
+         */
+        fun unescapeAndDecode(chars: String, outChars: StringBuilder, sourceOffsets: IntArray?): Boolean {


note: I am not entirely happy with having two implementations to parse/unescape strings now. Anyway, that is not important right now. I may merge them at some point in the future.

- fix outdated javadoc - fix outdated valid injection host check

cottand · 2024-07-13T17:19:40Z

Thanks for your review 🙏

Of course you are welcome to try anything you please!

As for the escaping issue, you are correct that it's a blocker. I think I might have to re-implement this with injections directly in the text fragment, but I have to learn how it works for Java and Kotlin first. I do not see another way to get around the problem you pointed out. Note Kotlin (which has ${} interpolatinos) should suffer from the same

JojOatXGME · 2024-07-15T23:58:33Z

Just to give you some status update: I noticed that there were also some problems regarding the decoding of the strings. Most noticeably, the indentation is not detected correctly, and I think empty strings would cause issues. (I think the following assumption is not correct.)

nix-idea/src/main/java/org/nixos/idea/psi/NixStringLiteralEscaper.kt

Lines 75 to 77 in 7d2f79b

    
           if (c == '\n' && index < chars.length - 1) { 
        
               // we know that the next n chars are going to be whitespace indent 
        
               index += minIndent

After my first attempt to detect the indentation correctly (af39db9, only detection of indent), I got the idea that I could adjust the lexer to make the implementation much easier. I have now implemented the correct removal of the indentation at f47a403. I also made a first attempt to use this decoding in NixStringLiteralEscaper at 328d644, but it seems unpleasantly complex, and it doesn't work. (The indices I write into outSourceOffsets seem incorrect.)

I didn't yet manage to look into the main issue regarding the interpolations. I may also not have time for this topic for the second half of this week. :(

If you like, I can push these changes on your branch. I wasn't sure if you would like that, as some of my changes are quite different compared to the code you have touched so far, and pushing them into the PR would mean that they would eventually end up in "your" commit after squashing the PR.

cottand · 2024-07-20T09:05:11Z

hey @JojOatXGME ! sorry for the late replies. I will be a bit unresponsive the coming week but I am keeping an eye on this PR.

If you like, I can push these changes on your branch

Feel free to push into mine, if we need my work we can always branch off at 7d2f79b later.

The indices I write into outSourceOffsets seem incorrect

I had a lot of this in my initially. Over the coming week I will try to look at your new approach and see where the wrong offsets might be!

At any rate thanks for your continued support

JojOatXGME · 2024-07-28T18:26:31Z

While looking at this topic again, I noticed the following statement from the Javadoc of MultiHostInjector.

For the more high-level API consider implementing the LanguageInjectionContributor and/or LanguageInjectionPerformer

I gave it a try and implemented LanguageInjectionPerformer at ce8ee54. It worked much better than expected. Looks like IDEA implements some magic to make the rest just work. Note that I skipped some parts of what JetBrains' implementations of the interface do, like calling InjectorUtils.registerSupport. Not sure for what it is, seems to work without it.

Anyway, while looking into this topic, I also noticed further edge cases and bugs I tried to resolve. This is why the commit is much bigger. One remaining bug is that I get a strange RuntimeException when I don't start with an empty line. At some point, I started to look into tests, so that I can put the scenarios into tests before I continue to investigate them. However, I haven't come very far with this yet. See NixInjectionPerformerTest. I just pushed what I have right now. I will probably not look into this for the next two or three weeks.

See exception

(Looks like the indent of the first line is missing in the PSI/AST after patching, whatever patching means in this case.)

java.lang.RuntimeException: After patch: doc:
'  (__interpolation0__)
  first_of_array=''${ARRAY[0]}
  from_nix=(__interpolation1__)
  (__interpolation2__)
  (__interpolation3__)
  def
  (__interpolation4__)'
---PSI:
'(__interpolation0__)
  first_of_array=''${ARRAY[0]}
  from_nix=(__interpolation1__)
  (__interpolation2__)
  (__interpolation3__)
  def
  (__interpolation4__)'
---chars:
'(__interpolation0__)
first_of_array=${ARRAY[0]}
from_nix=(__interpolation1__)
(__interpolation2__)
(__interpolation3__)
def
(__interpolation4__)'.
OK let's see. Host file: Nix File in 'C:\Users\jojo\Entwicklung\nixpkgs\test.nix' (Language: Nix) 
Was injected Language: Shell Script at ranges: [Shred suffix='(__interpolation0__)' in NixStringTextImpl(STRING_TEXT) with range (40,42) (class org.nixos.idea.psi.impl.NixStringTextImpl) inside range (0,2), Shred suffix='(__interpolation1__)' in NixStringTextImpl(STRING_TEXT) with range (68,111) (class org.nixos.idea.psi.impl.NixStringTextImpl) inside range (0,43), Shred suffix='(__interpolation2__)' in NixStringTextImpl(STRING_TEXT) with range (137,140) (class org.nixos.idea.psi.impl.NixStringTextImpl) inside range (0,3), Shred suffix='(__interpolation3__)' in NixStringTextImpl(STRING_TEXT) with range (166,169) (class org.nixos.idea.psi.impl.NixStringTextImpl) inside range (0,3), Shred suffix='(__interpolation4__)' in NixStringTextImpl(STRING_TEXT) with range (195,204) (class org.nixos.idea.psi.impl.NixStringTextImpl) inside range (0,9), Shred in NixStringTextImpl(STRING_TEXT) with range (230,230) (class org.nixos.idea.psi.impl.NixStringTextImpl) inside range (0,0)]
	at com.intellij.psi.impl.source.tree.injected.InjectionRegistrarImpl.parseFile(InjectionRegistrarImpl.java:695)
	at com.intellij.psi.impl.source.tree.injected.InjectionRegistrarImpl.createAndRegisterInjected(InjectionRegistrarImpl.java:254)
	at com.intellij.psi.impl.source.tree.injected.InjectionRegistrarImpl.doneInjecting(InjectionRegistrarImpl.java:219)
	at org.nixos.idea.psi.NixInjectionPerformer.performInjection(NixInjectionPerformer.java:55)
	at ...

I also noticed that JetBrains' documentation at https://plugins.jetbrains.com/docs/intellij/language-injection.html mentions that language plugins should implement AbstractLanguageInjectionSupport. I ignored that so far. Doesn't look like we actually need it. Note that to implement it, we would need to add a dependency to the plugin org.intellij.intelliLang. (Btw, the same dependency is required to access InjectorUtils.)

cottand · 2024-09-28T17:58:06Z

Hey @JojOatXGME I am sorry for the late reply. First off, congrats on your progress with ce8ee54 it does mostly work.

This afternoon I have tried to make sense of the problem you are seeing and implement NixInjectionPerformerTest (also unsuccessfully). I am sorry I have not been much use other than with my flakey implementation of the offset-tracking. I would still love to see this feature in Nix-IDEA but I am seriously struggling to understand IDEA's internals. Again, apologies for the radio silence.

JojOatXGME · 2024-09-28T20:12:26Z

No worries. I am also struggling to understand IDEA from time to time. You were already a great help. 👍 While I may have changed some parts of your initial implementation, we wouldn't have brought it that far without your initiative to create some prototype. Unfortunately, I didn't have time over the last two months. Not entirely sure how my next weeks are going to look, but I believe, we can make it work in the not so far future. 😄

FYI, my hypothesis regarding the exception is that IDEA is confused with the first ignored line of the string. The following code in the lexer tells IntelliJ that the first line is insignificant whitespace and can be ignored during parsing.

nix-idea/src/main/lang/Nix.flex

Lines 77 to 80 in ce8ee54

    
           <IND_STRING_START> { 
        
             // The first line is ignored in case it is empty 
        
             [\ ]*\n               { replaceState(IND_STRING_START, IND_STRING_INDENT); return com.intellij.psi.TokenType.WHITE_SPACE; } 
        
           }

My best guess is that some part of IntelliJ is then implicitly truncating this insignificant whitespace during some internal processing, which then causes a different part of IntelliJ to fail, as it didn't expect that the whitespaces are missing now. At least this is the only idea I have right now which could explain the exception. Anyway, I would still favor to have some tests, even when we have resolved the exception.

Also, if you have any question about the change I did, feel free to ask. After all, I changed some code you haven't touched in this PR before.

cottand · 2024-09-28T21:15:36Z

Thanks! I agree and your hypothesis would make sense, but I don't understand the grammar enough to suggest a change that could test your theory. I also agree that proper tests would be ideal!

I think the only question I have is around this test case (and similar ones) that fail (as of ce8ee54). I started looking into fixing them but I do not think I fully understand why you expect it to not have the leading whitespace in the test.

JojOatXGME · 2024-09-28T22:10:09Z

I think the only question I have is around this test case (and similar ones) that fail (as of ce8ee54). I started looking into fixing them but I do not think I fully understand why you expect it to not have the leading whitespace in the test.

I don't remember if I tried to implement this case yet. I primarily wrote down what I expect as a user in the tests. Adding leading whitespace to an empty line would be unnecessary, as there is nothing to indent. At the same time, this whitespace would also be trailing whitespace. I personally don't like trailing whitespace and try to avoid it. I don't know if my tendency to avoid trailing whitespace is just a personal thing, or a bigger convention. I only know that Git classifies them as “whitespace errors” by default, but that only means that they are highlighted in diffs.

FYI: I think you should also be able to create review comments. This way, you can write your comments on the specific lines in the diff where you have the questions. You can also put your permanent code links into a separate line, GitHub will then actually inline a code snipped showing the code in your comment. (Only works if the link points to code in the same repository.)

cottand added 4 commits June 16, 2024 15:21

boilerplate for manipulator, fix bad unescaping of indStrings

4448751

split files, get highlighting working

02610cb

fix bad escaping of whitespace chars, handle one line string

1848120

more testing for escaping

6b6df0e

cottand marked this pull request as draft June 16, 2024 19:41

JojOatXGME requested changes Jun 17, 2024

View reviewed changes

cottand added 2 commits June 17, 2024 19:31

optimise imports in NixIndStringUtilTest

a442904

address review comments:

d709114

change wording in comment undo change signature of NixStringUtil.parse remove remnant in NixIndStringUtilTest make NixIndStringUtil.escape JvmStatic

cottand commented Jun 17, 2024

View reviewed changes

cottand added 2 commits July 6, 2024 20:14

remove leftover type mismatch

88c8dce

first iteration of offset tracking

9bc2749

cottand force-pushed the lang-injections2 branch from 4fd9e59 to 9bc2749 Compare July 6, 2024 22:31

cottand added 6 commits July 6, 2024 23:40

fix not covering happy path that has no escaping

82d8b4c

add test case for $ escapings

6d88fd6

fix not being able to inject in-fragment

d8086c8

fix bad replacing where quotes where duplicated

f97fee3

support indent in fragment editor

43664e2

fix backtracking with ' and remove minIndent parsing in decode

d675347

cottand requested a review from JojOatXGME July 7, 2024 11:00

cottand marked this pull request as ready for review July 7, 2024 11:00

JojOatXGME requested changes Jul 13, 2024

View reviewed changes

address review:

ade4a64

- fix outdated javadoc - fix outdated valid injection host check

replace system line separator with '`n'

7d2f79b

JojOatXGME added 2 commits July 16, 2024 00:49

Use lexer to help detect indentation of strings

f47a403

Use NixStringUtil in NixStringLiteralEscaper

328d644

Refactor to use NixInjectionPerformer (still buggy)

ce8ee54


		class NixStringLiteralEscaper(host: AbstractNixString) : LiteralTextEscaper<PsiLanguageInjectionHost>(host) {

		override fun isOneLine(): Boolean = false

Implement basic one-off language inections #82

Are you sure you want to change the base?

Implement basic one-off language inections #82

Conversation

cottand commented Jun 16, 2024 • edited Loading

cottand commented Jun 16, 2024

JojOatXGME commented Jun 17, 2024

JojOatXGME left a comment

Choose a reason for hiding this comment

JojOatXGME Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JojOatXGME Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cottand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JojOatXGME commented Jun 22, 2024 • edited Loading

cottand commented Jul 6, 2024

cottand commented Jul 7, 2024 • edited Loading

JojOatXGME commented Jul 7, 2024

JojOatXGME left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cottand Jul 13, 2024 • edited Loading

Choose a reason for hiding this comment

JojOatXGME Jul 13, 2024 • edited Loading

Choose a reason for hiding this comment

cottand commented Jul 13, 2024

JojOatXGME commented Jul 15, 2024 • edited Loading

cottand commented Jul 20, 2024

JojOatXGME commented Jul 28, 2024 • edited Loading

cottand commented Sep 28, 2024 • edited Loading

JojOatXGME commented Sep 28, 2024 • edited Loading

cottand commented Sep 28, 2024 • edited Loading

JojOatXGME commented Sep 28, 2024 • edited Loading

cottand commented Jun 16, 2024 •

edited

Loading

JojOatXGME Jun 17, 2024 •

edited

Loading

JojOatXGME Jun 17, 2024 •

edited

Loading

JojOatXGME commented Jun 22, 2024 •

edited

Loading

cottand commented Jul 7, 2024 •

edited

Loading

JojOatXGME left a comment •

edited

Loading

cottand Jul 13, 2024 •

edited

Loading

JojOatXGME Jul 13, 2024 •

edited

Loading

JojOatXGME commented Jul 15, 2024 •

edited

Loading

JojOatXGME commented Jul 28, 2024 •

edited

Loading

cottand commented Sep 28, 2024 •

edited

Loading

JojOatXGME commented Sep 28, 2024 •

edited

Loading

cottand commented Sep 28, 2024 •

edited

Loading

JojOatXGME commented Sep 28, 2024 •

edited

Loading