MEGA-ISSUE: Syntax highlighting in language-shellscript

[savetheclocktower]

IMPORTANT: Some issues have already been fixed!

If you’re still on the regular 1.113 release, you might be suffering from a problem that has already been fixed. Many fixes landed on master in #859. You are encouraged to download the latest rolling release — both (a) to see whether what you’re reporting has been fixed, and (b) so that you can enjoy the fixes that have been reported since the 1.113 release.

---

This will serve as a catch-all issue for any problems you may encounter with syntax highlighting in shell scripting (language-shellscript). If you have problems with the new syntax highlighting (or folds, or indents) that are specific to shell scripts, keep reading.

Something isn't highlighting correctly!

If there are any highlighting regressions since 1.113:

First, please scroll down and see if someone else has reported the issue. If not, please comment with

• A small amount of sample code to reproduce the issue
• Screenshots (before and after would be ideal, but just the “after” is OK for obvious things

I want to go back to the old highlighting!

You can easily opt out of the new Tree-sitter highlighting for any language, but first please:

• subscribe to this issue so you'll know when the problem is fixed
• remove the config change when the next release comes out so that you can enjoy the new grammar once again

To revert to the old Tree-sitter grammar for this language only, add the following to your config.cson:

".shell.source":
  core:
    useLegacyTreeSitter: true

To revert to the TextMate-style grammar for this language only, add the following to your config.cson:

".shell.source":
  core:
    useTreeSitterParsers: false

[savetheclocktower]

@Tamaranch reported that the while keyword is unhighlighted. This is on my radar and will be fixed very soon. (Fixed; see below.)

[savetheclocktower]

The while keyword is now highlighted; this commit contains two other small fixes. Subscribe to #859 to be notified when this fix (and many others) land.

[Tamaranch]

Some builtin commands not highlighted: declare, readonly.
I also think there is a bug with command substitution (here with One Dark theme):

Legacy:

Modern:

Some files for comparison: https://gitlab.xfce.org/apps/mousepad/-/tree/master/tests

[Tamaranch]

Mixing single and double quotes with parameter expansion breaks highlighting in various ways. These are bashisms though, I don't think it's reproducible with POSIX parameter expansions, but it used to be supported.

After this, the rest of the code is no longer highlighted:

var="${var/'a'*/b}"

After this everything (or almost everything) is then highlighted as strings:

var="${var/'a'*/'b'}"

[savetheclocktower]

I expect some of these to be bugs in tree-sitter-bash, sadly, but I won't know for sure until I try some of these tonight.

[savetheclocktower]

I also think there is a bug with command substitution (here with One Dark theme):

A few bugs here! One is that the $( is not scoped with a punctuation scope like ) is. One Dark is unusual in that it explicitly scopes punctuation scopes to be a plain text color in most cases, rather than just inheriting whatever color the surrounding construct is.

The legacy Tree-sitter grammar scopes var=$(cmd) strangely: the $( and ) have no scopes, and the cmd is scoped as entity.name.function.

The TextMate grammar scopes the whole thing as string.interpolated.dollar.shell, with proper punctuation scopes for the delimiters.

The new grammar scopes the whole thing as string.interpolation.backtick.shell, which is… wrong. The string.interpolation part is wrong in a couple of ways! Obviously the backtick part is wrong, and that's happening because both flavors of command substitution share a node name, so I'll have to distinguish those.

But interpolated is more correct than interpolation there. I'll make those changes.

[savetheclocktower]

After this, the rest of the code is no longer highlighted:

var="${var/'a'*/b}"

@Tamaranch, how would you describe what that line of code is doing? I know a bit about parameter expansion in bash, but I'm flummoxed by that example even after messing around with it in a sample script.

I ask because this is almost surely a tree-sitter-bash bug, and when I file it against the project, I want to be able to explain its purpose and why it's valid. The fact that it's a bashism wouldn't necessarily be a problem; I don't think they're limiting themselves to things in POSIX standards.

[savetheclocktower]

Meanwhile, everything else you mentioned is addressed in this commit. I'll open a ticket against tree-sitter-bash when I hear back from you about the parameter expansion thing.

[Tamaranch]

how would you describe what that line of code is doing?

Replace the first occurrence of a followed by anything (i.e. the longest suffix of the form a*) with b in $var.
This can be seen as an extension of the POSIX parameter expansion ${var%%a*} which removes the longest suffix of the form a* (i.e. replaces it with nothing).

For reference, the actual lines of code I have locally are the following, but I've tried to extract the simplest example:

curf="${f%'-roff2html'*}.html"
reff="${f/'-roff2html'*/'-ref'}.html"

The first doesn't break highlighting, the second does.

Note that in all these cases, neither single nor double quotes are necessary (double quotes might be if there was concatenation with a string containing spaces, but single quotes never are I think).
However, their use is legal, and it may (or may not) be considered that their use makes the code more readable, particularly with correct highlighting.

[Tamaranch]

Meanwhile, everything else you mentioned is addressed in this commit.

I can confirm it's fixed, but I'm not sure if it's best to use the same color for command substitutions and strings. It makes it harder to distinguish between the command and its parameters when strings are involved, which is often the case (it seems to be theme-independent, here with One Dark):

[savetheclocktower]

Meanwhile, everything else you mentioned is addressed in this commit.

I can confirm it's fixed, but I'm not sure if it's best to use the same color for command substitutions and strings. It makes it harder to distinguish between the command and its parameters when strings are involved, which is often the case (it seems to be theme-independent, here with One Dark):

That's fair, but it's something that'll have to be fixed in themes. It's hard to conceive of backtick-style command substitution as a string (which it pretty much is) but $()-style command substitution as not a string, semantically speaking. I'll see what I can do about it in the builtin themes.

[savetheclocktower]

Filed the ticket about parameter expansion as tree-sitter-bash#242.

[savetheclocktower]

@Tamaranch, I bumped tree-sitter-bash and the parameter expansion example now appears to be parsing reasonably.

Before

After

I snuck it into #906 so it'll likely make it into 1.114 in a few days.

[Tamaranch]

It may be a little better, but it's not fixed. The case I added in tree-sitter/tree-sitter-bash#242 (comment) is not fixed:

reff="${f/'-roff2html'*/'-ref'}.html"

It no longer breaks the highlighting in the rest of the script I have locally though, but here's another one of the same type that breaks it:

manpages_path=("${manpages_path[@]/#[^'/']*/'/none'}")

[Tamaranch]

@savetheclocktower Also opening this file crashes the editor for me after this change: https://gitlab.xfce.org/apps/mousepad/-/blob/9733d85a611770c72b59c61525766187d99af0f3/tests/functions-test.sh

Attempting to call a function in a renderer window that has been closed or released.
Function provided here: worker-manager.js:294:26
Remote event names: crashed, destroyed

[savetheclocktower]

@Tamaranch The “good news” on the crash is that that file seems to come from from the github package, so it's unrelated to the parser change. But let me know if you can reproduce it consistently.

I've added a comment on the tree-sitter-bash issue pointing out the test case that's still broken.

[Tamaranch]

The “good news” on the crash is that that file seems to come from from the github package, so it's unrelated to the parser change. But let me know if you can reproduce it consistently.

Well, disabling the github package removes the above message, but not the crash. And the file mentioned refuses to open even though it opened correctly before.

Also, enabling Legacy Tree-sitter allows it to open again. Does this file open correctly for you?

[savetheclocktower]

Also, enabling Legacy Tree-sitter allows it to open again. Does this file open correctly for you?

It does, in all scenarios. That's… annoying.

To be clear: this just happened for the first time when you updated to a build that you downloaded from #906's CI artifacts?

I'll try some stuff tonight. If I still can't reproduce this, I'll remove that change from #906.

[Tamaranch]

To be clear: this just happened for the first time when you updated to a build that you downloaded from #906 CI artifacts?

Yes I was using Pulsar-1.113.2024020421.AppImage from #906 until yesterday, then I downloaded Pulsar-1.113.2024021307.AppImage (also from #906) to test the shell syntax patch, and it's with this version that the file refuses to open.

[savetheclocktower]

OK. I'll test that specific build tonight and see if I can reproduce this. Thanks as always for the report.

[savetheclocktower]

Sure enough!

The rest of the stack trace isn't relevant; this is just the very first parsing attempt. I don't think I'd even typed anything before it gave me this error. I probably used the wrong version of Emscripten or something stupid like that.

I rolled the change back in #906. @Tamaranch, my only consolation is that you've told me that this was barely a fix in the first place. At least that removes the urgency! I'll attempt this again in a few weeks.

[danfuzz]

FWIW it looks like https://github.com/tree-sitter/tree-sitter-bash has gotten some attention over the last month or so, and at least one of the bugs I filed on that project has gotten addressed, which still seems to be present in Pulsar's version. E.g., try this:

cat >x <<< 'x'
oops this line is highlighted as a string

[savetheclocktower]

OK, I'll give this another shot when I get a chance. Hopefully it goes better than last time.

[savetheclocktower]

@Tamaranch @danfuzz I've just updated our custom version of web-tree-sitter to 0.20.9. Once I did that, I was able to bump tree-sitter-bash to the most recent commit — the same commit as the one I'd tried a few weeks ago. Nothing appears to have exploded.

My best hypothesis as to why this happened is that I was generating these recent .wasm files with a newer version of Tree-sitter than the one we had in the repository, but I can only guess.

But you can try it yourself if you like by downloading a nightly build for your platform from the artifacts at the bottom of this page (once they're done building in a half-hour or so).

[Tamaranch]

I can confirm that #873 (comment) is fixed now 👍