bpo-30597: Shows expected input in custom 'print' error message by CuriousLearner · Pull Request #2009 · python/cpython

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

                            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

+                          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

+        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

+    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

+    */
+
+    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

+    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

+    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

+        "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

+    // 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

+            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

+_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

+
+    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

+{
+
+    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

+    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

+    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

+_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

+    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

+        }
+    }
+
+    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

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


Remove excessive empty lines.

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

+        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

+        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

+            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

+        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

+        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

 -----------------

+- 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

 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!

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!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!