[vim/vim] Fix hang when to string of object with recursive references. (PR #15082)
errael
And add a hang test and a compare test (they look very similar).
There's a significant whitespace difference in vim9class.c.
Similar to object equals issue. object2string was called directly from
echo_string_core. Patterned this change from dict_tv2string usage.
@LemonBoy @yegappan object_tv2string is getting numbuf, echo_style,
composite_val, unlike dict_tv2string. That keeps the arguments to
object2string unchanged. Also in object_tv2string I didn't think the case
else if (copyID != 0 && obj->obj_copyID == copyID
would be doing anything, but now the failing case outputs
object of C {nest1: object of C {...}, nest2: object of
C {nest1: object of C {...}, nest2: object of C {...}}}
This case was that last thing implemented.
If the case is commented out, then the test outputs
E724: Variable nested too deep for displaying
which is the situation that caused the hang.
(2nd crash in two days, might be a record for me :) )
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/15082
Commit Summary
- 5e4b50a Fix hang when to string of object object with recursive references.
File Changes
(3 files)
- M src/eval.c (59)
- M src/testdir/test_vim9_class.vim (44)
- M src/vim9class.c (85)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
@errael pushed 1 commit.
- ec83a9b Fix hang of string(object) when object has recursive references.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
errael
Rebased, last vim crash I know of
@chrisbra @yegappan @LemonBoy Hope someone can take a look and get this merged.
Can do side by side comparisons of
object_tv2string dict_tv2string
object2string dict2string
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Thanks. @yegappan are you fine with this?
—
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.
In src/vim9class.c:
> } - ga_concat(&ga, (char_u *)"}"); + line_breakcheck();
Why is the line_breakcheck() needed here? This may process other pending events that may change the value that is displayed here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
errael
@errael commented on this pull request.
Look at a side by side of this function, object2string with dict2string, that's why it is there. In fact, when doing my last check before opening the PR, I put this in, without knowing exactly why it's there.
I'm happy to take it out, LMK.
@yegappan Another thing I was unsure of, in dict_tv2string there is
else if (copyID != 0 && tv->vval.v_dict->dv_copyID == copyID && tv->vval.v_dict->dv_hashtab.ht_used != 0)
which I converted as
else if (copyID != 0 && obj->obj_copyID == copyID
&& obj->obj_refcount != 0)But I think the check against refcount is bogus and can/should just be taken out. Do you agree?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
errael
@errael commented on this pull request.
In src/vim9class.c:
> } - ga_concat(&ga, (char_u *)"}"); + line_breakcheck();
Ahh, instead if that bogus refcount check, the following is probably the equivalent check
else if (copyID != 0 && obj->obj_copyID == copyID
&& obj->obj_class->class_obj_member_count != 0)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
errael
BTW, when the bug this issue addresses is encountered, a force kill is required. I'm guess that is because there some kind of nested signal handling going on, IDK.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
@yegappan Changed && obj->obj_refcount != 0 to && obj->obj_class->class_obj_member_count != 0 as discussed above.
Any further comment about line_breakcheck()?
—
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.
Yes. The check for obj->obj_class->class_obj_member_count is the correct check.
—
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 Changed
&& obj->obj_refcount != 0to&& obj->obj_class->class_obj_member_count != 0as discussed above.Any further comment about
line_breakcheck()?
We can keep the line_breakcheck() call to be consistent with dict2string().
—
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.
errael
Rebased, only docs have changed.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.