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.

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

Improve AST safety parsing error message #2304

Merged
merged 9 commits into from Jul 13, 2021

Conversation

felix-hilden
Copy link
Collaborator

Closes #2178, alternative to #2218: This PR continues on the work of Hasan Ramezani in the alternative PR to improve the error messages when parsing formatted AST. His original commit is till at the beginning of the branch, which I then modified (Thank you for the initial push!)

A number of non-obvious details and questions:

  • As suggested by Richard in the linked issue, I removed the error messages (and a test) for Python 2 parsing
  • Previously versions 3.6 and 3.7 were parsed twice, first by the builtin ast and then by typed_ast. On Discord we determined this was an accident so I removed it.
  • The parsing was refactored a bit for clearer flow. It's now two functions: one for parsing a single version, and the wrapping function for constructing a list of versions to parse and then doing it. Probably easier to view the source than the diff on that one.
  • Mypy linting fails, even though none of the type signatures were changed. Should we ignore it or is there some trickery that could be done to make Mypy recognise the types?

Everything's in separate commits for an easier review. I'll leave this as a draft at least until the Mypy issue has been resolved.

@felix-hilden felix-hilden marked this pull request as draft June 3, 2021 20:13
@felix-hilden felix-hilden marked this pull request as ready for review June 3, 2021 20:16
@felix-hilden
Copy link
Collaborator Author

Oops. Didn't realise tests wouldn't run in a draft!

@JelleZijlstra
Copy link
Collaborator

Not sure what's going on with the mypy error. Can look in more detail in a few hours.

src/black/parsing.py Outdated Show resolved Hide resolved
@ichard26
Copy link
Collaborator

ichard26 commented Jun 3, 2021

I said this on PyDis but I'll say it here for the record and also just in case someone misses it:

@[felix] I just looked into it and the mypy error seems to be about how ast.parse only started to accept feature_version in 3.8 and higher

ast.parse(source, filename='', mode='exec', *, type_comments=False, feature_version=None)
[...]
Changed in version 3.8: Added type_comments, mode='func_type' and feature_version.

And mypy isn't smart enough to know that ast will only be used on versions where feature_version is supported :slight_smile:

The easiest solution would be just adding an assert that makes sure we're running on 3.8 in that branch. Other than that, I can't think of a solution that wouldn't require redoing and restructuring the patch which is kinda defeating the point :/

edit: and yes mypy is configured to treat the runtime version as 3.6:

python_version=3.6

@felix-hilden
Copy link
Collaborator Author

Thanks for digging around! I see now why the error happens only now. So a couple of things we could do:

  • # type: ignore, since we know that the calling code has made sure we are running on 3.8+ (tested and works)
  • Assert or redundantly test in the if sys.version_info once more (tested and works)
  • Revert the refactoring, if you're not too keen on having it (I know it's only a slight improvement)

I'll leave it up to you!

@felix-hilden
Copy link
Collaborator Author

I corrected the parsing logic now! I think this could be ready for another round of review (hopefully the last as well) 👌

Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

I don't see any issue with this and it's a nice readability improvement while implementing a nice enhancement. Thank you!

src/black/parsing.py Show resolved Hide resolved
src/black/parsing.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a more useful error when parsing fails during AST safety checks
5 participants