Merge pull request #1426 from kareltucek/arg_template_validation_fixes

Arg template validation fixes

Author: kareltucek

.gitignore

@@ -3,4 +3,5 @@ node_modules
 .cache
 build
 .vscode/settings.json
+.jj
 mcux_include.json

doc-dev/reference-manual.md

@@ -188,6 +188,7 @@ COMMAND = set battery.chargeLimit { full | optimizeHealth }
 COMMAND = set bluetooth.enabled BOOL
 COMMAND = set bluetooth.alwaysAdvertiseHid BOOL
 COMMAND = set modifierLayerTriggers.{shift|alt|super|ctrl} {left|right|both}
+COMMAND = &macroArg.<macro argument index (INT)>
 CONDITION = <condition>
 CONDITION = if (EXPRESSION)
 CONDITION = else
@@ -218,7 +219,6 @@ MODIFIER = postponeKeys
 MODIFIER = final
 MODIFIER = autoRepeat
 MODIFIER = oneShot
-TEMPLATE = $macroArg.<macro argument index (INT)>
 IFSHORTCUT_OPTIONS = noConsume | transitive | anyOrder | orGate | timeoutIn <time in ms (INT)> | cancelIn <time in ms(INT)>
 DIRECTION = {left|right|up|down}
 LAYERID = {fn|mouse|mod|base|fn2|fn3|fn4|fn5|alt|shift|super|ctrl}|last|previous|current
@@ -227,7 +227,7 @@ KEYMAPID = <short keymap abbreviation(IDENTIFIER)>|last|current
 MACROID = last | <single char slot identifier(CHAR)> | <single number slot identifier(INT)>
 OPERATOR = + | - | * | / | % | < | > | <= | >= | == | != | && | ||
 VARIABLE_EXPANSION = $<variable name(IDENTIFIER)> | $<config value name>
-VARIABLE_EXPANSION = $currentAddress | $currentTime | $thisKeyId | $queuedKeyId.<queue index (INT)> | $keyId.KEYID_ABBREV | $uhk.name
+VARIABLE_EXPANSION = $currentAddress | $currentTime | $thisKeyId | $queuedKeyId.<queue index (INT)> | $keyId.KEYID_ABBREV | $uhk.name | $macroArg.<macro argument index (INT)>
 EXPRESSION = <expression> | (EXPRESSION) | INT | BOOL | FLOAT | VARIABLE_EXPANSION | EXPRESSION OPERATOR EXPRESSION | !EXPRESSION | min(EXPRESSION [, EXPRESSION]+) | max(EXPRESSION [, EXPRESSION]+)
 EXPRESSION = STRING == STRING | STRING != STRING
 PARENTHESSED_EXPRESSION = (EXPRESSION)
@@ -583,6 +583,15 @@ Internally, values are saved in one of the following types, and types are automa
 - `BOOL` - 1 or 0 value
 - `STRING` - a string _reference_. These strings can use interpolation, but this interpolation is always applied at the expansion site / time.
 
+### Argument expansion:
+
+Key actions can be parametrized with macro arguments. These arguments can be expanded in two ways:
+
+- `$macroArg.<idx>` - in which case they are parsed as a single value. This is the normal and safe variant that prevents context corruption.
+- `&macroArg.<idx>` - this variant substitutes the argument into current parser context. These allow substituing any string, including full commands or their parts. This is an experimental feature and might be unsafe in some contexts. Following limitations apply:
+  - the argument bounds must correspond to token bounds in the fully expanded string
+  - the argument cannot span multiple lines
+
 ### Configuration options:
 
 - `set stickyModifiers {never|smart|always}` globally turns on or off sticky modifiers. This affects only standard scancode actions. Macro actions (both gui and command ones) are always nonsticky, unless `sticky` flag is included in `tapKey|holdKey|pressKey` commands. Default value is `smart`, which is the official behaviour - i.e., `<alt/ctrl/gui> + <tab/arrows>` are sticky.

right/src/config_parser/parse_keymap.c

