refactor(autocmds): remove indirection #32291

Problem:
`AucmdExecutable` adds 2 layers of indirection. Although formalizing
a `union` is good practice for shared interfaces, this struct is mainly
for `autocmd_register` which is internal to this module.

Solution:
- Store the cmd/fn fields directly on the `AutoCmd` struct.
- Drop `AucmdExecutable` and related structures.

src/nvim/api/autocmd.c

@@ -290,10 +290,12 @@ Array nvim_get_autocmds(Dict(get_autocmds) *opts, Arena *arena, Error *err)
         PUT_C(autocmd_info, "desc", CSTR_AS_OBJ(ac->desc));
       }
 
-      if (ac->exec.type == CALLABLE_CB) {
+      if (ac->handler_cmd) {
+        PUT_C(autocmd_info, "command", CSTR_AS_OBJ(ac->handler_cmd));
+      } else {
         PUT_C(autocmd_info, "command", STRING_OBJ(STRING_INIT));
 
-        Callback *cb = &ac->exec.callable.cb;
+        Callback *cb = &ac->handler_fn;
         switch (cb->type) {
         case kCallbackLua:
           if (nlua_ref_is_function(cb->data.luaref)) {
@@ -307,8 +309,6 @@ Array nvim_get_autocmds(Dict(get_autocmds) *opts, Arena *arena, Error *err)
         case kCallbackNone:
           abort();
         }
-      } else {
-        PUT_C(autocmd_info, "command", CSTR_AS_OBJ(ac->exec.callable.cmd));
       }
 
       PUT_C(autocmd_info, "pattern", CSTR_AS_OBJ(ap->pat));
@@ -322,23 +322,6 @@ Array nvim_get_autocmds(Dict(get_autocmds) *opts, Arena *arena, Error *err)
         PUT_C(autocmd_info, "buflocal", BOOLEAN_OBJ(false));
       }
 
-      // TODO(sctx): It would be good to unify script_ctx to actually work with lua
-      //  right now it's just super weird, and never really gives you the info that
-      //  you would expect from this.
-      //
-      //  I think we should be able to get the line number, filename, etc. from lua
-      //  when we're executing something, and it should be easy to then save that
-      //  info here.
-      //
-      //  I think it's a big loss not getting line numbers of where options, autocmds,
-      //  etc. are set (just getting "Sourced (lua)" or something is not that helpful.
-      //
-      //  Once we do that, we can put these into the autocmd_info, but I don't think it's
-      //  useful to do that at this time.
-      //
-      // PUT_C(autocmd_info, "sid", INTEGER_OBJ(ac->script_ctx.sc_sid));
-      // PUT_C(autocmd_info, "lnum", INTEGER_OBJ(ac->script_ctx.sc_lnum));
-
       kvi_push(autocmd_list, DICT_OBJ(autocmd_info));
     }
   }
