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

autoload ファイルを手動で source するとautoload関数定義時に2重で追加のsourceが発生する #1120

Closed
haya14busa opened this Issue Nov 23, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@haya14busa
Member

haya14busa commented Nov 23, 2017

質問・報告の内容

再現方法

vim --clean --cmd 'set rtp+=.' -S autoload/hoge.vim

autoload/hoge.vim

echom 'start sourcing autoload/hoge.vim'

echom 'define s:f'
function! s:f() abort
endfunction

echom 'define hoge#foo'
function! hoge#foo() abort
endfunction
echom 'end define hoge#foo'

echom 'delfunction <SID>f'
delfunction <SID>f

echom 'end sourcing autoload/hoge.vim'

出力

start sourcing autoload/hoge.vim
define s:f
define hoge#foo
start sourcing autoload/hoge.vim
define s:f
define hoge#foo
end define hoge#foo
delfunction <SID>f
end sourcing autoload/hoge.vim
end define hoge#foo
delfunction <SID>f
Error detected while processing /home/haya14busa/src/github.com/vim/vim/autoload/hoge.vim:
line   13:
E130: Unknown function: <SID>f
end sourcing autoload/hoge.vim

出力結果を見ると、 最初のsource で hoge#foo を定義する時点で2つめの source がファイルの先頭から発生し最後まで sourceされたあと、もとのsourceが hoge#foo を定義したあとから実行されます。
これによって、autoload/hoge.vim のように スクリプトローカル関数定義 -> autoload関数定義 -> スクリプトローカル関数を削除 といったコードを書くと、スクリプトローカル関数の定義が2回走ったあと, その削除が2回行われるので、2度目の削除時に Unknown function が出るという実害がでます。

Vimのバージョン

8.0.1257
7.3.500 でも試しましたが再現したのでもとからこの挙動っぽい。

OSの種類/ディストリ/バージョン

Ubuntu

その他

vim-jp/vital.vim#553 のpull-request でこの挙動に気づいて数時間ハマりました... > <

@h-east

This comment has been minimized.

Show comment
Hide comment
@h-east

h-east Nov 25, 2017

Member

質問です。

  1. delfunction!でエラーを回避した場合、他に問題がありますか?
  2. autoloadのロジックに手を入れるとした場合に何か案はありますか?(実際に変更可能かどうかはおいといて)
Member

h-east commented Nov 25, 2017

質問です。

  1. delfunction!でエラーを回避した場合、他に問題がありますか?
  2. autoloadのロジックに手を入れるとした場合に何か案はありますか?(実際に変更可能かどうかはおいといて)
@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 27, 2017

Member

dict func かどうかで挙動が違ってそうですね。

autoload/foo.vim

let foo#bar = {}

echo 'loading foo#bar'
function! foo#bar.do_something()
endfunction

このスクリプトを :so する場合、loading foo#bar は1回しか表示されません。

echo 'loading foo#bar'
function! foo#bar.do_something()
endfunction

この foo#bar を消した場合は undefined variable が出ますがこちらも loading foo#bar は1回しか表示されません。つまり foo#bar を確認する為に autoload していない事になります。かたや dict でない場合には

echo 'loading foo#bar'
function! foo#do_something()
endfunction

2回表示されます。つり合いが取れてないなという気がしますので、本来なら function コマンドの右項では autoload は実行されないのが正しい気がしています。

表題の件であれば以下のパッチで直りそうです。

diff --git a/src/userfunc.c b/src/userfunc.c
index e18768455..71b52d708 100644
--- a/src/userfunc.c
+++ b/src/userfunc.c
@@ -1886,7 +1886,7 @@ ex_function(exarg_T *eap)
      * g:func	    global function name, same as "func"
      */
     p = eap->arg;
