Doc strings no longer stored in body of AST

@brettcannon

BPO	32911
Nosy	@brettcannon, @birkenfeld, @rhettinger, @ncoghlan, @benjaminp, @ned-deily, @methane, @ambv, @markshannon, @serhiy-storchaka, @grimreaper, @minrk, @Carreau
PRs	bpo-32911: Add new AST node for docstring #5927 bpo-32911: Remove the docstring attribute of AST types #7121 [3.8] bpo-32911: Revert bpo-29463. (GH-7121) #7197 [3.7] bpo-32911: Update the historical note about the magic number. #7272 [3.8] bpo-32911: Add the historical note about the magic number. #7273

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-05-30.12:27:57.966>
created_at = <Date 2018-02-22.12:20:07.907>
labels = ['interpreter-core', 'type-bug', '3.7', 'release-blocker']
title = 'Doc strings no longer stored in body of AST'
updated_at = <Date 2018-05-31.06:12:17.993>
user = 'https://github.com/markshannon'

bugs.python.org fields:

activity = <Date 2018-05-31.06:12:17.993>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2018-05-30.12:27:57.966>
closer = 'ncoghlan'
components = ['Interpreter Core']
creation = <Date 2018-02-22.12:20:07.907>
creator = 'Mark.Shannon'
dependencies = []
files = []
hgrepos = []
issue_num = 32911
keywords = ['patch', '3.7regression']
message_count = 55.0
messages = ['312557', '312558', '312559', '312561', '312562', '312564', '312565', '312567', '312580', '312588', '312625', '312629', '312630', '312856', '312989', '312999', '313000', '313001', '313002', '313005', '313315', '313698', '313997', '314056', '315238', '315240', '315581', '315582', '317450', '317491', '317492', '317493', '317654', '317655', '317657', '317665', '317671', '317683', '317686', '317696', '317716', '317718', '317777', '317782', '317783', '317784', '317785', '317988', '317992', '318089', '318147', '318150', '318176', '318258', '318259']
nosy_count = 15.0
nosy_names = ['brett.cannon', 'georg.brandl', 'rhettinger', 'ncoghlan', 'benjamin.peterson', 'ned.deily', 'methane', 'lukasz.langa', 'Mark.Shannon', 'serhiy.storchaka', 'eitan.adler', 'minrk', 'mbussonn', 'flherne', 'Benjamin Ragan-Kelley']
pr_nums = ['5927', '7121', '7197', '7272', '7273']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32911'
versions = ['Python 3.7']

Python 3.7.0b1+ (heads/3.7:dfa1144, Feb 22 2018, 12:10:59)

>>> m = ast.parse("def foo():\n   'help'")
>>> m.body[0].body
[]

Correct behaviour (3.6 or earlier)

>>> m = ast.parse("def foo():\n   'help'")
>>> m.body[0].body
[<_ast.Expr object at 0x7fb9eeb1d4e0>]

AST is changed slightly from Python 3.7.
ast.get_docstring() works for both of 3.6 and 3.7.

