Incorrect highlighting with .fish files

[orf]

Given the following small snippet of fish code:

test -n "$_CONDA_EXE"; or set _CONDA_EXE "$PWD/shell/bin/conda"

test -n "$CONDA_SHLVL"; or set -gx CONDA_SHLVL "0"

function __conda_add_prompt
  if set -q CONDA_DEFAULT_ENV
      set_color normal
      echo -n '('
      set_color -o green
      echo -n $CONDA_DEFAULT_ENV
      set_color normal
      echo -n ') '
  end
end

Syntect seems to render the following, at least through bat:

Not sure if this should be reported here or in bat, so sorry if this is the wrong place 👍

[keith-hall]

for reference, it seems like bat is using the ShellScript/bash syntax from https://github.com/sublimehq/Packages to highlight .fish files. I'm not at a computer right now to see how it looks in Sublime Text, to help determine if it is a problem with the syntax definition or syntect, but if nobody beats me to it, I will hopefully be able to check in about 12 hours time. Thanks for reporting btw.

[sharkdp]

This is how it looks like in Sublime Text:

It looks slightly different (I'm not quite sure if my Sublime Text uses the same version of sublimehq/Packages as bat) but the main issue is definitely that the Fish syntax is quite different from Bash syntax. So it really looks like a sublimehq/Packages issue.

Syntect users could potentially solve the problem by adding a dedicated Fish syntax (and possibly also Zsh), but I'm not sure how file_extension conflicts are handled (which .sublime-syntax takes precedence?)

[keith-hall]

I believe precedence works in Sublime Text by lexicographical order https://github.com/guillermooo/sublime-undocs/blob/a571f5ba987176607f12bb141965bf4596775eeb/source/extensibility/packages.rst#id62, so a package named ShellScriptFish would take precedence over ShellScriptBash (for example) were they to have conflicting file extension mappings. Users of Sublime Text can also set a preference to open particular extensions with a particular syntax, which always takes priority over what is in the syntax definition.

[keith-hall]

in syntect, it seems the syntax definitions are searched in the order they are added to the syntax set, and the first one with a matching file extension is used. Currently, I believe the syntax set builder which loads from a folder doesn't load the definitions in a deterministic order. We could perhaps change it to behave similar to how syntest orders/sorts the syntax test files. Then, perform the search in reverse to get the same behavior as Sublime Text of lexicographical order overrides.

[robinst]

Sounds good. I'm not sure if that's the loading code that you're talking about, but there is sorting here:

https://github.com/trishume/syntect/blob/master/src/parsing/syntax_set.rs#L336

So maybe it's just about doing the search in reverse.

[keith-hall]

You're right, somehow I didn't notice that earlier. 👍 It looks like the first_line_match caching will be a bit more complicated, due to the way syntaxes can be added after some first line match entries are cached, so just caching in reverse isn't quite going to be suitable I think?

[robinst]

Ah, we can actually change how that code works now that SyntaxSet is immutable and we have SyntaxSetBuilder. The length check is no longer necessary, we just have to initialize it lazily.

If you're not comfortable changing it, I can look at raising a PR to change it tomorrow.

[keith-hall]

oops, just realized I forgot to comment - if you could spare a few mins to raise a PR for this, that'd be grand @robinst, thanks - save me botching it and thus taking more of our time to review and fix it ;)

[robinst]

@keith-hall done :) #216

I'll leave the actual logic change to you, so that I don't botch it and take your time to review and fix it ;)

[Enselic]

I haven't double checked, but sounds as if the merged PR #217 fixed this problem, so I'm closing this issue now.

Feel free to reopen if I am mistaken of course.