[vim/vim] [vim9script] Regression after fixing constructor argument type checking bug (Issue #13102)
Lifepillar
Steps to reproduce
Patch 9.0.1724 (“vim9class constructor argument type checking bug”) has introduced a regression in one of my Vim 9 libraries. I have a test suite that caught that, but I was able to nail down the issue to the following self-contained script:
vim9script def ListifyKeys(keys: any): list<list<string>> if type(keys) == v:t_string return [[keys]] endif return keys enddef class Rel this.keys: list<list<string>> def new(keys: any) const keys_: list<list<string>> = ListifyKeys(keys) enddef endclass def Test() var R = Rel.new('A') enddef Test()
This fails with E1013: Argument 1: type mismatch, expected list<list<string>> but got string.
Interestingly, if the this.keys attribute is removed, no error occurs.
Expected behaviour
I would expect the script above to run without errors. That was the case before patch 9.0.1724.
Version of Vim
9.0.1899
Environment
macOS 13.5.1
Apple Terminal
xterm-256color
zsh 5.9
Logs and stack traces
No response
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
ping @h-east
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
Steps to reproduce
Patch 9.0.1724 (“vim9class constructor argument type checking bug”) has introduced a regression in one of my Vim 9 libraries. I have a test suite that caught that, but I was able to nail down the issue to the following self-contained script:
vim9script def ListifyKeys(keys: any): list<list<string>> if type(keys) == v:t_string return [[keys]] endif return keys enddef class Rel this.keys: list<list<string>> def new(keys: any) const keys_: list<list<string>> = ListifyKeys(keys) enddef endclass def Test() var R = Rel.new('A') enddef Test()This fails with
E1013: Argument 1: type mismatch, expected list<list<string>> but got string.Interestingly, if the
this.keysattribute is removed, no error occurs.Expected behaviour
I would expect the script above to run without errors. That was the case before patch 9.0.1724.
index ccec0bcc3..7af478f71 100644
--- a/src/vim9instr.c
+++ b/src/vim9instr.c
@@ -1838,9 +1838,13 @@ generate_CALL(
{
class_T *clp = mtype->tt_class;
char_u *aname = ((char_u **)ufunc->uf_args.ga_data)[i];
- ocmember_T *m = object_member_lookup(clp, aname, 0, NULL);
- if (m != NULL)
- expected = m->ocm_type;
+ if (STRNCMP(aname, "this.", 5) == 0)
+ {
+ aname += 5;
+ ocmember_T *m = object_member_lookup(clp, aname, 0, NULL);
+ if (m != NULL)
+ expected = m->ocm_type;
+ }
}
}
else if (ufunc->uf_va_type == NULL
vim-dev ML
On Sat, Sep 16, 2023 at 1:30 AM Lifepillar ***@***.***>
wrote:
> Steps to reproduce
>
> Patch 9.0.1724 (“vim9class constructor argument type checking bug”) has
> introduced a regression in one of my Vim 9 libraries. I have a test suite
> that caught that
> script:
>
> vim9script
> def ListifyKeys(keys: any): list<list<string>>
> if type(keys) == v:t_string
> return [[keys]]
> endif
> return keysenddef
> class Rel
> this.keys: list<list<string>>
>
> def new(keys: any)
> const keys_: list<list<string>> = ListifyKeys(keys)
> enddef
> endclass
> def Test()
> var R = Rel.new('A')enddef
> Test()
>
> This fails with E1013: Argument 1: type mismatch, expected
> list<list<string>> but got string.
>
> Interestingly, if the this.keys attribute is removed, no error occurs.
> Expected behaviour
>
> I would expect the script above to run without errors. That was the case
> before patch 9.0.1724.
>
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
Hi, Rough investigation and test code creation has been completed.
Please wait a few hours as I will issue a PR.
By the way, even in the case of object member, aname does not include "this.".
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Lifepillar
The code currently checks for the type of arguments to the new() constructor, even if the argument name does not start with "this". In the above example, if you rename the "keys" argument to the new() function, then you will not see this issue.
Right, that works. I also confirm that the proposed patch fixes the issue. Thanks a lot!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
so, do we need to fix it or are you fine with the current behaviour?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
h_east
With Yegappan's patch, it looks like the following tests and existing tests are not passing.
diff --git a/src/testdir/test_vim9_class.vim b/src/testdir/test_vim9_class.vim index 1796f5f72..5b32a2337 100644 --- a/src/testdir/test_vim9_class.vim +++ b/src/testdir/test_vim9_class.vim @@ -766,6 +766,27 @@ def Test_class_new_with_object_member() Check() END v9.CheckSourceFailure(lines, 'E1013:') + + lines =<< trim END + vim9script + + class C + this.str: string + def new(str: any) + enddef + endclass + + def Check() + try + var c = C.new(1) + catch + assert_report($'Unexpected exception was caught: {v:exception}') + endtry + enddef + + Check() + END + v9.CheckSourceSuccess(lines) enddef def Test_class_object_member_inits()
$ make test_vim9_class
Found errors in Test_class_new_with_object_member():
command line..script /home/h_east/samba/github/vim/src/testdir/runtest.vim[601]..function RunTheTest[54]..Test_class_new_with_object_member[51]..<SNR>9_CheckSourceFailure[4]..script :source buffer=123[18]..function <SNR>133_Check line 4: Unexpected exception was caught: Vim:E1012: Type mismatch; expected string but got number
command line..script /home/h_east/samba/github/vim/src/testdir/runtest.vim[601]..function RunTheTest[54]..Test_class_new_with_object_member[51]..<SNR>9_CheckSourceFailure line 4: ['vim9script', '', 'class C', ' this.str: string', ' this.num: number', ' def new(this.str, this.num)', ' enddef', 'endclass', '', 'def Check()', ' try', ' var c = C.new(1, 2)', ' catch', ' assert_report($''Unexpected exception was caught: {v:exception}'')', ' endtry', 'enddef', '', 'Check()']: Expected 'E1013:' but got '[unknown]': ['vim9script', '', 'class C', ' this.str: string', ' this.num: number', ' def new(this.str, this.num)', ' enddef', 'endclass', '', 'def Check()', ' try', ' var c = C.new(1, 2)', ' catch', ' assert_report($''Unexpected exception was caught: {v:exception}'')', ' endtry', 'enddef', '', 'Check()']
command line..script /home/h_east/samba/github/vim/src/testdir/runtest.vim[601]..function RunTheTest[54]..Test_class_new_with_object_member[73]..<SNR>9_CheckSourceFailure[4]..script :source buffer=124[18]..function <SNR>134_Check line 4: Unexpected exception was caught: Vim:E1012: Type mismatch; expected number but got string
command line..script /home/h_east/samba/github/vim/src/testdir/runtest.vim[601]..function RunTheTest[54]..Test_class_new_with_object_member[73]..<SNR>9_CheckSourceFailure line 4: ['vim9script', '', 'class C', ' this.str: string', ' this.num: number', ' def newVals(this.str, this.num)', ' enddef', 'endclass', '', 'def Check()', ' try', ' var c = C.newVals(''dogs'', ''apes'')', ' catch', ' assert_report($''Unexpected exception was caught: {v:exception}'')', ' endtry', 'enddef', '', 'Check()']: Expected 'E1013:' but got '[unknown]': ['vim9script', '', 'class C', ' this.str: string', ' this.num: number', ' def newVals(this.str, this.num)', ' enddef', 'endclass', '', 'def Check()', ' try', ' var c = C.newVals(''dogs'', ''apes'')', ' catch', ' assert_report($''Unexpected exception was caught: {v:exception}'')', ' endtry', 'enddef', '', 'Check()']
make[1]: *** [Makefile:70: test_vim9_class] Error 1
make[1]: Leaving directory '/home/h_east/samba/github/vim/src/testdir'
make: *** [Makefile:2253: test_vim9_class] Error 2
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
Something has to be fixed. There is currently different behavior between :def and script level. And depending on the type specified for the new(arg: some_type) it works or doesn't work. See test below. IIUC, with Yegappan's patch things work as documented, for example :hellp vim9-simple-class shows this.lnum = lnum in the constructor.
vim9script
class A
this.val1: number = 1
this.valstring: string
def new(val1: any) # <<<<< Change type to string and things work.
this.valstring = val1
enddef
endclass
var o1 = A.new('bar') # <<<<< This works.
echo o1.valstring
def F()
var o = A.new('baz') # <<<<< This fails compilation.
echo o.valstring
enddef
F()
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
h_east
@errael
Thanks for the advice.
I see the fix points of the patch.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
h_east
PR modification has been completed.
Ready to review.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
Closed #13102 as completed via b895b0f.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.