Repository: python/cpython
Pull request: bpo-40334: Fix error location upon parsing an invalid string literal
When parsing a string with an invalid escape, the old parser used to
point to the beginning of the invalid string. This PR changes the new
parser to match that behavior, since it's currently pointing to the
end of the string (or to be more precise, to the beginning of the next
token).

Closes we-like-parsers/cpython#64.

<!--
Thanks for your contribution!
Please read this comment in its entirety. It's quite important.

# Pull Request title

It should be in the following format:

```
bpo-NNNN: Summary of the changes made
```

Where: bpo-NNNN refers to the issue number in the https://bugs.python.org.

Most PRs will require an issue number. Trivial changes, like fixing a typo, do not need an issue.

# Backport Pull Request title

If this is a backport PR (PR made against branches other than `master`),
please ensure that the PR title is in the following format:

```
[X.Y] <title from the original PR> (GH-NNNN)
```

Where: [X.Y] is the branch name, e.g. [3.6].

GH-NNNN refers to the PR number from `master`.

-->


<!-- issue-number: [bpo-40334](https://bugs.python.org/issue40334) -->
https://bugs.python.org/issue40334
<!-- /issue-number -->


Problem statement:
Failure in test_string_literals: Unexpected amount of warnings
Current Parser:
```
╰─ ./python       
Python 3.9.0a5+ (heads/pegen:99a8e2fa08, Apr  9 2020, 16:19:04) 
[GCC 9.2.1 20191008] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings
>>> with warnings.catch_warnings(record=True) as w:
...     warnings.simplefilter('always', category=DeprecationWarning)
...     eval("'''\n\\z'''")
... 
'\n\\z'
>>> len(w)
1
```

Pegen:
```
╰─ ./python -p new
Python 3.9.0a5+ (heads/pegen:99a8e2fa08, Apr  9 2020, 16:19:04) 
[GCC 9.2.1 20191008] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings
>>> with warnings.catch_warnings(record=True) as w:
...     warnings.simplefilter('always', category=DeprecationWarning)
...     eval("'''\n\\z'''")
... 
'\n\\z'
>>> len(w)
2
```

Hints from issue discussion:
Simpler repro (the eval is just confusing matters):

Old parser:
```
>>> '\z'
<stdin>:1: DeprecationWarning: invalid escape sequence \z
'\\z'
>>> 
```

New parser:
```
>>> '\z'
<stdin>:1: DeprecationWarning: invalid escape sequence \z
<stdin>:1: DeprecationWarning: invalid escape sequence \z
<stdin>:1: DeprecationWarning: invalid escape sequence \z
'\\z'
>>> 
```
If we memoize the `atom` rule, this goes away, since this is only happening, because the parser tries to parse a string more than once, before it completely fails and it backtracks out of everything.

Should we do it?
It seems that there is another mismatch here, that I had missed.

Current Parser:
```
>>> with warnings.catch_warnings(record=True) as w:
...     eval("'''\n\\z'''")
... 
'\n\\z'
>>> w[0].lineno
1
```

Pegen:
```
>>> with warnings.catch_warnings(record=True) as w:
...     eval("'''\n\\z'''")
... 
'\n\\z'
>>> w[0].lineno
2
```

Not sure which is wrong.
Neither points to the line with the offending `\z` on it -- the old parser points to the start of the string, the new parser to the end. Let's say that the old parser is right then.
Reopening this until we disable the line checking test.
Turns out pegen does not point to the end of the string, but to the `ENDMARKER` token here. The problem is that `STRING` nodes are parsed in a loop, which means that `p->fill - 1` in
https://github.com/python/cpython/blob/531d1e541284bfd7944f8c66a5e8c3c3234afaff/Parser/pegen/pegen.c#L389 will always point to the node after a `STRING`, which I think is a bug and we need to find a way to resolve.

Commit message: Update test_cmd_line_script as well

Patch to review:
diff --git a/Lib/test/test_cmd_line_script.py b/Lib/test/test_cmd_line_script.py
index 1fc9500738f352..171340581af228 100644
--- a/Lib/test/test_cmd_line_script.py
+++ b/Lib/test/test_cmd_line_script.py
@@ -648,7 +648,7 @@ def test_syntaxerror_invalid_escape_sequence_multi_line(self):
             self.assertEqual(
                 stderr.splitlines()[-3:],
                 [   b'    foo = """\\q"""',
-                    b'                 ^',
+                    b'          ^',
                     b'SyntaxError: invalid escape sequence \\q'
                 ],
             )
diff --git a/Lib/test/test_string_literals.py b/Lib/test/test_string_literals.py
index 5b5477d14d467d..a65fba3c1b40be 100644
--- a/Lib/test/test_string_literals.py
+++ b/Lib/test/test_string_literals.py
@@ -118,8 +118,7 @@ def test_eval_str_invalid_escape(self):
             eval("'''\n\\z'''")
         self.assertEqual(len(w), 1)
         self.assertEqual(w[0].filename, '<string>')
-        if use_old_parser():
-            self.assertEqual(w[0].lineno, 1)
+        self.assertEqual(w[0].lineno, 1)
 
         with warnings.catch_warnings(record=True) as w:
             warnings.simplefilter('error', category=DeprecationWarning)
@@ -128,8 +127,7 @@ def test_eval_str_invalid_escape(self):
             exc = cm.exception
         self.assertEqual(w, [])
         self.assertEqual(exc.filename, '<string>')
-        if use_old_parser():
-            self.assertEqual(exc.lineno, 1)
+        self.assertEqual(exc.lineno, 1)
 
     def test_eval_str_raw(self):
         self.assertEqual(eval(""" r'x' """), 'x')
diff --git a/Parser/pegen/parse_string.c b/Parser/pegen/parse_string.c
index d96303dc183fa7..c489d9647088fb 100644
--- a/Parser/pegen/parse_string.c
+++ b/Parser/pegen/parse_string.c
@@ -12,7 +12,7 @@
 // file (like "_PyPegen_raise_syntax_error").
 
 static int
-warn_invalid_escape_sequence(Parser *p, unsigned char first_invalid_escape_char)
+warn_invalid_escape_sequence(Parser *p, unsigned char first_invalid_escape_char, Token *t)
 {
     PyObject *msg =
         PyUnicode_FromFormat("invalid escape sequence \\%c", first_invalid_escape_char);
@@ -20,12 +20,19 @@ warn_invalid_escape_sequence(Parser *p, unsigned char first_invalid_escape_char)
         return -1;
     }
     if (PyErr_WarnExplicitObject(PyExc_DeprecationWarning, msg, p->tok->filename,
-                                 p->tok->lineno, NULL, NULL) < 0) {
+                                 t->lineno, NULL, NULL) < 0) {
         if (PyErr_ExceptionMatches(PyExc_DeprecationWarning)) {
             /* Replace the DeprecationWarning exception with a SyntaxError
                to get a more accurate error report */
             PyErr_Clear();
+
+            /* This is a hack, in order for the SyntaxError to point to the token t,
+               since _PyPegen_raise_error always uses p->tokens[p->fill - 1] for the
+               error location. */
+            Token *old_end = p->tokens[p->fill - 1];
+            p->tokens[p->fill - 1] = t;
             RAISE_SYNTAX_ERROR("invalid escape sequence \\%c", first_invalid_escape_char);
+            p->tokens[p->fill - 1] = old_end;
         }
         Py_DECREF(msg);
         return -1;
@@ -47,7 +54,7 @@ decode_utf8(const char **sPtr, const char *end)
 }
 
 static PyObject *
