bpo-32911: Remove the docstring attribute of AST types by serhiy-storchaka · Pull Request #7121 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
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.
| return 1; |
|---|
| } |
| int docstring = isdocstring((stmt_ty)asdl_seq_GET(stmts, 0)); |
| CALL_SEQ(astfold_stmt, stmt_ty, stmts); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop from 1 seems simpler than JoinedStr hack.
for (int i = docstring; i < asdl_seq_LEN(stmts); i++) {
stmt_ty st = (stmt_ty)asdl_seq_GET(stmts, i);
if (st != NULL && !astfold_stmt(st, ctx_, optimize_)) {
return 0;
}
}Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A JoinedStr hack is needed for preventing interpreting a new constant string created by optimization as a docstring:
def foo(): 'Not a' + ' docstring' pass
but we still want to apply optimizations to the first statement.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"which was added"
| @@ -0,0 +1,4 @@ |
|---|
| Reverted :issue:`29463`. ``docstring`` field is removed from Module, ClassDef, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should say why we are doing this. Perhaps start the entry with:
"Due to unexpected compatibility issues discovered during downstream beta testing, reverted ... "
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
I have made the requested changes; please review again.
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced 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
serhiy-storchaka added a commit that referenced 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
Kodiologist added a commit to Kodiologist/hy that referenced this pull request
carlwgeorge added a commit to carlwgeorge/jedi that referenced this pull request
davidhalter pushed a commit to davidhalter/jedi that referenced this pull request
