bpo-32028: Fix custom print suggestion having leading whitespace in print statement #4688

CuriousLearner · 2017-12-03T17:01:54Z

This fixes the newly added print suggestion for cases when there is leading whitespace in the initial data.

I've also added a test case for this. @ncoghlan Can you please check this?

https://bugs.python.org/issue32028

…rint statement

ncoghlan

The new test case is good, and the implementation changes are headed in the right general direction. However, it should be sufficient to reorder the existing operations, rather than calling _PyUnicode_XStrip twice.

ncoghlan · 2017-12-04T01:39:05Z

Objects/exceptions.c

    Py_ssize_t text_len = PyUnicode_GET_LENGTH(self->text);
-    PyObject *data = PyUnicode_Substring(self->text, PRINT_OFFSET, text_len);
+    // Issue 32028: Handle case when whitespace is used with print call
+    PyObject *initial_data = _PyUnicode_XStrip(self->text, 2, strip_sep_obj);


Rather than adding a new call to _PyUnicode_XStrip, just reorder the existing operations:

First strip any surrounding ASCII whitespace (i.e. do this first, instead of last)

Then get the length of the result (rather than the length of the original text)

Then skip over PRINT_OFFSET characters at the beginning

A minor point I missed when reviewing the original PR: defining and using const int STRIP_BOTH = 2; will make the _PyUnicode_XStrip call more self explanatory.

So we may as well make that change now, since the code is being modified anyway.

+1 for the const int STRIP_BOTH = 2;. I was explaining my patch to a few folks and I had to consult my blog for that particular thing. Few months down the line I didn't even remember that.

So, yes that would be great.

@ncoghlan So, while I modified the code, one of the test cases fails with re-ordering the code. That test case is:

test_string_with_excessive_whitespace. Since we're only stripping the the data at the beginning, and then directly trying to extract the sub-string.

I think we may need to strip the leading chars to make the previous test case pass. What do you say?

cc @serhiy-storchaka

I've pushed my current code which re-orders the existing operations & the test_string_with_excessive_whitespace is failing. I'm waiting for your reply on this. I think we might want to strip the leading whitespace from data.

ncoghlan · 2017-12-04T01:52:11Z

Lib/test/test_print.py


+    def test_string_with_leading_whitespace(self):
+        python2_print_str = '''if 1:
+    print "Hello World"


Since we're stripping all leading whitespace, you may as well indent this 4 spaces relative to the target variable name (so 12 leading spaces total)

bedevere-bot · 2017-12-04T01:54:21Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka

Don't forget to check for errors and count references.

serhiy-storchaka · 2017-12-04T14:05:05Z

Objects/exceptions.c

+    const int STRIP_BOTH = 2;
+    // Issue 32028: Handle case when whitespace is used with print call
+    PyObject *initial_data = _PyUnicode_XStrip(self->text, STRIP_BOTH, strip_sep_obj);
+    Py_ssize_t text_len = PyUnicode_GET_LENGTH(initial_data);


initial_data can be NULL.

serhiy-storchaka · 2017-12-04T14:06:54Z

Objects/exceptions.c

+    PyObject *initial_data = _PyUnicode_XStrip(self->text, STRIP_BOTH, strip_sep_obj);
+    Py_ssize_t text_len = PyUnicode_GET_LENGTH(initial_data);
+    PyObject *data = _PyUnicode_XStrip( \
+        PyUnicode_Substring(initial_data, PRINT_OFFSET, text_len), \


The result of PyUnicode_Substring() can be NULL.
Otherwise it will be leaked.

CuriousLearner · 2017-12-04T15:59:03Z

@serhiy-storchaka Nice catch! Sorry, I missed those memory leaks. I've tried to address those issues. Can you please take a pass.

serhiy-storchaka

Please remove redundant empty lines.

CuriousLearner · 2017-12-04T17:44:52Z

@serhiy-storchaka Fixed :)

ncoghlan

I'd missed that the second _PyUnicode_XStrip call handled excess whitespace between print and the expression being printed, so I was wrong about that being redundant (and the regression test suite was right).

I've adjusted the NEWS entry wording, so +1 from me (I'll merge once CI finishes).

CuriousLearner · 2017-12-05T07:09:35Z

Thank you so much @ncoghlan & @serhiy-storchaka :)

CuriousLearner · 2017-12-16T14:26:36Z

@serhiy-storchaka @ncoghlan I guess we can merge this now :)

Also needs a backport to 3.6 label on this one.

CuriousLearner · 2018-01-17T17:26:04Z

Hi @serhiy-storchaka @ncoghlan

Do we need something else here? Please let me know and I'll do that :)

ncoghlan · 2018-01-18T04:38:05Z

Just closing & reopending to restart the CI (Appveyor's a required check now, and it didn't run properly)

miss-islington · 2018-01-20T03:12:24Z

Thanks @CuriousLearner for the PR, and @ncoghlan for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

The suggested replacement for print statements previously failed to account for leading whitespace and hence could end up including unwanted text in the proposed call to the print builtin. Patch by Sanyam Khurana. (cherry picked from commit d57f26c)

bedevere-bot · 2018-01-20T03:12:33Z

GH-5249 is a backport of this pull request to the 3.6 branch.

bpo-32028: Fix custom print suggestion having leading whitespace in p…

99c26e8

…rint statement

the-knights-who-say-ni added the CLA signed label Dec 3, 2017

bedevere-bot added the awaiting review label Dec 3, 2017

ncoghlan requested changes Dec 4, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Dec 4, 2017

CuriousLearner added 2 commits December 4, 2017 19:06

Address Nick's review

44c28b2

Use XStrip again to modify

74ed54e

serhiy-storchaka requested changes Dec 4, 2017

View reviewed changes

CuriousLearner added 2 commits December 4, 2017 21:25

Address Serhiy's review

2c330a5

Handle memory leak for

3e5b704

serhiy-storchaka reviewed Dec 4, 2017

View reviewed changes

Remove redundant lines

aaa850b

serhiy-storchaka approved these changes Dec 4, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Dec 4, 2017

Adjust wording of NEWS entry

2abeecd

ncoghlan approved these changes Dec 5, 2017

View reviewed changes

ncoghlan closed this Jan 18, 2018

ncoghlan reopened this Jan 18, 2018

ncoghlan added the needs backport to 3.6 label Jan 18, 2018

ncoghlan merged commit d57f26c into python:master Jan 20, 2018

bedevere-bot removed the awaiting merge label Jan 20, 2018

bedevere-bot removed the needs backport to 3.6 label Jan 20, 2018

Uh oh!

bpo-32028: Fix custom print suggestion having leading whitespace in print statement #4688

bpo-32028: Fix custom print suggestion having leading whitespace in print statement #4688

Uh oh!

Conversation

CuriousLearner commented Dec 3, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncoghlan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bedevere-bot commented Dec 4, 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!

CuriousLearner commented Dec 4, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

CuriousLearner commented Dec 4, 2017

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

CuriousLearner commented Dec 5, 2017

Uh oh!

CuriousLearner commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CuriousLearner commented Jan 17, 2018

Uh oh!

ncoghlan commented Jan 18, 2018

Uh oh!

miss-islington commented Jan 20, 2018

Uh oh!

bedevere-bot commented Jan 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CuriousLearner commented Dec 3, 2017 •

edited by bedevere-bot

Loading

ncoghlan left a comment •

edited

Loading

CuriousLearner commented Dec 16, 2017 •

edited

Loading