Syntax highlighting error when \ used to put if/unless on a new line

[kaldrenon]

Example:

def foo
  return false if call_some_function(args)
end

In the above case, the end will be correctly highlighted to match the def

def foo
  return false \
    if call_some_function(args)
end

Now, with the if on the line below, the end will be incorrectly highlighted as the match for the if

The code still performs correctly (the syntax is valid in Ruby) but the highlighting gets confused. This is more problematic when the block is nested further, and it is harder to visually detect the correct nesting without the help of the highlight.

May not present a problem in all color schemes, if the color is the same for class and if. Problem presented when using the wombat256mod colorscheme

[AndrewRadev]

The problem here is that we don't care for line continuations. When vim-ruby sees the if-clause with nothing but whitespace before it, it assumes it's a full if-clause with a matching "end".

What I could do is squeeze a \%(\\\n\s*\)\<@! at the front of the relevant pattern. In short, what this does is assert that the if-clause pattern does not follow a backslash at the end of the previous line. However, I'm not sure how far-reaching this would be, and I'm not sure how much it would affect performance for large files. There was recently some talk about poor syntax highlighting performance for ruby in the vim mailing list and it's true that we use some pretty complex regexes to catch a bunch of edge cases.

At this time, I'm strongly inclined to just not support line continuations. I believe they're not encountered so often in ruby code and they would complicate handling of both this case and possibly many others. A related issue is that your example also doesn't indent correctly, which means that should also be tackled.

In this case, I'd personally just use a multiline if-clause:

def foo
  if call_some_function(args)
    return false
  end
end

Is there some reason to prefer your variant as opposed to the multiline one?

[kaldrenon]

At the moment, my only real motive for supporting line continuations is that I work somewhere which treats it as preferred style (even though I personally do not). It can be frustrating to have closing end keywords be mis-colored, and for = operations to fail because an end keyword is being mistreated.

If the possible negative effects of supporting the format are too significant, I can understand not supporting it. In a broader style sense, I dislike line continuations, but if I change existing ones in code at work, they'll get reverted or questioned.

I may try dropping your suggestion into my local version of vim-ruby and look for any negative side effects.

[AndrewRadev]

I work somewhere which treats it as preferred style (even though I personally do not).

I see, this was what I was afraid of. It's much more likely to convince a person than a corporation :). Even so, I think we both agree this style is not particularly conventional, so I'm not keen on the idea of supporting it. For now, do try the change and see how it turns out.

The relevant line is here: https://github.com/vim-ruby/vim-ruby/blob/master/syntax/ruby.vim#L229. The replacement for it would be:

syn region rubyConditionalExpression matchgroup=rubyConditional start="\%(\%(\\\n\s*\)\<@!\%(^\|\.\.\.\=\|[{:,;([<>~\*/%&^|+=-]\|\%(\<[_[:lower:]][_[:alnum:]]*\)\@<![?!]\)\s*\)\@<=\%(if\|unless\)\>" end="\%(\%(\%(\.\@<!\.\)\|::\)\s*\)\@<!\<end\>" contains=ALLBUT,@rubyNotTop fold

[ilya-bobyr]

I think that if the \%(\\\n\s*\)\<@! exception is inserted after the \@<= that is already there the performance hit should be rather small. Only in case when there is an "if" modifier Vim will do the check to see if it is not following a line continuation character. With \@<= filtering out most cases already.

I have created a pull request with the change: #267

[AndrewRadev]

The performance issue is not the big problem here, it's that we just don't support backslashes like this. As I said, that particular example breaks indentation as well (as far as I remember). Even if we fix this issue, we just don't have much of a plan for dealing with these kind of constructs. Most ruby codebases can just use a different syntax.

Still, I won't close the PR out of fear of (unknown) bad consequences. I'll go ahead and merge it, but bear in mind that if we end up with related issues I may revert it.