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


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

…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.)


        use strchr to compare

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


        check on single

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.


        apply suggestions


        Tweak comment text


        Tweak news message

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.
🐍🍒⛏🤖


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

…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.


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

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


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

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


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

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

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 Dec 1, 2019

gvanrossum reviewed Dec 1, 2019

View 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…

Loading status checks…

ecf7b35

…espace

isidentical force-pushed the isidentical:bpo-38673 branch from c9d117e to ecf7b35 Dec 2, 2019

isidentical requested a review from gvanrossum Dec 2, 2019

pablogsal reviewed Dec 2, 2019

View changes

Parser/tokenizer.c Outdated

if (tok->nextprompt != NULL) {

if (newtok != NULL && (*newtok == '#' || *newtok == ' ' || *newtok == '\t'))

tok->nextprompt = NULL;

else

This comment has been minimized.

Sign in to view

Copy link

pablogsal Dec 2, 2019
Member

PEP 7: Please add braces

pablogsal reviewed Dec 2, 2019

View 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 changes

use strchr to compare

Loading status checks…

5e632fb

isidentical force-pushed the isidentical:bpo-38673 branch from 0b0fb24 to 5e632fb Dec 3, 2019

gvanrossum reviewed Dec 6, 2019

View changes

check on single

Loading status checks…

0b97cf3

gvanrossum reviewed Dec 7, 2019

View changes

Parser/tokenizer.c Outdated Show resolved Hide resolved

apply suggestions

Loading status checks…

0e0da91

isidentical requested a review from gvanrossum Dec 8, 2019

gvanrossum added 2 commits Dec 8, 2019

Tweak comment text

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

24d0caa

Tweak news message

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

4d15dfb

gvanrossum approved these changes Dec 8, 2019

View changes

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

gvanrossum added the 🤖 automerge label 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

python/cpython

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

isidentical commented Nov 30, 2019 •

edited by miss-islington

This comment has been minimized.

terryjreedy commented Dec 1, 2019

gvanrossum left a comment

This comment has been minimized.

isidentical commented Dec 2, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pablogsal commented Dec 2, 2019

This comment has been minimized.

gvanrossum commented Dec 2, 2019

gvanrossum left a comment

This comment has been minimized.

isidentical commented Dec 7, 2019

gvanrossum left a comment

This comment has been minimized.

gvanrossum commented Dec 8, 2019

This comment has been minimized.

isidentical commented Dec 8, 2019

gvanrossum left a comment

This comment has been minimized.

terryjreedy commented Dec 9, 2019

This comment has been minimized.

miss-islington commented Dec 9, 2019

This comment has been minimized.

bedevere-bot commented Dec 9, 2019

Sponsor python/cpython

python/cpython

Join GitHub today

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

Conversation

isidentical commented Nov 30, 2019 • edited by miss-islington

This comment has been minimized.

terryjreedy commented Dec 1, 2019

gvanrossum left a comment

This comment has been minimized.

isidentical commented Dec 2, 2019

This comment has been minimized.

pablogsal Dec 2, 2019

This comment has been minimized.

pablogsal Dec 2, 2019

This comment has been minimized.

pablogsal Dec 2, 2019

This comment has been minimized.

isidentical Dec 3, 2019

This comment has been minimized.

pablogsal commented Dec 2, 2019

This comment has been minimized.

gvanrossum commented Dec 2, 2019

gvanrossum left a comment

This comment has been minimized.

isidentical commented Dec 7, 2019

gvanrossum left a comment

This comment has been minimized.

gvanrossum commented Dec 8, 2019

This comment has been minimized.

isidentical commented Dec 8, 2019

gvanrossum left a comment

This comment has been minimized.

terryjreedy commented Dec 9, 2019

This comment has been minimized.

miss-islington commented Dec 9, 2019

This comment has been minimized.

bedevere-bot commented Dec 9, 2019

isidentical commented Nov 30, 2019 •

edited by miss-islington