bpo-38673: dont switch to ps2 if the line starts with comment or whitespace #17421

isidentical · 2019-11-30T21:43:09Z

https://bugs.python.org/issue38673

Automerge-Triggered-By: @gvanrossum

terryjreedy · 2019-12-01T01:30:16Z

The patch fixes the issue on master on Windows. I can't tell if this is the best fix or if there could be an issue not caught in the tests.

gvanrossum

I'm not sure that this is the right approach yet. While you prevent the prompt from changing to "...", the code still keeps accumulating input, which means the line count will be off. Example:

>>> #
>>> )
  File "<stdin>", line 2
SyntaxError: unmatched ')'

I suspect you may have to doctor the caller of tok_nextc(), or perhaps its caller...

Parser/tokenizer.c

…espace

isidentical · 2019-12-02T17:59:53Z

Thanks for suggestions @gvanrossum, current version fixed accumulation bug.

pablogsal · 2019-12-02T21:54:57Z

Parser/tokenizer.c

-            if (tok->nextprompt != NULL)
-                tok->prompt = tok->nextprompt;
+            if (tok->nextprompt != NULL) {
+                if (newtok != NULL && (*newtok == '#' || *newtok == ' ' || *newtok == '\t'))


PEP 7: Please add braces

pablogsal · 2019-12-02T21:55:03Z

Parser/tokenizer.c

+            if (tok->nextprompt != NULL) {
+                if (newtok != NULL && (*newtok == '#' || *newtok == ' ' || *newtok == '\t'))
+                    tok->nextprompt = NULL;
+                else


PEP 7: Please add braces

Misc/NEWS.d/next/Core and Builtins/2019-12-01-00-17-44.bpo-38673.K_Tze-.rst

pablogsal · 2019-12-02T22:02:19Z

Parser/tokenizer.c

-            if (tok->nextprompt != NULL)
-                tok->prompt = tok->nextprompt;
+            if (tok->nextprompt != NULL) {
+                if (newtok != NULL && (*newtok == '#' || *newtok == ' ' || *newtok == '\t'))


Suggestion: You may be able to simplify this with strchr (check that is not slower....maybe is faster if inlined).

Thank you for suggestions, I'll apply them.

pablogsal · 2019-12-02T22:03:30Z

Left some comment regarding the code itself, I didn't check the solution itself yet.

gvanrossum · 2019-12-02T22:12:01Z

(I am working through a backlog, I should pay attention to reviews and other tasks that have been waiting longer first.)

gvanrossum

Better, but there's still a regression. Look here:

Python 3.9.0a1+ (heads/pr/17421:5e632fb43a, Dec  6 2019, 14:23:30) 
[Clang 11.0.0 (clang-1100.0.33.8)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> if 1:
...   # comment
  File "<stdin>", line 2
    # comment
    ^
IndentationError: expected an indented block
>>>

Previously this would just give a ... prompt:

Python 3.8.0 (v3.8.0:fa919fdf25, Oct 14 2019, 10:23:27) 
[Clang 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> if 1:
...   # comment
...   pass
... 
>>>

isidentical · 2019-12-07T10:42:27Z

I think it works now but i am not sure if it is an ideal solution or not.

gvanrossum

I have to think about this more. It seems to work but the changes look ad-hoc (and not at all what I had expected they would be when I wrote up the bug description). I need to do some deep thinking about whether this is the right place in the code to implement this behavior.

Ping me if I haven't replied in a week -- I don't want to keep you hanging forever!

Parser/tokenizer.c

gvanrossum · 2019-12-08T05:10:37Z

I have a different suggestion:

--- a/Parser/tokenizer.c
+++ b/Parser/tokenizer.c
@@ -1148,6 +1148,12 @@ tok_get(struct tok_state *tok, char **p_start, char **p_end)
             if (col == 0 && c == '\n' && tok->prompt != NULL) {
                 blankline = 0; /* Let it through */
             }
+            else if (tok->prompt != NULL && tok->lineno == 1) {
+                /* In interactive mode, if the first line contains
+                   only spaces and a comment, let it through. */
+                blankline = 0;
+                col = altcol = 0;
+            }
             else {
                 blankline = 1; /* Ignore completely */

isidentical · 2019-12-08T08:16:18Z

Thanks for your suggestions Guido.

gvanrossum

LGTM. Can one of the other reviewers double-check there's nothing amiss?

terryjreedy · 2019-12-09T03:10:56Z

The 2 error cases on the issue and 2 above and otherw work for me. I presume

if 1:
... # k
... # k
... a = 2
... )
File "", line 5
SyntaxError: unmatched ')'

is correct.

miss-islington · 2019-12-09T04:38:07Z

Thanks @isidentical for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

…espace (pythonGH-17421) https://bugs.python.org/issue38673 (cherry picked from commit 109fc27) Co-authored-by: Batuhan Taşkaya <47358913+isidentical@users.noreply.github.com>

bedevere-bot · 2019-12-09T04:38:17Z

GH-17516 is a backport of this pull request to the 3.8 branch.

…espace (GH-17421) https://bugs.python.org/issue38673 (cherry picked from commit 109fc27) Co-authored-by: Batuhan Taşkaya <47358913+isidentical@users.noreply.github.com>

…espace (pythonGH-17421) https://bugs.python.org/issue38673 (cherry picked from commit 109fc27) Co-authored-by: Batuhan Taşkaya <47358913+isidentical@users.noreply.github.com> (cherry picked from commit 184a381) Co-authored-by: Miss Islington (bot) <31488909+miss-islington@users.noreply.github.com>

…espace (GH-17421) (GH-17522) https://bugs.python.org/issue38673 (cherry picked from commit 109fc27) Co-authored-by: Batuhan Taşkaya <47358913+isidentical@users.noreply.github.com>

…espace (pythonGH-17421) https://bugs.python.org/issue38673

the-knights-who-say-ni added the CLA signed label Nov 30, 2019

bedevere-bot added the awaiting review label Nov 30, 2019

terryjreedy requested a review from gvanrossum December 1, 2019 01:28

gvanrossum reviewed Dec 1, 2019

View reviewed changes

Parser/tokenizer.c Outdated Show resolved Hide resolved

Parser/tokenizer.c Outdated Show resolved Hide resolved

bpo-38673: dont switch to ps2 if the line starts with comment or whit…

ecf7b35

…espace

isidentical force-pushed the bpo-38673 branch from c9d117e to ecf7b35 Compare December 2, 2019 17:57

isidentical requested a review from gvanrossum December 2, 2019 18:52

pablogsal reviewed Dec 2, 2019

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2019-12-01-00-17-44.bpo-38673.K_Tze-.rst Outdated Show resolved Hide resolved

pablogsal reviewed Dec 2, 2019

View reviewed changes

use strchr to compare

5e632fb

isidentical force-pushed the bpo-38673 branch from 0b0fb24 to 5e632fb Compare December 3, 2019 14:48

gvanrossum reviewed Dec 6, 2019

View reviewed changes

check on single

0b97cf3

gvanrossum reviewed Dec 7, 2019

View reviewed changes

Parser/tokenizer.c Outdated Show resolved Hide resolved

apply suggestions

0e0da91

isidentical requested a review from gvanrossum December 8, 2019 08:20

gvanrossum added 2 commits December 8, 2019 11:37

Tweak comment text

24d0caa

Tweak news message

4d15dfb

gvanrossum approved these changes Dec 8, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Dec 8, 2019

gvanrossum added the 🤖 automerge label Dec 9, 2019

miss-islington merged commit 109fc27 into python:master Dec 9, 2019

bedevere-bot removed the awaiting merge label Dec 9, 2019

gvanrossum added the needs backport to 3.8 label Dec 9, 2019

bedevere-bot removed the needs backport to 3.8 label Dec 9, 2019

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-38673: dont switch to ps2 if the line starts with comment or whit…

1119d98

…espace (pythonGH-17421) https://bugs.python.org/issue38673

Uh oh!

bpo-38673: dont switch to ps2 if the line starts with comment or whitespace #17421

bpo-38673: dont switch to ps2 if the line starts with comment or whitespace #17421

Uh oh!

Conversation

isidentical commented Nov 30, 2019 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryjreedy commented Dec 1, 2019

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

isidentical commented Dec 2, 2019

Uh oh!

pablogsal Dec 2, 2019

Choose a reason for hiding this comment

Uh oh!

pablogsal Dec 2, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pablogsal Dec 2, 2019

Choose a reason for hiding this comment

Uh oh!

isidentical Dec 3, 2019

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Dec 2, 2019

Uh oh!

gvanrossum commented Dec 2, 2019

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

isidentical commented Dec 7, 2019

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gvanrossum commented Dec 8, 2019

Uh oh!

isidentical commented Dec 8, 2019

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

terryjreedy commented Dec 9, 2019

Uh oh!

miss-islington commented Dec 9, 2019

Uh oh!

bedevere-bot commented Dec 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

isidentical commented Nov 30, 2019 •

edited by miss-islington

Loading