[vim/vim] Fix import and class extends E1001 issue (Try again) (PR #16602)
h_east
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/16602
Commit Summary
- 084abd8 Fix import and extends E1001 issue
File Changes
(2 files)
- M src/testdir/test_vim9_import.vim (36)
- M src/vim9compile.c (33)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
@h-east pushed 1 commit.
- d3c5d90 Fix import and extends E1001 issue
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
@yegappan Could you review this PR?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan Could you review this PR?
Yes. Reviewing the changes.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
This PR doesn't fix the issue when multi-level imports are used:
test.vim:
vim9script
import './cccc.vim'
class MyView extends cccc.View
def new(v: string)
this._content.value = v
enddef
endclass
var myView = MyView.new('This should be ok')
echo myView
cccc.vim:
vim9script
import "./bbbb.vim"
export class View
var _content = bbbb.Data.new()
endclass
bbbb.vim:
vim9script
import "./aaaa.vim"
export class Data
var _d = aaaa.Property.new('xxx')
endclass
aaaa.vim:
vim9script
export class Property
public var value: string = 'aaa'
endclass
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
This PR doesn't fix the issue when multi-level imports are used:
Ignore this comment. I made a mistake in the script. After making the below changes to the script, the PR does work.
test.vim:
vim9script import './cccc.vim' class MyView extends cccc.View def new(v: string) this._content.value = v
This should be "this._content.d.value = v".
enddefendclass
var myView = MyView.new('This should be ok')
echo myViewcccc.vim:vim9script
import "./bbbb.vim"
export class View
var _content = bbbb.Data.new()
endclassbbbb.vim:vim9script
import "./aaaa.vim"
export class Data
var _d = aaaa.Property.new('xxx')
This should be "var d = ...".
endclass
aaaa.vim:vim9script
export class Property
public var value: string = 'aaa'
endclass
When you source test.vim, the message 'E1326: Variable "value" not found in object "Data"' is displayed.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
> + && cl_extends->class_class_function_count_child > 0)
+ check_extends = TRUE;
+
+ if (check_extends)
+ ++emsg_skip;
+ ret = compile_expr0(&expr, cctx);
+ if (check_extends)
+ --emsg_skip;
+ if (ret == FAIL && check_extends)
+ {
+ sctx_T current_sctx_save = current_sctx;
+
+ expr = m->ocm_init;
+ current_sctx =
+ cl_extends->class_class_functions[0]->uf_script_ctx;
+ //current_sctx.sc_seq = current_sctx_save.sc_seq;
The commented out line should be removed?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
> + && cl_extends->class_class_function_count_child > 0)
+ check_extends = TRUE;
+
+ if (check_extends)
+ ++emsg_skip;
+ ret = compile_expr0(&expr, cctx);
+ if (check_extends)
+ --emsg_skip;
+ if (ret == FAIL && check_extends)
+ {
+ sctx_T current_sctx_save = current_sctx;
+
+ expr = m->ocm_init;
+ current_sctx =
+ cl_extends->class_class_functions[0]->uf_script_ctx;
+ //current_sctx.sc_seq = current_sctx_save.sc_seq;
Yes, I'll remove it after I get home.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
> + && cl_extends->class_class_function_count_child > 0)
+ check_extends = TRUE;
+
+ if (check_extends)
+ ++emsg_skip;
+ ret = compile_expr0(&expr, cctx);
+ if (check_extends)
+ --emsg_skip;
+ if (ret == FAIL && check_extends)
+ {
+ sctx_T current_sctx_save = current_sctx;
+
+ expr = m->ocm_init;
+ current_sctx =
+ cl_extends->class_class_functions[0]->uf_script_ctx;
+ //current_sctx.sc_seq = current_sctx_save.sc_seq;
Done.
Thanks for reviewing 👍
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
are we good here?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
are we good here?
Perhaps we could address it deeper inside (e.g. compile_expr9() or compile_load() or etc...) rather than before compile_expr0().
That would probably make more sense.
I'll try it.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
I'll try it.
Done.
Actually, this issue might need to be addressed within find_imported(), but since there are many call points,
So, I created find_imported_from_extends() and handled only the necessary parts for this issue.
If similar issues arise in the future, it may be necessary to consider addressing them in find_imported().
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
@yegappan Could you please re-review it?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
ASAN error has occurred.
Since 9.1.1097 !?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
might be the newly added test and how we run system('vim --log /Xfoobar'). I am trying to find a better way to test it.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
@h-east pushed 2 commits.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@yegappan do you approve?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan approved this pull request.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan do you approve?
@chrisbra Yes.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
> + * Find "name" in imported items of extended base class of the class to which
+ * the context :def function belongs.
+ */
+ imported_T *
+find_imported_from_extends(cctx_T *cctx, char_u *name, size_t len, int load)
+{
+ imported_T *ret = NULL;
+ class_T *cl_extends;
+
+ if (cctx == NULL || cctx->ctx_ufunc == NULL
+ || cctx->ctx_ufunc->uf_class == NULL)
+ return NULL;
+
+ cl_extends = cctx->ctx_ufunc->uf_class->class_extends;
+
+ if (cl_extends != NULL && cl_extends->class_class_function_count_child > 0)
Can you use early return here also?
if (cl_extends == NULL || cl_extends->class_class_function_count_child <= 0)
return NULL;
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> + * Find "name" in imported items of extended base class of the class to which
+ * the context :def function belongs.
+ */
+ imported_T *
+find_imported_from_extends(cctx_T *cctx, char_u *name, size_t len, int load)
+{
+ imported_T *ret = NULL;
+ class_T *cl_extends;
+
+ if (cctx == NULL || cctx->ctx_ufunc == NULL
+ || cctx->ctx_ufunc->uf_class == NULL)
+ return NULL;
+
+ cl_extends = cctx->ctx_ufunc->uf_class->class_extends;
+
+ if (cl_extends != NULL && cl_extends->class_class_function_count_child > 0)
I'll include that change, thanks
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#16602)Thank you for the review and the follow-up 👍
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.