[termdebug vim9script] problems with saving/restoring options

[errael]

Steps to reproduce

Refer to #14951, in the comments to that PR, it is pointed out that the problem fixed by the PR may be a general problem in the vim9script port of termdebug.

So this issue potentially covers more than that PR. Since the new code is in operation, I gave it a brief try, there's a chance there may be only a few isolated problems.

The following script mimics the termdebug code. Each call to SaveRestoreMM should do both a save and a restore. Run the following and see that

• If the saved value is '' it is not restored.
• The wrong value ends up in &mousemodel.

vim9script

var mm = ''

def SaveRestoreMM()
    echom printf("save '%s'", &mousemodel)

    mm = &mousemodel
    &mousemodel = 'popup_setpos'

    if mm != ''
        echom printf("restore '%s'", mm)
        &mousemodel = mm
        mm = null_string
    endif
enddef

SaveRestoreMM()

&mousemodel = ''

SaveRestoreMM()

SaveRestoreMM()

&mousemodel = ''

SaveRestoreMM()

Output

save 'extend'
restore 'extend'
save ''
save 'popup_setpos'
restore 'popup_setpos'
save ''

Expected behaviour

This is a fix, from #14951, for &mousemodel option, see :help null-anomalies and the PR for more information

--- a/runtime/pack/dist/opt/termdebug/plugin/termdebug.vim
+++ b/runtime/pack/dist/opt/termdebug/plugin/termdebug.vim
@@ -112,1 +112,1 @@
-var saved_mousemodel = ''
+var saved_mousemodel = null_string
@@ -1223,1 +1223,1 @@
-    if saved_mousemodel != ''
+    if saved_mousemodel isnot null_string

Here is the test code with the fix

vim9script

var mm = null_string

def SaveRestoreMM()
    echom printf("save '%s'", &mousemodel)

    mm = &mousemodel
    &mousemodel = 'popup_setpos'

    if mm isnot null_string
        echom printf("restore '%s'", mm)
        &mousemodel = mm
        mm = null_string
    endif
enddef

SaveRestoreMM()

&mousemodel = ''

SaveRestoreMM()

Version of Vim

9.1.472

Environment

linux/gtk

Logs and stack traces

No response

[errael]

The PR is merged. @ubaldot The important remaining stuff for this issue is

I think evalexpr and few other variables are initialized as '' and then tested against such a value and that perhaps happens with some more variables.

[ubaldot]

I just figured that something similar may happen with plus_map_saved, minus_map_saved and k_map_saved. In-spite mappings test pass, what if a user change e.g. 'K' mapping during a termdebug sessions? Also, I noticed a useless exists() in DeleteCommands() function.

[errael]

Just looking at termdebug.vim noticed that some artifacts from legacy could be cleaned up.

var map = 1 could be var map: bool = true or simply var map = true. Similarly var pup = 1 should be var pup = true.

[ubaldot]

Just looking at termdebug.vim noticed that some artifacts from legacy could be cleaned up.

var map = 1 could be var map: bool = true or simply var map = true. Similarly var pup = 1 should be var pup = true.

Yes, I am aware of it. There are a lot of 0 and 1 that should be true amd false. There is room for a good refactoring in general.

A bit of history on how we ended here but you can check my PR as well: at the very beginning I was in need of some customizations but not not knowing legacy vim (I only know Vim9) I made the step "zero" of trying to convert as-is in Vim9 and then start to refactor, change, update and so on. Since I am not familiar how to contribute to open source, I literally copied termdebug.vim on my repo and did some work.

At some point, I thought that given that I took from the community, it would be nice if I give something back, so I opened a PR with the the porting to Vim9 as-is, without any other things that I have implemented. The PR passed all the tests but being a huge work, obviously some "imperfection" slipped out, in-fact, and not surprisingly, there is a number of PR opened, but fortunately none of them is major. So I guess we finally made it.

However, as stated before, I had already did additional work on my repo. The idea now that the porting to Vim9 is in the main distribution, is to take features from my repo and to step-wise adding them though a number of PR:s here, provided that the community will accept them! This just to inform you given that you are showing interest: more PR will come and at some point I will shutoff my repo and work directly on the vim fork :)

