viml/parser/expressions: Fix determining invalid commas/colons

3 files changed, 192 insertions, 57 deletions

diff --git a/src/nvim/viml/parser/expressions.c b/src/nvim/viml/parser/expressions.c
index 1713d0c89f..c283241cb4 100644
--- a/src/nvim/viml/parser/expressions.c
+++ b/src/nvim/viml/parser/expressions.c
@@ -13,6 +13,7 @@
 #include "nvim/types.h"
 #include "nvim/charset.h"
 #include "nvim/ascii.h"
+#include "nvim/assert.h"
 #include "nvim/lib/kvec.h"
 
 #include "nvim/viml/parser/expressions.h"
@@ -37,6 +38,32 @@ typedef enum {
   kENodeArgumentSeparator,
 } ExprASTWantedNode;
 
+/// Operator priority level
+typedef enum {
+  kEOpLvlInvalid = 0,
+  kEOpLvlComplexIdentifier,
+  kEOpLvlParens,
+  kEOpLvlArrow,
+  kEOpLvlComma,
+  kEOpLvlColon,
+  kEOpLvlTernary,
+  kEOpLvlOr,
+  kEOpLvlAnd,
+  kEOpLvlComparison,
+  kEOpLvlAddition,  ///< Addition, subtraction and concatenation.
+  kEOpLvlMultiplication,  ///< Multiplication, division and modulo.
+  kEOpLvlUnary,  ///< Unary operations: not, minus, plus.
+  kEOpLvlSubscript,  ///< Subscripts.
+  kEOpLvlValue,  ///< Values: literals, variables, nested expressions, …
+} ExprOpLvl;
+
+/// Operator associativity
+typedef enum {
+  kEOpAssNo= 'n',  ///< Not associative / not applicable.
+  kEOpAssLeft = 'l',  ///< Left associativity.
+  kEOpAssRight = 'r',  ///< Right associativity.
+} ExprOpAssociativity;
+
 #ifdef INCLUDE_GENERATED_DECLARATIONS
 # include "viml/parser/expressions.c.generated.h"
 #endif
