Syntax highlighting broken when a variable is inserted in a heredoc

[fletchto99]

Description

Syntax highlighting is broken after the heredoc when a variable is inserted if it's not surrounded by double quotes. See the examples provided below. The only change was on line 6, I removed the " around the pr_number variable.

Ruby LSP Information

Ruby LSP Information

VS Code Version

1.92.2

Ruby LSP Extension Version

0.7.17

Ruby LSP Server Version

0.17.16

Ruby LSP Addons

Ruby Version

3.2.2

Ruby Version Manager

none

Installed Extensions

Click to expand

• github-markdown-preview (0.3.0)
• markdown-checkbox (0.4.0)
• markdown-emoji (0.3.0)
• markdown-footnotes (0.1.1)
• markdown-mermaid (1.23.1)
• markdown-preview-github-styles (2.0.4)
• markdown-yaml-preamble (0.1.0)
• markdown-table-prettify (3.6.0)
• vscode-markdownlint (0.55.0)
• languagetool-linter (0.21.4)
• vscode-eslint (3.0.10)
• prettier-vscode (11.0.0)
• markdown-table-formatter (3.0.0)
• copilot (1.223.0)
• copilot-chat (0.18.2)
• vscode-github-actions (0.26.3)
• vscode-pull-request-github (0.94.0)
• go (0.42.0)
• vscode-docker (1.29.2)
• debugpy (2024.10.0)
• isort (2023.10.1)
• python (2024.12.3)
• vscode-pylance (2024.8.2)
• azurecli (0.6.0)
• vscode-typescript-next (5.7.20240825)
• wordcount (0.1.0)
• vsliveshare (1.0.5936)
• vscode-xml (0.27.1)
• vscode-yaml (1.15.0)
• remove-tabs-on-save (1.2.4)
• ruby-extensions-pack (0.1.11)
• ruby-lsp (0.7.17)
• vscode-fileutils (3.10.3)
• sorbet-vscode-extension (0.3.36)
• code-spell-checker (3.0.1)
• vscode-stylelint (1.4.0)
• shellcheck (0.37.1)
• alex-linter (0.6.6)
• write-good-linter (0.1.5)
• markdown-all-in-one (3.6.2)

Ruby LSP Settings

Click to expand

Workspace

{}

User

{
  "enableExperimentalFeatures": false,
  "enabledFeatures": {
    "codeActions": true,
    "diagnostics": true,
    "documentHighlights": true,
    "documentLink": true,
    "documentSymbols": true,
    "foldingRanges": true,
    "formatting": true,
    "hover": true,
    "inlayHint": true,
    "onTypeFormatting": true,
    "selectionRanges": true,
    "semanticHighlighting": true,
    "completion": true,
    "codeLens": true,
    "definition": true,
    "workspaceSymbol": true,
    "signatureHelp": true,
    "typeHierarchy": true
  },
  "featuresConfiguration": {},
  "rubyVersionManager": {
    "identifier": "none"
  },
  "customRubyCommand": "",
  "formatter": "auto",
  "linters": null,
  "bundleGemfile": "",
  "testTimeout": 30,
  "branch": "",
  "pullDiagnosticsOn": "both",
  "useBundlerCompose": false,
  "bypassTypechecker": false,
  "rubyExecutablePath": "",
  "indexing": {},
  "erbSupport": true
}

Reproduction steps

Write a heredoc with a variable that isn't surrounded by dobule quotes.

Normal highlighting:

Broken highlighting (but still valid ruby):

[snutij]

Hi @fletchto99, do we still have this issue, even with the latest version of VS Code (v1.95) and the ruby-lsp extension (v0.8.10)? I'm asking because I can't reproduce the same behavior. If you still experience this, could you try the extension bisect debug to point out the faulty one?

code snippets

class DemoOK
  def check_automerge_status(pr_number)
    query = <<~GRAPHQL
      query {
        repository(owner: "foo", name: "bar") {
          pullRequest(number: "#{pr_number}") {
            isInMergeQueue
          }
        }
      }
    GRAPHQL

    response = client.post("/graphql", { query: query }.to_json)
    response[:data][:repository][:pullRequest][:isInMergeQueue]
  end
end

class DemoKO
  def check_automerge_status(pr_number)
    query = <<~GRAPHQL
      query {
        repository(owner: "foo", name: "bar") {
          pullRequest(number: #{pr_number}) {
            isInMergeQueue
          }
        }
      }
    GRAPHQL

    response = client.post("/graphql", { query: query }.to_json)
    response[:data][:repository][:pullRequest][:isInMergeQueue]
  end
end

[vinistock]

The problem here is the intersection of grammars. Inside a heredoc using the GRAPHQL delimiter, we mark the language as graphql so that if you have an extension providing a grammar for GraphQL, it will highlight it properly.

However, if you interpolate something in the middle, then it's likely that it will break the GraphQL grammar as it will try to understand the interpolation, but that's not valid GraphQL.

I'm not sure if we could use semantic highlighting to inform the editor that only the variable interpolation is Ruby source and the rest is still GraphQL source.

[fletchto99]

