Add dedicated structs for `BindingKind` variants #3672

charliermarsh · 2023-03-22T19:00:47Z

Summary

This is a non-behavior-changing refactor to move the BindingKind variants to use dedicated structs.

Test Plan

Verify that all automated checks pass as expected.

charliermarsh · 2023-03-22T19:01:28Z

crates/ruff_python_ast/src/scope.rs

+    pub name: &'a str,
+    /// The full name of the submodule being imported.
+    /// Given `import foo.bar`, `full_name` would be "foo.bar".
+    pub full_name: &'a str,


I think we can improve some of these abstractions (better names, shared traits). This PR doesn't attempt to improve the object hierarchy at all -- it's just a "straight" migration.

Sounds good. I hoped to find a name in the python specification but didn't find any other than dotted name. What came to my mind is that this is the path but there's no precedence for this term

charliermarsh · 2023-03-22T19:02:37Z

crates/ruff_python_ast/src/scope.rs

+    StarImportation(StarImportation),
+    Importation(Importation<'a>),
+    FromImportation(FromImportation<'a>),
+    SubmoduleImportation(SubmoduleImportation<'a>),


Learning opportunity for me: when would we want to Box these?

When we want the enum to have the size of a box instead of the biggest struct, I think.

Boxing a variant reduces the size of that variant to a single pointer instead of the size of the inner struct + padding.

I recommend boxing when one variant is significantly larger than all other variants.

github-actions · 2023-03-22T19:45:21Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.01     13.8±0.07ms     2.9 MB/sec    1.00     13.6±0.06ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.6±0.01ms     4.7 MB/sec    1.00      3.6±0.01ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    492.1±0.98µs     6.0 MB/sec    1.01    495.1±1.76µs     6.0 MB/sec
linter/all-rules/pydantic/types.py         1.01      6.0±0.03ms     4.2 MB/sec    1.00      6.0±0.02ms     4.3 MB/sec
linter/default-rules/large/dataset.py      1.02      7.3±0.02ms     5.5 MB/sec    1.00      7.2±0.03ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1638.6±2.45µs    10.2 MB/sec    1.00   1631.5±8.71µs    10.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    179.1±0.44µs    16.5 MB/sec    1.03    183.7±4.14µs    16.1 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.4±0.02ms     7.5 MB/sec    1.00      3.4±0.01ms     7.5 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.01     21.2±1.01ms  1966.1 KB/sec    1.00     20.9±0.80ms  1992.2 KB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      5.5±0.25ms     3.0 MB/sec    1.00      5.5±0.35ms     3.0 MB/sec
linter/all-rules/numpy/globals.py          1.03   719.5±57.55µs     4.1 MB/sec    1.00   700.8±62.16µs     4.2 MB/sec
linter/all-rules/pydantic/types.py         1.01      9.1±0.39ms     2.8 MB/sec    1.00      9.0±0.38ms     2.8 MB/sec
linter/default-rules/large/dataset.py      1.00     10.2±0.69ms     4.0 MB/sec    1.15     11.7±0.80ms     3.5 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.03      2.5±0.20ms     6.7 MB/sec    1.00      2.4±0.10ms     6.9 MB/sec
linter/default-rules/numpy/globals.py      1.00   279.7±20.23µs    10.5 MB/sec    1.02   284.0±16.68µs    10.4 MB/sec
linter/default-rules/pydantic/types.py     1.00      5.1±0.25ms     5.0 MB/sec    1.06      5.3±0.40ms     4.8 MB/sec

MichaReiser

🎸🎸

MichaReiser · 2023-03-22T22:39:29Z

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

-                        BindingKind::Importation(.., full_name) => full_name,
-                        BindingKind::FromImportation(.., full_name) => full_name.as_str(),
-                        BindingKind::SubmoduleImportation(.., full_name) => full_name,
+                        BindingKind::Importation(Importation { full_name, .. }) => full_name,


Nit. This could be moved to a 'full_name' method on BindingKind to avoid repetition

It doesn't exist on all variants though -- so would it return Option<&str>?

Yeah, it would have to return an Option

MichaReiser · 2023-03-22T22:41:40Z

crates/ruff_python_ast/src/scope.rs

@@ -366,6 +385,54 @@ impl nohash_hasher::IsEnabled for BindingId {}
 //        StarImportation
 //        FutureImportation

+#[derive(Clone, Debug)]
+pub struct Export {
+    /// The names of the bindings exported via `__all__`.


Thanks for commenting the fields.

MichaReiser · 2023-03-22T22:47:28Z

crates/ruff_python_ast/src/scope.rs

+    pub name: &'a str,
+    /// The full name of the submodule being imported.
+    /// Given `import foo.bar`, `full_name` would be "foo.bar".
+    pub full_name: &'a str,


Sounds good. I hoped to find a name in the python specification but didn't find any other than dotted name. What came to my mind is that this is the path but there's no precedence for this term

MichaReiser · 2023-03-22T22:49:38Z

crates/ruff_python_ast/src/scope.rs

+    StarImportation(StarImportation),
+    Importation(Importation<'a>),
+    FromImportation(FromImportation<'a>),
+    SubmoduleImportation(SubmoduleImportation<'a>),


Boxing a variant reduces the size of that variant to a single pointer instead of the size of the inner struct + padding.

I recommend boxing when one variant is significantly larger than all other variants.

charliermarsh requested review from MichaReiser and konstin March 22, 2023 19:00

charliermarsh commented Mar 22, 2023

View reviewed changes

Add dedicated structs for BindingKind variants

8759785

charliermarsh force-pushed the charlie/structs branch from edcdf3f to 8759785 Compare March 22, 2023 19:20

MichaReiser approved these changes Mar 22, 2023

View reviewed changes

charliermarsh merged commit 189c9d4 into main Mar 22, 2023
13 checks passed

charliermarsh deleted the charlie/structs branch March 22, 2023 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dedicated structs for `BindingKind` variants #3672

Add dedicated structs for `BindingKind` variants #3672

charliermarsh commented Mar 22, 2023

charliermarsh Mar 22, 2023

MichaReiser Mar 22, 2023

charliermarsh Mar 22, 2023

JonathanPlasse Mar 22, 2023

MichaReiser Mar 22, 2023

github-actions bot commented Mar 22, 2023 •

edited

MichaReiser left a comment

MichaReiser Mar 22, 2023

charliermarsh Mar 22, 2023

MichaReiser Mar 23, 2023

MichaReiser Mar 22, 2023

MichaReiser Mar 22, 2023

MichaReiser Mar 22, 2023

Add dedicated structs for BindingKind variants #3672

Add dedicated structs for BindingKind variants #3672

Conversation

charliermarsh commented Mar 22, 2023

Summary

Test Plan

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

github-actions bot commented Mar 22, 2023 • edited

PR Check Results

Ecosystem

Benchmark

Linux

Windows

MichaReiser 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add dedicated structs for `BindingKind` variants #3672

Add dedicated structs for `BindingKind` variants #3672

github-actions bot commented Mar 22, 2023 •

edited