Lint vertical spacing in Rust code (#1572)

* Lint missing vertical spacing in Rust files

* scripts/lint.py --fix

* cleanup

* Put `type` declarations on their own line

* py-format

* fix

crates/re_data_store/src/entity_properties.rs

@@ -51,6 +51,7 @@ pub struct EntityProperties {
     ///
     /// See [`Self::color_mapper`] to select an actual mapping.
     pub color_mapping: bool,
+
     /// What kind of color mapping should be applied (none, map, texture, transfer..)?
     pub color_mapper: EditableAutoValue<ColorMapper>,
 
@@ -64,12 +65,15 @@ pub struct EntityProperties {
     /// Only applies to tensors with meaning=depth that are affected by a pinhole transform when
     /// in a spatial view, using 3D navigation.
     pub backproject_depth: bool,
+
     /// Entity path of the pinhole transform used for the backprojection.
     ///
     /// `None` means backprojection is disabled.
     pub backproject_pinhole_ent_path: Option<EntityPath>,
+
     /// Used to scale the resulting point cloud.
     pub backproject_scale: EditableAutoValue<f32>,
+
     /// Used to scale the radii of the points in the resulting point cloud.
     pub backproject_radius_scale: EditableAutoValue<f32>,
 }

crates/re_log_types/src/component_types/arrow_convert_shims.rs

@@ -25,6 +25,7 @@ impl BinaryBuffer {
 
 impl Index<usize> for BinaryBuffer {
     type Output = u8;
+
     fn index(&self, i: usize) -> &u8 {
         &self.0[i]
     }

crates/re_renderer/examples/depth_cloud.rs

@@ -403,6 +403,7 @@ struct DepthTexture {
     dimensions: glam::UVec2,
     data: DepthCloudDepthData,
 }
+
 impl DepthTexture {
     pub fn spiral(dimensions: glam::UVec2) -> Self {
         let size = (dimensions.x * dimensions.y) as usize;
@@ -429,6 +430,7 @@ struct AlbedoTexture {
     dimensions: glam::UVec2,
     rgba8: Vec<u8>,
 }
+
 impl AlbedoTexture {
     pub fn spiral(dimensions: glam::UVec2) -> Self {
         let size = (dimensions.x * dimensions.y) as usize;

crates/re_renderer/src/allocator/uniform_buffer_fill.rs

@@ -5,6 +5,7 @@ use crate::{wgpu_resources::BindGroupEntry, DebugLabel, RenderContext};
 struct UniformBufferAlignmentCheck<T> {
     pub _marker: std::marker::PhantomData<T>,
 }
+
 impl<T> UniformBufferAlignmentCheck<T> {
     /// wgpu requires uniform buffers to be aligned to up to 256 bytes.
     ///
@@ -33,6 +34,7 @@ impl<T> UniformBufferAlignmentCheck<T> {
         "Uniform buffers need to be aligned to 256 bytes. Use `#[repr(C, align(256))]`"
     );
 }
+
 /// Utility for fast & efficient creation of uniform buffers from a series of structs.
 ///
 /// For subsequent frames, this will automatically not allocate any resources (thanks to our buffer pooling mechanism).

crates/re_renderer/src/renderer/mesh_renderer.rs

@@ -86,7 +86,9 @@ mod gpu_data {
 #[derive(Clone)]
 struct MeshBatch {
     mesh: GpuMesh,
+
     count: u32,
+
     /// Number of meshes out of `count` which have outlines.
     /// We put all instances with outlines at the start of the instance buffer range.
     count_with_outlines: u32,

crates/re_renderer/src/renderer/outlines.rs

@@ -115,6 +115,7 @@ pub struct OutlineConfig {
 
     /// Premultiplied RGBA color for the first outline layer.
     pub color_layer_a: crate::Rgba,
+
     /// Premultiplied RGBA color for the second outline layer.
     pub color_layer_b: crate::Rgba,
 }
@@ -143,6 +144,7 @@ mod gpu_data {
     #[derive(Clone, Copy, bytemuck::Pod, bytemuck::Zeroable)]
     pub struct JumpfloodingStepUniformBuffer {
         pub step_width: wgpu_buffer_types::U32RowPadded,
+
         /// All this padding hurts. `step_width` be a PushConstant but they are not widely supported enough!
         pub end_padding: [wgpu_buffer_types::PaddingRow; 16 - 1],
     }

crates/re_renderer/src/wgpu_resources/dynamic_resource_pool.rs

@@ -202,6 +202,7 @@ where
             .load(std::sync::atomic::Ordering::Relaxed)
     }
 }
+
 impl<Handle, Desc, Res> Drop for DynamicResourcePool<Handle, Desc, Res>
 where
     Handle: Key,

crates/re_renderer/src/wgpu_resources/texture_pool.rs

@@ -90,6 +90,7 @@ impl DynamicResourcesDesc for TextureDesc {
         true
     }
 }
+
 #[derive(Default)]
 pub struct GpuTexturePool {
     pool: DynamicResourcePool<GpuTextureHandle, TextureDesc, GpuTextureInternal>,

crates/re_viewer/src/remote_viewer_app.rs

@@ -6,8 +6,10 @@ pub struct RemoteViewerApp {
     app_env: crate::AppEnvironment,
     startup_options: crate::StartupOptions,
     re_ui: re_ui::ReUi,
+
     /// The url of the remote server.
     url: String,
+
     app: Option<(re_ws_comms::Connection, App)>,
 }
 

crates/re_viewer/src/ui/view_spatial/ui_renderer_bridge.rs

@@ -73,6 +73,7 @@ fn create_and_fill_view_builder(
 }
 
 slotmap::new_key_type! { pub struct ViewBuilderHandle; }
+
 type ViewBuilderMap = slotmap::SlotMap<ViewBuilderHandle, ViewBuilder>;
 
 fn renderer_paint_callback(

examples/rust/api_demo/src/main.rs

@@ -82,6 +82,7 @@ fn demo_extension_components(session: &Session) -> anyhow::Result<()> {
     #[derive(arrow2_convert::ArrowField, arrow2_convert::ArrowSerialize)]
     #[arrow_field(transparent)]
     struct Confidence(f32);
+
     impl Component for Confidence {
         fn name() -> ComponentName {
             "ext.confidence".into()
@@ -102,14 +103,17 @@ fn demo_extension_components(session: &Session) -> anyhow::Result<()> {
     #[derive(arrow2_convert::ArrowField, arrow2_convert::ArrowSerialize)]
     #[arrow_field(transparent)]
     struct Corner(String);
+
     impl Component for Corner {
         fn name() -> ComponentName {
             "ext.corner".into()
         }
     }
+
     #[derive(arrow2_convert::ArrowField, arrow2_convert::ArrowSerialize)]
     #[arrow_field(transparent)]
     struct Training(bool);
+
     impl Component for Training {
         fn name() -> ComponentName {
             "ext.training".into()

examples/rust/objectron/src/main.rs

@@ -32,6 +32,7 @@ struct ArFrame {
     index: usize,
     timepoint: TimePoint,
 }
+
 impl ArFrame {
     fn from_raw(
         dir: PathBuf,

scripts/lint.py

@@ -10,7 +10,9 @@
 import os
 import re
 import sys
-from typing import Optional
+from typing import Any, List, Optional, Tuple
+
+# -----------------------------------------------------------------------------
 
 todo_pattern = re.compile(r"TODO([^(]|$)")
 debug_format_of_err = re.compile(r"\{\:#?\?\}.*, err")
@@ -65,7 +67,7 @@ def lint_line(line: str) -> Optional[str]:
     return None
 
 
-def test_lint() -> None:
+def test_lint_line() -> None:
     assert lint_line("hello world") is None
 
     should_pass = [
@@ -109,7 +111,137 @@ def test_lint() -> None:
         assert lint_line(line) is not None, f'expected "{line}" to fail'
 
 
-def lint_file(filepath: str) -> int:
+# -----------------------------------------------------------------------------
+
+re_declaration = re.compile(r"^\s*((pub(\(\w*\))? )?((impl|fn|struct|enum|union|trait|type)\b))")
+re_attribute = re.compile(r"^\s*\#\[(error|derive)")
+re_docstring = re.compile(r"^\s*///")
+
+
+def is_missing_blank_line_between(prev_line: str, line: str) -> bool:
+    def is_empty(line: str) -> bool:
+        return (
+            line == ""
+            or line.startswith("#")
+            or line.startswith("//")
+            or line.endswith("{")
+            or line.endswith("(")
+            or line.endswith("\\")
+            or line.endswith('r"')
+            or line.endswith("]")
+        )
+
+    """Only for Rust files."""
+    if re_declaration.match(line) or re_attribute.match(line) or re_docstring.match(line):
+        line = line.strip()
+        prev_line = prev_line.strip()
+
+        if is_empty(prev_line):
+            return False
+
+        if line.startswith("fn ") and line.endswith(";"):
+            return False  # maybe a trait function
+
+        if line.startswith("type ") and prev_line.endswith(";"):
+            return False  # many type declarations in a row is fine
+
+        if prev_line.endswith(",") and line.startswith("impl"):
+            return False
+
+        if prev_line.endswith("*"):
+            return False  # maybe in a macro
+
+        return True
+
+    return False
+
+
+def lint_vertical_spacing(lines_in: List[str]) -> Tuple[List[str], List[str]]:
+    """Only for Rust files."""
+    prev_line = None
+
+    errors = []
+    lines_out = []
+
+    for line_nr, line in enumerate(lines_in):
+        line_nr = line_nr + 1
+
+        if prev_line is not None and is_missing_blank_line_between(prev_line, line):
+            errors.append(f"{line_nr}: for readability, add newline before `{line.strip()}`")
+            lines_out.append("\n")
+
+        lines_out.append(line)
+        prev_line = line
+
+    return errors, lines_out
+
+
+def test_lint_vertical_spacing() -> None:
+    should_pass = [
+        "hello world",
+        """
+        /// docstring
+        foo
+
+        /// docstring
+        bar
+        """,
+        """
+        trait Foo {
+            fn bar();
+            fn baz();
+        }
+        """,
+        # macros:
+        """
+        $(#[$meta])*
+        #[derive(Clone, Copy, Hash, PartialEq, Eq, PartialOrd, Ord)]
+        """,
+        """
+        Item = (
+            &PointCloudBatchInfo,
+            impl Iterator<Item = &PointCloudVertex>,
+        ),
+        """,
+        """
+        type Response = Response<Body>;
+        type Error = hyper::Error;
+        """,
+    ]
+
+    should_fail = [
+        """
+        /// docstring
+        foo
+        /// docstring
+        bar
+        """,
+        """
+        Foo,
+        #[error]
+        Bar,
+        """,
+        """
+        slotmap::new_key_type! { pub struct ViewBuilderHandle; }
+        type ViewBuilderMap = slotmap::SlotMap<ViewBuilderHandle, ViewBuilder>;
+        """,
+    ]
+
+    for code in should_pass:
+        errors, _ = lint_vertical_spacing(code.split("\n"))
+        assert len(errors) == 0, f"expected this to pass:\n{code}\ngot: {errors}"
+
+    for code in should_fail:
+        errors, _ = lint_vertical_spacing(code.split("\n"))
+        assert len(errors) > 0, f"expected this to fail:\n{code}"
+
+    pass
+
+
+# -----------------------------------------------------------------------------
+
+
+def lint_file(filepath: str, args: Any) -> int:
     with open(filepath) as f:
         lines_in = f.readlines()
 
@@ -121,11 +253,26 @@ def lint_file(filepath: str) -> int:
             num_errors += 1
             print(f"{filepath}:{line_nr+1}: {error}")
 
+    if filepath.endswith(".rs"):
+        errors, lines_out = lint_vertical_spacing(lines_in)
+
+        for error in errors:
+            print(f"{filepath}{error}")
+
+        if args.fix and lines_in != lines_out:
+            with open(filepath, "w") as f:
+                f.writelines(lines_out)
+            print(f"{filepath} fixed.")
+
+        num_errors = len(errors)
+
     return num_errors
 
 
 def main() -> None:
-    test_lint()  # Make sure we are bug free before we run!
+    # Make sure we are bug free before we run:
+    test_lint_line()
+    test_lint_vertical_spacing()
 
     parser = argparse.ArgumentParser(description="Lint code with custom linter.")
     parser.add_argument(
@@ -135,14 +282,15 @@ def main() -> None:
         nargs="*",
         help="File paths. Empty = all files, recursively.",
     )
+    parser.add_argument("--fix", dest="fix", action="store_true", help="Automatically fix some problems.")
 
     args = parser.parse_args()
 
     num_errors = 0
 
     if args.files:
         for filepath in args.files:
-            num_errors += lint_file(filepath)
+            num_errors += lint_file(filepath, args)
     else:
         script_dirpath = os.path.dirname(os.path.realpath(__file__))
         root_dirpath = os.path.abspath(f"{script_dirpath}/..")
@@ -154,6 +302,7 @@ def main() -> None:
 
         exclude_paths = {
             "./CODE_STYLE.md",
+            "./examples/rust/objectron/src/objectron.rs",  # auto-generated
             "./scripts/lint.py",  # we contain all the patterns we are linting against
             "./web_viewer/re_viewer_debug.js",  # auto-generated by wasm_bindgen
             "./web_viewer/re_viewer.js",  # auto-generated by wasm_bindgen
@@ -166,13 +315,13 @@ def main() -> None:
                 if extension in extensions:
                     filepath = os.path.join(root, filename)
                     if filepath not in exclude_paths:
-                        num_errors += lint_file(filepath)
+                        num_errors += lint_file(filepath, args)
 
     if num_errors == 0:
-        print("lint.py finished without error")
+        print(f"{sys.argv[0]} finished without error")
         sys.exit(0)
     else:
-        print(f"{num_errors} errors.")
+        print(f"{sys.argv[0]} found {num_errors} errors.")
         sys.exit(1)