Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix clippy lints and run clippy in CI #29

Merged
merged 11 commits into from
Jan 30, 2021
Merged

Fix clippy lints and run clippy in CI #29

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d23492c
Fix clippy lints
smklein Jan 27, 2021
03b905b
Merge branch 'master' into clippy
smklein Jan 29, 2021
e3640c3
Add Clippy to README, github workflow (expect failure)
smklein Jan 29, 2021
d16e9f2
Use non-conflicting workflow names
smklein Jan 29, 2021
f2e6269
Welp no unstable options on stable, go figure
smklein Jan 29, 2021
87c3617
Update clippy flags
smklein Jan 29, 2021
3de03b5
Comments
smklein Jan 29, 2021
ee40f54
Patch the workflow name
smklein Jan 29, 2021
2f562f2
Dangit yaml
smklein Jan 29, 2021
08679d0
Welp that lint doesn't exist on stable
smklein Jan 29, 2021
a32caee
re: code review feedback
smklein Jan 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions api_identity/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,9 @@ fn do_api_identity(item: TokenStream) -> Result<TokenStream, syn::Error> {
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"),
smklein marked this conversation as resolved.
Show resolved Hide resolved
),
_ => false,
} {
return Err(syn::Error::new_spanned(
Expand Down
6 changes: 3 additions & 3 deletions src/datastore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<KeyType, Arc<ValueType>>,
pagparams: &'b DataPageParams<'_, KeyType>,
pub fn collection_page<KeyType, ValueType>(
search_tree: &BTreeMap<KeyType, Arc<ValueType>>,
pagparams: &DataPageParams<'_, KeyType>,
) -> ListResult<ValueType>
where
KeyType: std::cmp::Ord,
Expand Down
24 changes: 12 additions & 12 deletions src/http_pagination.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
}

Expand Down Expand Up @@ -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<S>(
limit: NonZeroUsize,
pag_params: &'a PaginationParams<S, ApiPageSelector<S, S::MarkerValue>>,
) -> Result<DataPageParams<'a, S::MarkerValue>, HttpError>
pag_params: &PaginationParams<S, ApiPageSelector<S, S::MarkerValue>>,
) -> Result<DataPageParams<S::MarkerValue>, HttpError>
where
S: ScanParams,
{
Expand Down Expand Up @@ -295,7 +295,7 @@ impl ScanParams for ApiScanById {
PaginationOrder::Ascending
}
fn marker_for_item<T: ApiObjectIdentity>(&self, item: &T) -> Uuid {
item.identity().id.clone()
item.identity().id
}
fn from_query(p: &ApiPaginatedById) -> Result<&Self, HttpError> {
Ok(match p.page {
Expand Down Expand Up @@ -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),
}
}

Expand Down Expand Up @@ -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<DataPageParams<'a, ApiName>, HttpError> {
pag_params: &ApiPaginatedByNameOrId,
) -> Result<DataPageParams<ApiName>, HttpError> {
let data_page = data_page_params_with_limit(limit, pag_params)?;
let direction = data_page.direction;
let marker = match data_page.marker {
Expand Down Expand Up @@ -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<DataPageParams<'a, Uuid>, HttpError> {
pag_params: &ApiPaginatedByNameOrId,
) -> Result<DataPageParams<Uuid>, HttpError> {
let data_page = data_page_params_with_limit(limit, pag_params)?;
let direction = data_page.direction;
let marker = match data_page.marker {
Expand Down
10 changes: 5 additions & 5 deletions src/oxide_controller/oxide_controller.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,19 +739,19 @@ impl OxideController {
fn disk_attachment_for(
instance: &Arc<ApiInstance>,
disk: &Arc<ApiDisk>,
) -> CreateResult<ApiDiskAttachment> {
) -> Arc<ApiDiskAttachment> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change. Is clippy saying that this thing only ever returns success, so we should drop the Result from the type? That's true, but the caller always wraps the return value in a Result, so it makes sense to me to just do that here, in one place.

If I'm understanding right, I think this is one of those examples of clippy's advice being pretty subjective, which is why we don't run it automatically all the time. I don't really mind this change if there's value in being clippy-clean, but my past experience has been that it's not really worth it (see oxidecomputer/dropshot#4). What about you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the context with that link.

Clippy itself is pretty configurable - it has different "lint levels" (allow / warn / deny) and "lint groups" (correctness, style, pedantic, complexity, perf, etc) that can be turned on or off depending on invocation.

The default is:

  • correctness group: deny
  • style: warn
  • complexity: warn
  • perf: warn
  • pedantic: allow

But this can be (and probably should be, depending on a project's requirements) adjusted, at a broad category but also a per-lint basis.

As an example, take a look at the "deny" lint level on https://rust-lang.github.io/rust-clippy/master/index.html - a lot of these things are in the bucket of "technically not a compiler error, but are almost always inadvisable".

Even if there is opposition to some of the on-the-fence lints, I'd still advocate for some threshold of analysis here - the cost is pretty low.

That being said, if there's a preference for (as @ahl suggested) "run[ning] clippy periodically", that seems fine too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to this specific lint, yeah, clippy is identifying that the function type signature is misleading.

For what it's worth, I do not think this lint would be raised on a public function (where API stability matters), but in the context of a private function, my gut instinct agrees with the lint: the prototype is imprecise regarding the behavior of the code - the use of the function implies that an error could be returned, and should be handled, but this is never the case. All the current uses propagate the Result directly, but a future invocation might have more complexity, and error-handling around this function would become impossible-to-reach code.

(I also totally admit, this is a weakly-held opinion, if you feel strongly, I'd be fine reverting the suggestions based on this lint)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good rule of thumb and it's useful that clippy can identify that. Here's another one: if you have a private function, and it has N callers, and all N callers apply the same transformation to the returned value, then the function ought to apply that transformation instead for the usual DRY reasons. I could imagine a clippy lint for this, too. 🤷

How about this: if you think it's worthwhile for us to pick a set of lints and stay clippy-clean, want to update this PR to do that as part of CI and document it in the README? It's up to you if you think this is worth it. I haven't run it on this code base in months, and I'm mostly indifferent to what it found here (though matches! one is a nice readability improvement!).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmkay - I added clippy to the readme, and have the invocation within .github/workflows/rust.yml, so it'll run on new PRs too.

Disabling a lint is really easy, just add a -D clippy::lint_name, and it'll be ignored. I think it's super super reasonable to look at a lint, decide it's irritating, and throw it in that bucket - like your stance on unnecessary_wraps (which I think is reasonable - removed those changes).

For now the linter is catching defaults + filtering out some undesired lints - WDYT? I'm flexible here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the historical record, this specific lint came up again under #38. (It was also pedantic in the latest version of clippy.)

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(
Expand Down Expand Up @@ -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));
}

/*
Expand Down Expand Up @@ -838,7 +838,7 @@ impl OxideController {
ApiDiskStateRequested::Attached(*instance_id),
)
.await?;
disk_attachment_for(&instance, &disk)
Ok(disk_attachment_for(&instance, &disk))
}

/**
Expand Down