bpo-30597: Shows expected input in custom 'print' error message #2009

CuriousLearner · 2017-06-08T17:53:04Z

This is a WIP for issue: http://bugs.python.org/issue30597

lisroach

Can you add some unit tests for this?

lisroach · 2017-06-08T18:28:30Z

There appears to be a newline character between the error_msg and the error_msg_end:

>>> print "helllo world"
  File "<stdin>", line 1
    print "helllo world"
                       ^
SyntaxError: Missing parentheses in call to 'print'. Did you mean 'print("hello world"
)'?

CuriousLearner · 2017-06-09T05:06:04Z

Hi @lisroach, yes, I'm on my way to figuring that out; so it's still a work in progress. Also, I need to do additional checks which @ncoghlan mentioned on the bug. I'll update this patch soon.

About the unit test, what is the best place to write them at? Probably I can refer to some earlier test cases written and would be able to write this up too.

ncoghlan

The basic approach here looks good to me, just a few structural and technical comments inline.

ncoghlan · 2017-06-09T04:41:17Z

Objects/exceptions.c

@@ -2897,8 +2897,24 @@ _check_for_legacy_statements(PySyntaxErrorObject *self, Py_ssize_t start)
    }
    if (PyUnicode_Tailmatch(self->text, print_prefix,
                            start, text_len, -1)) {
-        Py_XSETREF(self->msg,
-                  PyUnicode_FromString("Missing parentheses in call to 'print'"));
+        char *error_msg_start = "Missing parentheses in call to 'print'. Did you mean 'print(";


This is getting complex enough now that I think it makes sense to separate it out as a second static helper: _set_legacy_print_statement_msg(PySyntaxErrorObject *self, Py_ssize_t start)

ncoghlan · 2017-06-09T04:43:17Z

Objects/exceptions.c

+                          strlen(error_msg_end)) + 1;
+
+        strcat(error_msg, error_msg_start);
+        strcat(error_msg, msg);


As per the discussion on the issue tracker, we want to check for a trailing , here, and if so, also append end=' ' to request the Python 3 equivalent of Python 2's soft-space behaviour.

(That will also impact the working memory allocation - the simplest option would be to always allocate enough space to handle the suggest_end_arg case, since the working string will be null-terminated anyway)

ncoghlan · 2017-06-09T05:02:54Z

Objects/exceptions.c

+        char *msg = data + PRINT_OFFSET;
+        char *error_msg_end = ")'?";
+
+        char *error_msg = malloc(strlen(error_msg_start) + \


This should use PyMem_Malloc and PyMem_Free rather than the direct C APIs. Also, it should actually free the allocated working memory :)

We may want to include a short block comment explaining that data is UTF-8 encoded, so we mostly treat it as an indivisible opaque blob, except that we know it starts with print and might end with ,. This is also why we work directly with char * and convert to a Python Unicode object at the end, rather than pre-allocating the object via PyUnicode_New and populating it incrementally.

ncoghlan · 2017-06-09T05:19:43Z

@CuriousLearner My guess would be that the misbehaviour @lisroach noted comes from data including the statement's trailing newline, so you'll need to deal with that. I'd also suggest testing a case like print "message"; pass to see how that behaves.

As far as test cases go, it looks like I didn't add any the first time around ("Missing parentheses" doesn't show up anywhere in the test directory for me).

So I'd suggest adding a new test class to Lib/test/test_print.py (e.g. TestPy2MigrationHint) that uses exec() to provoke SyntaxError and checks for the expected details.

CuriousLearner · 2017-06-09T19:28:22Z

@ncoghlan I've revised the patch. I've also included test for the new code. Can you take a pass?

Also, on the bug you mentioned that the right shift operator case is distinctive enough. So, would it be needed to included here?

Please let me know and I'll do the changes. Thanks :)

ncoghlan · 2017-06-10T03:36:55Z

@CuriousLearner Could you file a separate bug on bugs.python.org about the right-shift operator question? The search results indicate that people are hitting it reasonably frequently, so we may be able to save them a trip to Google by adding a custom 'Did you mean "print(<output>, file={:100!r})'.format(rhs)"? message when the LHS is the print builtin.