Here is the list of the changes I did: https://github.com/ubaldot/experiments/issues/1 and here is the code: https://github.com/ubaldot/experiments/blob/main/termdebug9.vim

As you can see, I refactored it a bit (btw, the mousemodel issue is not present there but it is good to add a test for it in the main distribution) similar to the following

vim9script

&mousemodel = ''
Termdebug
sleep 2
echom "termdebug_mousemodel:" .. &mousemodel

wincmd b
quit!

echom "restored_mousemodel:" .. &mousemodel

with some adjustments, like using some assert instead of the echom, etc.

If you want to take a look at the code, the architecture is not rocket science. The entry point is the StartDebug() function. You can skip everything related to the "prompt" version at first instance and look only to the terminal version. Then you should be able to follow along. I think Bram did an outstanding job.

[errael]

@ubaldot I think everything mentioned in here has been addressed, closing.

If you want this open for some reason, LMK.

[errael]

I just figured that something similar may happen with plus_map_saved, minus_map_saved and k_map_saved. In-spite mappings test pass, what if a user change e.g. 'K' mapping during a termdebug sessions? Also, I noticed a useless exists() in DeleteCommands() function.

I foolishly closed this, thinking @ubaldot would have fixed the problems he identified some days before that big check-in yesterday. But now I see that @Shane-XB-Qian has opened a PR to fix them.

@chrisbra @yegappan I personally would like to see everything converted to vim9script. But given a large plugin that works, replacing it with buggy untested code, and the author seems to put fixing bugs that are introduced at a low priority, seems the wrong direction.

[errael]

See #15013.

I guess anything mentioned in here needs to be looked at more carefully, reopening.

[ubaldot]

I just figured that something similar may happen with plus_map_saved, minus_map_saved and k_map_saved. In-spite mappings test pass, what if a user change e.g. 'K' mapping during a termdebug sessions? Also, I noticed a useless exists() in DeleteCommands() function.

I foolishly closed this, thinking @ubaldot would have fixed the problems he identified some days before that big check-in yesterday. But now I see that @Shane-XB-Qian has opened a PR to fix them.

@chrisbra @yegappan I personally would like to see everything converted to vim9script. But given a large plugin that works, replacing it with buggy untested code, and the author seems to put fixing bugs that are introduced at a low priority, seems the wrong direction.

Respectfully, you should notice the following sequence of events:

Nobody: @errael: "I think everything mentioned in here has been addressed." Nobody: @errael: "the author seems to put fixing bugs that are introduced at a low priority, seems the wrong direction."

It looks like you are doing everything by yourself. :) As said, if someone wants to become the termdebug maintainer is welcome, FYI: I declined the offer. :)

Having said that, I have noticed these saved variable issues. And if you look at the code more carefully, there are others (small hint: sign); And there is even more: check what TermDebugBalloonExpr() returns (note: this was the same in the legacy version of the script), just to cite some. :)

All in all, I think the best approach is to follow standard change management practice, which at page zero, says to address large changes incrementally. What do you think?

P.S. Why I haven't fixed the map saved? Because I plan a PR that extend the mapping feature and that change would automatically fix the saving issue.

[errael]

Nobody: @errael: "I think everything mentioned in here has been addressed."

I assumed (I should know better) that when you did not respond to something directed at you, there was no issue.

Nobody: @errael: "the author seems to put fixing bugs that are introduced at a low priority, seems the wrong direction."

I believe that is an accurate statement.

As I said

I foolishly [thought] @ubaldot would have fixed the problems he identified

I acknowledged that I made a mistake. I reopened the issue.

It looks like you are doing everything by yourself. :)

I'm not sure what you mean "everything by yourself". I fixed a bug, opened some bug reports; those are things one does by themselves hoping that the author of the bugs will take an interest (and by taking an interest, I mean fixing bugs, not simply pointing out more problems). It did feel like I was by myself, left with a broken tool and the author doesn't seem to care.

As said, if someone wants to become the termdebug maintainer is welcome, FYI: I declined the offer. :)

@chrisbra Should an important plugin be replaced by untested buggy code that is not maintained?

