MEGA-ISSUE: Syntax highlighting in `language-shellscript`

savetheclocktower commented 10 months ago

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 commented 10 months ago

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

savetheclocktower commented 10 months ago

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 commented 10 months ago

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

Legacy: legacy

Modern: modern

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

Tamaranch commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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):

cmd-subst

savetheclocktower commented 10 months ago

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 commented 10 months ago

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

savetheclocktower commented 9 months ago

@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 commented 9 months ago

It may be a little better, but it's not fixed. The case I added in https://github.com/tree-sitter/tree-sitter-bash/issues/242#issuecomment-1906662462 is not fixed:

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

param-exp-bash

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'}")

highlight-broken

Tamaranch commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

To be clear: this just happened for the first time when you updated to a build that you downloaded from https://github.com/pulsar-edit/pulsar/pull/906's 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 commented 9 months ago

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

savetheclocktower commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

savetheclocktower commented 8 months ago

@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 commented 8 months ago

I can confirm that https://github.com/pulsar-edit/pulsar/issues/873#issuecomment-1941375562 is fixed now :+1:

pulsar-edit / pulsar