Merge pull request #27 from DataDog/feat/more-function-lints

feat(function-lint): add Self-return patterns and MustNotExist rule for builder-style APIs

Author: scottgerring

cargo_pup_lint_config/src/function_lint/builder.rs

@@ -123,6 +123,12 @@ impl<'a> FunctionConstraintBuilder<'a> {
         self
     }
 
+    /// Require that no function matching the selector exists
+    pub fn must_not_exist(mut self) -> Self {
+        self.add_rule_internal(FunctionRule::MustNotExist(self.current_severity));
+        self
+    }
+
     /// Create a new MaxLength rule with the current severity
     pub fn create_max_length_rule(&self, length: usize) -> FunctionRule {
         FunctionRule::MaxLength(length, self.current_severity)

cargo_pup_lint_config/src/function_lint/matcher.rs

@@ -60,6 +60,21 @@ impl FunctionMatcher {
             pattern.into(),
         )))
     }
+
+    /// Matches functions that return `Self` by value
+    pub fn returns_self(&self) -> FunctionMatchNode {
+        FunctionMatchNode::Leaf(FunctionMatch::ReturnsType(ReturnTypePattern::SelfValue))
+    }
+
+    /// Matches functions that return `&Self`
+    pub fn returns_self_ref(&self) -> FunctionMatchNode {
+        FunctionMatchNode::Leaf(FunctionMatch::ReturnsType(ReturnTypePattern::SelfRef))
+    }
+
+    /// Matches functions that return `&mut Self`
+    pub fn returns_self_mut_ref(&self) -> FunctionMatchNode {
+        FunctionMatchNode::Leaf(FunctionMatch::ReturnsType(ReturnTypePattern::SelfMutRef))
+    }
 }
 
 /// Node in the matcher expression tree

cargo_pup_lint_config/src/function_lint/types.rs

@@ -16,6 +16,12 @@ pub enum ReturnTypePattern {
     Regex(String),
     /// Match Result<T, E> where E implements Error trait
     ResultWithErrorImpl,
+    /// Match when the function returns `Self` by value (e.g., a consuming builder-style method)
+    SelfValue,
+    /// Match when the function returns `&Self` (immutable reference, e.g., fluent interface)
+    SelfRef,
+    /// Match when the function returns `&mut Self` (mutable reference, e.g., classic builder setter)
+    SelfMutRef,
 }
 
 /// Specifies how to match functions for linting
@@ -52,6 +58,8 @@ pub enum FunctionRule {
     MaxLength(usize, Severity),
     /// Enforces that Result error types must implement the Error trait
     ResultErrorMustImplementError(Severity),
+    /// Enforces that a function matching the selector must not exist at all
+    MustNotExist(Severity),
 }
 
 // Helper methods for FunctionRule

cargo_pup_lint_impl/src/lints/function_lint/lint.rs

@@ -12,6 +12,16 @@ use rustc_middle::ty::TyKind;
 use rustc_session::impl_lint_pass;
 use rustc_span::BytePos;
 
+// Helper: retrieve the concrete Self type of the impl the method belongs to, if any
+fn get_self_type<'tcx>(
+    ctx: &LateContext<'tcx>,
+    fn_def_id: rustc_hir::def_id::DefId,
+) -> Option<rustc_middle::ty::Ty<'tcx>> {
+    ctx.tcx
+        .impl_of_method(fn_def_id)
+        .map(|impl_def_id| ctx.tcx.type_of(impl_def_id).instantiate_identity())
+}
+
 pub struct FunctionLint {
     name: String,
     matches: FunctionMatch,
@@ -143,6 +153,23 @@ fn evaluate_function_match(
                         Err(_) => false,
                     }
                 }
+                ReturnTypePattern::SelfValue => get_self_type(ctx, fn_def_id) == Some(return_ty),
+                ReturnTypePattern::SelfRef => {
+                    match (get_self_type(ctx, fn_def_id), return_ty.kind()) {
+                        (Some(self_ty), &TyKind::Ref(_, inner, rustc_hir::Mutability::Not)) => {
+                            inner == self_ty
+                        }
+                        _ => false,
+                    }
+                }
+                ReturnTypePattern::SelfMutRef => {
+                    match (get_self_type(ctx, fn_def_id), return_ty.kind()) {
+                        (Some(self_ty), &TyKind::Ref(_, inner, rustc_hir::Mutability::Mut)) => {
+                            inner == self_ty
+                        }
+                        _ => false,
+                    }
+                }
             }
         }
         FunctionMatch::AndMatches(left, right) => {
@@ -284,6 +311,21 @@ impl<'tcx> LateLintPass<'tcx> for FunctionLint {
                             }
                         }
                     }
+                    FunctionRule::MustNotExist(severity) => {
+                        let sig_span = item
+                            .span
+                            .with_hi(item.span.lo() + BytePos((item_name.len() + 5) as u32));
+
+                        span_lint_and_help(
+                            ctx,
+                            FUNCTION_LINT::get_by_severity(*severity),
+                            self.name().as_str(),
+                            sig_span,
+                            format!("Function '{item_name}' is forbidden by lint rule"),
+                            None,
+                            "Remove this function to satisfy the architectural rule",
+                        );
+                    }
                 }
             }
         }