All in all, I think the best approach is to follow standard change management practice, which at page zero, says to address large changes incrementally. What do you think?

I think things would be better if "standard change management practice" had been followed.

This should never have been merged as termdebug; it should have been left in Draft state. It obviously wasn't ready for primetime; I'm not sure it was even alpha release quality. It wasn't tested.

"standard change management"? An important rationale for making incremental changes is to not break things; it's too bad standard change management practice wasn't used. Testing before replacing...

[ubaldot]

When you make a large porting it is normal that something slips out in-spite the new software passes all the unit tests. I also assume that the ported version has been reviewed by some expert.

Typically, such anomalies are triggered by some corner cases (like saving the mousemodel as an empty string - not sure how many people do that, and tbh I am not even sure if that was only due to the null thing in Vim9 or if it was an issue also in the old version). On top of that, consider a super weird way of handling null types which, btw, seems to be still under discussion. Is this issue an absolute no-no? But regardless, was that fixed promptly? We also have a test now for that.

Regarding other potential issues (like the mapping, which btw passed all the tests and perhaps didn't even suffer of the same issue as the mouseodel, I should double check) I was planning an even more robust solution to be implemented in few PRs. But it seems that someone wanted the job done... when? Yesterday? Otherwise the author don't care? I don't follow...

I pointed out that I am aware of such potential issue, plus other strange things present also in the old version and that are showing up during refactoring with the plan of investigating/fixing them during next week. But you also need to refactor sometimes to make things to show up, which is what I doing. No new features are added so far. Adding Init functions, sanity checks, etc. are for fixing/preventing bugs, they are not new features. I think here we are all appreciating your effort in finding bugs, but don't expect them to be fixed as soon as you open an issue.

In-spite these disturbances which are absolutely normal when you make a large porting, I think that the plugin is more than usable as it is in its current status - unless you are eager to save your mousemodel as an empty string. :p If you think that this version of the plugin is absolutely unusable, please keep in opening issues in the issue tracker, we would all appreciate that! It would be also great if you point out if the bug is present only in the Vim9 version or also in the legacy version.

And finally, just to be crystal clear, if the director decide to switch back to the previous version, I am fine anyway. This is the beauty of the open-source: you can take the code and modify at your will as long you are in the limit of the license. :)

[errael]

Typically, such anomalies are triggered by some corner cases

I don't consider using breakpoints in a debugger a "corner case"; IMHO broken breakpoint handling is not "more than usable" for a debugger. I had to interrupt my workflow to fix the issue and now need to maintain my own copy so I can use breakpoints reliably; not what I'd call usable. See #14994, it was first reported in the mailing list before I realized it related to breakpoints.

It would be also great if you point out if the bug is present only in the Vim9 version or also in the legacy version

I agree. That's why I asked several days ago that both the legacy and vim9 version are available in the release.

don't expect them to be fixed as soon as you open an issue

I don't. Using breakpoints caused several problems, I consider that high priority. After it was reported, there was a PR which didn't include the fix that was included in the issue report; very disappointing.

I'm putting together a PR that has both versions of termdebug.

[chrisbra]

Sorry, I might have merged that too early and I apologize. Let's make sure we fix the remaining problems. I also wonder why the test didn't catch the issue with the wrong argument type of the breakpoint.

[ubaldot]

Breakpoints are tested through unit-tests. They all passed. Plus I tested them manually. I am sure that other people is using this version and I would be surprised if they are using a debugger without breakpoints. Hence, I would not say they are untested. We had no complaints so far. You found a case where breakpoints won't work. Well, I think that is just great so that we can fix it!

Why the implicit type conversion has not been included in the previous PR? Because the PR was opened before the issue related to breakpoints was opened and already had a big change - adding a Init function is big and it relates to bugs. The PR was about to be completed and we agreed in general to don't overload PR:s. Your proposed bugfix would had been fixed in 3-4 days anyways. Too much? I could understand your point if I was pushing several PR:s with new features by neglecting your bug reports. But that was not the case. I also wonder how unacceptable is to fix bugs after 3-4 days after they have been signalled. Nevertheless, @Shane-XB-Qian happened to fix it even earlier. In my view it's even better then!

