bpo-32911: Remove the docstring attribute of AST types#7121
Uh oh!
There was an error while loading. Please reload this page.
Conversation
ncoghlan left a comment
ncoghlan left a comment There was a problem hiding this comment.
Thanks for this Serhiy. In addition to the b5 NEWS entry, the only other item I see missing here is a magic number bump for the bytecode (so any debug and opt-level 1 bytecode generated with the earlier alphas and betas gets regenerated with docstrings included again).
bedevere-bot commented May 27, 2018
When you're done making the requested changes, leave the comment: |
ncoghlan commented May 27, 2018
Note that while the previous 3.7.0a1 NEWS entry is modified by the current patch, there'll need to be a dedicated NEWS entry for the reversion that will then appear under 3.7.0b5. |
| return1; | ||
| } | ||
| intdocstring=isdocstring((stmt_ty)asdl_seq_GET(stmts, 0)); | ||
| CALL_SEQ(astfold_stmt, stmt_ty, stmts); |
There was a problem hiding this comment.
Loop from 1 seems simpler than JoinedStr hack.
for (inti=docstring; i<asdl_seq_LEN(stmts); i++){stmt_tyst= (stmt_ty)asdl_seq_GET(stmts, i); if (st!=NULL&& !astfold_stmt(st, ctx_, optimize_)){return0} }There was a problem hiding this comment.
A JoinedStr hack is needed for preventing interpreting a new constant string created by optimization as a docstring:
deffoo(): 'Not a'+' docstring'passbut we still want to apply optimizations to the first statement.
serhiy-storchaka commented May 28, 2018
Sorry, I don't know Git well still, and loss a news entry when applied Inada's patch. I didn't think that a magic number bumping is needed, because this PR affects AST. But it also affects the first line number of nodes with docstrings. I'll bump a magic number. |
ned-deily left a comment
ned-deily left a comment There was a problem hiding this comment.
A couple of wording improvements
| @@ -0,0 +1,4 @@ | |||
| Reverted :issue:`29463`. ``docstring`` field is removed from Module, ClassDef, | |||
| FunctionDef, and AsyncFunctionDef ast nodes which is added in 3.7a1. Docstring | |||
| @@ -0,0 +1,4 @@ | |||
| Reverted :issue:`29463`. ``docstring`` field is removed from Module, ClassDef, | |||
There was a problem hiding this comment.
We should say why we are doing this. Perhaps start the entry with:
"Due to unexpected compatibility issues discovered during downstream beta testing, reverted ... "
bedevere-bot commented May 28, 2018
When you're done making the requested changes, leave the comment: |
serhiy-storchaka commented May 29, 2018
I have made the requested changes; please review again. |
bedevere-bot commented May 29, 2018
Thanks for making the requested changes! @methane, @ned-deily, @ncoghlan: please review the changes made to this pull request. |
Remove the docstring attribute of AST types and restore docstring expression as a first stmt in their body. Co-authored-by: INADA Naoki <methane@users.noreply.github.com>
ncoghlan commented May 29, 2018
Thanks @serhiy-storchaka! |
Earlier development versions of Python 3.7 added the docstring field to AST nodes. This was later reverted in Python 3.7.0b5. https://bugs.python.org/issue29463python/cpython#7121
Earlier development versions of Python 3.7 added the docstring field to AST nodes. This was later reverted in Python 3.7.0b5. https://bugs.python.org/issue29463python/cpython#7121
7 participants
It is based on #5927 and goes one step further to Python 3.6. It doesn't introduce a new AST type DocString.
https://bugs.python.org/issue32911