Skip to content

/cpython

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from Dec 9, 2019
Merged

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

merged 6 commits into from Dec 9, 2019
+7 −0

Conversation

@isidentical
Copy link
Contributor

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

https://bugs.python.org/issue38673

Automerge-Triggered-By: @gvanrossum
@terryjreedy terryjreedy requested a review from gvanrossum Dec 1, 2019
@terryjreedy

This comment has been minimized.

Sign in to view
Copy link
Member

terryjreedy commented Dec 1, 2019

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
gvanrossum reviewed Dec 1, 2019
View changes
Copy link
Member

gvanrossum left a comment

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 Outdated Show resolved Hide resolved
Parser/tokenizer.c Outdated Show resolved Hide resolved
@isidentical
bpo-38673: dont switch to ps2 if the line starts with comment or whit…
Loading status checks…
ecf7b35 
…espace
@isidentical isidentical force-pushed the isidentical:bpo-38673 branch from c9d117e to ecf7b35 Dec 2, 2019
@isidentical

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

isidentical commented Dec 2, 2019

Thanks for suggestions @gvanrossum, current version fixed accumulation bug.
@isidentical isidentical requested a review from gvanrossum Dec 2, 2019
@pablogsal
pablogsal reviewed Dec 2, 2019
View changes
Parser/tokenizer.c Outdated
if (tok->nextprompt != NULL)
tok->prompt = tok->nextprompt;
if (tok->nextprompt != NULL) {
if (newtok != NULL && (*newtok == '#' || *newtok == ' ' || *newtok == '\t'))

This comment has been minimized.

Sign in to view
Copy link
@pablogsal

pablogsal Dec 2, 2019

Member

PEP 7: Please add braces
@pablogsal
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

pablogsal Dec 2, 2019

Member

PEP 7: Please add braces
@pablogsal
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
pablogsal reviewed Dec 2, 2019
View changes
Parser/tokenizer.c Outdated
if (tok->nextprompt != NULL)
tok->prompt = tok->nextprompt;
if (tok->nextprompt != NULL) {
if (newtok != NULL && (*newtok == '#' || *newtok == ' ' || *newtok == '\t'))

This comment has been minimized.

Sign in to view
Copy link
@pablogsal

pablogsal Dec 2, 2019

Member

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

This comment has been minimized.

Sign in to view
Copy link
@isidentical

isidentical Dec 3, 2019

Author Contributor

Thank you for suggestions, I'll apply them.
@pablogsal

This comment has been minimized.

Sign in to view
Copy link
Member

pablogsal commented Dec 2, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

gvanrossum commented Dec 2, 2019

(I am working through a backlog, I should pay attention to reviews and other tasks that have been waiting longer first.)
@isidentical
use strchr to compare
Loading status checks…
5e632fb
@isidentical isidentical force-pushed the isidentical:bpo-38673 branch from 0b0fb24 to 5e632fb Dec 3, 2019
@gvanrossum
gvanrossum reviewed Dec 6, 2019
View changes
Copy link
Member

gvanrossum left a comment

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
check on single
Loading status checks…
0b97cf3
@isidentical

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

isidentical commented Dec 7, 2019

I think it works now but i am not sure if it is an ideal solution or not.
@gvanrossum
gvanrossum reviewed Dec 7, 2019
View changes
Copy link
Member

gvanrossum left a comment

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 Outdated Show resolved Hide resolved
@gvanrossum

This comment has been minimized.

Sign in to view
Copy link
Member

gvanrossum commented Dec 8, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

isidentical commented Dec 8, 2019

Thanks for your suggestions Guido.
@isidentical
apply suggestions
Loading status checks…
0e0da91
@isidentical isidentical requested a review from gvanrossum Dec 8, 2019
gvanrossum added 2 commits Dec 8, 2019
@gvanrossum
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
@gvanrossum
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
gvanrossum approved these changes Dec 8, 2019
View changes
Copy link
Member

gvanrossum left a comment

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

This comment has been minimized.

Sign in to view
Copy link
Member

terryjreedy commented Dec 9, 2019

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 miss-islington merged commit 109fc27 into python:master Dec 9, 2019
4 checks passed
4 checks passed
Azure Pipelines PR #20191208.20 succeeded
Details
bedevere/issue-number Issue number 38673 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Dec 9, 2019

Thanks @isidentical for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖
miss-islington added a commit to miss-islington/cpython that referenced this pull request Dec 9, 2019
@isidentical @miss-islington
bpo-38673: dont switch to ps2 if the line starts with comment or whit…
b5c2992 
…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

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Dec 9, 2019

GH-17516 is a backport of this pull request to the 3.8 branch.
miss-islington added a commit that referenced this pull request Dec 9, 2019
@miss-islington @isidentical
bpo-38673: dont switch to ps2 if the line starts with comment or whit…
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…
184a381 
…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>
miss-islington added a commit to miss-islington/cpython that referenced this pull request Dec 9, 2019
@miss-islington
bpo-38673: dont switch to ps2 if the line starts with comment or whit…
0ef5a7f 
…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>
ned-deily added a commit that referenced this pull request Dec 9, 2019
@miss-islington @isidentical @ned-deily
bpo-38673: dont switch to ps2 if the line starts with comment or whit…
Loading status checks…
188d5ae 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal

@gvanrossum gvanrossum

Assignees
No one assigned
Labels
🤖 automerge CLA signed
Projects
None yet
Milestone
No milestone
7 participants
@isidentical @terryjreedy @pablogsal @gvanrossum @miss-islington @bedevere-bot @the-knights-who-say-ni
You can’t perform that action at this time.