Fix unnamable types #1094

Lorak-mmk · 2024-10-17T12:59:35Z

This PR enables the unnameable_types lint. This lint catches types that are

pub
Used in some public API
Not exported - and thus unusable by the user of the driver.

The only situation when this is desired that I know of is creating a sealed trait.
This PR fixes all occurrences of unnameable types and enables the lint so that
we don't introduce more in the future.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
~~I added relevant tests for new features and bug fixes.~~
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
~~I have provided docstrings for the public items that I want to introduce.~~
~~I have adjusted the documentation in ./docs/source/.~~
I added appropriate Fixes: annotations to PR description.

github-actions · 2024-10-17T13:04:52Z

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: 8a3e60d

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 1ec602028831a900fd10daae121fdfa6e81c3393
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 1ec602028831a900fd10daae121fdfa6e81c3393
     Cloning 1ec602028831a900fd10daae121fdfa6e81c3393
     Parsing scylla v0.14.0 (current)
      Parsed [  20.483s] (current)
     Parsing scylla v0.14.0 (baseline)
      Parsed [  20.205s] (baseline)
    Checking scylla v0.14.0 -> v0.14.0 (no change)
     Checked [   0.105s] 94 checks: 91 pass, 3 fail, 0 warn, 0 skip

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_missing.ron

Failed in:
  enum scylla::transport::topology::UntranslatedEndpoint, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-1ec602028831a900fd10daae121fdfa6e81c3393/8c1b498f4bac0b4f0119ba9f6f149324a4963ba8/scylla/src/transport/topology.rs:80

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_variant_missing.ron

Failed in:
  variant KnownNode::CloudEndpoint, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-1ec602028831a900fd10daae121fdfa6e81c3393/8c1b498f4bac0b4f0119ba9f6f149324a4963ba8/scylla/src/transport/node.rs:231

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/inherent_method_missing.ron

