Skip to content

Commit

Permalink
Run clippy on public API too (#2596)
Browse files Browse the repository at this point in the history
### What
I discovered the clippy option
[`avoid-breaking-exported-api`](https://doc.rust-lang.org/nightly/clippy/lint_configuration.html#avoid-breaking-exported-api).
By setting it to `false`, we enable linting of our public API:s.

The most interesting thing that popped up was a couple of functions that
returned `Result` despite never return `Err`. There is
`DataCellInner::try_from_arrow` and `DataCellInner::try_from_arrow` and
`try_from_arrow_empty`, which just lacks a check that should be there
(poke @teh-cmc). More interestingly there is `MsgSender::send`, which
doesn't do any error checking yet returns a `Result`. I decided to
silence these three warnings for now because we might add checks to them
in the future.

I also enabled a couple of more lints

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/2596) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/2596)
- [Docs
preview](https://rerun.io/preview/pr%3Aemilk%2Fmore-clippy-lints/docs)
- [Examples
preview](https://rerun.io/preview/pr%3Aemilk%2Fmore-clippy-lints/examples)
  • Loading branch information
emilk authored Jul 4, 2023
1 parent b964108 commit c7d7687
Show file tree
Hide file tree
Showing 32 changed files with 89 additions and 52 deletions.
2 changes: 2 additions & 0 deletions Cranky.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ warn = [
"clippy::iter_on_empty_collections",
"clippy::iter_on_single_items",
"clippy::large_digit_groups",
"clippy::large_include_file",
"clippy::large_stack_arrays",
"clippy::large_types_passed_by_value",
"clippy::let_unit_value",
Expand Down Expand Up @@ -99,6 +100,7 @@ warn = [
"clippy::string_to_string",
"clippy::suspicious_xor_used_as_pow",
"clippy::todo",
"clippy::too_many_lines",
"clippy::trailing_empty_array",
"clippy::trait_duplication_in_bounds",
"clippy::unchecked_duration_subtraction",
Expand Down
16 changes: 16 additions & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
@@ -1,9 +1,25 @@
# There is also a scripts/clippy_wasm/clippy.toml which forbids some methods that are not available in wasm.

# -----------------------------------------------------------------------------
# Section identical to the main scripts/clippy_wasm/clippy.toml:

msrv = "1.69"

allow-unwrap-in-tests = true

# https://doc.rust-lang.org/nightly/clippy/lint_configuration.html#avoid-breaking-exported-api
# We want suggestions, even if it changes public API.
avoid-breaking-exported-api = false

max-fn-params-bools = 2 # TODO(emilk): decrease this to 1

# https://rust-lang.github.io/rust-clippy/master/index.html#/large_include_file
max-include-file-size = 1000000

too-many-lines-threshold = 600 # TODO(emilk): decrease this

# -----------------------------------------------------------------------------

# https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_macros
disallowed-macros = [
'dbg',
Expand Down
6 changes: 3 additions & 3 deletions crates/re_arrow_store/src/store_arrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ fn serialize_control_columns(
// NOTE: Optional column, so make sure it's actually there:
if !col_insert_id.is_empty() {
let (insert_id_field, insert_id_column) =
DataTable::serialize_primitive_column(COLUMN_INSERT_ID, col_insert_id, None)?;
DataTable::serialize_primitive_column(COLUMN_INSERT_ID, col_insert_id, None);
schema.fields.push(insert_id_field);
columns.push(insert_id_column);
}
Expand All @@ -158,13 +158,13 @@ fn serialize_control_columns(
timeline.name(),
col_time,
timeline.datatype().into(),
)?;
);
schema.fields.push(time_field);
columns.push(time_column);
}

let (num_instances_field, num_instances_column) =
DataTable::serialize_primitive_column(COLUMN_NUM_INSTANCES, col_num_instances, None)?;
DataTable::serialize_primitive_column(COLUMN_NUM_INSTANCES, col_num_instances, None);
schema.fields.push(num_instances_field);
columns.push(num_instances_column);

Expand Down
3 changes: 1 addition & 2 deletions crates/re_arrow_store/src/store_polars.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,7 @@ impl IndexedBucket {
self.timeline.name(),
col_time,
self.timeline.datatype().into(),
)
.unwrap();
);
let num_rows = times.len();

let insert_ids = config
Expand Down
2 changes: 1 addition & 1 deletion crates/re_components/src/color.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ impl ColorRGBA {
}

#[inline]
pub fn to_array(&self) -> [u8; 4] {
pub fn to_array(self) -> [u8; 4] {
[
(self.0 >> 24) as u8,
(self.0 >> 16) as u8,
Expand Down
2 changes: 2 additions & 0 deletions crates/re_components/src/coordinates.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![allow(clippy::wrong_self_convention)] // TODO(emilk): re-enable

use arrow2::datatypes::DataType;
use arrow2_convert::{
deserialize::ArrowDeserialize,
Expand Down
2 changes: 2 additions & 0 deletions crates/re_components/src/rect.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![allow(clippy::upper_case_acronyms)]

use arrow2_convert::{ArrowDeserialize, ArrowField, ArrowSerialize};

use super::Vec4D;
Expand Down
1 change: 1 addition & 0 deletions crates/re_components/src/tensor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ impl ArrowDeserialize for TensorId {
/// ```
#[derive(Clone, PartialEq, ArrowField, ArrowSerialize, ArrowDeserialize)]
#[arrow_field(type = "dense")]
#[allow(clippy::upper_case_acronyms)] // TODO(emilk): Rename to `Jpeg`.
pub enum TensorData {
U8(Buffer<u8>),
U16(Buffer<u16>),
Expand Down
2 changes: 1 addition & 1 deletion crates/re_components/src/transform3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ impl Transform3D {
#[cfg(feature = "glam")]
impl Transform3D {
#[inline]
pub fn to_parent_from_child_transform(&self) -> glam::Affine3A {
pub fn to_parent_from_child_transform(self) -> glam::Affine3A {
let transform: glam::Affine3A = self.transform.into();
if self.from_parent {
transform.inverse()
Expand Down
3 changes: 2 additions & 1 deletion crates/re_log_encoding/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ impl EncodingOptions {
}
}

pub fn to_bytes(&self) -> [u8; 4] {
pub fn to_bytes(self) -> [u8; 4] {
[
self.compression as u8,
self.serializer as u8,
Expand All @@ -93,6 +93,7 @@ impl EncodingOptions {

/// On failure to decode [`EncodingOptions`]
#[derive(thiserror::Error, Debug)]
#[allow(clippy::enum_variant_names)]
pub enum OptionsError {
#[error("Reserved bytes not zero")]
UnknownReservedBytes,
Expand Down
4 changes: 2 additions & 2 deletions crates/re_log_types/src/data_cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ impl DataCell {
///
/// Fails if the array is not a valid list of components.
#[inline]
#[allow(clippy::unnecessary_wraps)] // TODO(cmc): check that it is indeed a component datatype
pub fn try_from_arrow(
name: ComponentName,
values: Box<dyn arrow2::array::Array>,
Expand Down Expand Up @@ -267,12 +268,11 @@ impl DataCell {
///
/// Fails if the datatype is not a valid component type.
#[inline]
#[allow(clippy::unnecessary_wraps)] // TODO(cmc): check that it is indeed a component datatype
pub fn try_from_arrow_empty(
name: ComponentName,
datatype: arrow2::datatypes::DataType,
) -> DataCellResult<Self> {
// TODO(cmc): check that it is indeed a component datatype

let mut inner = DataCellInner {
name,
size_bytes: 0,
Expand Down
6 changes: 3 additions & 3 deletions crates/re_log_types/src/data_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ impl DataTable {
COLUMN_NUM_INSTANCES,
col_num_instances.as_slice(),
None,
)?;
);
schema.fields.push(num_instances_field);
columns.push(num_instances_column);

Expand Down Expand Up @@ -730,7 +730,7 @@ impl DataTable {
name: &str,
values: &[T],
datatype: Option<DataType>,
) -> DataTableResult<(Field, Box<dyn Array>)> {
) -> (Field, Box<dyn Array>) {
re_tracing::profile_function!();

let data = PrimitiveArray::from_slice(values);
Expand All @@ -747,7 +747,7 @@ impl DataTable {
.extend([("ARROW:extension:name".to_owned(), name)]);
}

Ok((field, data))
(field, data)
}

/// Serializes all data columns into an arrow payload and schema.
Expand Down
2 changes: 1 addition & 1 deletion crates/re_log_types/src/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl Time {
}

/// Returns the absolute datetime if applicable.
pub fn to_datetime(&self) -> Option<OffsetDateTime> {
pub fn to_datetime(self) -> Option<OffsetDateTime> {
let ns_since_epoch = self.nanos_since_epoch();
if self.is_absolute_date() {
OffsetDateTime::from_unix_timestamp_nanos(ns_since_epoch as i128).ok()
Expand Down
4 changes: 2 additions & 2 deletions crates/re_renderer/examples/2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,8 @@ impl framework::Example for Render2D {
);
}

let line_strip_draw_data = line_strip_builder.to_draw_data(re_ctx).unwrap();
let point_draw_data = point_cloud_builder.to_draw_data(re_ctx).unwrap();
let line_strip_draw_data = line_strip_builder.into_draw_data(re_ctx).unwrap();
let point_draw_data = point_cloud_builder.into_draw_data(re_ctx);

let image_scale = 4.0;
let rectangle_draw_data = RectangleDrawData::new(
Expand Down
4 changes: 2 additions & 2 deletions crates/re_renderer/examples/depth_cloud.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ impl RenderDepthClouds {
std::iter::empty::<re_renderer::PickingLayerInstanceId>(),
);

builder.to_draw_data(re_ctx).unwrap()
builder.into_draw_data(re_ctx)
};

let mut view_builder = ViewBuilder::new(
Expand Down Expand Up @@ -305,7 +305,7 @@ impl framework::Example for RenderDepthClouds {
glam::Vec3::ONE * 0.5,
));
}
builder.to_draw_data(re_ctx).unwrap()
builder.into_draw_data(re_ctx).unwrap()
};

let image_draw_data = RectangleDrawData::new(
Expand Down
4 changes: 2 additions & 2 deletions crates/re_renderer/examples/multiview.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ fn build_lines(re_ctx: &mut RenderContext, seconds_since_startup: f32) -> LineDr
.radius(Size::new_scene(0.1))
.flags(LineStripFlags::FLAG_CAP_END_TRIANGLE);

builder.to_draw_data(re_ctx).unwrap()
builder.into_draw_data(re_ctx).unwrap()
}

enum CameraControl {
Expand Down Expand Up @@ -334,7 +334,7 @@ impl Example for Multiview {
std::iter::empty::<re_renderer::PickingLayerInstanceId>(),
);

let point_cloud = builder.to_draw_data(re_ctx).unwrap();
let point_cloud = builder.into_draw_data(re_ctx);
let meshes = build_mesh_instances(
re_ctx,
&self.model_mesh_instances,
Expand Down
2 changes: 1 addition & 1 deletion crates/re_renderer/examples/picking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ impl framework::Example for Picking {
point_set.picking_ids.iter().cloned(),
);
}
view_builder.queue_draw(point_builder.to_draw_data(re_ctx).unwrap());
view_builder.queue_draw(point_builder.into_draw_data(re_ctx));

let instances = self
.model_mesh_instances
Expand Down
1 change: 1 addition & 0 deletions crates/re_renderer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ impl DeviceCaps {
}

/// Required features for the given device tier.
#[allow(clippy::unused_self)]
pub fn features(&self) -> wgpu::Features {
wgpu::Features::empty()
}
Expand Down
1 change: 1 addition & 0 deletions crates/re_renderer/src/file_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@ mod file_server_impl {
f(&mut Self)
}

#[allow(clippy::unused_self)]
pub fn collect<Fs: FileSystem>(
&mut self,
_resolver: &mut FileResolver<Fs>,
Expand Down
2 changes: 1 addition & 1 deletion crates/re_renderer/src/line_strip_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ impl LineStripSeriesBuilder {
}

/// Finalizes the builder and returns a line draw data with all the lines added so far.
pub fn to_draw_data(
pub fn into_draw_data(
self,
ctx: &mut crate::context::RenderContext,
) -> Result<LineDrawData, LineDrawDataError> {
Expand Down
10 changes: 2 additions & 8 deletions crates/re_renderer/src/point_cloud_builder.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
use crate::{
allocator::CpuWriteGpuReadBuffer,
draw_phases::PickingLayerObjectId,
renderer::{
PointCloudBatchFlags, PointCloudBatchInfo, PointCloudDrawData, PointCloudDrawDataError,
PointCloudVertex,
},
renderer::{PointCloudBatchFlags, PointCloudBatchInfo, PointCloudDrawData, PointCloudVertex},
Color32, DebugLabel, DepthOffset, OutlineMaskPreference, PickingLayerInstanceId, RenderContext,
Size,
};
Expand Down Expand Up @@ -100,10 +97,7 @@ impl PointCloudBuilder {
}

/// Finalizes the builder and returns a point cloud draw data with all the points added so far.
pub fn to_draw_data(
self,
ctx: &mut crate::context::RenderContext,
) -> Result<PointCloudDrawData, PointCloudDrawDataError> {
pub fn into_draw_data(self, ctx: &mut crate::context::RenderContext) -> PointCloudDrawData {
PointCloudDrawData::new(ctx, self)
}
}
Expand Down
13 changes: 5 additions & 8 deletions crates/re_renderer/src/renderer/point_cloud.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,7 @@ impl PointCloudDrawData {
/// Number of vertices and colors has to be equal.
///
/// If no batches are passed, all points are assumed to be in a single batch with identity transform.
pub fn new(
ctx: &mut RenderContext,
mut builder: PointCloudBuilder,
) -> Result<Self, PointCloudDrawDataError> {
pub fn new(ctx: &mut RenderContext, mut builder: PointCloudBuilder) -> Self {
re_tracing::profile_function!();

let mut renderers = ctx.renderers.write();
Expand All @@ -201,11 +198,11 @@ impl PointCloudDrawData {
let batches = builder.batches.as_slice();

if vertices.is_empty() {
return Ok(PointCloudDrawData {
return PointCloudDrawData {
bind_group_all_points: None,
bind_group_all_points_outline_mask: None,
batches: Vec::new(),
});
};
}

let fallback_batches = [PointCloudBatchInfo {
Expand Down Expand Up @@ -491,11 +488,11 @@ impl PointCloudDrawData {
}
}

Ok(PointCloudDrawData {
PointCloudDrawData {
bind_group_all_points: Some(bind_group_all_points),
bind_group_all_points_outline_mask: Some(bind_group_all_points_outline_mask),
batches: batches_internal,
})
}
}
}

Expand Down
1 change: 1 addition & 0 deletions crates/re_sdk/src/msg_sender.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ impl MsgSender {

/// Consumes, packs, sanity checks and finally sends the message to the currently configured
/// target of the SDK.
#[allow(clippy::unnecessary_wraps)] // We might want to return errors in the future.
pub fn send(self, rec_stream: &RecordingStream) -> Result<(), DataTableError> {
if !rec_stream.is_enabled() {
return Ok(()); // silently drop the message
Expand Down
1 change: 1 addition & 0 deletions crates/re_sdk/src/recording_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ impl RecordingStreamBuilder {
self
}

#[allow(clippy::wrong_self_convention)]
#[doc(hidden)]
pub fn is_official_example(mut self, is_official_example: bool) -> Self {
self.is_official_example = is_official_example;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,19 @@ impl SharedRenderBuilders {
) -> Vec<re_renderer::QueueableDrawData> {
let mut result = Vec::new();
result.extend(self.lines.take().and_then(
|l| match l.into_inner().to_draw_data(render_ctx) {
|l| match l.into_inner().into_draw_data(render_ctx) {
Ok(d) => Some(d.into()),
Err(err) => {
re_log::error_once!("Failed to build line strip draw data: {err}");
None
}
},
));
result.extend(self.points.take().and_then(
|l| match l.into_inner().to_draw_data(render_ctx) {
Ok(d) => Some(d.into()),
Err(err) => {
re_log::error_once!("Failed to build point draw data: {err}");
None
}
},
));
result.extend(
self.points
.take()
.map(|l| l.into_inner().into_draw_data(render_ctx).into()),
);
result
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/re_space_view_spatial/src/ui_3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ pub fn view_3d(
}

// Commit ui induced lines.
match line_builder.to_draw_data(ctx.render_ctx) {
match line_builder.into_draw_data(ctx.render_ctx) {
Ok(line_draw_data) => {
view_builder.queue_draw(line_draw_data);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/re_types/src/components/color_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ impl Color {
}

#[inline]
pub fn to_array(&self) -> [u8; 4] {
pub fn to_array(self) -> [u8; 4] {
[
(self.0 >> 24) as u8,
(self.0 >> 16) as u8,
Expand Down
Loading

0 comments on commit c7d7687

Please sign in to comment.