Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Inconsistent ast.parse result for python<3.9 and PEP614 #96427

Closed
sobolevn opened this issue Aug 30, 2022 · 5 comments
Closed

Inconsistent ast.parse result for python<3.9 and PEP614 #96427

sobolevn opened this issue Aug 30, 2022 · 5 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@sobolevn
Copy link
Member

PEP614 introduces relaxed decorator grammar in 3.9: https://peps.python.org/pep-0614/

Right now, ast.parse with feature_version=(3, 8) and bellow behave differently from specified python versions themself.

For example, main branch is able to parse this code:

Python 3.12.0a0 (heads/main-dirty:e860e521ec, Aug 28 2022, 21:36:18) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import ast
>>> code = """
... @decs[0].func
... def some(): pass
... """
>>> ast.parse(code)
<ast.Module object at 0x102db8150>
>>> ast.parse(code, feature_version=(3, 8))
<ast.Module object at 0x102d9d320>

While python==3.8.x is not able to:

Python 3.8.9 (default, May  3 2021, 12:15:25) 
[Clang 11.0.0 (clang-1100.0.33.16)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import ast
>>> code = """
... @decs[0].func
... def some(): pass
... """
>>> ast.parse(code)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/sobolev/.pyenv/versions/3.8.9/lib/python3.8/ast.py", line 47, in parse
    return compile(source, filename, mode, flags,
  File "<unknown>", line 2
    @decs[0].func
         ^
SyntaxError: invalid syntax

When looking at we-love-parsers/pegen - it does have a version check for this: https://github.com/we-like-parsers/pegen/blame/599255a5ba3930e86367cd3fc78fae91ee184dd5/data/python.gram#L660-L662

Pros and Cons

I think that there are several key points to this, pros:

  1. Correct parsing of older versions
  2. Consistency with other parser like we-love-parsers/pegen

However, there are some serious cons to it:

  1. Grammar is made quite significanly more complex
  2. There might be new bugs in new behaviour: locations / performance / etc

I think it is up to @pablogsal and other core devs to decide.

Proposed solution

I've modified an existing we-love-parsers/pegen's solution a bit, here's what it looks like:

decorators[asdl_expr_seq*]: a[asdl_expr_seq*]=decorator+
decorator[expr_ty]:
    | a=('@' f=dec_maybe_call NEWLINE { f }) { a }
    | a=('@' f=named_expression NEWLINE { f }) {
        CHECK_VERSION(expr_ty, 9, "Relaxed decorator syntax is", a) }
dec_maybe_call[expr_ty]:
    | a=dec_primary '(' b=[arguments] ')' {
        _PyAST_Call(a,
                    (b) ? ((expr_ty) b)->v.Call.args : NULL,
                    (b) ? ((expr_ty) b)->v.Call.keywords : NULL,
                    EXTRA) }
    | dec_primary
dec_primary[expr_ty]:
    | a=dec_primary '.' b=NAME { _PyAST_Attribute(a, b->v.Name.id, Load, EXTRA) }
    | a=NAME

I will send a PR with all the changes / tests / auto-generated files, so we can take a closer look and decide whether we want this or not.
@sobolevn sobolevn added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 30, 2022
sobolevn added a commit to sobolevn/cpython that referenced this issue Aug 30, 2022
@sobolevn
pythongh-96427: Raise SyntaxError for PEP614 on older python vers…
2e6ca88 
…ions
@bedevere-bot bedevere-bot mentioned this issue Aug 30, 2022
Closed
@pablogsal
Copy link
Member

I fear that this is too much complexity unfortunately. Unless @isidentical or @lysnikolaou have string opinions about this, I prefer to mark is as won't fix.

This whole idea of rejecting new constructs on compatibility mode is quite a lot of burden for a use case that's not clear (at least to me) and was not really discussed a lot. I don't think this is even fully documented...

In any case, thanks for the issue and the work @sobolevn and sorry that we need to reject it :(

@sobolevn
Copy link
Member Author

I fear that this is too much complexity unfortunately.

No worries! I felt almost the same.
At least I had a chance to work with all the parser tooling :)

Let's wait for others to comment.

@lysnikolaou
Copy link
Member

No strong opinion on my side. Plus I agree with @pablogsal. Should we maybe do a bit more research as to how feature_version is used?

@sobolevn
Copy link
Member Author

Should we maybe do a bit more research as to how feature_version is used?

I would be glad to. First, I will start with searching for real-life use-cases on github.

@sobolevn
Copy link
Member Author

sobolevn commented Aug 30, 2022 •
edited
Loading

Mypy

My main use-case is mypy parsing of python files.
You can specify target version of python you want to check. For example, you can check python3.8 while actually running python3.10

It is possible via mypy yourfile.py --python-version=3.8
Here's the source code we use in mypy: https://github.com/python/mypy/blob/master/mypy/fastparse.py#L275-L303

So, the initial problem was:

  1. I have a file that is written with relaxed decorator syntax
  2. I run mypy with --python-version=3.8 (target version for my project), ideally it should find all problems with my source code. However, it passes and produces a false-negative
  3. In runtime executing python yourfile.py fails with syntax error

black

It is also used by black for some similar stuff: https://github.com/psf/black/blob/44d5da00b520a05cd56e58b3998660f64ea59ebd/src/black/parsing.py#L149-L169

    if sys.version_info >= (3, 8) and version >= (3,):
        return ast.parse(src, filename, feature_version=version, type_comments=True)

So, I think we can conlcude that this is a very specific feature for static analysis tools.

Other projects

It is also used by 100+ other projects (including some forks). https://cs.github.com/?q=feature_version%3D%20language%3APython&scopeName=All%20repos&scope=#

@sobolevn sobolevn mentioned this issue Sep 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sobolevn @pablogsal @lysnikolaou