Ahh I think I found the issue. I also had the GraphQL: Syntax Highlighting plugin enabled, which supports highlighting in ruby files. That's what seems to break the highlighting. I guess this would be a bug with the GraphQL Syntax highlighting plugin then?

With it enabled:

With it disabled:

[vinistock]

Does it support highlighting in Ruby files? Or does it register for the graphql language ID? If it's the former, then it's a bug in that extension. If it's the latter, then it's the issue I mentioned.

Inside the heredoc, the language ID is graphql, which means the grammar is applied, but a string interpolation is not valid GraphQL, which leads to the incorrect highlighting.

This will happen when using any heredoc with a language identifier and interpolation.

# Inside this block, the language ID is SQL. If you have an extension registered for it, it will
# attempt to run its grammar on this code. But the interpolation syntax #{} is not valid SQL code
# so that will lead to highlighting errors
<<~SQL
  SELECT #{some_var} FROM #{some_table}
SQL

[SampsonCrowley]

I don't even have a GraphQL extension installed whatsoever, but heredoc highlighting is severely broken with interpolation...

Thankfully it only seems to last for the duration of the line, but the areas within the doc aren't even recognized as being a string...

[vinistock]

I don't even have a GraphQL extension installed whatsoever

A GraphQL extension wouldn't affect a SQL block since the language ID there is sql and not graphql. It's likely a SQL extension that provides the grammar for that.

I don't believe there's an easy way to improve this. We would need to make the text mate grammar, which is based on regexes, be able to parse Ruby code embedded in the middle of another language, identified by the heredoc delimiter.

Which would also mean, being able to parse with the grammar whatever language is identified by the delimiter (SQL, GraphQL or whatever). This would essentially mean trying to support every possible language someone might want to put in a heredoc in our grammar file, which does not scale. But beyond that, it doesn't actually fix the issue since there's runtime dependencies.

The problem here is not that the Ruby grammar we provide is failing to parse the heredoc. It's that the grammars responsible for the other language IDs don't expect the interpolation and therefore fail to highlight the content. And they have no way of understanding the interpolation since it may depend on runtime aspects.

Consider this

<<~SQL
  SELECT #{columns_i_want.reject { |c| IGNORED_COLUMNS.any?(c) }.join(",")} FROM users;
SQL

This SQL statement is only valid SQL code after the interpolation happens, which depends on runtime values. There's no way for a text mate grammar to process this and know with confidence if this is valid SQL or not.

[SampsonCrowley]

I don't believe there's an easy way to improve this. We would need to make the text mate grammar, which is based on regexes, be able to parse Ruby code embedded in the middle of another language, identified by the heredoc delimiter.

It's all ruby first

I don't think it should be that big of a lift that a ruby heredoc is treated as a ruby heredoc. Also known as a string

Would you be so cavalier in saying "there's nothing we can do" is a plain double quote string had the same problem just because the string contains SQL?

No one asked for it to correctly parse interpolated SQL. We asked for it to correctly display what it is. ruby

[midnightmonster]

@SampsonCrowley the "try to highlight this as SQL (or GraphQL, or whatever)" behavior is driven by the tag you use for the heredoc. If you want Ruby string highlighting, use a tag that doesn't imply some other grammar. E.g., the app I'm working on has to talk to Salesforce, which uses its own dialect of SQL that it calls SOQL. A bit of query composition code in the app looks like this (don't worry, we control all the pieces that go into the build—this is not a S(O)QL injection vulnerability):

With SOQL tag (which doesn't imply another grammar), ruby highlighting is correct

With SQL tag, ruby highlighting is broken

FTR, I've only just finally understood this properly (I think!) in reacting to your comment. I had not noticed till now that what I thought was a bug was just a grammar fight and I could avoid it by changing my heredoc tags.

[vinistock]

I don't think it should be that big of a lift that a ruby heredoc is treated as a ruby heredoc. Also known as a string
Would you be so cavalier in saying "there's nothing we can do" is a plain double quote string had the same problem just because the string contains SQL?

I didn't say there's nothing we can do, I said I don't believe there's an easy way to improve this. If we can find a way, then I'm all for it.

However, treating heredocs as pure strings would be your preference, which isn't guaranteed to be everyone's. The moment we do that and people lose syntax highlighting for heredocs using the SQL and GraphQL identifier (and all the other ones we support), it would only be a matter of time until someone opened an issue complaining that the highlighting is gone. Especially given that this feature has existed and been carried over since this grammar was first imported to Atom from Text Mate. And then what do we do? How do we know what's the majority's preference in a feature that has existed for so long?

The ideal solution would be if we could detect that there's interpolation in the heredoc and then not add the language identifier in those cases, but I'm not sure if it's possible. It would essentially be a lookahead to detect interpolation before we assign the token to the entire heredoc block, which is bound to have corner cases and false positives/negatives.

There's tension between two facets for this issue. The first one is technical, how can we avoid undesired highlighting under interpolation scenarios? The second one is cultural, is fully removing this identifier behaviour the right move?

[vinistock]

In perfect timing, we just got a request to expand the support to handle ERB too #2863. I believe removing support for this would frustrate many users, but I hope it's feasible to detect the interpolation somehow and turn it off in those cases.