google / vim-maktaba

Consistent Vimscript
Apache License 2.0
591 stars 42 forks source link

maktaba#ensure#IsCallable and maktaba#value#IsEnum broken in Vim >= 7.4.1875, breaking Maktaba entirely #172

Closed lsls01 closed 8 years ago

lsls01 commented 8 years ago

I'm an Arch Linux user, just after I updated Vim to the latest version by pacman, maktaba fails. I have already updated maktaba to latest version by vundle, sadly still got same error messages below. For this reason, I cannot use codefmt in Vim.

ERROR(BadValue): Dictionary with keys ['dict', 'func', 'Apply', 'Call', 'WithContext', 'arglist', 'WithArgs'] is not a maktaba function. ERROR(NotFound): Flag "clang_format_executable" not defined in codefmt. ERROR(NotFound): Flag "yapf_executable" not defined in codefmt. ERROR(NotFound): Flag "js_beautify_executable" not defined in codefmt. ERROR(NotFound): Flag "gofmt_executable" not defined in codefmt.

malcolmr commented 8 years ago

Brining over most of https://github.com/google/vim-codefmt/issues/73:

This regressed in Vim 7.4.1875 (https://github.com/vim/vim/commit/8e759ba8651428995b338b66c615367259f79766).

It breaks our tests too:

$ vroom vroom/function.vroom
vroom/function.vroom
FAILED on line 037: Multiple failures:
Suspected error message:
Error detected while processing function maktaba#function#DoApply[1]..maktaba#function#Call[1]..maktaba#ensure#IsCallable:

Unexpected message:
line    9:

Suspected error message:
E605: Exception not caught: ERROR(BadValue): Dictionary with keys ['dict', 'func', 'Apply', 'Call', 'WithContext', 'arglist', 'WithArgs'] is not a maktaba function.
[...]

The following fixes most of our tests, but I'm not sure it's the right thing to do:

diff --git a/autoload/maktaba/function.vim b/autoload/maktaba/function.vim
index 5c1e72d..baf466a 100644
--- a/autoload/maktaba/function.vim
+++ b/autoload/maktaba/function.vim
@@ -46,7 +46,7 @@ let s:DoWithContext = function('maktaba#function#DoWithContext')
 function! maktaba#function#IsWellFormedDict(F) abort
   return maktaba#value#IsDict(a:F)
       \ && has_key(a:F, 'Call')
-      \ && maktaba#value#IsEqual(a:F.Call, s:DoCall)
+      " \ && maktaba#value#IsEqual(a:F.Call, s:DoCall)
 endfunction

(though I think maktaba#value#IsEnum may still be broken, which I guess isn't surprising, since it calls maktaba#value#IsEqual(a:Value.Name, function('maktaba#enum#Name'))

It looks like the Vim patch above breaks function equality in some not-entirely-obvious fashion.

dbarnett commented 8 years ago

How about we change maktaba#value#IsEqual to compare string(X) for functions?

  if maktaba#value#IsFuncRef(a:X) && maktaba#value#IsFuncRef(a:Y)
    return string(a:X) == string(a:Y)
  endif
  return …
dbarnett commented 8 years ago

Vim's help explicitly guarantees the behavior we had been counting on:

                                                    E694

A Funcref can only be compared with a Funcref and only "equal" and "not equal" can be used. Case is never ignored. Whether arguments or a Dictionary are bound (with a partial) is ignored. This is so that when a function is made a member of a Dictionary it is still considered to be the same function. To compare partials to see if they bind the same argument and Dictionary values use string(): echo string(Partial1) == string(Partial2)

But the comparison fails even though the function names match:

string(a:F.Call): 'function(''maktaba#function#DoCall'', {''dict'': {}, ''func'': ''maktaba#function#EvalExpr'', ''Apply'': function(''maktaba#function#DoApply''), ''Call'': function(''maktaba#function#DoCall''), ''WithContext'': function(''maktaba#function#DoWithContext''), ''arglist'': [''codefmt#InvalidateAutopep8Version()''], ''WithArgs'': function(''maktaba#function#DoWithArgs'')})'
string(s:DoCall): 'function(''maktaba#function#DoCall'')'

We should keep maktaba#value#IsEqual in sync with intended behavior. Pbb a bug, so we should update IsEqual to compare strings up to the first comma. Other possibility is it's an intended behavior change and the mistake was forgetting to update the docs.