Add a method to Checker for cached parsing of stringified type annotations#13158
Add a method to Checker for cached parsing of stringified type annotations#13158AlexWaygood merged 6 commits intomainfrom
Checker for cached parsing of stringified type annotations#13158Conversation
|
This doesn't seem to have any impact on the Codspeed benchmarks one way or another as a standalone PR (but I don't know how many stringified annotations we have in those benchmarks?) |
|
8ff61ad to
3a3b810
Compare
I don't think we have any benchmark containing stringified annotations. You could try to run some hyperfine benchmarks over a project that does use stringified annotations. |
MichaReiser
left a comment
There was a problem hiding this comment.
Nice. Looks good to me.
The only part that's unclear to me is why we need to reset the cache. It would be nice if it could be avoided. If not, then it would help to extend the comment so that it explains what data gets invalidated.
|
This is more of a general comment, since I thought about adding something similar for my ongoing work on flake8-type-checking. I am not sure to what degree this would be viable, but why not just eagerly parse string annotations, store them all on the checker as a struct that contains both the string and the parsed expression add that struct to the deferred string annotations vector instead of just the string and then visit the expression where we currently parse the annotation in the checker. That should preserve the same semantics for bindings and references. I realize there are some corner cases with nested strings like |
|
I thought about that too because we always end up parsing all type annotations. My conclusion was that doing it inside of |
…t from `visit_deferred_string_type_definitions`
c4d222e to
6fa7a5b
Compare
Summary
This PR adds a method to
ruff_linter::checkers::ast::Checkerfor cached parsing of stringified type annotations. It was decided in review of #12951 that this would be desirable, sinceruff_python_parser::typing::parse_type_annotationcan be quite expensive. However, since we already have a number of linter rules that callruff_python_parser::typing::parse_type_annotation, it seemed to me like this would make sense as a standalone PR. (Adding caching was also more complicated than I expected, so separating this into its own PR should make life easier for reviewers.) If this is accepted, I'll rebase #12951 on top of it.The PR should be easiest to review commit-by-commit. Each commit on its own passes the entire test suite. The first two commits do some refactoring to lay the groundwork for adding the new method. The third commit adds the new method and makes use of it in
crates/ruff_linter/src/checkers/ast/mod.rs; the final commit makes use of the new method in various linter rules that currently useruff_python_parser::typing::parse_type_annotation.Test Plan
cargo test