patch 8.2.1845: Vim9: function defined in a block can't use block variables

Problem:    Vim9: function defined in a block can't use variables defined in
            that block.
Solution:   First step: Make a second hashtab that holds all script variables,
            also block-local ones, with more information.

src/evalvars.c

@@ -2882,7 +2882,7 @@ vars_clear_ext(hashtab_T *ht, int free_val)
 	}
     }
     hash_clear(ht);
-    ht->ht_used = 0;
+    hash_init(ht);
 }
 
 /*
@@ -3142,30 +3142,10 @@ set_var_const(
 	if (flags & ASSIGN_CONST)
 	    di->di_flags |= DI_FLAGS_LOCK;
 
+	// A Vim9 script-local variable is also added to sn_all_vars and
+	// sn_var_vals.
 	if (is_script_local && vim9script)
-	{
+	    add_vim9_script_var(di, tv, type);
-	    scriptitem_T *si = SCRIPT_ITEM(current_sctx.sc_sid);
-
-	    // Store a pointer to the typval_T, so that it can be found by
-	    // index instead of using a hastab lookup.
-	    if (ga_grow(&si->sn_var_vals, 1) == OK)
-	    {
-		svar_T *sv = ((svar_T *)si->sn_var_vals.ga_data)
-						      + si->sn_var_vals.ga_len;
-		sv->sv_name = di->di_key;
-		sv->sv_tv = &di->di_tv;
-		if (type == NULL)
-		    sv->sv_type = typval2type(tv, &si->sn_type_list);
-		else
-		    sv->sv_type = type;
-		sv->sv_const = (flags & ASSIGN_CONST);
-		sv->sv_export = is_export;
-		++si->sn_var_vals.ga_len;
-
-		// let ex_export() know the export worked.
-		is_export = FALSE;
-	    }
-	}
     }
 
     if (copy || tv->v_type == VAR_NUMBER || tv->v_type == VAR_FLOAT)

src/ex_eval.c

@@ -914,41 +914,35 @@ enter_block(cstack_T *cstack)
 {
     ++cstack->cs_idx;
     if (in_vim9script())
-	cstack->cs_script_var_len[cstack->cs_idx] =
+    {
-			  SCRIPT_ITEM(current_sctx.sc_sid)->sn_var_vals.ga_len;
+	scriptitem_T *si = SCRIPT_ITEM(current_sctx.sc_sid);
+
+	cstack->cs_script_var_len[cstack->cs_idx] = si->sn_var_vals.ga_len;
+	cstack->cs_block_id[cstack->cs_idx] = ++si->sn_current_block_id;
+    }
 }
 
     static void
 leave_block(cstack_T *cstack)
 {
-    int i;
+    if (in_vim9script() && SCRIPT_ID_VALID(current_sctx.sc_sid))
-
-    if (in_vim9script())
     {
 	scriptitem_T	*si = SCRIPT_ITEM(current_sctx.sc_sid);
-	hashtab_T	*ht = get_script_local_ht();
+	int		i;
 
-	if (ht != NULL)
+	for (i = cstack->cs_script_var_len[cstack->cs_idx];
-	{
-	    for (i = cstack->cs_script_var_len[cstack->cs_idx];
 					       i < si->sn_var_vals.ga_len; ++i)
-	    {
+	{
-		svar_T	*sv = ((svar_T *)si->sn_var_vals.ga_data) + i;
+	    svar_T	*sv = ((svar_T *)si->sn_var_vals.ga_data) + i;
-		hashitem_T	*hi;
 
-		if (sv->sv_name != NULL)
+	    if (sv->sv_name != NULL)
-		{
+		// Remove a variable declared inside the block, if it still
-		    // Remove a variable declared inside the block, if it still
+		// exists, from sn_vars and move the value into sn_all_vars.
-		    // exists.
+		hide_script_var(si, sv);
-		    hi = hash_find(ht, sv->sv_name);
-		    if (!HASHITEM_EMPTY(hi))
-		    {
-			delete_var(ht, hi);
-			sv->sv_name = NULL;
-		    }
-		}
-	    }
 	}
+
+	// TODO: is this needed?
+	cstack->cs_script_var_len[cstack->cs_idx] = si->sn_var_vals.ga_len;
     }
     --cstack->cs_idx;
 }

src/proto/vim9script.pro

@@ -3,11 +3,14 @@ int in_vim9script(void);
 void ex_vim9script(exarg_T *eap);
 int not_in_vim9(exarg_T *eap);
 void ex_export(exarg_T *eap);
-void free_imports(int sid);
+void free_imports_and_script_vars(int sid);
 void ex_import(exarg_T *eap);
 int find_exported(int sid, char_u *name, ufunc_T **ufunc, type_T **type);
 char_u *handle_import(char_u *arg_start, garray_T *gap, int import_sid, evalarg_T *evalarg, void *cctx);
 char_u *vim9_declare_scriptvar(exarg_T *eap, char_u *arg);
+void add_vim9_script_var(dictitem_T *di, typval_T *tv, type_T *type);
+void hide_script_var(scriptitem_T *si, svar_T *sv);
+void free_all_script_vars(scriptitem_T *si);
 svar_T *find_typval_in_script(typval_T *dest);
 int check_script_var_type(typval_T *dest, typval_T *value, char_u *name);
 /* vim: set ft=c : */