@@ -372,6 +414,21 @@ impl<'tcx> LateLintPass<'tcx> for FunctionLint {
                             }
                         }
                     }
+                    FunctionRule::MustNotExist(severity) => {
+                        let sig_span = impl_item
+                            .span
+                            .with_hi(impl_item.span.lo() + BytePos((item_name.len() + 5) as u32));
+
+                        span_lint_and_help(
+                            ctx,
+                            FUNCTION_LINT::get_by_severity(*severity),
+                            self.name().as_str(),
+                            sig_span,
+                            format!("Function '{item_name}' is forbidden by lint rule"),
+                            None,
+                            "Remove this function to satisfy the architectural rule",
+                        );
+                    }
                 }
             }
         }

test_app/Cargo.lock

@@ -240,4 +240,24 @@ warning: Struct 'MyBadlyNamedThing' is public, but must be private
    = note: Applied by cargo-pup rule 'trait_restrictions'.
    = note: `#[warn(struct_lint_must_be_private)]` on by default
 
+error: Function 'with_width' is forbidden by lint rule
+ --> src/builder_style/mod.rs:8:5
+  |
+8 |     pub fn with_width(self, width: u32) -> Self {
+  |     ^^^^^^^^^^^^^^^
+  |
+  = help: Remove this function to satisfy the architectural rule
+  = note: Applied by cargo-pup rule 'builder_style_with_consuming_forbidden'.
+  = note: `#[deny(function_lint)]` on by default
+
+error: Function 'set_height' is forbidden by lint rule
+  --> src/builder_style/mod.rs:13:5
+   |
+13 |     pub fn set_height(self, height: u32) -> Self {
+   |     ^^^^^^^^^^^^^^^
+   |
+   = help: Remove this function to satisfy the architectural rule
+   = note: Applied by cargo-pup rule 'builder_style_set_consuming_forbidden'.
+
 warning: `test_app` (bin "test_app") generated 20 warnings
+error: could not compile `test_app` (bin "test_app") due to 2 previous errors; 20 warnings emitted

test_app/expected_output

@@ -99,5 +99,25 @@
                 ),
             ],
         )),
+        Function((
+            name: "builder_style_with_consuming_forbidden",
+            matches: AndMatches(
+                NameRegex("^with_.*"),
+                ReturnsType(SelfValue)
+            ),
+            rules: [
+                MustNotExist(Error),
+            ],
+        )),
+        Function((
+            name: "builder_style_set_consuming_forbidden",
+            matches: AndMatches(
+                NameRegex("^set_.*"),
+                ReturnsType(SelfValue)
+            ),
+            rules: [
+                MustNotExist(Error),
+            ],
+        )),
     ],
 )

test_app/pup.ron

@@ -0,0 +1,28 @@
+pub struct WidgetBuilder {
+    width: u32,
+    height: u32,
+}
+
+impl WidgetBuilder {
+    // BAD: Consuming builder – should be caught by the lint (returns `Self`)
+    pub fn with_width(self, width: u32) -> Self {
+        Self { width, ..self }
+    }
+
+    // BAD: Consumes self but uses a `set_` prefix – also forbidden
+    pub fn set_height(self, height: u32) -> Self {
+        Self { height, ..self }
+    }
+
+    // GOOD: Reference-based setter – allowed (`&mut self` → `&mut Self`)
+    pub fn set_width(&mut self, width: u32) -> &mut Self {
+        self.width = width;
+        self
+    }
+
+    // GOOD: Reference-based "with_" method – allowed
+    pub fn with_height(&mut self, height: u32) -> &mut Self {
+        self.height = height;
+        self
+    }
+} 

test_app/src/builder_style/mod.rs

@@ -12,6 +12,7 @@ mod module_usage;
 mod must_be_empty;
 mod result_error;
 mod trait_impl;
+mod builder_style;
 
 fn main() {
     println!("Hello, world!");

test_app/src/main.rs

@@ -98,6 +98,26 @@ fn test_lint_config() {
         .enforce_error_trait_implementation()
         .build();
 
+    // ------------------------------------------------------------------
+    // Builder style lint rules (demonstrates consuming vs reference pattern)
+    // ------------------------------------------------------------------
+
+    builder
+        .function_lint()
+        .lint_named("builder_style_with_consuming_forbidden")
+        .matching(|m| m.name_regex("^with_.*").and(m.returns_self()))
+        .with_severity(Severity::Error)
+        .must_not_exist()
+        .build();
+
+    builder
+        .function_lint()
+        .lint_named("builder_style_set_consuming_forbidden")
+        .matching(|m| m.name_regex("^set_.*").and(m.returns_self()))
+        .with_severity(Severity::Error)
+        .must_not_exist()
+        .build();
+
     // Write the configuration to pup.ron using the fixed write_to_file method
     builder
         .write_to_file("pup.ron")