refactor: Use `QualifiedName` for `Imported::call_path` #10214

MichaReiser · 2024-03-03T18:56:38Z

Summary

When you try to remove an internal representation leaking into another type and end up rewriting a simple version of smallvec.

The goal of this PR is to replace the Box<[&'a str]> with Box<QualifiedName> to avoid that the internal QualifiedName representation leaks (and it gives us a nicer API too). However, doing this when QualifiedName uses SmallVec internally gives us all sort of funny lifetime errors. I was lost but @BurntSushi came to rescue me. He figured out that smallvec has a variance problem which is already tracked in servo/rust-smallvec#146

To fix the variants problem, I could use the smallvec-2-alpha-4 or implement our own smallvec. I went with implementing our own small vec for this specific problem. It obviously isn't as sophisticated as smallvec (only uses safe code), e.g. it doesn't perform any size optimizations, but it does its job.

Other changes:

Removed Imported::qualified_name (the version that returns a String). This can be replaced by calling ToString on the qualified name.
Renamed Imported::call_path to qualified_name and changed its return type to &QualifiedName.
Renamed QualifiedName::imported to user_defined which is the more common term when talking about builtins vs the rest/user defined functions.

Test plan

cargo test

codspeed-hq · 2024-03-04T20:28:09Z

CodSpeed Performance Report

Merging #10214 will not alter performance

_{Comparing qualified-name-lifetimes (9b04828) with main (d441338)}

Summary

✅ 30 untouched benchmarks

github-actions · 2024-03-04T20:41:53Z

`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 · 2024-03-05T08:46:37Z

crates/ruff_python_ast/src/name.rs

-        Self {
-            segments: iter.into_iter().collect(),
+impl Display for QualifiedName<'_> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {


Inlined from format_qualified_name_segments. We no longer need format_qualified_name_segments because the code that used Box<[&'astr]> can now call into this display implementation.

MichaReiser · 2024-03-05T08:48:58Z

crates/ruff_python_ast/src/name.rs

-                w.write_char('.')?;
+impl<'a> UnqualifiedName<'a> {
+    /// Convert an `Expr` to its [`UnqualifiedName`] (like `["typing", "List"]`).
+    pub fn from_expr(expr: &'a Expr) -> Option<Self> {


Inlined from collect_segments (see separate commit)

MichaReiser · 2024-03-05T08:49:43Z

crates/ruff_python_ast/src/name.rs

+        let mut segments = Vec::with_capacity(SMALL_LEN * 2);
+
+        let mut current = &*attr8.value;
+
+        loop {
+            current = match current {
+                Expr::Attribute(attr) => {
+                    segments.push(attr.attr.as_str());
+                    &*attr.value
+                }
+                Expr::Name(nodes::ExprName { id, .. }) => {
+                    segments.push(id.as_str());
+                    break;
+                }
+                _ => break,
            }


I rewrote this to no longer require recursion (which creates new SegmentsVecs internally, which seems unnecessary when we know that we need a Vec anyway).

MichaReiser · 2024-03-05T09:08:22Z

crates/ruff_python_semantic/src/binding.rs

-    fn call_path(&self) -> &[&'a str] {
-        self.qualified_name.as_ref()
+    fn qualified_name(&self) -> &QualifiedName<'a> {
+        &self.qualified_name
    }

    /// For example, given `import foo.bar`, returns `["foo"]`.
    fn module_name(&self) -> &[&'a str] {


It would be nice if we could return QualifiedNameRef or similar from here because the module name is a qualified name too. But I consider this out of the scope of this PR.

BurntSushi

w00t! LGTM!

BurntSushi · 2024-03-05T12:28:55Z

crates/ruff_python_ast/src/name.rs

+struct SegmentsStack<'a> {
+    segments: [&'a str; SMALL_LEN],
+    len: usize,
+}


It's possible arrayvec could simplify things a bit here, although it may have the same issue as smallvec.

Unfortunately, the arrayvec crate doesn't support the most complex operation, extending from an Iterator. That's why I think its use is limited. The main advantage I see is that it avoids initializing unused memory.

I gave it a quick try. Funny enough, it has a variance issue, but it manifests differently than smallvec

BurntSushi · 2024-03-05T12:29:18Z

crates/ruff_python_ast/src/name.rs

+            heap.as_slice(),
+            &["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l"]
+        );
+    }


Nice tests. :)

@BurntSushi

) ## Summary When you try to remove an internal representation leaking into another type and end up rewriting a simple version of `smallvec`. The goal of this PR is to replace the `Box<[&'a str]>` with `Box<QualifiedName>` to avoid that the internal `QualifiedName` representation leaks (and it gives us a nicer API too). However, doing this when `QualifiedName` uses `SmallVec` internally gives us all sort of funny lifetime errors. I was lost but @BurntSushi came to rescue me. He figured out that `smallvec` has a variance problem which is already tracked in servo/rust-smallvec#146 To fix the variants problem, I could use the smallvec-2-alpha-4 or implement our own smallvec. I went with implementing our own small vec for this specific problem. It obviously isn't as sophisticated as smallvec (only uses safe code), e.g. it doesn't perform any size optimizations, but it does its job. Other changes: * Removed `Imported::qualified_name` (the version that returns a `String`). This can be replaced by calling `ToString` on the qualified name. * Renamed `Imported::call_path` to `qualified_name` and changed its return type to `&QualifiedName`. * Renamed `QualifiedName::imported` to `user_defined` which is the more common term when talking about builtins vs the rest/user defined functions. ## Test plan `cargo test`

MichaReiser changed the base branch from main to rename-call-path March 3, 2024 18:56

MichaReiser force-pushed the rename-call-path branch from bcb457d to 71dd02a Compare March 4, 2024 08:59

Base automatically changed from rename-call-path to main March 4, 2024 09:06

MichaReiser force-pushed the qualified-name-lifetimes branch 3 times, most recently from bd43f3d to 293adf4 Compare March 4, 2024 20:22

MichaReiser added the internal An internal refactor or improvement label Mar 4, 2024

MichaReiser force-pushed the qualified-name-lifetimes branch from 293adf4 to ab84658 Compare March 4, 2024 20:22

MichaReiser force-pushed the qualified-name-lifetimes branch 3 times, most recently from 89a3488 to 41b8fd2 Compare March 4, 2024 22:22

MichaReiser changed the title ~~qualified name lifetimes~~ refactor: Use QualifiedName for Imported::call_path Mar 4, 2024

MichaReiser force-pushed the qualified-name-lifetimes branch from 41b8fd2 to a852379 Compare March 4, 2024 22:35

MichaReiser added 3 commits March 5, 2024 08:08

QualifiedName lifetime issue

6a25dfa

Use own SmallVec for QualifiedName to work-around lifetime issue

29bd49f

Inline collect_segments and make it non-recursive

81d8996

MichaReiser force-pushed the qualified-name-lifetimes branch 2 times, most recently from ee0c9d8 to 87260fb Compare March 5, 2024 09:00

Cleanup, add tests

9b04828

MichaReiser force-pushed the qualified-name-lifetimes branch from 87260fb to 9b04828 Compare March 5, 2024 09:00

MichaReiser requested review from BurntSushi and charliermarsh March 5, 2024 09:07

MichaReiser commented Mar 5, 2024

View reviewed changes

MichaReiser marked this pull request as ready for review March 5, 2024 09:08

MichaReiser requested a review from AlexWaygood as a code owner March 5, 2024 09:08

BurntSushi approved these changes Mar 5, 2024

View reviewed changes

MichaReiser merged commit 8ea5b08 into main Mar 6, 2024
17 checks passed

MichaReiser deleted the qualified-name-lifetimes branch March 6, 2024 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Use `QualifiedName` for `Imported::call_path` #10214

refactor: Use `QualifiedName` for `Imported::call_path` #10214

MichaReiser commented Mar 3, 2024 •

edited

Loading

codspeed-hq bot commented Mar 4, 2024 •

edited

Loading

github-actions bot commented Mar 4, 2024 •

edited

Loading

MichaReiser Mar 5, 2024

MichaReiser Mar 5, 2024

MichaReiser Mar 5, 2024

MichaReiser Mar 5, 2024

BurntSushi left a comment

BurntSushi Mar 5, 2024

MichaReiser Mar 6, 2024

MichaReiser Mar 6, 2024

BurntSushi Mar 5, 2024

refactor: Use QualifiedName for Imported::call_path #10214

refactor: Use QualifiedName for Imported::call_path #10214

Conversation

MichaReiser commented Mar 3, 2024 • edited Loading

Summary

Test plan

codspeed-hq bot commented Mar 4, 2024 • edited Loading

Merging #10214 will not alter performance

Summary

github-actions bot commented Mar 4, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurntSushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor: Use `QualifiedName` for `Imported::call_path` #10214

refactor: Use `QualifiedName` for `Imported::call_path` #10214

MichaReiser commented Mar 3, 2024 •

edited

Loading

codspeed-hq bot commented Mar 4, 2024 •

edited

Loading

github-actions bot commented Mar 4, 2024 •

edited

Loading

`ruff-ecosystem` results