@@ -307,8 +307,6 @@ static parser_error_t parseKeyActions(uint8_t targetLayer, config_buffer_t *buff
     uint8_t noneBlockUntil = 0;
     rgb_t noneBlockColor = {0, 0, 0};
     uint8_t actionIdx = 0;
-    key_action_t currentAction = {};
-    bool currentActionHasArguments = false;
     for (uint8_t i = 0; i < actionCount; i++) {
         bool isNoneActionInNoneBlock = actionIdx < noneBlockUntil;
 
@@ -347,26 +345,13 @@ static parser_error_t parseKeyActions(uint8_t targetLayer, config_buffer_t *buff
         }
 
         if (wasAction) {
-            // validate previous action
-            if (currentAction.type == KeyActionType_PlayMacro && currentActionHasArguments && Macros_ValidationInProgress) {
-                //validate it
-                Macros_ValidateMacro(currentAction.playMacro.macroId, currentAction.playMacro.offset, currentActionHasArguments, moduleId, actionIdx-1, keymapIdx);
+            if (keyAction->type == KeyActionType_PlayMacro && Macros_ValidationInProgress) {
+                Macros_ValidateMacro(keyAction->playMacro.macroId, keyAction->playMacro.offset, moduleId, actionIdx, keymapIdx, targetLayer);
             }
 
             actionIdx++;
-
-            // cache current action
-            currentAction = *keyAction;
-            currentActionHasArguments = false;
-        } else {
-            currentActionHasArguments = true;
         }
     }
-    // validate previous action
-    if (currentAction.type == KeyActionType_PlayMacro && currentActionHasArguments && Macros_ValidationInProgress) {
-        //validate it
-        Macros_ValidateMacro(currentAction.playMacro.macroId, currentAction.playMacro.offset, currentActionHasArguments, moduleId, actionIdx-1, keymapIdx);
-    }
 
     /* default second touchpad action to right button */
     if (parseMode != ParseMode_DryRun && moduleId == ModuleId_TouchpadRight) {

right/src/debug.h

@@ -23,7 +23,7 @@
 #define DEBUG_BATTERY_TESTING true
 #define DEBUG_UHK60_SLEEPS false
 
-#define DEBUG_ROLL_STATUS_BUFFER true
+#define DEBUG_ROLL_STATUS_BUFFER false
 
 #ifdef __ZEPHYR__
     #include "logger.h"

right/src/macro_events.c

@@ -218,7 +218,7 @@ void MacroEvent_TriggerGenericEvent(generic_macro_event_t eventId)
 void MacroEvent_OnError() {
     static uint32_t last = 0;
 
-    if (Timer_GetCurrentTime() - last > 1000) {
+    if (Timer_GetCurrentTime() - last > 1000 && Macros_ValidationInProgress == false) {
         last = Timer_GetCurrentTime();
         MacroEvent_TriggerGenericEvent(GenericMacroEvent_OnError);
     }

right/src/macros/core.c

@@ -482,7 +482,7 @@ uint8_t Macros_StartMacro(uint8_t index, key_state_t *keyState, uint16_t argumen
     return slotIndex;
 }
 
-void Macros_ValidateMacro(uint8_t macroIndex, uint16_t argumentOffset, bool hasArgs, uint8_t moduleId, uint8_t keyIdx, uint8_t keymapIdx) {
+void Macros_ValidateMacro(uint8_t macroIndex, uint16_t argumentOffset, uint8_t moduleId, uint8_t keyIdx, uint8_t keymapIdx, uint8_t layerIdx) {
     bool wasValid = true;
     uint8_t slotIndex = initMacro(macroIndex, NULL, argumentOffset, 255, 255);
 
@@ -493,6 +493,7 @@ void Macros_ValidateMacro(uint8_t macroIndex, uint16_t argumentOffset, bool hasA
 
     scheduleSlot(slotIndex);
 
+    Macros_ParserError = false;
     bool macroHasNotEnded = AllMacros[macroIndex].macroActionsCount;
     while (macroHasNotEnded) {
         if (S->ms.currentMacroAction.type == MacroActionType_Command) {
@@ -506,14 +507,17 @@ void Macros_ValidateMacro(uint8_t macroIndex, uint16_t argumentOffset, bool hasA
     endMacro();
     S = NULL;
 
-    if (!wasValid && hasArgs && moduleId != 255 && keyIdx != 255 && keymapIdx != 255) {
+    if (!wasValid && moduleId != 255 && keyIdx != 255 && keymapIdx != 255) {
+        const char *name, *nameEnd;
+        FindMacroName(&AllMacros[macroIndex], &name, &nameEnd);
         uint8_t keyId = Utils_KeyCoordinatesToKeyId(ModuleIdToSlotId(moduleId), keyIdx);
         const char* keyAbbrev = MacroKeyIdParser_KeyIdToAbbreviation(keyId);
         const char* keymapAbbrev = AllKeymaps[keymapIdx].abbreviation;
         uint8_t keymapAbbrevLen = AllKeymaps[keymapIdx].abbreviationLen;
         const char* moduleName = ModuleIdToStr(moduleId);
-        Macros_ReportErrorPrintf(NULL, "> Bound at %.*s/%s/%s.\n", keymapAbbrevLen, keymapAbbrev, moduleName, keyAbbrev);
+        Macros_ReportErrorPrintf(NULL, ">     '%.*s' bound at %.*s/%s/%s/%s.\n", nameEnd - name, name, keymapAbbrevLen, keymapAbbrev, LayerNames[layerIdx], moduleName, keyAbbrev);
     }
+    Macros_ParserError = false;
 }
 
 /**
@@ -526,16 +530,10 @@ void Macros_ValidateAllMacros()
     macro_state_t* oldS = S;
     scheduler_state_t schedulerState = Macros_SchedulerState;
     memset(&Macros_SchedulerState, 0, sizeof Macros_SchedulerState);
+    LogU("Validating macros with arguments...\n");
     Macros_DryRun = true;
     Macros_ValidationInProgress = true;
-    // Validate macros without arguments
     uint32_t t1 = Timer_GetCurrentTime();
-    LogU("Validating macros without arguments...\n");
-    for (uint8_t macroIndex = 0; macroIndex < AllMacrosCount; macroIndex++) {
-        Macros_ValidateMacro(macroIndex, 0, false, 255, 255, 255);
-    }
-    LogU("Validating macros with arguments...\n");
-    // Validate macros that have arguments
     for (uint8_t keymapIndex = 0; keymapIndex < AllKeymapsCount; keymapIndex++) {
         DryParseKeymap(keymapIndex);
     }

right/src/macros/core.h

@@ -282,7 +282,7 @@
     void Macros_SignalInterrupt(void);
     void Macros_SignalUsbReportsChange();
     void Macros_ValidateAllMacros();
-    void Macros_ValidateMacro(uint8_t macroIndex, uint16_t argumentOffset, bool hasArgs, uint8_t moduleId, uint8_t keyIdx, uint8_t keymapIdx);
+    void Macros_ValidateMacro(uint8_t macroIndex, uint16_t argumentOffset, uint8_t moduleId, uint8_t keyIdx, uint8_t keymapIdx, uint8_t layerIdx);
     void Macros_WakeBecauseOfKeystateChange();
 
 #endif

right/src/macros/set_command.c

@@ -349,7 +349,7 @@ static macro_variable_t allowUnsecuredConnections(parser_context_t* ctx, set_com
 {
     ASSIGN_BOOL(Cfg.Bt_AllowUnsecuredConnections);
     if (Cfg.Bt_AllowUnsecuredConnections) {
-        Macros_PrintfWithPos(ctx->at, "Warning: insecure connections were allowed. This may allow eavesdropping on your keyboard input!");
+        Macros_PrintfWithPos(ctx, "Warning: insecure connections were allowed. This may allow eavesdropping on your keyboard input!");
     }
 
     return noneVar();

right/src/macros/status_buffer.c

@@ -262,9 +262,12 @@ static void reportErrorHeader(const char* status, uint16_t pos)
     }
 }
 
-static void reportCommandLocation(uint16_t line, uint16_t pos, const char* begin, const char* end, bool markPosition)
+static void reportCommandLocation(uint16_t line, uint16_t pos, const char* begin, const char* end, bool markPosition, uint8_t indent)
 {
     Macros_SetStatusString("> ", NULL);
+    for (uint8_t i = 0; i < indent; i++) {
+        Macros_SetStatusString(" ", NULL);
+    }
     uint16_t l = Buf.len;
     Macros_SetStatusNumSpaced(line, false);
     l = Buf.len - l;
@@ -273,7 +276,7 @@ static void reportCommandLocation(uint16_t line, uint16_t pos, const char* begin
     Macros_SetStatusString("\n", NULL);
     if (markPosition) {
         Macros_SetStatusString("> ", NULL);
-        for (uint8_t i = 0; i < l; i++) {
+        for (uint8_t i = 0; i < l + indent; i++) {
             Macros_SetStatusString(" ", NULL);
         }
         Macros_SetStatusString(" | ", NULL);
@@ -285,21 +288,23 @@ static void reportCommandLocation(uint16_t line, uint16_t pos, const char* begin
     }
 }
 
-static void reportLocationStackLevel(const parser_context_t* ctx, uint16_t line) {
+static void reportLocationStackLevel(const parser_context_t* ctx, uint16_t line, uint8_t indent) {
     uint16_t pos = ctx->at - ctx->begin;
     bool positionIsValid = ctx->begin <= ctx->at && ctx->at <= ctx->end;
     if (positionIsValid) {
-        reportCommandLocation(line, pos, ctx->begin, ctx->end, positionIsValid);
+        reportCommandLocation(line, pos, ctx->begin, ctx->end, positionIsValid, indent);
     } else {
         Macros_SetStatusString("> Position not available here.\n", NULL);
     }
 }
 
 static void reportLocationStack(parser_context_t* ctx, uint16_t line) {
+    uint8_t indent = 0;
     for (uint8_t level = 0; level < ctx->nestingLevel; level++) {
-        reportLocationStackLevel(ViewContext(level), line);
+        reportLocationStackLevel(ViewContext(level), line, indent);
+        indent = 0;
     }
-    reportLocationStackLevel(ctx, line);
+    reportLocationStackLevel(ctx, line, indent);
 }
 
 static void reportError(
@@ -321,7 +326,7 @@ static void reportError(
         const char* endOfLine = S->ms.currentMacroAction.cmd.text + S->ls->ms.commandEnd;
         uint16_t line = findCurrentCommandLine();
         if (startOfLine <= arg && arg <= endOfLine) {
-            reportCommandLocation(line, arg - startOfLine, startOfLine, endOfLine, argIsCommand);
+            reportCommandLocation(line, arg - startOfLine, startOfLine, endOfLine, argIsCommand, 0);
         } else if (ctx != NULL) {
             reportLocationStack(ctx, line);
         } else {
@@ -376,7 +381,7 @@ void Macros_ReportWarn(const char* err, const char* arg, const char *argEnd)
     reportError(err, arg, argEnd, NULL);
 }
 
-void Macros_PrintfWithPos(const char* pos, const char *fmt, ...)
+void Macros_PrintfWithPos(parser_context_t* ctx, const char *fmt, ...)
 {
     REENTRANCY_GUARD_BEGIN;
     va_list myargs;
@@ -385,9 +390,10 @@ void Macros_PrintfWithPos(const char* pos, const char *fmt, ...)
     vsprintf(buffer, fmt, myargs);
 
     indicateOut();
-    if (pos != NULL) {
-        reportErrorHeader("Out", findPosition(pos));
-        reportError(buffer, pos, pos, NULL);
+    if (ctx != NULL) {
+        reportErrorHeader("Out", findPositionCtx(ctx));
+        reportError(buffer, NULL, NULL, ctx);
+
     } else {
         Macros_SetStatusString(buffer, NULL);
     }

right/src/macros/status_buffer.h

@@ -37,7 +37,7 @@
     void Macros_ReportErrorNum(const char* err, int32_t num, const char* pos);
     void Macros_ReportErrorFloat(const char* err, float num, const char* pos);
     void Macros_ReportWarn(const char* err, const char* arg, const char *argEnd);
-    void Macros_PrintfWithPos(const char* pos, const char *fmt, ...);
+    void Macros_PrintfWithPos(parser_context_t* ctx, const char *fmt, ...);
     void Macros_SanitizedPut(const char* text, const char *textEnd);
 
     void MacroStatusBuffer_Validate(void);