serhiy-storchaka · 2017-06-10T05:54:20Z

Objects/exceptions.c

+    analogous to Python3 print syntax.
+    */
+
+    PyObject *strip_sep_obj = PyUnicode_FromString("\r\n");


Virtually any API call can fail and raise an exception. Always check the result.

serhiy-storchaka · 2017-06-10T05:55:51Z

Objects/exceptions.c

+    */
+
+    PyObject *strip_sep_obj = PyUnicode_FromString("\r\n");
+    PyObject *data = _PyUnicode_XStrip(self->text, 1, strip_sep_obj);


I would call _PyUnicode_XStrip() after PyUnicode_Substring().

@serhiy-storchaka Any specific reason to it that way?

More than one space after "print".

serhiy-storchaka · 2017-06-10T05:57:11Z

Objects/exceptions.c

+    data = PyUnicode_Substring(data, PRINT_OFFSET, text_len);
+    // gets the modified text_len after stripping `print `
+    text_len = PyUnicode_GET_LENGTH(data);
+    char *maybe_end_arg = " end=\" \"";


It is better to use const char * for immutable C strings.

serhiy-storchaka · 2017-06-10T06:00:26Z

Objects/exceptions.c

+    PyObject *error_msg = PyUnicode_FromFormat(
+        "Missing parentheses in call to 'print'. Did you mean print(%U)?",
+         data);
+    if (PyUnicode_Tailmatch(data, PyUnicode_FromString(","), start, text_len, 1))


You could just read the last character of the string by PyUnicode_READ_CHAR(str, PyUnicode_GET_LENGTH(str)-1) (don't forget to check that PyUnicode_GET_LENGTH(str) > 0).

serhiy-storchaka · 2017-06-10T06:02:08Z

Objects/exceptions.c

+        "Missing parentheses in call to 'print'. Did you mean print(%U)?",
+         data);
+    if (PyUnicode_Tailmatch(data, PyUnicode_FromString(","), start, text_len, 1))
+        error_msg = PyUnicode_FromFormat(


This is almost duplicates the former formatting. Both cases can be merged if use an empty string for maybe_end_arg in the first case.

serhiy-storchaka · 2017-06-10T06:02:50Z

Objects/exceptions.c

+    // PRINT_OFFSET is to remove print word from the data.
+    const int PRINT_OFFSET = 6;
+    Py_ssize_t text_len = PyUnicode_GET_LENGTH(data);
+    data = PyUnicode_Substring(data, PRINT_OFFSET, text_len);


The old value of data is leaked.

serhiy-storchaka · 2017-06-10T06:03:48Z

Objects/exceptions.c

+            data, maybe_end_arg);
+
+    Py_XSETREF(self->msg, error_msg);
+    return 1;


At that point multiple results of PyUnicode_XXX calls are leaked.

serhiy-storchaka · 2017-06-10T06:05:00Z

Objects/exceptions.c