$ ./python
Python 3.8.0a0 (heads/master-dirty:451d1edaf4, Feb 22 2018, 21:11:54)
[GCC 7.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import ast
>>> m = ast.parse("def foo():\n   'help'")
>>> ast.get_docstring(m.body[0])
'help'

This is an unnecessary and breaking change.
Changes like this should not be made unless necessary to fix a bug.

ref: https://bugs.python.org/issue29463

That issue has to do with bytecode generation, not the AST.

The AST should be an accurate representation of the Python source code.
Making a better representation of the source would be fine, but this makes it worse.

Doc-strings may be semantically distinct from other expressions, but they are syntactically the same.

We implemented AST-level constant folding (bpo-29469) based on the AST change (bpo-29463).

I feel it's possible to revert AST change without reverting AST constant folding:

Docstring is exposed from AST before calling AST optimizer.
AST optimizer removes unused constant.

But I don't remember how difficult it is because since this change is made one year ago (GH-46)...

This isn't a bug, but a feature. You no longer need to check and skip the first statement if it is a literal string. The body attribute now always represents a sequence of statements, and the docstring attribute represents a docstring.

Stating that "this is a feature not a bug" does not make it so.
This breaks existing code and reduces the capabilities of the ast module.
For example, how does one get the location of the docstring now?

From a syntactic point of view.
def foo():
"help"
and
def foo():
b'"help"
barely differ.
The S in AST stands for Syntax not Semantics.

It seems we have a difference of opinion here. Serhiy closed this issue so, Mark, if you feel strongly enough to pursue it, you should reopen it and solicit other opinions. The clock has just about run out to change the now current behavior for 3.7.0.

Just noting that I'm going to attempt an alternative, hopefully lower impact, approach at implementing the reversion, which will be to modify Parser/asdl_c.py to inject the following constructor postamble for ASDL nodes that define both a "body" and a "docstring" field:

If docstring is set, also insert it as the first entry in the node's body sequence (this is enough to get AST compilation back to behaving the way it did in 3.6, since the docstrings will be reinserted as body[0] even after the AST compilation process took them out)
If docstring is not set, check if the first entry is usable as a docstring, and if so, also set it as the docstring (this would restore compatibility with node manipulation code that expects to be able to pass a docstring as body[0])

In 3.8, we'd then decide how to eliminate the duplication of information between the two locations, but do so armed with the ability to emit deprecation warnings for code which isn't setting the node docstring attribute explicitly.

After looking at potential implementation approaches, I believe the following would be the ASDL code generator changes needed to present the docstring as part of the body in the Python API, while keeping it as a separate attribute in the C API:

in ast2obj, add code in the converter postamble to inject the docstring as body[0]
in obj2ast, add code in the converter preamble to pop and discard body[0] when docstring is not None
in ast_type_init, add code to either inject the docstring as body[0] (if the docstring is set), or else to extract and set the docstring based on body[0]

Since CPython's own AST optimizer works at the C API layer, we then wouldn't need to revert any of the changes that relied on the separation of the docstring out to its own attribute.

At the Python level, some differences would still be visible though:

there'd be a node for the docstring in the AST, but the value would still be optimised out of the resulting code object
the first line number of code objects would still change to be that of the first non-docstring line
the synthesised docstring node would always claim to be the line before the first non-docstring line (since escape characters mean we can't just count line feeds in the docstring)

Given the complexity of the required changes to the ASDL code generator, and the Python level API differences that would still remain, I think Serhiy's full reversion patch is going to be the more reliable option at this late stage of the release process.

While it isn't nice for the folks that already adapted their code to cope with the earlier beta releases, it's the lowest risk approach that lets us get 3.7.0 out the door without unintended consequences, while allowing the issue to be revisited for 3.8 with greater awareness of the potential backwards compatibility and code migration issues.

I'm +1 on PR-7121 too.

OK, since I believe everyone who has spoken up so far has chosen B or a variation on it, I think we can eliminate option A. And there also seems to be a consensus so far among the core developers who have spoken up for the approach in PR 7121. Before we commit to it and produce 3.7.0b5, I really would like to hear from at least one of the downsteamers that this approach seems OK for 3.7.0. mbussonn? MIN RK?

Thank you all for your (prompt) help so far with this!

That should work well for us. Our patches for this are all conditional on the module body being empty, so reverting causes us no issues at all.

Thank you!

New changeset 2641ee5 by Serhiy Storchaka in branch '3.7':
bpo-32911: Revert bpo-29463. (GH-7121)
2641ee5

New changeset 73cbe7a by Serhiy Storchaka in branch 'master':
bpo-32911: Revert bpo-29463. (GH-7121) (GH-7197)
73cbe7a

Thank you all for your input and thank you, Inada-san and Serhiy, for the PRs. Is there anything more that needs to go in for 3.7.0b5 tomorrow so that this can be tested before the release candidate? If not, can we now close this again? If new issues arise with b5 or later, we should probably track with another issue.

I think it's OK.

I concur.

In that case, closing this as resolved. Folks interested in the idea of a "suite" compilation mode to compile a series of statements without special-casing the first one as a potential docstring, as well as the idea of a dedicated DocString AST node may want to open new RFEs for 3.8 :)

New changeset 941ec21 by Serhiy Storchaka in branch 'master':
bpo-32911: Add the historical note about the magic number. (GH-7273)
941ec21

New changeset d6bbb57 by Serhiy Storchaka in branch '3.7':
bpo-32911: Update the historical note about the magic number. (GH-7272)
d6bbb57

Navigation Menu

Repository navigation

Doc strings no longer stored in body of AST #77092

42 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Doc strings no longer stored in body of AST #77092

Description

Activity

markshannon commented on Feb 22, 2018

methane commented on Feb 22, 2018

markshannon commented on Feb 22, 2018

methane commented on Feb 22, 2018

markshannon commented on Feb 22, 2018

methane commented on Feb 22, 2018

serhiy-storchaka commented on Feb 22, 2018

markshannon commented on Feb 22, 2018

ned-deily commented on Feb 22, 2018

42 remaining items

ncoghlan commented on May 27, 2018

ncoghlan commented on May 27, 2018

methane commented on May 27, 2018

ned-deily commented on May 27, 2018

BenjaminRagan-Kelley commented on May 27, 2018

serhiy-storchaka commented on May 29, 2018

serhiy-storchaka commented on May 29, 2018

ned-deily commented on May 29, 2018

methane commented on May 30, 2018

serhiy-storchaka commented on May 30, 2018

ncoghlan commented on May 30, 2018

serhiy-storchaka commented on May 31, 2018

serhiy-storchaka commented on May 31, 2018

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions