-
-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix coercion broken by a user-defined Func
function
#68
Conversation
Func
existsFunc
function
endif | ||
let s = type(a:self) == type({}) ? a:self : {} | ||
if type(Func) == type(function('tr')) | ||
return call(Func,a:000,s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet you can fix the issue by changing this line and this line alone. Can you confirm/refute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I don't know much about git or github, so when you say "this line", I'm not sure what line I should change.
Just above your comment I can see these 2 lines in red:
if type(Func) == type(function('tr'))
return call(Func,a:000,s)
So, I assume you want me to only scope the first 4 occurrences of Func
like this:
if type(a:func) == type('') || type(a:func) == type(0)
let l:Func = get(a:self,a:func,'')
else
let l:Func = a:func
endif
if type(l:Func) == type(function('tr'))
return call(l:Func,a:000,s)
If that's what you meant, then yes, I've just made a test and it fixes the issue.
Do you want me to push another commit and only change these 4 occurrences, and leave the 10 other ones alone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the return call(Func,a:000,s)
line is what I was thinking. If that works then yes, I'd favor the single line diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've just made a test, the only change being this line:
return call(Func,a:000,s)
Replaced by this line:
return call(l:Func,a:000,s)
But unfortunately, the issue still persists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it must be the 2 assignment lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I think the error comes from the assignments, that's why I scoped them with l:
in the commit.
And since I did that for the assignments, I felt it was necessary to go on and do the same for all the other occurrences, otherwise I thought that Func
would refer to the user-defined function, instead of the local funcref.
If I only scope the assignments, when I press cru
, something weird happens (the word is deleted, or the cursor moves and a character is deleted somewhere else); it depends on what the current line contains...
However, as I said in the previous post, if I scope only the first four occurrences, then this issue is fixed.
So if you want me to scope only them, and leave the other alone, I'll do it and push another commit.
Although, I'm not sure a similar issue would not happen if s:send()
had to process the following elseif
statements where Func
is unscoped.
FWIW, I always scope a funcref with l:
to avoid having headaches trying to understand what Vim will understand. But I admit I have far less experience in VimL than you, so if it's fine by you, it's fine by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I favor the minimum possible diff which appears to be the 2 assignment lines. Since you closed this I'll take care of it. Thanks for the recon!
Merge pull request #374 from easymotion/reviewdog CI: run vint with reviewdog Moved styles into their own css file. It's a bit of a phaff to find the path to the CSS file. The method I'm using works when the markdown-preview plugin is loaded with Pathogen.vim, but I'm not sure if it will work if the plugin is loaded the traditional way. Second attempt to fix YouCompleteMe issue (#92) Squashed from two commits: * Second attempt to fix YouCompleteMe issue * VimEnter also has to be ignored Fix blank lines issue In #254, probably because the tag is followed by <CR>, cstta<CR> sets regtype 'V'(linewise) though its keeper doesn't end with <CR>. And setreg() adds extra <CR> at @". Remove this extra <CR> and fix adding <CR> method. Version 1.1.1: debug (stop beeping) explicitly set locale to C for sorting so README sorts same on all systems (#348) back to boooring Documentation update part 2: content Merge branch 'revert-marks' Close gh-59. Merge pull request #3 from joekur/support-js-jsx Support for js and jsx specs Merge pull request #190 from hhatto/improve-managepy improve manage.py completion #188 Remove trailing slash from auto-closing tags (#1081) This breaks compatibility with XHTML (see #845). The slashes are even forbidden in HTML4 (but optional in HTML5). Source: <http://xahlee.info/js/html5_non-closing_tag.html> Merge pull request #7 from assaflavie/master copy paste error fixed in readme README: document that ZSH 4.3 or newer is required closes #24 Try to fix issue #17 (async command execution on Windows without DLL) See issue 17 on GitHub: xolox/vim-misc#17 Add doc/tags to git ignores. Merge pull request #125 from acatton/fix-121 Fix #121: sudo syntax highlighting incorrectly Mention that a new window will be opened for :NR! if hidden is not set Merge pull request #55 from ldong/master Added only use file_line upon files not directory Merge pull request #10 from sheerun/patch-1 Set proper comment string on tmux.conf Rust closures with bracket counting Version 070503: Fixed problem with 'smartcase'. Checksum 726910263 Manual: update URL for foodcritic. Merge pull request #31 from boatrite/zb-fix-describe Highlight describe in RSpec.describe Add a help tag for :Tabularize Fixes #45 Add copyright notice and vim license Create the Normal highlight if it doesn't exist This can happen in versions of vim built without X support, like vim.nox in Debian. Fixes #9 Merge pull request #851 from xTire/master Fix misspellings in README_ZH_CN.md Merge branch 'prepare-for-0.3.3' Document usage of multiple platforms (issue #85) See also issue #85 on GitHub: xolox/vim-session#85 Merge pull request #1 from taybin/patch-1 use function! instead of function Fix :0Dispatch Closes tpope/vim-rake#36 Version 1.13 Use a defaultdict for easier appending. Pass around dir for found tags file for better integration with fugitive (thanks to Markus Bertheau). Remove mapping of @ This key should not be stolen! It's used for macros and YR does something bad with it, causing us to have to hit enter to use the macro. Merge branch 'marabesi-master' * marabesi-master: Use bash syntax highlighting for terminal commands in the readme Add empty lines after fenced code blocks Update README.markdown gitcommit: use string width not character count Merge pull request #605 from jsit/jsit/css-min-keyword-604 Change css, scss, and sass Omni input pattern to match min_keyword_length delete global functions after the test case Version 1.52 - FIX: Correct forward-to-end motion over lowercase part in "lowerCamel". Found this by chance in GitHub fork by Kevin Lee (bkad). - BUG: Correct wrong stop on second letter of ACRONYM at the beginning of a word "AXBCText". - The motion functionality is automatically tested via a runVimTests (vimscript #2565) test suite. update zshenv Add skip_forgery_protection to syntax highlighting Improve parser version documentation See #248. Merge pull request #670 from jteplitz602/jteplitz602-readme Update README.md to reflect project status Use real visual mode mappings. The normal mode mappings that start with "v" introduce a mapping delay when entering visual mode; this is irritating. Instead, define the mappings directly in visual mode, and disregard the previous selection with <C-u> prepended to the function call, so it works the same. docs fix last commit. Thanks to z0rc Merge pull request #7 from BurningLutz/master support async modifier Remove FugitiveGenerate() in favor of FugitiveFind() Version 0.6: Initial upload Updated documentation Fix PATH string building (#10) References tpope/vim-rvm#5 Force sort to recalculate the cached sortKey. (#898) * Force sort to recalculate the cached sortKey. The problem in issue #880 was caused by the sort using the old sortKey. For example, given nodes A, B, and C, if B were renamed to D, the sort was still using B as its sortKey, thus not moving it. It's a bit of a hack, but if we set g:NERDTreeOldSortOrder to an empty list, the cached sortKey will be recalculated. I did the same thing for both the Copy and Add functions as well. * Add a comment to explain the let ... = [] statement. Restore legacy maps Update README.md Version 1.13.2 Another bug fix: either 1.13 or 1.13.1 broke the behavior of finding a matching construct that starts after the cursor. Add deprecation notice Merge pull request #34 from Jagua/fix-built-in-function-check fix method for detecting whether exists built-in function. SneakEnter, SneakLeave events closes #128 fix replacing Namespace::Exception => ex Merge branch 'master' of github:xsunsmile/showmarks Only define <Leader>* mappings if available If user has defined <Leader>* mappings in their vimrc, the mappings here won't apply. YAML has meaningful indentation. Merge pull request #230 from Hinidu/patch-1 Support for the terraform filetype Merge branch 'prepare-for-0.0.4' Merge branch 'prepare-for-0.4.0' Merge pull request #221 from thinca/remove-autocmd Remove unnecessary autocmd update vimup Merge pull request #7 from joshuacronemeyer/master support for v3 iterm applescript dictionary changes Version 12 * changed conc -> cole for vim 7.3's built-in conceal mode * included some additional escape sequences Improve visual select to retain code highlighting colors Version 2.0 removed "'echo hi'" from the script. added rake to the gemfile as it is needed by travis Merge pull request #20 from BrandonMathis/master Tripple ` for named markdownCodeDelimiter Note terminal mode incompatability Add support for alternative real path API Merge pull request #260 from aschrab/command-window Do setup for command windows Updates output Remove outdated plugin references add CTRL-M for multi-line markdown code snippets Set and unset completeopt in htmlEn and htmlDis Fucking Bitbucket Merge pull request #26 from bovine3dom/master Ensure Cygwin uses cygstart rather than *-open Handle case where global Func() exists Closes tpope/vim-abolish#68 Simplify regex using \k Signed-off-by: jesson Foo <jessonfoo@gmail.com>
Merge pull request #374 from easymotion/reviewdog CI: run vint with reviewdog Moved styles into their own css file. It's a bit of a phaff to find the path to the CSS file. The method I'm using works when the markdown-preview plugin is loaded with Pathogen.vim, but I'm not sure if it will work if the plugin is loaded the traditional way. Second attempt to fix YouCompleteMe issue (#92) Squashed from two commits: * Second attempt to fix YouCompleteMe issue * VimEnter also has to be ignored Fix blank lines issue In #254, probably because the tag is followed by <CR>, cstta<CR> sets regtype 'V'(linewise) though its keeper doesn't end with <CR>. And setreg() adds extra <CR> at @". Remove this extra <CR> and fix adding <CR> method. Version 1.1.1: debug (stop beeping) explicitly set locale to C for sorting so README sorts same on all systems (#348) back to boooring Documentation update part 2: content Merge branch 'revert-marks' Close gh-59. Merge pull request #3 from joekur/support-js-jsx Support for js and jsx specs Merge pull request #190 from hhatto/improve-managepy improve manage.py completion #188 Remove trailing slash from auto-closing tags (#1081) This breaks compatibility with XHTML (see #845). The slashes are even forbidden in HTML4 (but optional in HTML5). Source: <http://xahlee.info/js/html5_non-closing_tag.html> Merge pull request #7 from assaflavie/master copy paste error fixed in readme README: document that ZSH 4.3 or newer is required closes #24 Try to fix issue #17 (async command execution on Windows without DLL) See issue 17 on GitHub: xolox/vim-misc#17 Add doc/tags to git ignores. Merge pull request #125 from acatton/fix-121 Fix #121: sudo syntax highlighting incorrectly Mention that a new window will be opened for :NR! if hidden is not set Merge pull request #55 from ldong/master Added only use file_line upon files not directory Merge pull request #10 from sheerun/patch-1 Set proper comment string on tmux.conf Rust closures with bracket counting Version 070503: Fixed problem with 'smartcase'. Checksum 726910263 Manual: update URL for foodcritic. Merge pull request #31 from boatrite/zb-fix-describe Highlight describe in RSpec.describe Add a help tag for :Tabularize Fixes #45 Add copyright notice and vim license Create the Normal highlight if it doesn't exist This can happen in versions of vim built without X support, like vim.nox in Debian. Fixes #9 Merge pull request #851 from xTire/master Fix misspellings in README_ZH_CN.md Merge branch 'prepare-for-0.3.3' Document usage of multiple platforms (issue #85) See also issue #85 on GitHub: xolox/vim-session#85 Merge pull request #1 from taybin/patch-1 use function! instead of function Fix :0Dispatch Closes tpope/vim-rake#36 Version 1.13 Use a defaultdict for easier appending. Pass around dir for found tags file for better integration with fugitive (thanks to Markus Bertheau). Remove mapping of @ This key should not be stolen! It's used for macros and YR does something bad with it, causing us to have to hit enter to use the macro. Merge branch 'marabesi-master' * marabesi-master: Use bash syntax highlighting for terminal commands in the readme Add empty lines after fenced code blocks Update README.markdown gitcommit: use string width not character count Merge pull request #605 from jsit/jsit/css-min-keyword-604 Change css, scss, and sass Omni input pattern to match min_keyword_length delete global functions after the test case Version 1.52 - FIX: Correct forward-to-end motion over lowercase part in "lowerCamel". Found this by chance in GitHub fork by Kevin Lee (bkad). - BUG: Correct wrong stop on second letter of ACRONYM at the beginning of a word "AXBCText". - The motion functionality is automatically tested via a runVimTests (vimscript #2565) test suite. update zshenv Add skip_forgery_protection to syntax highlighting Improve parser version documentation See #248. Merge pull request #670 from jteplitz602/jteplitz602-readme Update README.md to reflect project status Use real visual mode mappings. The normal mode mappings that start with "v" introduce a mapping delay when entering visual mode; this is irritating. Instead, define the mappings directly in visual mode, and disregard the previous selection with <C-u> prepended to the function call, so it works the same. docs fix last commit. Thanks to z0rc Merge pull request #7 from BurningLutz/master support async modifier Remove FugitiveGenerate() in favor of FugitiveFind() Version 0.6: Initial upload Updated documentation Fix PATH string building (#10) References tpope/vim-rvm#5 Force sort to recalculate the cached sortKey. (#898) * Force sort to recalculate the cached sortKey. The problem in issue #880 was caused by the sort using the old sortKey. For example, given nodes A, B, and C, if B were renamed to D, the sort was still using B as its sortKey, thus not moving it. It's a bit of a hack, but if we set g:NERDTreeOldSortOrder to an empty list, the cached sortKey will be recalculated. I did the same thing for both the Copy and Add functions as well. * Add a comment to explain the let ... = [] statement. Restore legacy maps Update README.md Version 1.13.2 Another bug fix: either 1.13 or 1.13.1 broke the behavior of finding a matching construct that starts after the cursor. Add deprecation notice Merge pull request #34 from Jagua/fix-built-in-function-check fix method for detecting whether exists built-in function. SneakEnter, SneakLeave events closes #128 fix replacing Namespace::Exception => ex Merge branch 'master' of github:xsunsmile/showmarks Only define <Leader>* mappings if available If user has defined <Leader>* mappings in their vimrc, the mappings here won't apply. YAML has meaningful indentation. Merge pull request #230 from Hinidu/patch-1 Support for the terraform filetype Merge branch 'prepare-for-0.0.4' Merge branch 'prepare-for-0.4.0' Merge pull request #221 from thinca/remove-autocmd Remove unnecessary autocmd update vimup Merge pull request #7 from joshuacronemeyer/master support for v3 iterm applescript dictionary changes Version 12 * changed conc -> cole for vim 7.3's built-in conceal mode * included some additional escape sequences Improve visual select to retain code highlighting colors Version 2.0 removed "'echo hi'" from the script. added rake to the gemfile as it is needed by travis Merge pull request #20 from BrandonMathis/master Tripple ` for named markdownCodeDelimiter Note terminal mode incompatability Add support for alternative real path API Merge pull request #260 from aschrab/command-window Do setup for command windows Updates output Remove outdated plugin references add CTRL-M for multi-line markdown code snippets Set and unset completeopt in htmlEn and htmlDis Fucking Bitbucket Merge pull request #26 from bovine3dom/master Ensure Cygwin uses cygstart rather than *-open Handle case where global Func() exists Closes tpope/vim-abolish#68 Simplify regex using \k Signed-off-by: jesson Foo <jessonfoo@gmail.com>
MWE:
Start Vim like this:
Execute on the command-line:
^@
is a NUL inserted by pressingC-v C-j
.Insert a word in the buffer:
Escape to get back to normal mode, and press
cru
to make it uppercase.It raises the following error:
I think the issue comes from an assignment in the
s:send()
function.Usually, the
l:
scope is useless unless you use one of these 5 variables:Because they're the only ones with the
VV_COMPAT
flag.But
l:
is also useful when you create a funcref inside a function, as its name can conflict with an outer public function.In
s:send()
, I think that the variableFunc
refers to a funcref, and so should be scoped withl:
to avoid a conflict with a user-defined function namedFunc
.This is an issue which I encounters often as I'm frequently creating a function
Func
when prototyping a new function and I don't want to think about a name yet.I'm not familiar with the code, so I don't know whether the PR breaks something else, but in my limited usage of the plugin, it didn't seem to cause an issue.
Feel free to close the PR if there's another way of fixing the issue.
Thank you very much for your plugin.