Note that I asked at least two-three times to point out what are the problems present in the Vim9 version that are not present in the legacy version to those complaining about stability issues. I received zero answers. If you want to provide such a list you are more than welcome, we are all ears and I believe that the whole community would benefit from that.

[ubaldot]

Sorry, I might have merged that too early and I apologize. Let's make sure we fix the remaining problems. I also wonder why the test didn't catch the issue with the wrong argument type of the breakpoint.

@chrisbra Further notice that the function PlaceSign expects number type for the first two arguments. Hence, I would have expected to have a E1013 type mismatch, expected number but got string.. In that way such a bug would have been immediately caught.

[Shane-XB-Qian]

@ubaldot i knew your effort, and thanks; but as i said at: // https://github.com/vim/vim/pull/14978#issuecomment-2164769948 and its following comments e.g: // https://github.com/vim/vim/pull/14978#issuecomment-2168416733 since you raised a huge change to an important and essential native plugin, good manner was to fix it timely.

but i knew this is an open source project, so cannot say too much, let's see what's going on, and better having more verify at your own locally before PR, let's try to keep it as a stable way, vs sth you proposed plan e.g 'vim9 import' or 'DAP' etc seems not a very matter to termdebug. // or trying to create another independent plugin for them maybe ok, would not impact others at least, if so.

thanks your effort anyway.

-- shane.xb.qian

[errael]

Sorry, I might have merged that too early and I apologize. Let's make sure we fix the remaining problems. I also wonder why the test didn't catch the issue with the wrong argument type of the breakpoint.

@chrisbra Further notice that the function PlaceSign expects number type for the first two arguments. Hence, I would have expected to have a E1013 type mismatch, expected number but got string.. In that way such a bug would have been immediately caught.

Looking at #14994, E1013 is in fact the error that is produced. Since it comes out a BufRead autocommand, I'm guessing the error shows up when you hit a breakpoint that is not in the currently displayed file. I guess that is an untested case; though quite common in normal usage. The code says

# Handle a BufRead autocommand event: place any signs.
def BufRead()

So that probably explain why the gdb up/down commands also show erratic display behavior.

I think we can all agree that the tests could use plenty of work.

[errael]

The PR was about to be completed and we agreed in general to don't overload PR:s.

A one line fix for a critical problem? I'm reminded of https://ptproductsonline.com/industry-news/research-development/people-actually-can-walk-and-chew-gum-at-the-same-time-heres-how/

Your proposed bugfix would had been fixed in 3-4 days anyways. Too much?

Yes, IMHO it's a critical issue. That's 3-4 days from report to now, another 3-4 days for PR, another 1 or more days (depending on how much other stuff is in the PR); over a week. Maybe you don't agree that it's a critical issue.

Note that I asked at least two-three times to point out what are the problems present in the Vim9 version that are not present in the legacy version to those complaining about stability issues. I received zero answers.

What? There have been bug reports, PRs, discussion about having both versions readily available.

I don't know about most people, but I use termdebug rarely in bursts (weeks or months apart); I rarely use "C". And I, and I suspect like many, go to termdebug as a last resort. In addition, most people don't update their install every day.

If you want to provide such a list you are more than welcome, we are all ears and I believe that the whole community would benefit from that.

How do you propose such a list be obtained? From what I can tell seeing some comments, termdebug/terminal is a big thing with many features. I use it infrequently with minimal features (I rearely use "C").

I wholeheartedly agree that such a list would be useful, I find it surprising that you think the list should come from a group separate than the primary developers.

[chrisbra]

guys please. Let's please concentrate on fixing the remaining issues for termdebug (are there still issues?) instead of blaming each other. That's not helpful. Thanks!

[ubaldot]

@chrisbra I can't agree more. A PR is under preparation. ;)

[ubaldot]

'DAP' etc seems not a very matter to termdebug.

https://github.com/vim/vim/issues/11605

[errael]

@ubaldot Can this be closed? I'll wait a couple days, then close it if I don't hear back.

[ubaldot]

(EDIT) I thought I was replying to something else.

Yes, I guess it can be closed now.

[errael]

Closing. Termdebug option handling is way past this now.