@@ -411,8 +394,8 @@ Integer nvim_create_autocmd(uint64_t channel_id, Object event, Dict(create_autoc
 {
   int64_t autocmd_id = -1;
   char *desc = NULL;
-  AucmdExecutable aucmd = AUCMD_EXECUTABLE_INIT;
+  char *handler_cmd = NULL;
-  Callback cb = CALLBACK_NONE;
+  Callback handler_fn = CALLBACK_NONE;
 
   Array event_array = unpack_string_or_array(event, "event", true, arena, err);
   if (ERROR_SET(err)) {
@@ -437,13 +420,13 @@ Integer nvim_create_autocmd(uint64_t channel_id, Object event, Dict(create_autoc
         goto cleanup;
       });
 
-      cb.type = kCallbackLua;
+      handler_fn.type = kCallbackLua;
-      cb.data.luaref = callback->data.luaref;
+      handler_fn.data.luaref = callback->data.luaref;
       callback->data.luaref = LUA_NOREF;
       break;
     case kObjectTypeString:
-      cb.type = kCallbackFuncref;
+      handler_fn.type = kCallbackFuncref;
-      cb.data.funcref = string_to_cstr(callback->data.string);
+      handler_fn.data.funcref = string_to_cstr(callback->data.string);
       break;
     default:
       VALIDATE_EXP(false, "callback", "Lua function or Vim function name",
@@ -451,12 +434,8 @@ Integer nvim_create_autocmd(uint64_t channel_id, Object event, Dict(create_autoc
         goto cleanup;
       });
     }
-
-    aucmd.type = CALLABLE_CB;
-    aucmd.callable.cb = cb;
   } else if (HAS_KEY(opts, create_autocmd, command)) {
-    aucmd.type = CALLABLE_EX;
+    handler_cmd = string_to_cstr(opts->command);
-    aucmd.callable.cmd = string_to_cstr(opts->command);
   } else {
     VALIDATE(false, "%s", "Required: 'command' or 'callback'", {
       goto cleanup;
@@ -496,7 +475,6 @@ Integer nvim_create_autocmd(uint64_t channel_id, Object event, Dict(create_autoc
     int retval;
 
     FOREACH_ITEM(patterns, pat, {
-      // See: TODO(sctx)
       WITH_SCRIPT_CONTEXT(channel_id, {
         retval = autocmd_register(autocmd_id,
                                   event_nr,
@@ -506,7 +484,8 @@ Integer nvim_create_autocmd(uint64_t channel_id, Object event, Dict(create_autoc
                                   opts->once,
                                   opts->nested,
                                   desc,
-                                  aucmd);
+                                  handler_cmd,
+                                  &handler_fn);
       });
 
       if (retval == FAIL) {
@@ -517,7 +496,11 @@ Integer nvim_create_autocmd(uint64_t channel_id, Object event, Dict(create_autoc
   });
 
 cleanup:
-  aucmd_exec_free(&aucmd);
+  if (handler_cmd) {
+    XFREE_CLEAR(handler_cmd);
+  } else {
+    callback_free(&handler_fn);
+  }
 
   return autocmd_id;
 }

src/nvim/autocmd.c

@@ -74,7 +74,6 @@ static const char e_autocommand_nesting_too_deep[]
 // Naming Conventions:
 //  - general autocmd behavior start with au_
 //  - AutoCmd start with aucmd_
-//  - Autocmd.exec stat with aucmd_exec
 //  - AutoPat start with aupat_
 //  - Groups start with augroup_
 //  - Events start with event_
@@ -254,24 +253,24 @@ static void au_show_for_event(int group, event_T event, const char *pat)
         return;
       }
 
-      char *exec_to_string = aucmd_exec_to_string(ac, ac->exec);
+      char *handler_str = aucmd_handler_to_string(ac);
       if (ac->desc != NULL) {
         size_t msglen = 100;
         char *msg = xmallocz(msglen);
-        if (ac->exec.type == CALLABLE_CB) {
+        if (ac->handler_cmd) {
-          msg_puts_hl(exec_to_string, HLF_8, false);
+          snprintf(msg, msglen, "%s [%s]", handler_str, ac->desc);
-          snprintf(msg, msglen, " [%s]", ac->desc);
         } else {
-          snprintf(msg, msglen, "%s [%s]", exec_to_string, ac->desc);
+          msg_puts_hl(handler_str, HLF_8, false);
+          snprintf(msg, msglen, " [%s]", ac->desc);
         }
         msg_outtrans(msg, 0, false);
         XFREE_CLEAR(msg);
-      } else if (ac->exec.type == CALLABLE_CB) {
+      } else if (ac->handler_cmd) {
-        msg_puts_hl(exec_to_string, HLF_8, false);
+        msg_outtrans(handler_str, 0, false);
       } else {
-        msg_outtrans(exec_to_string, 0, false);
+        msg_puts_hl(handler_str, HLF_8, false);
       }
-      XFREE_CLEAR(exec_to_string);
+      XFREE_CLEAR(handler_str);
       if (p_verbose > 0) {
         last_set_msg(ac->script_ctx);
       }
@@ -303,7 +302,11 @@ static void aucmd_del(AutoCmd *ac)
     xfree(ac->pat);
   }
   ac->pat = NULL;
-  aucmd_exec_free(&ac->exec);
+  if (ac->handler_cmd) {
+    XFREE_CLEAR(ac->handler_cmd);
+  } else {
+    callback_free(&ac->handler_fn);
+  }
   XFREE_CLEAR(ac->desc);
 
   au_need_clean = true;
@@ -899,8 +902,8 @@ void do_all_autocmd_events(const char *pat, bool once, int nested, char *cmd, bo
 // If *cmd == NUL: show entries.
 // If forceit == true: delete entries.
 // If group is not AUGROUP_ALL: only use this group.
-int do_autocmd_event(event_T event, const char *pat, bool once, int nested, char *cmd, bool del,
+int do_autocmd_event(event_T event, const char *pat, bool once, int nested, const char *cmd,
-                     int group)
+                     bool del, int group)
   FUNC_ATTR_NONNULL_ALL
 {
   // Cannot be used to show all patterns. See au_show_for_event or au_show_for_all_events
@@ -958,10 +961,8 @@ int do_autocmd_event(event_T event, const char *pat, bool once, int nested, char
     }
 
     if (is_adding_cmd) {
-      AucmdExecutable exec = AUCMD_EXECUTABLE_INIT;
+      Callback handler_fn = CALLBACK_INIT;
-      exec.type = CALLABLE_EX;
+      autocmd_register(0, event, pat, patlen, group, once, nested, NULL, cmd, &handler_fn);
-      exec.callable.cmd = cmd;
-      autocmd_register(0, event, pat, patlen, group, once, nested, NULL, exec);
     }
 
     pat = aucmd_next_pattern(pat, (size_t)patlen);
@@ -972,8 +973,13 @@ int do_autocmd_event(event_T event, const char *pat, bool once, int nested, char
   return OK;
 }
 
+/// Registers an autocmd. The handler may be a Ex command or callback function, decided by
+/// the `handler_cmd` or `handler_fn` args.
+///
+/// @param handler_cmd Handler Ex command, or NULL if handler is a function (`handler_fn`).
+/// @param handler_fn Handler function, ignored if `handler_cmd` is not NULL.
 int autocmd_register(int64_t id, event_T event, const char *pat, int patlen, int group, bool once,
-                     bool nested, char *desc, AucmdExecutable aucmd)
+                     bool nested, char *desc, const char *handler_cmd, Callback *handler_fn)
 {
   // 0 is not a valid group.
   assert(group != 0);
@@ -1081,7 +1087,12 @@ int autocmd_register(int64_t id, event_T event, const char *pat, int patlen, int
   AutoCmd *ac = kv_pushp(autocmds[(int)event]);
   ac->pat = ap;
   ac->id = id;
-  ac->exec = aucmd_exec_copy(aucmd);
+  if (handler_cmd) {
+    ac->handler_cmd = xstrdup(handler_cmd);
+  } else {
+    ac->handler_cmd = NULL;
+    callback_copy(&ac->handler_fn, handler_fn);
+  }
   ac->script_ctx = current_sctx;
   ac->script_ctx.sc_lnum += SOURCING_LNUM;
   nlua_set_sctx(&ac->script_ctx);
@@ -2028,7 +2039,7 @@ static void aucmd_next(AutoPatCmd *apc)
 /// Executes an autocmd callback function (as opposed to an Ex command).
 static bool au_callback(const AutoCmd *ac, const AutoPatCmd *apc)
 {
-  Callback callback = ac->exec.callable.cb;
+  Callback callback = ac->handler_fn;
   if (callback.type == kCallbackLua) {
     MAXSIZE_TEMP_DICT(data, 7);
     PUT_C(data, "id", INTEGER_OBJ(ac->id));
@@ -2093,10 +2104,10 @@ char *getnextac(int c, void *cookie, int indent, bool do_concat)
 
   if (p_verbose >= 9) {
     verbose_enter_scroll();
-    char *exec_to_string = aucmd_exec_to_string(ac, ac->exec);
+    char *handler_str = aucmd_handler_to_string(ac);
-    smsg(0, _("autocommand %s"), exec_to_string);
+    smsg(0, _("autocommand %s"), handler_str);
     msg_puts("\n");  // don't overwrite this either
-    XFREE_CLEAR(exec_to_string);
+    XFREE_CLEAR(handler_str);
     verbose_leave_scroll();
   }
 
@@ -2107,11 +2118,9 @@ char *getnextac(int c, void *cookie, int indent, bool do_concat)
   apc->script_ctx = current_sctx;
 
   char *retval;
-  switch (ac->exec.type) {
+  if (ac->handler_cmd) {
-  case CALLABLE_EX:
+    retval = xstrdup(ac->handler_cmd);
-    retval = xstrdup(ac->exec.callable.cmd);
+  } else {
-    break;
-  case CALLABLE_CB: {
     AutoCmd ac_copy = *ac;
     // Mark oneshot handler as "removed" now, to prevent recursion by e.g. `:doautocmd`. #25526
     ac->pat = oneshot ? NULL : ac->pat;
@@ -2133,11 +2142,6 @@ char *getnextac(int c, void *cookie, int indent, bool do_concat)
     //      and instead we loop over all the matches and just execute one-by-one.
     //          However, my expectation would be that could be expensive.
     retval = xcalloc(1, 1);
-    break;
-  }
-  case CALLABLE_NONE:
-  default:
-    abort();
   }
 
   // Remove one-shot ("once") autocmd in anticipation of its execution.
@@ -2456,60 +2460,14 @@ bool autocmd_delete_id(int64_t id)
   return success;
 }
 
-// ===========================================================================
+/// Gets an (allocated) string representation of an autocmd command/callback.
-//  AucmdExecutable Functions
+char *aucmd_handler_to_string(AutoCmd *ac)
-// ===========================================================================
-
-/// Generate a string description for the command/callback of an autocmd
-char *aucmd_exec_to_string(AutoCmd *ac, AucmdExecutable acc)
   FUNC_ATTR_PURE
 {
-  switch (acc.type) {
+  if (ac->handler_cmd) {
-  case CALLABLE_EX:
+    return xstrdup(ac->handler_cmd);
-    return xstrdup(acc.callable.cmd);
-  case CALLABLE_CB:
-    return callback_to_string(&acc.callable.cb, NULL);
-  case CALLABLE_NONE:
-    return "This is not possible";
   }
-
+  return callback_to_string(&ac->handler_fn, NULL);
-  abort();
-}
-
-void aucmd_exec_free(AucmdExecutable *acc)
-{
-  switch (acc->type) {
-  case CALLABLE_EX:
-    XFREE_CLEAR(acc->callable.cmd);
-    break;
-  case CALLABLE_CB:
-    callback_free(&acc->callable.cb);
-    break;
-  case CALLABLE_NONE:
-    return;
-  }
-
-  acc->type = CALLABLE_NONE;
-}
-
-AucmdExecutable aucmd_exec_copy(AucmdExecutable src)
-{
-  AucmdExecutable dest = AUCMD_EXECUTABLE_INIT;
-
-  switch (src.type) {
-  case CALLABLE_EX:
-    dest.type = CALLABLE_EX;
-    dest.callable.cmd = xstrdup(src.callable.cmd);
-    return dest;
-  case CALLABLE_CB:
-    dest.type = CALLABLE_CB;
-    callback_copy(&dest.callable.cb, &src.callable.cb);
-    return dest;
-  case CALLABLE_NONE:
-    return dest;
-  }
-
-  abort();
 }
 
 bool au_event_is_empty(event_T event)

src/nvim/autocmd_defs.h

@@ -37,10 +37,11 @@ typedef struct {
 } AutoPat;
 
 typedef struct {
-  AucmdExecutable exec;     ///< Command or callback function
   AutoPat *pat;             ///< Pattern reference (NULL when autocmd was removed)
   int64_t id;               ///< ID used for uniquely tracking an autocmd
   char *desc;               ///< Description for the autocmd
+  char *handler_cmd;        ///< Handler Ex command (NULL if handler is a function).
+  Callback handler_fn;      ///< Handler callback (ignored if `handler_cmd` is not NULL).
   sctx_T script_ctx;        ///< Script context where it is defined
   bool once;                ///< "One shot": removed after execution
   bool nested;              ///< If autocommands nest here

src/nvim/ex_cmds_defs.h

@@ -94,35 +94,6 @@ typedef struct exarg exarg_T;
 typedef void (*ex_func_T)(exarg_T *eap);
 typedef int (*ex_preview_func_T)(exarg_T *eap, int cmdpreview_ns, handle_T cmdpreview_bufnr);
 
-// NOTE: These possible could be removed and changed so that
-// Callback could take a "command" style string, and simply
-// execute that (instead of it being a function).
-//
-// But it's still a bit weird to do that.
-//
-// Another option would be that we just make a callback reference to
-// "execute($INPUT)" or something like that, so whatever the user
-// sends in via autocmds is just executed via this.
-//
-// However, that would probably have some performance cost (probably
-// very marginal, but still some cost either way).
-typedef enum {
-  CALLABLE_NONE,
-  CALLABLE_EX,
-  CALLABLE_CB,
-} AucmdExecutableType;
-
-typedef struct aucmd_executable_t AucmdExecutable;
-struct aucmd_executable_t {
-  AucmdExecutableType type;
-  union {
-    char *cmd;
-    Callback cb;
-  } callable;
-};
-
-#define AUCMD_EXECUTABLE_INIT { .type = CALLABLE_NONE }
-
 typedef char *(*LineGetter)(int, void *, int, bool);
 
 /// Structure for command definition.