Re: [vim/vim] Make sessions compatible with Vim9script expression mappings (PR #20152)

github-advanced-security[bot]

unread,

May 6, 2026, 6:25:46 PM (yesterday) May 6

to vim/vim, Subscribed

@github-advanced-security[bot] commented on this pull request.

> @@ -1294,12 +1313,24 @@
 #endif
 	    failed |= (makemap(fd, NULL) == FAIL
 					 || makeset(fd, flags, FALSE) == FAIL);
+
+#if defined(FEAT_EVAL)
+	    // save delay load import modules
+	    for (sid = script_items.ga_len; sid > 0; --sid)
+	    {
+		si = SCRIPT_ITEM(sid);
+		if (si->sn_autoload_prefix &&
+		    (fprintf(fd, "import autoload '%s'", si->sn_name) == FAIL ||

CodeQL / Cleartext storage of sensitive information in file

This write into file 'fd' may contain unencrypted data from this source..

Show more details

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

5:06 PM (4 hours ago) 5:06 PM

to vim/vim, Subscribed

chrisbra left a comment (vim/vim#20152)

Thanks, so if I see this correctly, this will now write vim9script session files? Do we need to consider backwards compatibility or compatibility with Neovim?

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.

Copilot

unread,

5:14 PM (3 hours ago) 5:14 PM

to vim/vim, Subscribed

@Copilot commented on this pull request.

Pull request overview

This PR updates session generation so that :mksession emits Vim9script, preserving the correct script-version context for <expr> mappings (Vim9 vs legacy) and ensuring Vim9 autoload-based mappings can work after restoring a session.

Changes:

Make generated session scripts Vim9script-friendly (e.g., add required : before ranges/line-address forms; replace legacy concatenation/let patterns with Vim9 equivalents where needed).
Preserve expression mapping evaluation context by emitting legacy for legacy <expr> mappings and by tracking script-version overrides (:legacy / :vim9cmd) when mappings are created.
Add test coverage for Vim9 and legacy expression mappings surviving a session round-trip, plus update existing session tests to match new output.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/session.c	Switch session output toward Vim9script, adjust emitted commands/variables, and attempt to add Vim9 autoload imports to sessions.
src/map.c	Track/serialize mapping expression script-version context and emit `legacy` for legacy `<expr>` mappings in generated output.
src/fold.c	Adjust emitted fold-restoration commands to be valid under Vim9script (notably `:` before line/range forms).
src/testdir/test_mksession.vim	Update expected session output and add new tests for Vim9/legacy expression mappings across sessions.
src/testdir/test_mksession_utf8.vim	Update expected output to match new Vim9script-style concatenation/variables.
src/testdir/test_autocmd.vim	Minor adjustment to `noswapfile` setup timing for a session-related autocmd test.

Comments suppressed due to low confidence (1)

src/session.c:518

put_view() writes var c: number = ... at script scope. When a session/view contains more than one window that hits this branch, the generated file will redeclare c and fail with E1054/E1041 (variable/script item already declared). Put this var c inside a { ... } block (like the l variable above), or declare c once and only assign to it in subsequent occurrences.

	    if (!wp->w_p_wrap && wp->w_leftcol > 0 && wp->w_width > 0)
	    {
		if (fprintf(fd,
			  "var c: number = %ld - ((%ld * winwidth(0) + %ld) / %ld)",
			    (long)wp->w_virtcol + 1,
			    (long)(wp->w_virtcol - wp->w_leftcol),
			    (long)wp->w_width / 2, (long)wp->w_width) < 0
			|| put_eol(fd) == FAIL
			|| put_line(fd, "if c > 0") == FAIL
			|| fprintf(fd,
			    "exe 'normal! ' .. c .. '|zs' .. %ld .. '|'",
			    (long)wp->w_virtcol + 1) < 0
			|| put_eol(fd) == FAIL
			|| put_line(fd, "else") == FAIL
			|| put_view_curpos(fd, wp, "  ") == FAIL
			|| put_line(fd, "endif") == FAIL)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

In src/map.c:

>  			if (did_cpo)
 			{
-			    if (fprintf(fd, "let s:cpo_save=&cpo") < 0
+			    if (fprintf(fd, "const cpo_save: string = &cpo") < 0
 				    || put_eol(fd) < 0
 				    || fprintf(fd, "set cpo&vim") < 0
 				    || put_eol(fd) < 0)
 				return FAIL;

makemap() now emits const cpo_save: string = &cpo when did_cpo becomes true. makemap() is called multiple times while generating a session (global + per-buffer mappings), so the resulting vim9script can end up with multiple const cpo_save declarations and fail when sourcing. Wrap the cpo save/restore in a { ... } block to keep cpo_save scoped per makemap() emission, or avoid redeclaring by using an assignment to a previously-declared variable.

In src/session.c:

> +	// Enforce vim9script
+	if (put_line(fd, "vim9script") == FAIL)
+	    failed = TRUE;
+
 	// Write the version command for :mkvimrc
 	if (eap->cmdidx == CMD_mkvimrc)
 	    (void)put_line(fd, "version 6.0");

ex_mkrc() currently writes vim9script unconditionally for all of :mkexrc, :mkvimrc, :mkview, and :mksession. The PR description only mentions changing :mksession output; forcing vim9script for mkexrc/mkvimrc is a behavioral change and may break consumers of those generated files. Consider limiting this to CMD_mksession (and possibly CMD_mkview if needed) instead of all mkrc variants.

In src/session.c:

> +	    for (sid = script_items.ga_len; sid > 0; --sid)
+	    {
+		si = SCRIPT_ITEM(sid);
+		if (si->sn_autoload_prefix &&
+		    (fprintf(fd, "import autoload '%s'", si->sn_name) == FAIL ||
+			put_eol(fd) == FAIL))
+		    failed = TRUE;
+	    }

The new autoload import emission has two issues: (1) fprintf() error handling is wrong (fprintf(...) == FAIL will not catch write errors; it should check < 0 like other uses in this file). (2) This block runs for non-session outputs too (e.g. :mkexrc/:mkvimrc), which will add import autoload lines unrelated to those files’ purposes. Consider gating this logic to CMD_mksession only and fixing the fprintf() check.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.

MiguelBarro

unread,

7:38 PM (1 hour ago) 7:38 PM

to vim/vim, Subscribed

@MiguelBarro commented on this pull request.

In src/session.c:

> @@ -1294,12 +1313,24 @@
 #endif
 	    failed |= (makemap(fd, NULL) == FAIL
 					 || makeset(fd, flags, FALSE) == FAIL);
+
+#if defined(FEAT_EVAL)
+	    // save delay load import modules
+	    for (sid = script_items.ga_len; sid > 0; --sid)
+	    {
+		si = SCRIPT_ITEM(sid);
+		if (si->sn_autoload_prefix &&
+		    (fprintf(fd, "import autoload '%s'", si->sn_name) == FAIL ||

The unencrypted data are filepaths to the autoload vim9scripts located either at 'packpath' or 'runtimepath' locations.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.

MiguelBarro

unread,

7:50 PM (1 hour ago) 7:50 PM

to vim/vim, Subscribed

MiguelBarro left a comment (vim/vim#20152)

Thanks, so if I see this correctly, this will now write vim9script session files? Do we need to consider backwards compatibility or compatibility with Neovim?

On the vim side:

people sourcing the session/rc/view files will not be affected.
those copy/pasting the session/rc/view contents will be affected.
On the neovim side:
To my knowledge vim9script is not in the roadmap.
It is somehow an issue because @tjdevries created a vim9script->lua converter.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.

MiguelBarro

unread,

7:57 PM (1 hour ago) 7:57 PM

to vim/vim, Subscribed

@MiguelBarro commented on this pull request.

In src/map.c:

>  			if (did_cpo)
 			{
-			    if (fprintf(fd, "let s:cpo_save=&cpo") < 0
+			    if (fprintf(fd, "const cpo_save: string = &cpo") < 0
 				    || put_eol(fd) < 0
 				    || fprintf(fd, "set cpo&vim") < 0
 				    || put_eol(fd) < 0)
 				return FAIL;

The 'cpo' option prolog and epilog are only used once.
All other variables used multiple times are either:

Referenced once on top of the session file:
https://github.com/MiguelBarro/vim/blob/34ab6c82ac44b47c9899d4f8528531f2d9e6874d/src/session.c#L652-L658
Defined inside a scope block:
https://github.com/MiguelBarro/vim/blob/34ab6c82ac44b47c9899d4f8528531f2d9e6874d/src/session.c#L472-L485

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.

MiguelBarro

unread,

8:01 PM (1 hour ago) 8:01 PM

to vim/vim, Subscribed

@MiguelBarro commented on this pull request.

In src/session.c:

> +	// Enforce vim9script
+	if (put_line(fd, "vim9script") == FAIL)
+	    failed = TRUE;
+
 	// Write the version command for :mkvimrc
 	if (eap->cmdidx == CMD_mkvimrc)
 	    (void)put_line(fd, "version 6.0");

vim9script is enforced in rc/vimrc/view files.
ex_mkrc() is used for them all because a session file is created by aggregation of rc and view files.
Keeping legacy and vim9script implementations will make maintenance more complex.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.

MiguelBarro

unread,

8:08 PM (1 hour ago) 8:08 PM

to vim/vim, Subscribed

@MiguelBarro commented on this pull request.

In src/session.c:

> +	    for (sid = script_items.ga_len; sid > 0; --sid)
+	    {
+		si = SCRIPT_ITEM(sid);
+		if (si->sn_autoload_prefix &&
+		    (fprintf(fd, "import autoload '%s'", si->sn_name) == FAIL ||
+			put_eol(fd) == FAIL))
+		    failed = TRUE;
+	    }

The fprintf() error handling must be fixed.
The import statements are linked to the mappings. They are only included if mappings are included. Only missing in views.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.

MiguelBarro

unread,

8:20 PM (20 minutes ago) 8:20 PM

to vim/vim, Push

@MiguelBarro pushed 1 commit.

b2c0132 addressing copilot review comments

—
View it on GitHub or unsubscribe.

Triage notifications on the go with GitHub Mobile for iOS or Android.

You are receiving this because you are subscribed to this thread.

github-advanced-security[bot]

unread,

8:29 PM (11 minutes ago) 8:29 PM

to vim/vim, Subscribed

@github-advanced-security[bot] commented on this pull request.

In src/session.c:

> @@ -1294,12 +1313,24 @@
 #endif
 	    failed |= (makemap(fd, NULL) == FAIL
 					 || makeset(fd, flags, FALSE) == FAIL);
+
+#if defined(FEAT_EVAL)
+	    // save delay load import modules
+	    for (sid = script_items.ga_len; sid > 0; --sid)
+	    {
+		si = SCRIPT_ITEM(sid);
+		if (si->sn_autoload_prefix &&
+		    (fprintf(fd, "import autoload '%s'", si->sn_name) < 0 ||