Skip to content

[bpo-37433] Fix SyntaxError indicator printing too many spaces for multi-line strings#14433

Merged
pablogsal merged 1 commit intopython:masterfrom
asottile:bpo_37433
Jul 29, 2019
Merged

[bpo-37433] Fix SyntaxError indicator printing too many spaces for multi-line strings#14433
pablogsal merged 1 commit intopython:masterfrom
asottile:bpo_37433

Conversation

@asottile
Copy link
Copy Markdown
Contributor

@asottile asottile commented Jun 27, 2019 •
edited by bedevere-bot
Loading

@mangrisano
Copy link
Copy Markdown
Contributor

mangrisano commented Jun 28, 2019

/cc @benjaminp

terryjreedy
terryjreedy reviewed Jun 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the example in the new test, I verified the problem and fix on Win 10.

I am not familiar enough with C and CPython C code to be sure that fix is exactly what is needed.

Comment thread Lib/test/test_cmd_line_script.py Outdated
@asottile
Copy link
Copy Markdown
Contributor Author

asottile commented Jun 29, 2019

Using the example in the new test, I verified the problem and fix on Win 10.

I am not familiar enough with C and CPython C code to be sure that fix is exactly what is needed.

to explain a little better in words what's happening in this block:

  • initially there is a buffer which is allocated, multi_line_start and line_start point at the beginning of that lines
  • at some point, the tokenizer realises it needs to allocate more and read more to fill out the buffer for a multi line token
  • this causes a reallocation of the original buffer into a new buffer, all of the pointers are copied over and adjusted for the new positions and offset -- however the mutli_line_start was not being copied and adjusted -- this patch fixes that bit

@asottile asottile force-pushed the bpo_37433 branch 2 times, most recently from d74e412 to f0f85c3 Compare July 16, 2019 15:48
@vstinner vstinner mentioned this pull request Jul 16, 2019
Closed
vstinner
vstinner approved these changes Jul 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But I would prefer that at least another core dev reviews the change.

pablogsal
pablogsal approved these changes Jul 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I checked the examples provided and tok->multi_line_start does not point anymore to random memory (the one that is there before the PyMem_REALLOC happens).

I would suggest adding more tests for this that do not involve f-strings, maybe in a different PR.

@pablogsal pablogsal self-assigned this Jul 29, 2019
@pablogsal
Copy link
Copy Markdown
Member

pablogsal commented Jul 29, 2019

@asottile Does this affect 3.7?

@pablogsal pablogsal merged commit 5b94f35 into python:master Jul 29, 2019
@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Jul 29, 2019

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

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jul 29, 2019

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 29, 2019
@asottile @miss-islington
Fix SyntaxError indicator printing too many spaces for multi-line s…
248eee7 
…trings (pythonGH-14433)

(cherry picked from commit 5b94f35)

Co-authored-by: Anthony Sottile <[email protected]>
@asottile
Copy link
Copy Markdown
Contributor Author

asottile commented Jul 29, 2019

@pablogsal nope, only 3.8+

@asottile asottile deleted the bpo_37433 branch July 29, 2019 14:00
@pablogsal
Copy link
Copy Markdown
Member

pablogsal commented Jul 29, 2019

Thank you very much @asottile for the fix :)

miss-islington added a commit that referenced this pull request Jul 29, 2019
@miss-islington @asottile
Fix SyntaxError indicator printing too many spaces for multi-line s…
cf52bd0 
…trings (GH-14433)

(cherry picked from commit 5b94f35)

Co-authored-by: Anthony Sottile <[email protected]>
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@asottile @lisroach
Fix SyntaxError indicator printing too many spaces for multi-line s…
a94dd83 
…trings (pythonGH-14433)
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@asottile @DinoV
Fix SyntaxError indicator printing too many spaces for multi-line s…
c24ad11 
…trings (pythonGH-14433)
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@asottile @websurfer5
Fix SyntaxError indicator printing too many spaces for multi-line s…
bf5115c 
…trings (pythonGH-14433)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terryjreedy terryjreedy terryjreedy left review comments

@vstinner vstinner vstinner approved these changes

@pablogsal pablogsal pablogsal approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees

@pablogsal pablogsal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@asottile @mangrisano @pablogsal @miss-islington @bedevere-bot @vstinner @terryjreedy @the-knights-who-say-ni