Failed in:
  ClusterData::get_datacenters_info, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-1ec602028831a900fd10daae121fdfa6e81c3393/8c1b498f4bac0b4f0119ba9f6f149324a4963ba8/scylla/src/transport/cluster.rs:414

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [  40.847s] scylla
     Parsing scylla-cql v0.3.0 (current)
      Parsed [   9.995s] (current)
     Parsing scylla-cql v0.3.0 (baseline)
      Parsed [  10.046s] (baseline)
    Checking scylla-cql v0.3.0 -> v0.3.0 (no change)
     Checked [   0.103s] 94 checks: 94 pass, 0 skip
     Summary no semver update required
    Finished [  20.191s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

Lorak-mmk · 2024-10-17T13:28:50Z

Fixed compilation errors when building without cloud feature.

scylla/src/transport/node.rs

scylla/src/transport/session.rs

wprzytula · 2024-10-17T13:46:19Z

scylla/src/transport/cluster.rs

 impl ClusterData {
-    // Updates information about rack count in each datacenter
-    fn update_rack_count(datacenters: &mut HashMap<String, Datacenter>) {
-        for datacenter in datacenters.values_mut() {
-            datacenter.rack_count = datacenter
-                .nodes
-                .iter()
-                .filter_map(|node| node.rack.as_ref())
-                .unique()
-                .count();
-        }
-    }
-
    pub(crate) async fn wait_until_all_pools_are_initialized(&self) {


wprzytula · 2024-10-17T13:46:37Z

scylla/src/transport/cluster.rs


-            if let Some(dc) = &node.datacenter {
-                match datacenters.get_mut(dc) {
-                    Some(v) => v.nodes.push(node.clone()),
-                    None => {
-                        let v = Datacenter {
-                            nodes: vec![node.clone()],
-                            rack_count: 0,
-                        };
-                        datacenters.insert(dc.clone(), v);
-                    }
-                }
-            }
-
            for token in peer_tokens {


wprzytula · 2024-10-17T13:47:34Z

scylla/src/transport/cluster.rs


-        Self::update_rack_count(&mut datacenters);
-
        let keyspaces = metadata.keyspaces;


...was that really pointless?! I can't believe it has always been... Could you please locate the commit that made datacenters unused?

This code was introduced in this commit: 8d82b04#diff-fc8ab4fd7742b17e4767ade6844fd697e5a7b0050bbadb414f52d19f6245bbac (PR: #172 )
You can see that the result is saved in ClusterData.

Then the field in ClusterData was removed by the same author 2 years later: 0db57c1 - but the calculation in ClusterData::new remained, introducing uselessness. PR: #612

I'm pretty sure that even thought there was no useless calculation before #612 this API was still useless because Datacenter struct was never truly public (= pub and exported)

scylla/src/transport/cluster.rs

scylla/src/transport/session_builder.rs

It was public but not exported, despite being returned from methods that are part of public API.

`ResolvedContactPoint` was unnameable. It was part of public API, because it was in a variant of `UntranslatedEndpoint`, which itself was public. `UntranslatedEndpoint` is not used in any public API, so can be made `pub(crate)`. Thanks to that `ResolvedContactPoint` can also be made `pub(crate)` as it no longer appears in any public API.

It was not possible for user to create / retrieve `KnownNode::CloudEndpoint` variant: - `CloudEndpoint` was unnameable type, which means user can't create an instance of it. - No public API allowed user to create this variant or CloudEndpoint struct: they are only used internally. This commit splits `KnownNode` struct into two: - `KnownNode`, without `CloudEndpoint`, which is part of public API - `InternalKnownNode` which is only used internally and thus `pub(crate)`. Thanks to that, `CloudEndpoint` is no longer part of any public API and can be made `pub(crate)`.

This struct is a part of public API: it is present in a field of `SessionConfig`. It was not exported - so it was an unnameable type. It has a pub method that allows its construction by parsing a yaml file, so user could (if this struct was public) create it and put it in several `SessionConfigs`. This commit exports this struct so it is a proper part of public API.

`ClusterData::new()` was creating a datacenter hashmap, populating it with all the nodes, updating rack counts in it, and then dropping it. This whole work is useless and can be just removed.

scylla/src/transport/session_test.rs

This method does some things that are unnecessary here: - For each DC it allocates a vector with it's nodes - It calculates rack count (which requires .unique() call which allocates) For the purpose of this test - and I'm pretty sure most of possible usages of get_datacenters_info - it is enough to get DC names from replica locator.

This method seems to have very limited usability: - For iterating through datacenter names user can call `ReplicaLocator::datacenter_names, which is constant time. - Nothing else was available to the user because `Datacenter` struct is not exported (so is unnameable). This also means all the work done by this method was a waste. - For more advanced tasks there are `ReplicaLocator::unique_nodes_in_datacenter_ring` and `ClusterData::get_nodes_info`. I don't think it is worth it to have this method which allocates a lot on each call and calculates rack counts. It could make sense to have rack counts precalculated when creating `ClusterData` - I'll think about it during metadata refactor. I'm pretty sure that the original intention was to calculate this whole hash map in `ClusterData::new`, but for some reason the calculation there was thrown out and redone here. I don't think its worth it repair this API now considering it was basically unavailable to users.`

It is no longer used anywhere.

Such types should not appear anywhere - except for sealed traits. Previous commits removed all occurrences of such types, so now we can enable the lint. The one legit occurrence - a sealed trait - is marked with `#[allow]`.

wprzytula · 2024-10-21T16:48:42Z

scylla/src/transport/session_builder.rs

@@ -29,6 +29,10 @@ use openssl::ssl::SslContext;
 use tracing::warn;

 mod sealed {
+    // This is a sealed trait - its whole purpose is to be unnameable.
+    // This means we need to disable the check.
+    #[allow(unknown_lints)] // Rust 1.66 doesn't know this lint


We no longer support Rust 1.66. Does 1.70 know this lint?

Unfortunately not - it was introduced in 1.79, so we would need to bump MSRV much higher for that.

Lorak-mmk requested review from wprzytula and muzarski October 17, 2024 12:59

Lorak-mmk self-assigned this Oct 17, 2024

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Oct 17, 2024

Lorak-mmk force-pushed the fix-unnamable-types branch from f42b184 to 0275ef4 Compare October 17, 2024 13:28

wprzytula requested changes Oct 17, 2024

View reviewed changes

Lorak-mmk force-pushed the fix-unnamable-types branch from 0275ef4 to e22f437 Compare October 21, 2024 11:42

lib.rs: export MetricsError

f2b358a

It was public but not exported, despite being returned from methods that are part of public API.

Lorak-mmk force-pushed the fix-unnamable-types branch from e22f437 to 0d871db Compare October 21, 2024 12:48

Lorak-mmk added 4 commits October 21, 2024 16:00

ClusterData::new: don't needlessly create Datacenter structs.

d7ba747

`ClusterData::new()` was creating a datacenter hashmap, populating it with all the nodes, updating rack counts in it, and then dropping it. This whole work is useless and can be just removed.

Lorak-mmk force-pushed the fix-unnamable-types branch 2 times, most recently from e25419d to a56b55b Compare October 21, 2024 14:25

muzarski approved these changes Oct 21, 2024

View reviewed changes

scylla/src/transport/session_test.rs Show resolved Hide resolved

Lorak-mmk added 4 commits October 21, 2024 17:24

cluster.rs: Remove Datacenter struct

ef2bba2

It is no longer used anywhere.

Cargo.toml: warn on unnameable types

8a3e60d

Such types should not appear anywhere - except for sealed traits. Previous commits removed all occurrences of such types, so now we can enable the lint. The one legit occurrence - a sealed trait - is marked with `#[allow]`.

Lorak-mmk force-pushed the fix-unnamable-types branch from a56b55b to 8a3e60d Compare October 21, 2024 15:24

Lorak-mmk requested a review from wprzytula October 21, 2024 15:24

wprzytula reviewed Oct 21, 2024

View reviewed changes

wprzytula approved these changes Oct 21, 2024

View reviewed changes

Lorak-mmk merged commit 025342c into scylladb:main Oct 21, 2024
11 checks passed

muzarski mentioned this pull request Oct 31, 2024

Datacenter struct not exported. #618

Closed

wprzytula mentioned this pull request Nov 14, 2024

Release 0.15 #1125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unnamable types #1094

Fix unnamable types #1094

Lorak-mmk commented Oct 17, 2024

github-actions bot commented Oct 17, 2024 •

edited

Loading

Lorak-mmk commented Oct 17, 2024

wprzytula Oct 17, 2024

wprzytula Oct 17, 2024

wprzytula Oct 17, 2024

Lorak-mmk Oct 17, 2024

Lorak-mmk Oct 17, 2024

wprzytula Oct 21, 2024

Lorak-mmk Oct 21, 2024


		Self::update_rack_count(&mut datacenters);

		let keyspaces = metadata.keyspaces;

Fix unnamable types #1094

Fix unnamable types #1094

Conversation

Lorak-mmk commented Oct 17, 2024

Pre-review checklist

github-actions bot commented Oct 17, 2024 • edited Loading

Lorak-mmk commented Oct 17, 2024

wprzytula Oct 17, 2024

Choose a reason for hiding this comment

wprzytula Oct 17, 2024

Choose a reason for hiding this comment

wprzytula Oct 17, 2024

Choose a reason for hiding this comment

Lorak-mmk Oct 17, 2024

Choose a reason for hiding this comment

Lorak-mmk Oct 17, 2024

Choose a reason for hiding this comment

wprzytula Oct 21, 2024

Choose a reason for hiding this comment

Lorak-mmk Oct 21, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2024 •

edited

Loading