src/scriptfile.c

@@ -1348,8 +1348,8 @@ do_source(
 		}
 	}
 
-	// old imports are no longer valid
+	// old imports and script variables are no longer valid
-	free_imports(sid);
+	free_imports_and_script_vars(sid);
 
 	// in Vim9 script functions are marked deleted
 	if (is_vim9)
@@ -1375,6 +1375,7 @@ do_source(
 	    // Allocate the local script variables to use for this script.
 	    new_script_vars(script_items.ga_len);
 	    ga_init2(&si->sn_var_vals, sizeof(svar_T), 10);
+	    hash_init(&si->sn_all_vars.dv_hashtab);
 	    ga_init2(&si->sn_imports, sizeof(imported_T), 10);
 	    ga_init2(&si->sn_type_list, sizeof(type_T), 10);
 # ifdef FEAT_PROFILE
@@ -1592,7 +1593,7 @@ free_scriptnames(void)
 	vim_free(si->sn_vars);
 
 	vim_free(si->sn_name);
-	free_imports(i);
+	free_imports_and_script_vars(i);
 	free_string_option(si->sn_save_cpo);
 #  ifdef FEAT_PROFILE
 	ga_clear(&si->sn_prl_ga);

src/structs.h

@@ -889,6 +889,7 @@ typedef struct {
     }		cs_pend;
     void	*cs_forinfo[CSTACK_LEN]; // info used by ":for"
     int		cs_line[CSTACK_LEN];	// line nr of ":while"/":for" line
+    int		cs_block_id[CSTACK_LEN];    // block ID stack
     int		cs_script_var_len[CSTACK_LEN];	// value of sn_var_vals.ga_len
 						// when entering the block
     int		cs_idx;			// current entry, or -1 if none
@@ -1701,18 +1702,47 @@ struct funccal_entry {
  * Holds the hashtab with variables local to each sourced script.
  * Each item holds a variable (nameless) that points to the dict_T.
  */
-typedef struct
+typedef struct {
-{
     dictitem_T	sv_var;
     dict_T	sv_dict;
 } scriptvar_T;
 
+/*
+ * Entry for "sn_all_vars".  Contains the s: variables from sn_vars plus the
+ * block-local ones.
+ */
+typedef struct sallvar_S sallvar_T;
+struct sallvar_S {
+    sallvar_T	*sav_next;	  // var with same name but different block
+    int		sav_block_id;	  // block ID where declared
+    int		sav_var_vals_idx; // index in sn_var_vals
+
+    // So long as the variable is valid (block it was defined in is still
+    // active) "sav_di" is used.  It is set to NULL when leaving the block,
+    // then sav_tv and sav_flags are used.
+    dictitem_T *sav_di;		// dictitem with di_key and di_tv
+    typval_T	sav_tv;		// type and value of the variable
+    char_u	sav_flags;	// DI_FLAGS_ flags (only used for variable)
+    char_u	sav_key[1];	// key (actually longer!)
+};
+
+/*
+ * In the sn_all_vars hashtab item "hi_key" points to "sav_key" in a sallvar_T.
+ * This makes it possible to store and find the sallvar_T.
+ * SAV2HIKEY() converts a sallvar_T pointer to a hashitem key pointer.
+ * HIKEY2SAV() converts a hashitem key pointer to a sallvar_T pointer.
+ * HI2SAV() converts a hashitem pointer to a sallvar_T pointer.
+ */
+#define SAV2HIKEY(sav) ((sav)->sav_key)
+#define HIKEY2SAV(p)  ((sallvar_T *)(p - offsetof(sallvar_T, sav_key)))
+#define HI2SAV(hi)     HIKEY2SAV((hi)->hi_key)
+
 /*
  * Entry for "sn_var_vals".  Used for script-local variables.
  */
 typedef struct {
-    char_u	*sv_name;	// points into "sn_vars" di_key
+    char_u	*sv_name;	// points into "sn_all_vars" di_key
-    typval_T	*sv_tv;		// points into "sn_vars" di_tv
+    typval_T	*sv_tv;		// points into "sn_vars" or "sn_all_vars" di_tv
     type_T	*sv_type;
     int		sv_const;
     int		sv_export;	// "export let var = val"
@@ -1742,12 +1772,27 @@ typedef struct
 {
     char_u	*sn_name;
 
-    scriptvar_T	*sn_vars;	// stores s: variables for this script
+    // "sn_vars" stores the s: variables currently valid.  When leaving a block
-    garray_T	sn_var_vals;	// same variables as a list of svar_T
+    // variables local to that block are removed.
+    scriptvar_T	*sn_vars;
+
+    // Specific for a Vim9 script.
+    // "sn_all_vars" stores all script variables ever declared.  So long as the
+    // variable is still valid the value is in "sn_vars->sv_dict...di_tv".
+    // When the block of a declaration is left the value is moved to
+    // "sn_all_vars..sav_tv".
+    // Variables with duplicate names are possible, the sav_block_id must be
+    // used to check that which variable is valid.
+    dict_T	sn_all_vars;	// all script variables, dict of sallvar_T
+
+    // Stores the same variables as in "sn_all_vars" as a list of svar_T, so
+    // that they can be quickly found by index instead of a hash table lookup.
+    // Also stores the type.
+    garray_T	sn_var_vals;
 
     garray_T	sn_imports;	// imported items, imported_T
-
     garray_T	sn_type_list;	// keeps types used by variables
+    int		sn_current_block_id;  // Unique ID for each script block
 
     int		sn_version;	// :scriptversion
     int		sn_had_command;	// TRUE if any command was executed

src/version.c

@@ -750,6 +750,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    1845,
 /**/
     1844,
 /**/

src/vim9script.c

@@ -134,7 +134,7 @@ new_imported(garray_T *gap)
  * Free all imported items in script "sid".
  */
     void
-free_imports(int sid)
+free_imports_and_script_vars(int sid)
 {
     scriptitem_T    *si = SCRIPT_ITEM(sid);
     int		    idx;
@@ -146,7 +146,9 @@ free_imports(int sid)
 	vim_free(imp->imp_name);
     }
     ga_clear(&si->sn_imports);
-    ga_clear(&si->sn_var_vals);
+
+    free_all_script_vars(si);
+
     clear_type_list(&si->sn_type_list);
 }
 
@@ -564,6 +566,127 @@ vim9_declare_scriptvar(exarg_T *eap, char_u *arg)
     return p;
 }
 
+/*
+ * Vim9 part of adding a script variable: add it to sn_all_vars and
+ * sn_var_vals.
+ * When "type" is NULL use "tv" for the type.
+ */
+    void
+add_vim9_script_var(dictitem_T *di, typval_T *tv, type_T *type)
+{
+    scriptitem_T *si = SCRIPT_ITEM(current_sctx.sc_sid);
+
+    // Store a pointer to the typval_T, so that it can be found by
+    // index instead of using a hastab lookup.
+    if (ga_grow(&si->sn_var_vals, 1) == OK)
+    {
+	svar_T *sv = ((svar_T *)si->sn_var_vals.ga_data)
+						      + si->sn_var_vals.ga_len;
+	hashitem_T *hi;
+	sallvar_T *newsav = (sallvar_T *)alloc_clear(
+				       sizeof(sallvar_T) + STRLEN(di->di_key));
+
+	if (newsav == NULL)
+	    return;
+
+	sv->sv_tv = &di->di_tv;
+	if (type == NULL)
+	    sv->sv_type = typval2type(tv, &si->sn_type_list);
+	else
+	    sv->sv_type = type;
+	sv->sv_const = (di->di_flags & DI_FLAGS_LOCK) ? ASSIGN_CONST : 0;
+	sv->sv_export = is_export;
+	newsav->sav_var_vals_idx = si->sn_var_vals.ga_len;
+	++si->sn_var_vals.ga_len;
+
+	STRCPY(&newsav->sav_key, di->di_key);
+	sv->sv_name = newsav->sav_key;
+	newsav->sav_di = di;
+	newsav->sav_block_id = si->sn_current_block_id;
+
+	hi = hash_find(&si->sn_all_vars.dv_hashtab, newsav->sav_key);
+	if (!HASHITEM_EMPTY(hi))
+	{
+	    sallvar_T *sav = HI2SAV(hi);
+
+	    // variable with this name exists in another block
+	    while (sav->sav_next != NULL)
+		sav = sav->sav_next;
+	    sav->sav_next = newsav;
+	}
+	else
+	    // new variable name
+	    hash_add(&si->sn_all_vars.dv_hashtab, newsav->sav_key);
+
+	// let ex_export() know the export worked.
+	is_export = FALSE;
+    }
+}
+
+    void
+hide_script_var(scriptitem_T *si, svar_T *sv)
+{
+    hashtab_T	*script_ht = get_script_local_ht();
+    hashtab_T	*all_ht = &si->sn_all_vars.dv_hashtab;
+    hashitem_T	*script_hi;
+    hashitem_T	*all_hi;
+
+    // Remove a variable declared inside the block, if it still exists.
+    // The typval is moved into the sallvar_T.
+    script_hi = hash_find(script_ht, sv->sv_name);
+    all_hi = hash_find(all_ht, sv->sv_name);
+    if (!HASHITEM_EMPTY(script_hi) && !HASHITEM_EMPTY(all_hi))
+    {
+	dictitem_T	*di = HI2DI(script_hi);
+	sallvar_T	*sav = HI2SAV(all_hi);
+
+	sav->sav_tv = di->di_tv;
+	di->di_tv.v_type = VAR_UNKNOWN;
+	sav->sav_flags = di->di_flags;
+	sav->sav_di = NULL;
+	delete_var(script_ht, script_hi);
+    }
+}
+
+/*
+ * Free the script variables from "sn_all_vars".
+ */
+    void
+free_all_script_vars(scriptitem_T *si)
+{
+    int		todo;
+    hashtab_T	*ht = &si->sn_all_vars.dv_hashtab;
+    hashitem_T	*hi;
+    sallvar_T	*sav;
+    sallvar_T	*sav_next;
+
+    hash_lock(ht);
+    todo = (int)ht->ht_used;
+    for (hi = ht->ht_array; todo > 0; ++hi)
+    {
+	if (!HASHITEM_EMPTY(hi))
+	{
+	    --todo;
+
+	    // Free the variable.  Don't remove it from the hashtab, ht_array
+	    // might change then.  hash_clear() takes care of it later.
+	    sav = HI2SAV(hi);
+	    while (sav != NULL)
+	    {
+		sav_next = sav->sav_next;
+		if (sav->sav_di == NULL)
+		    clear_tv(&sav->sav_tv);
+		vim_free(sav);
+		sav = sav_next;
+	    }
+	}
+    }
+    hash_clear(ht);
+    hash_init(ht);
+
+    ga_clear(&si->sn_var_vals);
+}
+
 /*
  * Find the script-local variable that links to "dest".
  * Returns NULL if not found.