@@ -747,6 +774,7 @@ static inline void viml_pexpr_debug_print_token(
 //
 // NVimParenthesis -> Delimiter
 //
+// NVimColon -> Delimiter
 // NVimComma -> Delimiter
 // NVimArrow -> Delimiter
 //
@@ -895,6 +923,32 @@ static const ExprOpAssociativity node_type_to_op_ass[] = {
   [kExprNodeListLiteral] = kEOpAssNo,
 };
 
+/// Get AST node priority level
+///
+/// Used primary to reduce line length, so keep the name short.
+///
+/// @param[in]  node  Node to get priority for.
+///
+/// @return Node priority level.
+static inline ExprOpLvl node_lvl(const ExprASTNode node)
+  FUNC_ATTR_ALWAYS_INLINE FUNC_ATTR_CONST FUNC_ATTR_WARN_UNUSED_RESULT
+{
+  return node_type_to_op_lvl[node.type];
+}
+
+/// Get AST node associativity, to be used for operator nodes primary
+///
+/// Used primary to reduce line length, so keep the name short.
+///
+/// @param[in]  node  Node to get priority for.
+///
+/// @return Node associativity.
+static inline ExprOpAssociativity node_ass(const ExprASTNode node)
+  FUNC_ATTR_ALWAYS_INLINE FUNC_ATTR_CONST FUNC_ATTR_WARN_UNUSED_RESULT
+{
+  return node_type_to_op_ass[node.type];
+}
+
 /// Handle binary operator
 ///
 /// This function is responsible for handling priority levels as well.
@@ -910,20 +964,19 @@ static void viml_pexpr_handle_bop(ExprASTStack *const ast_stack,
   assert(kv_size(*ast_stack));
   const ExprOpLvl bop_node_lvl = (bop_node->type == kExprNodeCall
                                   ? kEOpLvlSubscript
-                                  : node_type_to_op_lvl[bop_node->type]);
+                                  : node_lvl(*bop_node));
 #ifndef NDEBUG
   const ExprOpAssociativity bop_node_ass = (
       bop_node->type == kExprNodeCall
       ? kEOpAssLeft
-      : node_type_to_op_ass[bop_node->type]);
+      : node_ass(*bop_node));
 #endif
   do {
     ExprASTNode **new_top_node_p = kv_last(*ast_stack);
     ExprASTNode *new_top_node = *new_top_node_p;
     assert(new_top_node != NULL);
-    const ExprOpLvl new_top_node_lvl = node_type_to_op_lvl[new_top_node->type];
-    const ExprOpAssociativity new_top_node_ass = (
-        node_type_to_op_ass[new_top_node->type]);
+    const ExprOpLvl new_top_node_lvl = node_lvl(*new_top_node);
+    const ExprOpAssociativity new_top_node_ass = node_ass(*new_top_node);
     assert(bop_node_lvl != new_top_node_lvl
            || bop_node_ass == new_top_node_ass);
     if (top_node_p != NULL
@@ -1352,32 +1405,31 @@ viml_pexpr_parse_process_token:
           goto viml_pexpr_parse_invalid_comma;
         }
         for (size_t i = 1; i < kv_size(ast_stack); i++) {
-          const ExprASTNode *const *const eastnode_p =
-              (const ExprASTNode *const *)kv_Z(ast_stack, i);
-          if (!((*eastnode_p)->type == kExprNodeComma
-                || ((*eastnode_p)->type == kExprNodeColon
-                    && i == 1))
-              || i == kv_size(ast_stack) - 1) {
-            switch ((*eastnode_p)->type) {
-              case kExprNodeLambda: {
-                assert(want_node == kENodeArgumentSeparator);
-                break;
-              }
-              case kExprNodeDictLiteral:
-              case kExprNodeListLiteral:
-              case kExprNodeCall: {
-                break;
-              }
-              default: {
+          ExprASTNode *const *const eastnode_p =
+              (ExprASTNode *const *)kv_Z(ast_stack, i);
+          const ExprASTNodeType eastnode_type = (*eastnode_p)->type;
+          const ExprOpLvl eastnode_lvl = node_lvl(**eastnode_p);
+          if (eastnode_type == kExprNodeLambda) {
+            assert(want_node == kENodeArgumentSeparator);
+            break;
+          } else if (eastnode_type == kExprNodeDictLiteral
+                     || eastnode_type == kExprNodeListLiteral
+                     || eastnode_type == kExprNodeCall) {
+            break;
+          } else if (eastnode_type == kExprNodeComma
+                     || eastnode_type == kExprNodeColon
+                     || eastnode_lvl > kEOpLvlComma) {
+            // Do nothing
+          } else {
 viml_pexpr_parse_invalid_comma:
-                ERROR_FROM_TOKEN_AND_MSG(
-                    cur_token,
-                    _("E15: Comma outside of call, lambda or literal: %.*s"));
-                break;
-              }
-            }
+            ERROR_FROM_TOKEN_AND_MSG(
+                cur_token,
+                _("E15: Comma outside of call, lambda or literal: %.*s"));
             break;
           }
+          if (i == kv_size(ast_stack) - 1) {
+            goto viml_pexpr_parse_invalid_comma;
+          }
         }
         NEW_NODE_WITH_CUR_POS(cur_node, kExprNodeComma);
         viml_pexpr_handle_bop(&ast_stack, cur_node, &want_node);
@@ -1389,37 +1441,48 @@ viml_pexpr_parse_invalid_comma:
         if (kv_size(ast_stack) < 2) {
           goto viml_pexpr_parse_invalid_colon;
         }
+        bool is_ternary = false;
+        bool can_be_ternary = true;
         for (size_t i = 1; i < kv_size(ast_stack); i++) {
           ExprASTNode *const *const eastnode_p =
               (ExprASTNode *const *)kv_Z(ast_stack, i);
-          if ((*eastnode_p)->type != kExprNodeColon
-              || i == kv_size(ast_stack) - 1) {
-            switch ((*eastnode_p)->type) {
-              case kExprNodeUnknownFigure: {
-                SELECT_FIGURE_BRACE_TYPE((*eastnode_p), DictLiteral, Dict);
-                break;
-              }
-              case kExprNodeComma:
-              case kExprNodeDictLiteral:
-              case kExprNodeTernary: {
-                break;
-              }
-              default: {
+          const ExprASTNodeType eastnode_type = (*eastnode_p)->type;
+          const ExprOpLvl eastnode_lvl = node_lvl(**eastnode_p);
+          STATIC_ASSERT(kEOpLvlTernary > kEOpLvlComma,
+                        "Unexpected operator priorities");
+          if (can_be_ternary && eastnode_lvl == kEOpLvlTernary) {
+            assert(eastnode_type == kExprNodeTernary);
+            is_ternary = true;
+            break;
+          } else if (eastnode_type == kExprNodeUnknownFigure) {
+            SELECT_FIGURE_BRACE_TYPE(*eastnode_p, DictLiteral, Dict);
+            break;
+          } else if (eastnode_type == kExprNodeDictLiteral
+                     || eastnode_type == kExprNodeComma) {
+            break;
+          } else if (eastnode_lvl > kEOpLvlTernary) {
+            // Do nothing
+          } else if (eastnode_lvl > kEOpLvlComma) {
+            can_be_ternary = false;
+          } else {
 viml_pexpr_parse_invalid_colon:
-                ERROR_FROM_TOKEN_AND_MSG(
-                    cur_token,
-                    _("E15: Colon outside of dictionary or ternary operator: "
-                      "%.*s"));
-                break;
-              }
-            }
+            ERROR_FROM_TOKEN_AND_MSG(
+                cur_token,
+                _("E15: Colon outside of dictionary or ternary operator: "
+                  "%.*s"));
             break;
           }
+          if (i == kv_size(ast_stack) - 1) {
+            goto viml_pexpr_parse_invalid_colon;
+          }
         }
         NEW_NODE_WITH_CUR_POS(cur_node, kExprNodeColon);
         viml_pexpr_handle_bop(&ast_stack, cur_node, &want_node);
-        // FIXME: Handle ternary operator.
-        HL_CUR_TOKEN(Colon);
+        if (is_ternary) {
+          HL_CUR_TOKEN(TernaryColon);
+        } else {
+          HL_CUR_TOKEN(Colon);
+        }
         want_node = kENodeValue;
         break;
       }
diff --git a/src/nvim/viml/parser/expressions.h b/src/nvim/viml/parser/expressions.h
index 64abab9e41..01a51e4eda 100644
--- a/src/nvim/viml/parser/expressions.h
+++ b/src/nvim/viml/parser/expressions.h
@@ -144,10 +144,10 @@ typedef enum {
 typedef enum {
   kExprNodeMissing = 'X',
   kExprNodeOpMissing = '_',
-  kExprNodeTernary = '?',  ///< Ternary operator, valid one has three children.
-  kExprNodeRegister = '@',  ///< Register, no children.
-  kExprNodeSubscript = 's',  ///< Subscript, should have two or three children.
-  kExprNodeListLiteral = 'l',  ///< List literal, any number of children.
+  kExprNodeTernary = '?',  ///< Ternary operator.
+  kExprNodeRegister = '@',  ///< Register.
+  kExprNodeSubscript = 's',  ///< Subscript.
+  kExprNodeListLiteral = 'l',  ///< List literal.
   kExprNodeUnaryPlus = 'p',
   kExprNodeBinaryPlus = '+',
   kExprNodeNested = 'e',  ///< Nested parenthesised expression.
diff --git a/test/unit/viml/expressions/parser_spec.lua b/test/unit/viml/expressions/parser_spec.lua
index a2b76ccf8d..1f734c3c2a 100644
--- a/test/unit/viml/expressions/parser_spec.lua
+++ b/test/unit/viml/expressions/parser_spec.lua
@@ -181,14 +181,14 @@ child_call_once(function()
 end)
 
 describe('Expressions parser', function()
-  local function check_parsing(str, flags, exp_ast, exp_highlighting_fs,
-                               print_exp)
+  local function check_parsing(str, flags, exp_ast, exp_highlighting_fs)
     local pstate = new_pstate({str})
     local east = lib.viml_pexpr_parse(pstate, flags)
     local ast = east2lua(pstate, east)
     local hls = phl2lua(pstate)
-    if print_exp then
+    if exp_ast == nil then
       format_check(str, flags, ast, hls)
+      return
     end
     eq(exp_ast, ast)
     if exp_highlighting_fs then
@@ -2416,6 +2416,78 @@ describe('Expressions parser', function()
       hl('Curly', '}'),
       hl('Identifier', 'j'),
     })
+    check_parsing('{@a + @b : @c + @d, @e + @f : @g + @i}', 0, {
+      --           01234567890123456789012345678901234567
+      --           0         1         2         3
+      ast = {
+        {
+          'DictLiteral(-di):0:0:{',
+          children = {
+            {
+              'Comma:0:18:,',
+              children = {
+                {
+                  'Colon:0:8: :',
+                  children = {
+                    {
+                      'BinaryPlus:0:3: +',
+                      children = {
+                        'Register(name=a):0:1:@a',
+                        'Register(name=b):0:5: @b',
+                      },
+                    },
+                    {
+                      'BinaryPlus:0:13: +',
+                      children = {
+                        'Register(name=c):0:10: @c',
+                        'Register(name=d):0:15: @d',
+                      },
+                    },
+                  },
+                },
+                {
+                  'Colon:0:27: :',
+                  children = {
+                    {
+                      'BinaryPlus:0:22: +',
+                      children = {
+                        'Register(name=e):0:19: @e',
+                        'Register(name=f):0:24: @f',
+                      },
+                    },
+                    {
+                      'BinaryPlus:0:32: +',
+                      children = {
+                        'Register(name=g):0:29: @g',
+                        'Register(name=i):0:34: @i',
+                      },
+                    },
+                  },
+                },
+              },
+            },
+          },
+        },
+      },
+    }, {
+      hl('Dict', '{'),
+      hl('Register', '@a'),
+      hl('BinaryPlus', '+', 1),
+      hl('Register', '@b', 1),
+      hl('Colon', ':', 1),
+      hl('Register', '@c', 1),
+      hl('BinaryPlus', '+', 1),
+      hl('Register', '@d', 1),
+      hl('Comma', ','),
+      hl('Register', '@e', 1),
+      hl('BinaryPlus', '+', 1),
+      hl('Register', '@f', 1),
+      hl('Colon', ':', 1),
+      hl('Register', '@g', 1),
+      hl('BinaryPlus', '+', 1),
+      hl('Register', '@i', 1),
+      hl('Dict', '}'),
+    })
   end)
   -- FIXME: Test sequence of arrows inside and outside lambdas.
   -- FIXME: Test autoload character and scope in lambda arguments.