From d23492cf25dfe4f00d284a666b9e558838cbbf13 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 27 Jan 2021 10:51:23 -0500 Subject: [PATCH 01/10] Fix clippy lints --- api_identity/src/lib.rs | 9 +++------ src/datastore.rs | 6 +++--- src/http_pagination.rs | 24 ++++++++++++------------ src/oxide_controller/oxide_controller.rs | 10 +++++----- 4 files changed, 23 insertions(+), 26 deletions(-) diff --git a/api_identity/src/lib.rs b/api_identity/src/lib.rs index 3fc06247c1..697e2bf7d2 100644 --- a/api_identity/src/lib.rs +++ b/api_identity/src/lib.rs @@ -28,12 +28,9 @@ fn do_api_identity(item: TokenStream) -> Result { let name = &ast.ident; if !match ast.fields { - Fields::Named(ref fields) => { - fields.named.iter().any(|field| match &field.ident { - Some(ident) if ident.to_string() == "identity" => true, - _ => false, - }) - } + Fields::Named(ref fields) => fields.named.iter().any( + |field| matches!(&field.ident, Some(ident) if *ident == "identity"), + ), _ => false, } { return Err(syn::Error::new_spanned( diff --git a/src/datastore.rs b/src/datastore.rs index 52878f167c..25b1b08ab2 100644 --- a/src/datastore.rs +++ b/src/datastore.rs @@ -628,9 +628,9 @@ impl ControlDataStore { * TODO-cleanup this is only public because we haven't built Servers into the * datastore yet so the controller needs this interface. */ -pub fn collection_page<'a, 'b, KeyType, ValueType>( - search_tree: &'a BTreeMap>, - pagparams: &'b DataPageParams<'_, KeyType>, +pub fn collection_page( + search_tree: &BTreeMap>, + pagparams: &DataPageParams<'_, KeyType>, ) -> ListResult where KeyType: std::cmp::Ord, diff --git a/src/http_pagination.rs b/src/http_pagination.rs index e81a5a883d..2b71bf2ad6 100644 --- a/src/http_pagination.rs +++ b/src/http_pagination.rs @@ -156,7 +156,7 @@ where { ApiPageSelector { scan: scan_params.clone(), - last_seen: scan_params.marker_for_item(item).clone(), + last_seen: scan_params.marker_for_item(item), } } @@ -184,10 +184,10 @@ where * test the bulk of the logic without needing to cons up a Dropshot * `RequestContext` just to get the limit. */ -fn data_page_params_with_limit<'a, S>( +fn data_page_params_with_limit( limit: NonZeroUsize, - pag_params: &'a PaginationParams>, -) -> Result, HttpError> + pag_params: &PaginationParams>, +) -> Result, HttpError> where S: ScanParams, { @@ -295,7 +295,7 @@ impl ScanParams for ApiScanById { PaginationOrder::Ascending } fn marker_for_item(&self, item: &T) -> Uuid { - item.identity().id.clone() + item.identity().id } fn from_query(p: &ApiPaginatedById) -> Result<&Self, HttpError> { Ok(match p.page { @@ -394,7 +394,7 @@ impl ScanParams for ApiScanByNameOrId { let identity = item.identity(); match pagination_field_for_scan_params(self) { ApiPagField::Name => ApiNameOrIdMarker::Name(identity.name.clone()), - ApiPagField::Id => ApiNameOrIdMarker::Id(identity.id.clone()), + ApiPagField::Id => ApiNameOrIdMarker::Id(identity.id), } } @@ -447,10 +447,10 @@ pub fn data_page_params_nameid_name<'a>( data_page_params_nameid_name_limit(limit, pag_params) } -fn data_page_params_nameid_name_limit<'a>( +fn data_page_params_nameid_name_limit( limit: NonZeroUsize, - pag_params: &'a ApiPaginatedByNameOrId, -) -> Result, HttpError> { + pag_params: &ApiPaginatedByNameOrId, +) -> Result, HttpError> { let data_page = data_page_params_with_limit(limit, pag_params)?; let direction = data_page.direction; let marker = match data_page.marker { @@ -481,10 +481,10 @@ pub fn data_page_params_nameid_id<'a>( data_page_params_nameid_id_limit(limit, pag_params) } -fn data_page_params_nameid_id_limit<'a>( +fn data_page_params_nameid_id_limit( limit: NonZeroUsize, - pag_params: &'a ApiPaginatedByNameOrId, -) -> Result, HttpError> { + pag_params: &ApiPaginatedByNameOrId, +) -> Result, HttpError> { let data_page = data_page_params_with_limit(limit, pag_params)?; let direction = data_page.direction; let marker = match data_page.marker { diff --git a/src/oxide_controller/oxide_controller.rs b/src/oxide_controller/oxide_controller.rs index ebdc71d811..b93f32941e 100644 --- a/src/oxide_controller/oxide_controller.rs +++ b/src/oxide_controller/oxide_controller.rs @@ -739,19 +739,19 @@ impl OxideController { fn disk_attachment_for( instance: &Arc, disk: &Arc, - ) -> CreateResult { + ) -> Arc { let instance_id = &instance.identity.id; assert_eq!( instance_id, disk.runtime.disk_state.attached_instance_id().unwrap() ); - Ok(Arc::new(ApiDiskAttachment { + Arc::new(ApiDiskAttachment { instance_name: instance.identity.name.clone(), instance_id: *instance_id, disk_id: disk.identity.id, disk_name: disk.identity.name.clone(), disk_state: disk.runtime.disk_state.clone(), - })) + }) } fn disk_attachment_error( @@ -795,7 +795,7 @@ impl OxideController { * there's nothing else to do. */ ApiDiskState::Attached(id) if id == instance_id => { - return disk_attachment_for(&instance, &disk); + return Ok(disk_attachment_for(&instance, &disk)); } /* @@ -838,7 +838,7 @@ impl OxideController { ApiDiskStateRequested::Attached(*instance_id), ) .await?; - disk_attachment_for(&instance, &disk) + Ok(disk_attachment_for(&instance, &disk)) } /** From e3640c329fa57bfab1571575fc6e4ab37efa9acd Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 29 Jan 2021 11:59:37 -0500 Subject: [PATCH 02/10] Add Clippy to README, github workflow (expect failure) --- .github/workflows/rust.yml | 10 ++++++++++ README.adoc | 4 ++++ src/oxide_controller/oxide_controller.rs | 10 +++++----- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 3d06cf6b60..81c87e8039 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -14,6 +14,16 @@ jobs: - name: Check style run: cargo fmt -- --check + check-style: + runs-on: ubuntu-18.04 + steps: + # actions/checkout@v2 + - uses: actions/checkout@28c7f3d2b5162b5ddd3dfd9a45aa55eaf396478b + - name: Check style + # TODO: -Zunstable-options can be removed when + # https://github.com/rust-lang/rust-clippy/issues/4612 is fixed. + run: cargo clippy -Zunstable-options -- -D warnings + build-and-test: runs-on: ${{ matrix.os }} strategy: diff --git a/README.adoc b/README.adoc index 3f442f2eaf..16c282d956 100644 --- a/README.adoc +++ b/README.adoc @@ -59,6 +59,10 @@ suite runs cleanly and should remain clean. You can **format the code** using `cargo fmt`. Make sure to run this before pushing changes. The CI checks that the code is correctly formatted. +The https://github.com/rust-lang/rust-clippy[Clippy Linter] is used to help +keeep the codebase efficient and idiomatic, and can be executed with +`cargo clippy`. + To **run the system:** there are two executables: the `oxide_controller` (which is a real prototype backed by an in-memory store; this component manages an Oxide fleet), and the `sled_agent` (which is a simulation of the component that diff --git a/src/oxide_controller/oxide_controller.rs b/src/oxide_controller/oxide_controller.rs index 5ee3ece7e5..22e6423217 100644 --- a/src/oxide_controller/oxide_controller.rs +++ b/src/oxide_controller/oxide_controller.rs @@ -737,19 +737,19 @@ impl OxideController { fn disk_attachment_for( instance: &Arc, disk: &Arc, - ) -> Arc { + ) -> CreateResult { let instance_id = &instance.identity.id; assert_eq!( instance_id, disk.runtime.disk_state.attached_instance_id().unwrap() ); - Arc::new(ApiDiskAttachment { + Ok(Arc::new(ApiDiskAttachment { instance_name: instance.identity.name.clone(), instance_id: *instance_id, disk_id: disk.identity.id, disk_name: disk.identity.name.clone(), disk_state: disk.runtime.disk_state.clone(), - }) + })) } fn disk_attachment_error( @@ -791,7 +791,7 @@ impl OxideController { * there's nothing else to do. */ ApiDiskState::Attached(id) if id == instance_id => { - return Ok(disk_attachment_for(&instance, &disk)); + return disk_attachment_for(&instance, &disk); } /* @@ -834,7 +834,7 @@ impl OxideController { ApiDiskStateRequested::Attached(*instance_id), ) .await?; - Ok(disk_attachment_for(&instance, &disk)) + disk_attachment_for(&instance, &disk) } /** From d16e9f2899a9f35a03ca67fbc8e8ce19fc2970dd Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 29 Jan 2021 12:01:31 -0500 Subject: [PATCH 03/10] Use non-conflicting workflow names --- .github/workflows/rust.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 81c87e8039..175bb1ea2e 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -14,7 +14,7 @@ jobs: - name: Check style run: cargo fmt -- --check - check-style: + clippy-lint: runs-on: ubuntu-18.04 steps: # actions/checkout@v2 From f2e6269fe5abf827ee422ccdeb7f07c10318dc6a Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 29 Jan 2021 12:02:32 -0500 Subject: [PATCH 04/10] Welp no unstable options on stable, go figure --- .github/workflows/rust.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 175bb1ea2e..90edcb6659 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -20,9 +20,7 @@ jobs: # actions/checkout@v2 - uses: actions/checkout@28c7f3d2b5162b5ddd3dfd9a45aa55eaf396478b - name: Check style - # TODO: -Zunstable-options can be removed when - # https://github.com/rust-lang/rust-clippy/issues/4612 is fixed. - run: cargo clippy -Zunstable-options -- -D warnings + run: cargo clippy -- -D warnings build-and-test: runs-on: ${{ matrix.os }} From 87c36170edf2de95b3af094560bc46a12262822f Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 29 Jan 2021 12:27:59 -0500 Subject: [PATCH 05/10] Update clippy flags --- .github/workflows/rust.yml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 90edcb6659..ef0542de48 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -20,7 +20,15 @@ jobs: # actions/checkout@v2 - uses: actions/checkout@28c7f3d2b5162b5ddd3dfd9a45aa55eaf396478b - name: Check style - run: cargo clippy -- -D warnings + run: > + cargo clippy -- -D warnings + # TODO: Fixed by https://github.com/rust-lang/rust-clippy/issues/6344 + # Fix exists on nightly, not on stable. + -A clippy::field_reassign_with_default + # Many functions (internal to OXCP) propagate result types + # as an expected transformation, even when an error case is not possible. + # Avoiding this lint is intentional, as it avoids boilerplate. + -A clippy::unnecessary_wraps build-and-test: runs-on: ${{ matrix.os }} From 3de03b5c30f9c7381a9bca59f5b3e675e185e0a8 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 29 Jan 2021 12:29:51 -0500 Subject: [PATCH 06/10] Comments --- .github/workflows/rust.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index ef0542de48..6269de42a3 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -21,6 +21,8 @@ jobs: - uses: actions/checkout@28c7f3d2b5162b5ddd3dfd9a45aa55eaf396478b - name: Check style run: > + # TODO: Migrate these options to a config file in the workspace, + # once https://github.com/rust-lang/rust-clippy/issues/6625 is resolved. cargo clippy -- -D warnings # TODO: Fixed by https://github.com/rust-lang/rust-clippy/issues/6344 # Fix exists on nightly, not on stable. From ee40f540a98ec83d14f1ef66d733c95bdbbf9dc7 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 29 Jan 2021 12:31:21 -0500 Subject: [PATCH 07/10] Patch the workflow name --- .github/workflows/rust.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 6269de42a3..96dc6403b1 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -19,7 +19,7 @@ jobs: steps: # actions/checkout@v2 - uses: actions/checkout@28c7f3d2b5162b5ddd3dfd9a45aa55eaf396478b - - name: Check style + - name: Run Clippy Lints run: > # TODO: Migrate these options to a config file in the workspace, # once https://github.com/rust-lang/rust-clippy/issues/6625 is resolved. From 2f562f22cf27d958a98e13957286a4b2684254b0 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 29 Jan 2021 12:42:14 -0500 Subject: [PATCH 08/10] Dangit yaml --- .github/workflows/rust.yml | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 96dc6403b1..5a2e259136 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -20,16 +20,20 @@ jobs: # actions/checkout@v2 - uses: actions/checkout@28c7f3d2b5162b5ddd3dfd9a45aa55eaf396478b - name: Run Clippy Lints + # TODO: Migrate these options to a config file in the workspace, + # once https://github.com/rust-lang/rust-clippy/issues/6625 is resolved. + # + # - field_reassign_with_default: + # Fixed by https://github.com/rust-lang/rust-clippy/issues/6344 + # Fix exists on nightly, not on stable. + # + # - unnecessary_wraps: + # Many functions (internal to OXCP) propagate result types + # as an expected transformation, even when an error case is not possible. + # Avoiding this lint is intentional, as it avoids boilerplate. run: > - # TODO: Migrate these options to a config file in the workspace, - # once https://github.com/rust-lang/rust-clippy/issues/6625 is resolved. cargo clippy -- -D warnings - # TODO: Fixed by https://github.com/rust-lang/rust-clippy/issues/6344 - # Fix exists on nightly, not on stable. -A clippy::field_reassign_with_default - # Many functions (internal to OXCP) propagate result types - # as an expected transformation, even when an error case is not possible. - # Avoiding this lint is intentional, as it avoids boilerplate. -A clippy::unnecessary_wraps build-and-test: From 08679d0507f506c9f0ae8f9106c90f161335e972 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 29 Jan 2021 12:45:59 -0500 Subject: [PATCH 09/10] Welp that lint doesn't exist on stable --- .github/workflows/rust.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 5a2e259136..6e8c0ff539 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -26,15 +26,9 @@ jobs: # - field_reassign_with_default: # Fixed by https://github.com/rust-lang/rust-clippy/issues/6344 # Fix exists on nightly, not on stable. - # - # - unnecessary_wraps: - # Many functions (internal to OXCP) propagate result types - # as an expected transformation, even when an error case is not possible. - # Avoiding this lint is intentional, as it avoids boilerplate. run: > cargo clippy -- -D warnings -A clippy::field_reassign_with_default - -A clippy::unnecessary_wraps build-and-test: runs-on: ${{ matrix.os }} From a32caeef12f8dd0b77cf3681b796574a2738755b Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 29 Jan 2021 15:43:17 -0500 Subject: [PATCH 10/10] re: code review feedback --- .github/workflows/rust.yml | 10 +--------- README.adoc | 2 +- src/lib.rs | 4 ++++ 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 6e8c0ff539..e63ccb7673 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -20,15 +20,7 @@ jobs: # actions/checkout@v2 - uses: actions/checkout@28c7f3d2b5162b5ddd3dfd9a45aa55eaf396478b - name: Run Clippy Lints - # TODO: Migrate these options to a config file in the workspace, - # once https://github.com/rust-lang/rust-clippy/issues/6625 is resolved. - # - # - field_reassign_with_default: - # Fixed by https://github.com/rust-lang/rust-clippy/issues/6344 - # Fix exists on nightly, not on stable. - run: > - cargo clippy -- -D warnings - -A clippy::field_reassign_with_default + run: cargo clippy -- -D warnings build-and-test: runs-on: ${{ matrix.os }} diff --git a/README.adoc b/README.adoc index 16c282d956..15b972cb48 100644 --- a/README.adoc +++ b/README.adoc @@ -60,7 +60,7 @@ You can **format the code** using `cargo fmt`. Make sure to run this before pushing changes. The CI checks that the code is correctly formatted. The https://github.com/rust-lang/rust-clippy[Clippy Linter] is used to help -keeep the codebase efficient and idiomatic, and can be executed with +keep the codebase efficient and idiomatic, and can be executed with `cargo clippy`. To **run the system:** there are two executables: the `oxide_controller` (which diff --git a/src/lib.rs b/src/lib.rs index 2fd8ca5bb3..fff6993576 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -57,6 +57,10 @@ * it's expected that we'll have links to private items in the docs. */ #![allow(private_intra_doc_links)] +/* + * TODO(#32): Remove this exception once resolved. + */ +#![allow(clippy::field_reassign_with_default)] mod api_error; pub mod api_model;