mustache / vim-mustache-handlebars

mustache and handlebars mode for vim
mustache.github.io
455 stars 49 forks source link

Usage of `s:in_block_expr` causes non-deterministic behaviour #91

Closed AndrewRadev closed 4 years ago

AndrewRadev commented 5 years ago

I'm noticing issues with this particular snippet in the example file:

<div>
  {{#mustacheBlock
    param="foo"
    anotherParam="bar"
    }}
    {{mustacheBlock
      param="foo"
      anotherParam="bar"
      }}
    {{/mustacheBlock}}
</div>

The second closing curly bracket indents one shiftwidth too many, but this is affected by the mutation of s:in_block_expr -- it sort of depends on what line you indent before you indent that one, because repeated invocations that change s:in_block_expr are "remembered" and things get very confusing when I try to figure out this if-clause:

" unindent }} lines, and following lines if not inside a block expression
if currentLine =~# '\v^\s*\}\}\s*$' || (currentLine !~# '\v^\s*\{\{\/' && prevLine =~# '\v^\s*[^\{\} \t]+\}\}\s*$')
  if !s:in_block_expr
    let ind = ind - sw
  endif
endif

In fact, if you open the file and call GetHandleBarsIndent(95), you should get "E121: Undefined variable: s:in_block_expr". On the other hand, you stop getting the error the moment the variable is set the first time, but what it's set to depends on which line has been indented before indenting that one. It'd probably only work correctly when indenting from top to bottom, but manually executing == on various lines could break it.

I don't really know what to suggest, other than find a way to avoid maintaining that state. It might be jumping to the opening tag would do the trick, similar to https://github.com/mustache/vim-mustache-handlebars/pull/55/files#diff-eeee5d46b419bb17834d0b1a86a43f8fR109

jsit commented 5 years ago

Hm, I'm actually not seeing this on the latest master with a minimal .vimrc. I get this:

      <div>
          {{#mustacheBlock
              param="foo"
              anotherParam="bar"
              }}
              {{mustacheBlock
                  param="foo"
                  anotherParam="bar"
              }}
          {{/mustacheBlock}}
      </div>
" .vimrc
set nocompatible
filetype plugin on
filetype indent on
syntax on
set runtimepath+=~/.vim/bundle/vim-mustache-handlebars

Can you please delete any mkview stuff and try again? Thanks.`

jsit commented 5 years ago

It looks like this may be happening due to your hanging indent PR #55? I pulled that and I do see this problem there.

AndrewRadev commented 5 years ago

On master, open the example file directly with vim example.mustache. Jump to line 95, and then type == to reindent just that one line. For me, the }} gets indented one more shiftwidth. Does it not for you?

For a different expression of the same problem, open the example file with vim example.mustache, and then run :echo GetHandlebarsIndent(95). I get the following output:

Error detected while processing function GetHandlebarsIndent:
line   60:
E121: Undefined variable: s:in_block_expr
12

Do you not get this output?

jsit commented 5 years ago

No, this works fine for me. I'm on Vim 8.1.800 and am using the minimal .vimrc shown above. I'm on commit 915c93c of vim-mustache-handlebars.

jsit commented 5 years ago

Jump to line 95, and then type == to reindent just that one line. For me, the }} gets indented one more shiftwidth. Does it not for you?

Sorry, actually, yes, this does happen -- but if I == on the line above it, then == again on that line, it goes back to where it should be. If I visually select the whole block and do =, it indents correctly.

jsit commented 5 years ago

But, I'm not able to get E121: Undefined variable: s:in_block_expr.

AndrewRadev commented 5 years ago

The s:in_block_expr variable is effectively global state. The GetHandlebarsIndent function is called whenever Vim needs to get the indent of a line, and whether or not this line blows up depends on whether the function has been called in such a way that it enters one of the above if-clauses:

if currentLine =~# '\v^\s*\}\}\s*$' || (currentLine !~# '\v^\s*\{\{\/' && prevLine =~# '\v^\s*[^\{\} \t]+\}\}\s*$')
  if !s:in_block_expr
    let ind = ind - sw
  endif
endif

You could go elsewhere, run == on that line, go back to line 95 and run == and it might end up indenting differently. You might open a new handlebars/mustache file, and the unlet! s:in_block_expr at the top would remove that variable and now it would not indent correctly.

I really don't think this is fixable -- indent functions do not need to be called on consecutive lines. I'd look for a different way to determine whether a closing }} closes a block component or not. Maybe something like this?

  " unindent }} lines, and following lines if not inside a block expression
  let saved_pos = getpos('.')
  if currentLine =~# '\v^\s*\}\}\s*$' || (currentLine !~# '\v^\s*\{\{\/' && prevLine =~# '\v^\s*[^\{\} \t]+\}\}\s*$')
    call search('}}\s*$', 'Wb', lnum)
    let [line, col] = searchpairpos('{{', '', '}}', 'Wb')
    if line > 0
      if strpart(getline(line), col - 1, 3) != '{{#'
        let ind = ind - sw
      endif
    else
      call setpos('.', saved_pos)
    endif
  endif
jsit commented 5 years ago

Ahh, I'm starting to understand. You're right, I do get that error when calling the function before I ever indent anything. I misunderstood. I'll look into this; feel free to open a PR, too. Thanks!

jsit commented 5 years ago

Portions of that indent file were written with visual mode indenting in mind only, I'm remembering.

jsit commented 5 years ago

Your example rewrite of the function seems to be working for me. You can throw that into a PR and get rid of all s:in_block_expr references, I believe. Thanks so much!!

AndrewRadev commented 4 years ago

I guess this is closed by https://github.com/mustache/vim-mustache-handlebars/pull/93