-    name = trans_function_name(&p, eap->skip, 0, &fudi, NULL);
+    name = trans_function_name(&p, eap->skip, TFN_NO_AUTOLOAD, &fudi, NULL);
     paren = (vim_strchr(p, '(') != NULL);
     if (name == NULL && (fudi.fd_dict == NULL || !paren) && !eap->skip)
     {

テストは足していません

https://travis-ci.org/mattn/vim/builds/307763707

※表題にある様なテストケースは無いのでパスしました。

これで直るかなとは思いますが、ナイーブな部分なので今回のケース以外にトリッキーな事をやっていて、かつ今回の期待とは逆に2回読み込まれる事を期待する様なスクリプトが無いか心配です。

Member

mattn commented Nov 27, 2017

dict func かどうかで挙動が違ってそうですね。

autoload/foo.vim

let foo#bar = {}

echo 'loading foo#bar'
function! foo#bar.do_something()
endfunction

このスクリプトを :so する場合、loading foo#bar は1回しか表示されません。

echo 'loading foo#bar'
function! foo#bar.do_something()
endfunction

この foo#bar を消した場合は undefined variable が出ますがこちらも loading foo#bar は1回しか表示されません。つまり foo#bar を確認する為に autoload していない事になります。かたや dict でない場合には

echo 'loading foo#bar'
function! foo#do_something()
endfunction

2回表示されます。つり合いが取れてないなという気がしますので、本来なら function コマンドの右項では autoload は実行されないのが正しい気がしています。

表題の件であれば以下のパッチで直りそうです。

diff --git a/src/userfunc.c b/src/userfunc.c
index e18768455..71b52d708 100644
--- a/src/userfunc.c
+++ b/src/userfunc.c
@@ -1886,7 +1886,7 @@ ex_function(exarg_T *eap)
      * g:func	    global function name, same as "func"
      */
     p = eap->arg;
-    name = trans_function_name(&p, eap->skip, 0, &fudi, NULL);
+    name = trans_function_name(&p, eap->skip, TFN_NO_AUTOLOAD, &fudi, NULL);
     paren = (vim_strchr(p, '(') != NULL);
     if (name == NULL && (fudi.fd_dict == NULL || !paren) && !eap->skip)
     {

テストは足していません

https://travis-ci.org/mattn/vim/builds/307763707

※表題にある様なテストケースは無いのでパスしました。

これで直るかなとは思いますが、ナイーブな部分なので今回のケース以外にトリッキーな事をやっていて、かつ今回の期待とは逆に2回読み込まれる事を期待する様なスクリプトが無いか心配です。

@ichizok

This comment has been minimized.

Show comment
Hide comment
@ichizok

ichizok Nov 27, 2017

Member

@mattn call hoge#foo() した時に autoload されてしまいます。

Member

ichizok commented Nov 27, 2017

@mattn call hoge#foo() した時に autoload されてしまいます。

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 27, 2017

Member

あー、sourcing_name が同じか見ないといけないすねー。

Member

mattn commented Nov 27, 2017

あー、sourcing_name が同じか見ないといけないすねー。

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 27, 2017

Member

autoload/a.vim

call b#foo()

autoload/b.vim

call c#foo()

autoload/c.vim

call a#foo()

こんな場合を考えると script_name じゃなくて script_items を毎回見ないといけなくなって面倒ですね。

Member

mattn commented Nov 27, 2017

autoload/a.vim

call b#foo()

autoload/b.vim

call c#foo()

autoload/c.vim

call a#foo()

こんな場合を考えると script_name じゃなくて script_items を毎回見ないといけなくなって面倒ですね。

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 27, 2017

Member

ソースを一通り見てきましたが、:source さえしなければガードが掛かってるので、もしかしたら仕様じゃないかなと思い始めています。つまり↑のパッチだけで十分という意味です。

2重で読み込まれるのは :source した側の問題だと思ってて、これをやるのであれば :source でなく :runtime autoload/xxx.vim にすべきという事になるんじゃないかと思いました。

Member

mattn commented Nov 27, 2017

ソースを一通り見てきましたが、:source さえしなければガードが掛かってるので、もしかしたら仕様じゃないかなと思い始めています。つまり↑のパッチだけで十分という意味です。

2重で読み込まれるのは :source した側の問題だと思ってて、これをやるのであれば :source でなく :runtime autoload/xxx.vim にすべきという事になるんじゃないかと思いました。

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Nov 27, 2017

Member

あと :help autoload の最後にこんな事が書かれています。

Also note that if you have two script files, and one calls a function in the
other and vice versa, before the used function is defined, it won't work.
Avoid using the autoload functionality at the toplevel.
Member

mattn commented Nov 27, 2017

あと :help autoload の最後にこんな事が書かれています。

Also note that if you have two script files, and one calls a function in the
other and vice versa, before the used function is defined, it won't work.
Avoid using the autoload functionality at the toplevel.
@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Dec 7, 2017

Member

とりあえず状況説明
vim/vim#2423

Member

mattn commented Dec 7, 2017

とりあえず状況説明
vim/vim#2423

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Dec 8, 2017

Member

patch 8.0.1378

閉じるかどうかはお任せして良いですか?

Member

mattn commented Dec 8, 2017

patch 8.0.1378

閉じるかどうかはお任せして良いですか?

@haya14busa

This comment has been minimized.

Show comment
Hide comment
@haya14busa

haya14busa Dec 24, 2017

Member

見るの遅くなってすいません 💦
激LGTMです。ありがとうございます!

Member

haya14busa commented Dec 24, 2017

見るの遅くなってすいません 💦
激LGTMです。ありがとうございます!

@haya14busa haya14busa closed this Dec 24, 2017

@haya14busa

This comment has been minimized.

Show comment
Hide comment
@haya14busa

haya14busa Dec 24, 2017

Member

Hmm, not sure we should support sourcing an autoload script directly.

これもともとは atuoload関数が存在するかどうかチェックするには呼んでみるしかない or source するしかないというのが問題でそれがなかったらジッサイ直接 :source する機会はそんなになさそうかもですね

Member

haya14busa commented Dec 24, 2017

Hmm, not sure we should support sourcing an autoload script directly.

これもともとは atuoload関数が存在するかどうかチェックするには呼んでみるしかない or source するしかないというのが問題でそれがなかったらジッサイ直接 :source する機会はそんなになさそうかもですね

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment