Explicit `as_str` (no deref), add no allocation methods #8826

dhruvmanila · 2023-11-23T14:47:36Z

Summary

This PR is a follow-up to the AST refactor which does the following:

Remove Deref implementation on StringLiteralValue and use explicit as_str calls instead. The Deref implementation would implicitly perform allocations in case of implicitly concatenated strings. This is to make sure the allocation is explicit.
Now, certain methods can be implemented to do zero allocations which have been implemented in this PR. They are:
- is_empty
- len
- chars
- Custom PartialEq implementation to compare each character

Test Plan

Run the linter test suite and make sure all tests pass.

This commit implements the AST design to account for implicit concatenation in string nodes, specifically the `ExprFString`, `ExprStringLiteral`, and `ExprBytesLiteral` nodes.

This commit adds the new variants for the string parts to `AnyNode` and `AnyNodeRef` enums. These parts are `StringLiteral` (part of `ExprStringLiteral`), `BytesLiteral` (part of `ExprBytesLiteral`), and `FString` (part of `ExprFString`). The reason for this is to add visitor methods for these parts. This is done in the following commit. So, the visitor would visit the string as a whole first and then visit each part. ``` ExprStringLiteral - "foo" "bar" |- StringLiteral - "foo" |- StringLiteral - "bar" ``` The above tree helps understand the way visitor would work.

The visitor implementations are updated to visit each part nodes for the respective string nodes. The following example better highlights this: ``` ExprStringLiteral - "foo" "bar" |- StringLiteral - "foo" |- StringLiteral - "bar" ``` The `visit_expr` method would be use to visit the `ExprStringLiteral` while the `visit_string_literal` method would be use for the `StringLiteral` node. Similar methods are added for bytes and f-strings.

The generator is basically improved. Earlier, for an implicitly concatenated string we would produce the joined form. So, ```python "foo" "bar" "baz" ``` For the above example, the generator would give us: ```python "foobarbaz" ``` Now, as we have the information for each part, we will be producing the exact code back.

`Expr` is a general type for all expressions while `LiteralExpressionRef` is a type which includes only the literal expressions. The method is suited more for this type instead. This will also help in the formatter change.

As highlighted in the review: > If you have two `ConcatenatedStringLiteral` values where both have > equivalent values for `strings` but where one has `value` initialized > and the other does not, would you expect them to compare equal? > Semantically I think I would, since the alternative is that equality is > dependent on whether `as_str()` has been called, which seems incidental. #7927 (comment)

dhruvmanila · 2023-11-23T14:47:47Z

Current dependencies on/for this PR:

main
- PR Remove leftover constant tuple reference #8062
  - PR New Singleton enum for PatternMatchSingleton node #8063
    - PR Split Constant to individual literal nodes #8064
      - PR Update string nodes for implicit concatenation #7927
        
        PR Explicit as_str (no deref), add no allocation methods #8826 👈
        
        PR Remove #[allow(unused_variables)] from visitor methods #8828
        
        PR New AST nodes for f-string elements #8835
        
        PR Introduce StringLike enum #9016

This stack of pull requests is managed by Graphite.

github-actions · 2023-11-23T15:07:15Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

MichaReiser · 2023-11-28T02:35:47Z

What has been the main motivation for the change (just asking)? I assume it's for better performance but codespeed isn't loading for me right now.

Could we keep the Deref implementation in addition to as_str for case where the Deref lifetimes work? I'm asking because some of the code seems more complicated now, then when using Deref.

dhruvmanila · 2023-11-28T14:48:18Z

What has been the main motivation for the change (just asking)? I assume it's for better performance but codespeed isn't loading for me right now.

These methods are just some low hanging fruits which are simple to implement to work on each character instead of (probably) allocating a String. That said, I haven't seen any regression or improvements.

Could we keep the Deref implementation in addition to as_str for case where the Deref lifetimes work? I'm asking because some of the code seems more complicated now, then when using Deref.

Can you provide an example where you think the code is complicated? That would help me understand your perspective.

MichaReiser · 2023-11-28T23:53:33Z

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

                            if checker.enabled(Rule::StaticJoinToFString) {
-                                flynt::rules::static_join_to_fstring(checker, expr, string);
+                                flynt::rules::static_join_to_fstring(
+                                    checker,
+                                    expr,
+                                    string_value.as_str(),
+                                );
                            }


I think the previous code was easier to read than now where we have the explicit as_str call and using Deref seemed to work just fine?

The motivation here is that the Deref implementation includes an allocation -- the conversion here can allocate if the string is implicitly concatenated. So Deref is essentially hiding an allocation. I prefer that we do something explicit over an implicit allocation. It's not about the lifetimes or anything like that. It's intentionally more verbose.

I think we should then call the method to_str rather than as_str according to https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv Because I assume that a function called as to be free (or extremely cheap), whereas to might be cheap, but it depends, and into_ consumes self

Yeah, I think using to_str makes more sense. I was not aware of that. I'll create a follow-up PR to rename that.

MichaReiser · 2023-11-28T23:54:18Z

crates/ruff_linter/src/checkers/ast/mod.rs

                self.deferred.string_type_definitions.push((
                    expr.range(),
-                    value,
+                    value.as_str(),


Same for all these changes. If we could keep the Deref implementation in addition to the explicit as_str method, then code that doesn't need the more relaxed lifetime could be left as is.

dhruvmanila added 13 commits November 22, 2023 09:51

Update existing string nodes for implicit concatenation

86d799d

This commit implements the AST design to account for implicit concatenation in string nodes, specifically the `ExprFString`, `ExprStringLiteral`, and `ExprBytesLiteral` nodes.

Update parser for the new AST design

223380f

Update parser snapshots

21ed36c

Update ComparableExpr to account for each string parts

6be8eca

Update the AST helper methods

1c6dc52

misc: use Self consistently

6cdf149

Move is_implicit_concatenated to a narrow type

95f71da

`Expr` is a general type for all expressions while `LiteralExpressionRef` is a type which includes only the literal expressions. The method is suited more for this type instead. This will also help in the formatter change.

Update the linter code for the new AST design

6d6e1c7

Update the formatter code for the new AST design

bcabcca

This was referenced Nov 23, 2023

Remove leftover constant tuple reference #8062

Merged

New Singleton enum for PatternMatchSingleton node #8063

Merged

Split Constant to individual literal nodes #8064

Merged

Update string nodes for implicit concatenation #7927

Merged

dhruvmanila added 2 commits November 23, 2023 08:51

misc: use correct names for visitor arguments

1e72af2

Explicit as_str (no deref), add no allocation methods

8b261bc

dhruvmanila force-pushed the dhruv/custom-methods branch from 3b36f7f to 8b261bc Compare November 23, 2023 14:51

dhruvmanila mentioned this pull request Nov 23, 2023

Remove #[allow(unused_variables)] from visitor methods #8828

Merged

dhruvmanila added the internal An internal refactor or improvement label Nov 23, 2023

dhruvmanila requested a review from charliermarsh November 23, 2023 14:56

charliermarsh approved these changes Nov 24, 2023

View reviewed changes

dhruvmanila mentioned this pull request Nov 24, 2023

New AST nodes for f-string elements #8835

Merged

Base automatically changed from dhruv/implicit-str-concat-node to main November 24, 2023 23:55

Merge branch 'main' into dhruv/custom-methods

2155ecb

dhruvmanila enabled auto-merge (squash) November 24, 2023 23:58

dhruvmanila merged commit 626b057 into main Nov 25, 2023

dhruvmanila deleted the dhruv/custom-methods branch November 25, 2023 00:04

MichaReiser reviewed Nov 28, 2023

View reviewed changes

dhruvmanila mentioned this pull request Dec 5, 2023

Introduce StringLike enum #9016

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Explicit `as_str` (no deref), add no allocation methods #8826

Explicit `as_str` (no deref), add no allocation methods #8826

Uh oh!

dhruvmanila commented Nov 23, 2023

Uh oh!

dhruvmanila commented Nov 23, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Nov 23, 2023 •

edited

Loading

Uh oh!

MichaReiser commented Nov 28, 2023 •

edited

Loading

Uh oh!

dhruvmanila commented Nov 28, 2023

Uh oh!

MichaReiser Nov 28, 2023

Uh oh!

charliermarsh Nov 28, 2023

Uh oh!

MichaReiser Nov 29, 2023 •

edited

Loading

Uh oh!

dhruvmanila Nov 29, 2023

Uh oh!

MichaReiser Nov 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Explicit as_str (no deref), add no allocation methods #8826

Explicit as_str (no deref), add no allocation methods #8826

Uh oh!

Conversation

dhruvmanila commented Nov 23, 2023

Summary

Test Plan

Uh oh!

dhruvmanila commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

MichaReiser commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhruvmanila commented Nov 28, 2023

Uh oh!

MichaReiser Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

charliermarsh Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvmanila Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

MichaReiser Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Explicit `as_str` (no deref), add no allocation methods #8826

Explicit `as_str` (no deref), add no allocation methods #8826

dhruvmanila commented Nov 23, 2023 •

edited

Loading

github-actions bot commented Nov 23, 2023 •

edited

Loading

`ruff-ecosystem` results

MichaReiser commented Nov 28, 2023 •

edited

Loading

MichaReiser Nov 29, 2023 •

edited

Loading