+_set_legacy_print_statement_msg(PySyntaxErrorObject *self, Py_ssize_t start)
+{
+    /*
+    `data` is UTF-8 encoded and treated as an indivisible opaque blob. All


The comment is outdated.

serhiy-storchaka · 2017-06-10T06:06:28Z

Objects/exceptions.c

+
+    PyObject *strip_sep_obj = PyUnicode_FromString("\r\n");
+    PyObject *data = _PyUnicode_XStrip(self->text, 1, strip_sep_obj);
+    // PRINT_OFFSET is to remove print word from the data.


Be more explicit about what is removed ("print ").

serhiy-storchaka · 2017-06-13T05:49:03Z

Objects/exceptions.c

+{
+
+    PyObject *strip_sep_obj = PyUnicode_FromString("\r\n");
+    Py_UCS4 soft_space_check = PyUnicode_READ_CHAR(PyUnicode_FromString(","), 0);


The result of PyUnicode_FromString(",") is not checked for NULL and leaked.

You don't need this. You can use just soft_space_check = ','. Actually I think the named variable is not needed.

serhiy-storchaka · 2017-06-13T05:52:10Z

Objects/exceptions.c

+    PyObject *data = PyUnicode_Substring(self->text, PRINT_OFFSET, text_len);
+
+    if (data == NULL)
+        return -1;


strip_sep_obj is leaked here.

serhiy-storchaka · 2017-06-13T05:53:03Z

Objects/exceptions.c

+    const char *maybe_end_arg = "";
+
+    if (text_len > 0) {
+        if (PyUnicode_READ_CHAR(data, text_len-1) == soft_space_check) {


Two if's can be merged.

serhiy-storchaka · 2017-06-13T05:55:05Z

Objects/exceptions.c

+_set_legacy_print_statement_msg(PySyntaxErrorObject *self, Py_ssize_t start)
+{
+
+    PyObject *strip_sep_obj = PyUnicode_FromString("\r\n");


Add also a space and a tab.

serhiy-storchaka · 2017-06-13T05:56:22Z

Objects/exceptions.c

+    if (data == NULL)
+        return -1;
+
+    data = _PyUnicode_XStrip(data, 1, strip_sep_obj);


The result of _PyUnicode_XStrip() is not checked for error. Old value of data is leaked.

serhiy-storchaka · 2017-06-13T06:03:50Z

Objects/exceptions.c

+        }
+    }
+
+    PyObject *error_msg = PyUnicode_FromFormat(


The result is not checked for error.

CuriousLearner · 2017-06-20T06:05:27Z

@serhiy-storchaka , @ncoghlan , @lisroach

Possible, if you can take a look now?

serhiy-storchaka

Add an entry in Misc/NEWS and your name in Misc/ACKS.

serhiy-storchaka · 2017-06-20T06:11:41Z

Objects/exceptions.c

+static int
+_set_legacy_print_statement_msg(PySyntaxErrorObject *self, Py_ssize_t start)
+{
+


Remove excessive empty lines.

serhiy-storchaka · 2017-06-20T06:12:30Z

Lib/test/test_print.py

+        with self.assertRaises(SyntaxError) as context:
+            exec(python2_print_str)
+
+        self.assertTrue('print("Hello World")' in str(context.exception))


Use assertIn().

Or use assertRaisesRegex() with the argument r'print\("Hello World"\)' or re.escape('print("Hello World")').

serhiy-storchaka · 2017-06-20T06:15:54Z

Lib/test/test_print.py

+        with self.assertRaises(SyntaxError) as context:
+            exec(python2_print_str)
+
+        self.assertTrue('print("Hello World", end=" ")' in str(context.exception))


The same as above.

serhiy-storchaka · 2017-06-20T06:19:21Z

Lib/test/test_print.py

+            exec(python2_print_str)
+
+        self.assertTrue('print("Hello World", end=" ")' in str(context.exception))
+


Add also tests for excessive whitespaces: 'print "Hello World", '.

serhiy-storchaka · 2017-06-20T06:21:32Z

Objects/exceptions.c

+        return -1;
+    }
+
+    PyObject *new_data = _PyUnicode_XStrip(data, 1, strip_sep_obj);


Use 2 instead of 1.

@serhiy-storchaka

I don't have anything to add to @serhiy-storchaka's latest review, but also agree with Serhiy's requests for further updates

CuriousLearner · 2017-06-20T11:54:39Z

@serhiy-storchaka I've updated the patch. Also I've updated the branch to get even with master of upstream.

Can you please take a look now?

Also, for the ACKS, my name is already there.

serhiy-storchaka

The C code LGTM. Added few last minor comments.

May be it is worth to reformulate the Misc/NEWS entry (I don't like the titlecased Print). Nick, your thoughts?

serhiy-storchaka · 2017-06-20T11:49:27Z

Lib/test/test_print.py

+        self.assertIn('print("Hello World", end=" ")', str(context.exception))
+
+    def test_string_with_excessive_whitespace(self):
+        python2_print_str = 'print "Hello World", '


Add yet a space between print and its argument.

serhiy-storchaka · 2017-06-20T11:54:46Z

Misc/NEWS

@@ -10,9 +10,12 @@ What's New in Python 3.7.0 alpha 1?
 Core and Builtins
 -----------------

+- bpo-30597: `Print` now shows expected input in custom error message when
+  used as a Python 2 statement.


Add "Patch by Sanyam Khurana."

ncoghlan · 2017-06-20T12:16:31Z

Misc/NEWS

@@ -10,9 +10,12 @@ What's New in Python 3.7.0 alpha 1?
 Core and Builtins
 -----------------

+- bpo-30597: `Print` now shows expected input in custom error message when


Use print here.

ncoghlan · 2017-06-20T12:18:59Z

I adjusted the NEWS entry directly, so I think this is good to go once the CI gives it a passing grade.

CuriousLearner · 2017-06-20T13:23:54Z

Thanks @ncoghlan @serhiy-storchaka ! :)

On a second note, I just noticed that on the bug, Python 3.6 is also selected for this patch. So would this be back-ported? (As I don't see a backport tag with the PR right now).

serhiy-storchaka · 2017-06-20T13:29:10Z

This is a minor new feature. I don't think it should be backported.

serhiy-storchaka · 2017-06-20T13:33:06Z

Thank you for your contribution Sanyam!

CuriousLearner · 2017-06-20T13:34:27Z

Thanks for all your time @serhiy-storchaka :)

…pythonGH-2009). (cherry picked from commit 3a7f035)

bpo-30597: Shows expected input in custom 'print' error message

fcd1a05

the-knights-who-say-ni added the CLA signed label Jun 8, 2017

lisroach reviewed Jun 8, 2017

View reviewed changes

ncoghlan previously requested changes Jun 9, 2017

View reviewed changes

CuriousLearner added 3 commits June 9, 2017 14:37

bpo-30597: Address some review comments. Still WIP

737ff28

bpo-30597: Re-writes patch using already available utilities

3d08859

bpo-30597: Includes unittest for Py2 migration hint on print statement

80d96b2

CuriousLearner changed the title ~~bpo-30597: [WIP] Shows expected input in custom 'print' error message~~ bpo-30597: Shows expected input in custom 'print' error message Jun 10, 2017

serhiy-storchaka requested changes Jun 10, 2017

View reviewed changes

CuriousLearner added 2 commits June 13, 2017 10:32

bpo-30597: Address review comments

07d2943

bpo-30597: Minor change

1711975

serhiy-storchaka requested changes Jun 13, 2017

View reviewed changes

CuriousLearner added 2 commits June 13, 2017 13:17

bpo-30597: Address review comments

99ad8da

bpo-30597: Remove commented code

0a1de7d

serhiy-storchaka requested changes Jun 20, 2017

View reviewed changes

CuriousLearner added 2 commits June 20, 2017 16:58

bpo-30597: Address Review comments

3401409

Updates with master of python/cpython and resolves merge conflict

097aa87

serhiy-storchaka approved these changes Jun 20, 2017

View reviewed changes

bpo-30597: Address Review comments

825741b

ncoghlan reviewed Jun 20, 2017

View reviewed changes

Fix formatting in NEWS entry

d3efcd0

ncoghlan approved these changes Jun 20, 2017

View reviewed changes

serhiy-storchaka added the type-feature A feature request or enhancement label Jun 20, 2017

serhiy-storchaka merged commit 3a7f035 into python:master Jun 20, 2017

ncoghlan pushed a commit to ncoghlan/cpython that referenced this pull request Jul 2, 2017

[3.6] bpo-30597: Show expected input in custom 'print' error message. (…

a7f2cf2

…pythonGH-2009). (cherry picked from commit 3a7f035)

		exec(python2_print_str)

		self.assertTrue('print("Hello World", end=" ")' in str(context.exception))

Uh oh!

bpo-30597: Shows expected input in custom 'print' error message #2009

bpo-30597: Shows expected input in custom 'print' error message #2009

Uh oh!

Conversation

CuriousLearner commented Jun 8, 2017

Uh oh!

lisroach left a comment

Choose a reason for hiding this comment

Uh oh!

lisroach commented Jun 8, 2017

Uh oh!

CuriousLearner commented Jun 9, 2017

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncoghlan commented Jun 9, 2017

Uh oh!

CuriousLearner commented Jun 9, 2017

Uh oh!

ncoghlan commented Jun 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CuriousLearner commented Jun 20, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CuriousLearner commented Jun 20, 2017

Uh oh!