dracula / visual-studio-code

🧛🏻‍♂️ Dark theme for Visual Studio Code
https://draculatheme.com/visual-studio-code
MIT License
776 stars 208 forks source link

shellscript strings are not highlighted inside a `for ((` loop #212

Closed lilyball closed 1 month ago

lilyball commented 2 years ago

If I write a for (( exp1; exp2; exp3; )); do loop in shellscript, any strings in the body of the loop do not get colored.

screenshot demonstrating the lack of color on this type of shellscript for loop

The editor scope inspector says that the color is coming from meta.scope.for-loop.shell string, which appears to be the following rule:

{
    "name": "Edge cases (foreground color resets)",
    "scope": [
        "constant.other.symbol.hashkey.ruby",
        "keyword.operator.dereference.java",
        "keyword.operator.navigation.groovy",
        "meta.scope.for-loop.shell punctuation.definition.string.begin",
        "meta.scope.for-loop.shell punctuation.definition.string.end",
        "meta.scope.for-loop.shell string",
        "storage.modifier.import",
        "punctuation.section.embedded.begin.tsx",
        "punctuation.section.embedded.end.tsx",
        "punctuation.section.embedded.begin.jsx",
        "punctuation.section.embedded.end.jsx",
        "punctuation.separator.list.comma.css",
        "constant.language.empty-list.haskell"
    ],
    "settings": {
        "foreground": "#F8F8F2"
    }
}

I do not understand why this rule even exists. I haven't experimented with the other matching scopes here, but the meta.scope.for-loop.shell rules certainly seem out of place. meta.scope.for-loop.shell doesn't match anything else in the theme so I don't know what it's supposed to be resetting, and strings are not supposed to have the source foreground color.

dsifford commented 2 years ago

I couldn't recall either what the edge case was for, but after looking into it, it looks like without that set then the contents inside the (( )) of the loop are highlighted like this:

image

So one way or the other, something looks incorrect.

lilyball commented 2 years ago

@dsifford What you show is the highlighting that you would also get with just

(( a = 1; a <= 3; a++ ))

(except your parens aren't highlighted because I'm guessing you only disabled meta.scope.for-loop.shell string and not meta.scope.for-loop.shell punctuation.definition.string.begin/end)

In particular, this highlighting is because this construct is a math expression and ends up with the scope string.other.math.shell, which ends up with the base color being highlighted as string[^1]. So removing the meta.scope.for-loop.shell string rule (and punctuation variants) is still correct. I expect it's using a string base for the scope because it's assuming that this is usually going to be a $(( … )) construct even when it's not.

In any case there are certainly issues that should be fixed in the shellscript syntax definition, but with the current state of things it's perfectly correct for the theme to color for (( … )) exactly as it colors (( … )), and since there's no way to distinguish between the (( … )) expressions in for (( … )); do (( … )); done then the theme must treat them the same (and should treat them as a (( … )) expression outside of a for loop).

[^1]: Ideally the ; chars would actually be highlighted as operators in the for loop (but it looks like the for loop syntax doesn't bother giving the repeat expression its own scope so we can't actually color that differently without affecting math expressions in the loop body) but that's the only real quibble I have with this. Also this same scope isn't used for let a = 1 which also seems like a syntax definition bug as that's what (( … )) is sugar for.