Reduce memory usage of Docstring struct#16183
Conversation
|
MichaReiser
left a comment
There was a problem hiding this comment.
Nice, it helps clean up the code a lot. I only have a few nit comments.
| )); | ||
|
|
||
| if string_literal.value.is_implicit_concatenated() { | ||
| let [sole_string_part] = string_literal.value.as_slice() else { |
There was a problem hiding this comment.
I found the old explicit implicit concatenated string helped with readability. I wonder if we should add a method for non-implicit concatenated strings to string_literal.
Coming up with a good name is hard: let Some(sole_string_part) = string_literal.as_non_implicit()
There was a problem hiding this comment.
Yeah, good point about readability. I will defer this to another PR, though, as I think almost every current usage of the StringLiteralValue::as_slice() method would probably switch to using this new API -- and I agree that what we would call it isn't necessarily obvious. It seems worthy of considering as its own thing.
For now I'll just add a comment here.
| TextRange::new( | ||
| self.docstring.start() + self.docstring.flags().opener_len(), | ||
| self.docstring.end() - self.docstring.flags().closer_len(), | ||
| ) |
There was a problem hiding this comment.
Nit: It would be nice if we could avoid copying this computation. Consider adding a content_range to StringLiteral similar to
ruff/crates/ruff_python_ast/src/expression.rs
Lines 221 to 228 in e178148
There was a problem hiding this comment.
Hehe. I was planning this for a followup, since there are a number of other places where this method would be useful ;) But I'll add it in this PR, and switch other places in the codebase to this new method as the followup!
crates/ruff_linter/src/rules/pydocstyle/rules/blank_before_after_class.rs
Outdated
Show resolved
Hide resolved
459dbd3 to
ef91991
Compare
ef91991 to
fe17ea6
Compare
## Summary This PR makes the following changes: - It adjusts various callsites to use the new `ast::StringLiteral::contents_range()` method that was introduced in #16183. This is less verbose and more type-safe than using the `ast::str::raw_contents()` helper function. - It adds a new `ast::ExprStringLiteral::as_unconcatenated_literal()` helper method, and adjusts various callsites to use it. This addresses @MichaReiser's review comment at #16183 (comment). There is no functional change here, but it helps readability to make it clearer that we're differentiating between implicitly concatenated strings and unconcatenated strings at various points. - It renames the `StringLiteralValue::flags()` method to `StringLiteralFlags::first_literal_flags()`. If you're dealing with an implicitly concatenated string `string_node`, `string_node.value.flags().closer_len()` could give an incorrect result; this renaming makes it clearer that the `StringLiteralFlags` instance returned by the method is only guaranteed to give accurate information for the first `StringLiteral` contained in the `ExprStringLiteral` node. - It deletes the unused `BytesLiteralValue::flags()` method. This seems prone to misuse in the same way as `StringLiteralValue::flags()`: if it's an implicitly concatenated bytestring, the `BytesLiteralFlags` instance returned by the method would only give accurate information for the first `BytesLiteral` in the bytestring. ## Test Plan `cargo test`
Summary
I noticed that our
Docstringstruct, that we use for querying information on docstrings in various linter rules, eagerly stores a lot of information in the struct that could probably be lazily computed in methods on the struct. This PR reworks the API to store less information on the struct itself, reducing memory usage.In addition to this, it changes the struct to store an
ast::StringLiteralnode rather than anast::ExprStringLiteralnode. This:Docstringdefinitionast::StringLiteral::flagsattribute to query information directly from the underlying AST node, removing the need for quite a few.unwrap()calls (accompanied by// SAFETYcomments) across our docstring-related rulesTest Plan
cargo test