Repository: geldata/gel
Pull request: Make `$something` a token instead of two
Fixes #923

Problem statement:
Improve tokenization of variables
The current tokenizer separates `$` and ident or number tokens. This means the following is valid (apart from mixing positional and named arguments):
```
SELECT $   x, $   12, $   `test`
```
It's okay, but isn't needed per se.

It also means tuple access doesn't work sometimes:
```
edgedb> select $0.1;                   
EdgeQLSyntaxError: Unexpected '0.1'
### select $0.1;
###         ^
```

Making `$xxxx` a single token should fix the latter (and disallow spaces as a side-effect).

Hints from issue discussion:
Right, looks like among other things we need more tests using parametrized queries. Good catch!

Commit message: Make `$something` a token instead of two

Fixes #923

Patch to review:
diff --git a/edb/edgeql/parser/grammar/commondl.py b/edb/edgeql/parser/grammar/commondl.py
index a16f27208f1..715efe9a850 100644
--- a/edb/edgeql/parser/grammar/commondl.py
+++ b/edb/edgeql/parser/grammar/commondl.py
@@ -179,16 +179,16 @@ def reduce_Identifier(self, dp):
         self.val = dp.val
         self.context = dp.context
 
-    def reduce_DOLLAR_AnyIdentifier(self, dk, dp):
-        raise EdgeQLSyntaxError(
-            f"function parameters do not need a $ prefix, "
-            f"rewrite as '{dp.val}'",
-            context=dk.context)
-
-    def reduce_DOLLAR_ICONST(self, dk, di):
-        raise EdgeQLSyntaxError(
-            f'numeric parameters are not supported',
-            context=dk.context)
+    def reduce_ARGUMENT(self, dp):
+        if dp.val[1].isdigit():
+            raise EdgeQLSyntaxError(
+                f'numeric parameters are not supported',
+                context=dp.context)
+        else:
+            raise EdgeQLSyntaxError(
+                f"function parameters do not need a $ prefix, "
+                f"rewrite as '{dp.val[1:]}'",
+                context=dp.context)
 
 
 class FuncDeclArg(Nonterm):
diff --git a/edb/edgeql/parser/grammar/expressions.py b/edb/edgeql/parser/grammar/expressions.py
index 1b18b010b42..70fe6159251 100644
--- a/edb/edgeql/parser/grammar/expressions.py
+++ b/edb/edgeql/parser/grammar/expressions.py
@@ -895,11 +895,8 @@ def reduce_BaseBytesConstant(self, *kids):
 
 
 class ArgConstant(Nonterm):
-    def reduce_DOLLAR_ICONST(self, *kids):
-        self.val = qlast.Parameter(name=str(kids[1].val))
-
-    def reduce_DOLLAR_AnyIdentifier(self, *kids):
-        self.val = qlast.Parameter(name=kids[1].val)
+    def reduce_ARGUMENT(self, *kids):
+        self.val = qlast.Parameter(name=kids[0].val[1:])
 
 
 class BaseNumberConstant(Nonterm):
@@ -1197,16 +1194,16 @@ def reduce_AnyIdentifier_ASSIGN_Expr(self, *kids):
             kids[2].val,
         )
 
-    def reduce_DOLLAR_ICONST_ASSIGN_Expr(self, *kids):
-        raise EdgeQLSyntaxError(
-            f"numeric named arguments are not supported",
-            context=kids[0].context)
-
-    def reduce_DOLLAR_AnyIdentifier_ASSIGN_Expr(self, *kids):
-        raise EdgeQLSyntaxError(
-            f"named arguments do not need a '$' prefix: "
-            f"rewrite as '{kids[1].val} := ...'",
-            context=kids[0].context)
+    def reduce_ARGUMENT_ASSIGN_Expr(self, *kids):
+        if kids[0].val[1].isdigit():
+            raise EdgeQLSyntaxError(
+                f"numeric named arguments are not supported",
+                context=kids[0].context)
+        else:
+            raise EdgeQLSyntaxError(
+                f"named arguments do not need a '$' prefix, "
+                f"rewrite as '{kids[0].val[1:]} := ...'",
+                context=kids[0].context)
 
 
 class FuncCallArg(Nonterm):
