Skip to content

Improve AST safety parsing error message#2304

Merged
JelleZijlstra merged 9 commits intopsf:mainfrom
felix-hilden:improve-ast-parsing-errors
Jul 13, 2021
Merged

Improve AST safety parsing error message#2304
JelleZijlstra merged 9 commits intopsf:mainfrom
felix-hilden:improve-ast-parsing-errors

Conversation

@felix-hilden
Copy link
Copy Markdown
Collaborator

@felix-hilden felix-hilden commented Jun 3, 2021

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
Copy Markdown
Collaborator Author

felix-hilden commented Jun 3, 2021

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

@JelleZijlstra
Copy link
Copy Markdown
Collaborator

JelleZijlstra commented Jun 3, 2021

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

felix-hilden
felix-hilden commented Jun 3, 2021
View reviewed changes
Comment thread src/black/parsing.py Outdated
@ichard26
Copy link
Copy Markdown
Collaborator

ichard26 commented Jun 3, 2021 •
edited
Loading

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:

black/mypy.ini

Line 5 in a2b5ba2

python_version=3.6

@felix-hilden
Copy link
Copy Markdown
Collaborator Author

felix-hilden commented Jun 4, 2021

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 felix-hilden mentioned this pull request Jun 10, 2021
Closed
@felix-hilden
Copy link
Copy Markdown
Collaborator Author

felix-hilden commented Jun 13, 2021

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

ichard26
ichard26 approved these changes Jul 3, 2021
View reviewed changes
Copy link
Copy Markdown
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!

Comment thread src/black/parsing.py
Comment thread src/black/parsing.py
@JelleZijlstra JelleZijlstra merged commit 91773b8 into psf:main Jul 13, 2021
@ichard26 ichard26 mentioned this pull request Jul 13, 2021
Closed
@felix-hilden felix-hilden deleted the improve-ast-parsing-errors branch December 4, 2021 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ambv ambv ambv left review comments

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@ichard26 ichard26 ichard26 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@felix-hilden @JelleZijlstra @ichard26 @ambv @hramezani