-decode_unicode_with_escapes(Parser *parser, const char *s, size_t len)
+decode_unicode_with_escapes(Parser *parser, const char *s, size_t len, Token *t)
 {
     PyObject *v, *u;
     char *buf;
@@ -110,7 +117,7 @@ decode_unicode_with_escapes(Parser *parser, const char *s, size_t len)
     v = _PyUnicode_DecodeUnicodeEscape(s, len, NULL, &first_invalid_escape);
 
     if (v != NULL && first_invalid_escape != NULL) {
-        if (warn_invalid_escape_sequence(parser, *first_invalid_escape) < 0) {
+        if (warn_invalid_escape_sequence(parser, *first_invalid_escape, t) < 0) {
             /* We have not decref u before because first_invalid_escape points
                inside u. */
             Py_XDECREF(u);
@@ -123,7 +130,7 @@ decode_unicode_with_escapes(Parser *parser, const char *s, size_t len)
 }
 
 static PyObject *
-decode_bytes_with_escapes(Parser *p, const char *s, Py_ssize_t len)
+decode_bytes_with_escapes(Parser *p, const char *s, Py_ssize_t len, Token *t)
 {
     const char *first_invalid_escape;
     PyObject *result = _PyBytes_DecodeEscape(s, len, NULL, &first_invalid_escape);
@@ -132,7 +139,7 @@ decode_bytes_with_escapes(Parser *p, const char *s, Py_ssize_t len)
     }
 
     if (first_invalid_escape != NULL) {
-        if (warn_invalid_escape_sequence(p, *first_invalid_escape) < 0) {
+        if (warn_invalid_escape_sequence(p, *first_invalid_escape, t) < 0) {
             Py_DECREF(result);
             return NULL;
         }
@@ -147,7 +154,7 @@ decode_bytes_with_escapes(Parser *p, const char *s, Py_ssize_t len)
    string object.  Return 0 if no errors occurred.  */
 int
 _PyPegen_parsestr(Parser *p, const char *s, int *bytesmode, int *rawmode, PyObject **result,
-         const char **fstr, Py_ssize_t *fstrlen)
+                  const char **fstr, Py_ssize_t *fstrlen, Token *t)
 {
     size_t len;
     int quote = Py_CHARMASK(*s);
@@ -245,7 +252,7 @@ _PyPegen_parsestr(Parser *p, const char *s, int *bytesmode, int *rawmode, PyObje
             *result = PyBytes_FromStringAndSize(s, len);
         }
         else {
-            *result = decode_bytes_with_escapes(p, s, len);
+            *result = decode_bytes_with_escapes(p, s, len, t);
         }
     }
     else {
@@ -253,7 +260,7 @@ _PyPegen_parsestr(Parser *p, const char *s, int *bytesmode, int *rawmode, PyObje
             *result = PyUnicode_DecodeUTF8Stateful(s, len, NULL, NULL);
         }
         else {
-            *result = decode_unicode_with_escapes(p, s, len);
+            *result = decode_unicode_with_escapes(p, s, len, t);
         }
     }
     return *result == NULL ? -1 : 0;
@@ -637,7 +644,7 @@ fstring_compile_expr(Parser *p, const char *expr_start, const char *expr_end,
 */
 static int
 fstring_find_literal(Parser *p, const char **str, const char *end, int raw,
-                     PyObject **literal, int recurse_lvl)
+                     PyObject **literal, int recurse_lvl, Token *t)
 {
     /* Get any literal string. It ends when we hit an un-doubled left
        brace (which isn't part of a unicode name escape such as
@@ -660,7 +667,7 @@ fstring_find_literal(Parser *p, const char **str, const char *end, int raw,
                 }
                 break;
             }
-            if (ch == '{' && warn_invalid_escape_sequence(p, ch) < 0) {
+            if (ch == '{' && warn_invalid_escape_sequence(p, ch, t) < 0) {
                 return -1;
             }
         }
@@ -704,7 +711,7 @@ fstring_find_literal(Parser *p, const char **str, const char *end, int raw,
                                                     NULL, NULL);
         else
             *literal = decode_unicode_with_escapes(p, literal_start,
-                                                   s - literal_start);
+                                                   s - literal_start, t);
         if (!*literal)
             return -1;
     }
@@ -1041,7 +1048,7 @@ fstring_find_literal_and_expr(Parser *p, const char **str, const char *end, int
     assert(*literal == NULL && *expression == NULL);
 
     /* Get any literal string. */
-    result = fstring_find_literal(p, str, end, raw, literal, recurse_lvl);
+    result = fstring_find_literal(p, str, end, raw, literal, recurse_lvl, t);
     if (result < 0)
         goto error;
 
diff --git a/Parser/pegen/parse_string.h b/Parser/pegen/parse_string.h
index 4f2aa94fc19b05..78a60d32fb922e 100644
--- a/Parser/pegen/parse_string.h
+++ b/Parser/pegen/parse_string.h
@@ -35,7 +35,7 @@ typedef struct {
 
 void _PyPegen_FstringParser_Init(FstringParser *);
 int _PyPegen_parsestr(Parser *, const char *, int *, int *, PyObject **,
-             const char **, Py_ssize_t *);
+             const char **, Py_ssize_t *, Token *);
 int _PyPegen_FstringParser_ConcatFstring(Parser *, FstringParser *, const char **,
                                 const char *, int, int, Token *, Token *,
                                 Token *);
diff --git a/Parser/pegen/pegen.c b/Parser/pegen/pegen.c
index c311593af70f58..e4f47583264ae5 100644
--- a/Parser/pegen/pegen.c
+++ b/Parser/pegen/pegen.c
@@ -1977,7 +1977,7 @@ _PyPegen_concatenate_strings(Parser *p, asdl_seq *strings)
             goto error;
         }
 
-        if (_PyPegen_parsestr(p, this_str, &this_bytesmode, &this_rawmode, &s, &fstr, &fstrlen) != 0) {
+        if (_PyPegen_parsestr(p, this_str, &this_bytesmode, &this_rawmode, &s, &fstr, &fstrlen, t) != 0) {
             goto error;
         }
 


Code review comments:
diff_hunk: @@ -12,20 +12,27 @@
 // file (like "_PyPegen_raise_syntax_error").
 
 static int
-warn_invalid_escape_sequence(Parser *p, unsigned char first_invalid_escape_char)
+warn_invalid_escape_sequence(Parser *p, unsigned char first_invalid_escape_char, Token *t)
 {
     PyObject *msg =
         PyUnicode_FromFormat("invalid escape sequence \\%c", first_invalid_escape_char);
     if (msg == NULL) {
         return -1;
     }
     if (PyErr_WarnExplicitObject(PyExc_DeprecationWarning, msg, p->tok->filename,
-                                 p->tok->lineno, NULL, NULL) < 0) {
+                                 t->lineno, NULL, NULL) < 0) {
         if (PyErr_ExceptionMatches(PyExc_DeprecationWarning)) {
             /* Replace the DeprecationWarning exception with a SyntaxError
                to get a more accurate error report */
             PyErr_Clear();
+
+            /* This is a hack, in order for the SyntaxError to point to the token t,
+               since _PyPegen_raise_error always uses p->tokens[p->fill - 1] for the
+               error location. */
+            Token *old_end = p->tokens[p->fill - 1];
+            p->tokens[p->fill - 1] = t;
path: Parser/pegen/parse_string.c
text: @user1:
Instead, could we have a new  `Token*` attribute on the parser that (optionally) points to a token where the syntax error happens? Then, in the raise function, we take that token as a preference over `p->tokens[p->fill - 1]`. In this way, we don't need to modify the token string, which initially feels suboptimal (I have not thought about other side effects this may had).

@author:
You mean in the `Parser` struct?

@user1:
> You mean in the `Parser` struct?

Yup

@user1:
Maybe `p->known_err_token` or something similar.

@author:
Good idea! On it.

@author:
Done.

diff_hunk: @@ -1977,7 +1977,7 @@ _PyPegen_concatenate_strings(Parser *p, asdl_seq *strings)
             goto error;
         }
 
-        if (_PyPegen_parsestr(p, this_str, &this_bytesmode, &this_rawmode, &s, &fstr, &fstrlen) != 0) {
path: Parser/pegen/pegen.c
text: @user1:
If we pass the token, we can maybe get the value for `this_str` inside `_PyPegen_parsestr`

@author:
Done!