diff --git a/edb/edgeql/parser/grammar/lexer.py b/edb/edgeql/parser/grammar/lexer.py
index cadb59e0bcb..a7fe7437387 100644
--- a/edb/edgeql/parser/grammar/lexer.py
+++ b/edb/edgeql/parser/grammar/lexer.py
@@ -244,7 +244,15 @@ class EdgeQLLexer(lexer.Lexer):
 
         Rule(token='self',
              next_state=STATE_KEEP,
-             regexp=r'[\{\}$]'),
+             regexp=r'[\{\}]'),
+
+        Rule(token='ARGUMENT',
+             next_state=STATE_KEEP,
+             regexp=r'\$(?:[0-9]+|[^\W\d]\w*|`(?:[^`]|``)*`)'),
+
+        Rule(token='BADARGUMENT',
+             next_state=STATE_KEEP,
+             regexp=r'\$[0-9]+[^\W\d]\w*'),
     ]
 
     states = {
@@ -290,6 +298,17 @@ def token_from_text(self, rule_token, txt):
             elif '::' in txt:
                 self.handle_error(f'Identifiers cannot contain "::"',
                                   exact_message=True)
+        elif rule_token == 'ARGUMENT':
+            if txt == '$``':
+                self.handle_error(f'Backtick-quoted variable names '
+                                  f'cannot be empty',
+                                  exact_message=True)
+        elif rule_token == 'BADARGUMENT':
+            self.handle_error(f"Invalid argument name {txt!r} "
+                              f"should be either all digits or "
+                              f"start with letter",
+                              exact_message=True,
+                              exc_type=UnterminatedStringError)
 
         tok = super().token_from_text(rule_token, txt)
 
diff --git a/edb/edgeql/parser/grammar/tokens.py b/edb/edgeql/parser/grammar/tokens.py
index b299cb4ec95..251c5a3f586 100644
--- a/edb/edgeql/parser/grammar/tokens.py
+++ b/edb/edgeql/parser/grammar/tokens.py
@@ -135,7 +135,7 @@ class T_AT(Token, lextoken='@'):
     pass
 
 
-class T_DOLLAR(Token, lextoken='$'):
+class T_ARGUMENT(Token):
     pass
 
 
diff --git a/edb/server/defines.py b/edb/server/defines.py
index acfd5133b1e..995ea62c263 100644
--- a/edb/server/defines.py
+++ b/edb/server/defines.py
@@ -27,7 +27,7 @@
 EDGEDB_VISIBLE_METADATA_PREFIX = r'EdgeDB metadata follows, do not modify.\n'
 
 # Increment this whenever the database layout or stdlib changes.
-EDGEDB_CATALOG_VERSION = 20191128_00_00
+EDGEDB_CATALOG_VERSION = 20191205_00_00
 
 # Resource limit on open FDs for the server process.
 # By default, at least on macOS, the max number of open FDs
diff --git a/tests/test_edgeql_syntax.py b/tests/test_edgeql_syntax.py
index 47ff6e95279..6b7baffe96d 100644
--- a/tests/test_edgeql_syntax.py
+++ b/tests/test_edgeql_syntax.py
@@ -424,7 +424,7 @@ def test_edgeql_syntax_constants_41(self):
 
     def test_edgeql_syntax_constants_42(self):
         """
-        SELECT $ select;
+        SELECT $select;
         """
 
     def test_edgeql_syntax_constants_43(self):
@@ -1776,6 +1776,32 @@ def test_edgeql_syntax_path_28(self):
         SELECT TUP.1.1;
         """
 
+    def test_edgeql_syntax_path_29(self):
+        # legal when `$0`, `$1`, `$a` and `$abc` are tuples
+        """
+        SELECT $0.0;
+        SELECT $0.0.name;
+        SELECT $0.0.1.name;
+        SELECT $0.0.1.n;
+        SELECT $abc.0;
+        SELECT $abc.0.name;
+        SELECT $abc.0.1.name;
+        SELECT $abc.0.1.n;
+        SELECT $1.1.1;
+        SELECT $1.>1.1;
+        SELECT $1.>1.>1;
+        SELECT $a.1.1;
+        SELECT $a.>1.1;
+        SELECT $a.>1.>1;
+        """
+
+    @tb.must_fail(errors.EdgeQLSyntaxError, "Unexpected '\$'",
+                  line=2, col=16)
+    def test_edgeql_syntax_path_30(self):
+        """
+        SELECT $ a;
+        """
+
     def test_edgeql_syntax_type_interpretation_01(self):
         """
         SELECT Foo[IS Bar].spam;
@@ -2623,7 +2649,7 @@ def test_edgeql_syntax_function_07(self):
         """
 
     @tb.must_fail(errors.EdgeQLSyntaxError,
-                  r"named arguments do not need a '\$' prefix: "
+                  r"named arguments do not need a '\$' prefix, "
                   r"rewrite as 'a := \.\.\.'",
                   line=2, col=25)
     def test_edgeql_syntax_function_08(self):
diff --git a/tests/test_type_coverage.py b/tests/test_type_coverage.py
index 29f4e3d448b..71d94f89e7b 100644
--- a/tests/test_type_coverage.py
+++ b/tests/test_type_coverage.py
@@ -206,7 +206,7 @@ def test_cqa_type_coverage_common_markup(self) -> None:
         self.assertFunctionCoverage(EDB_DIR / "common" / "markup", 0)
 
     def test_cqa_type_coverage_edgeql(self) -> None:
-        self.assertFunctionCoverage(EDB_DIR / "edgeql", 41.06)
+        self.assertFunctionCoverage(EDB_DIR / "edgeql", 41.17)
 
     def test_cqa_type_coverage_edgeql_compiler(self) -> None:
         self.assertFunctionCoverage(EDB_DIR / "edgeql" / "compiler", 100.00)


Code review comments:
diff_hunk: @@ -1776,6 +1776,32 @@ def test_edgeql_syntax_path_28(self):
         SELECT TUP.1.1;
         """
 
+    def test_edgeql_syntax_path_29(self):
+        # legal when `$0`, `$1`, `$a` and `$abc` are tuples
+        """
+        SELECT $0.0;
+        SELECT $0.0.name;
+        SELECT $0.0.1.name;
+        SELECT $0.0.1.n;
+        SELECT $abc.0;
+        SELECT $abc.0.name;
+        SELECT $abc.0.1.name;
+        SELECT $abc.0.1.n;
+        SELECT $1.1.1;
+        SELECT $1.>1.1;
+        SELECT $1.>1.>1;
+        SELECT $a.1.1;
+        SELECT $a.>1.1;
+        SELECT $a.>1.>1;
+        """
+
+    @tb.must_fail(errors.EdgeQLSyntaxError, "Unexpected '\$'",
path: tests/test_edgeql_syntax.py
text: @user1:
Use a double backslash here, `\$` is not a valid escape sequence.

@user1:
Or if you didn't mean to keep the backslash, just drop it.