chore: Make clippy clean and harden helpers (#532)

- Promote common accessors (Expr/Rule span/eidx, ScopeContext constructors, Engine::set_rego_v0) to const
- Prefer Option combinators (map_or, then_some) and map_or_else
- Tighten engine logic: add missing semicolons, use checked u32::try_from, make boolean query evaluation avoid unchecked indexing,

Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>

src/ast.rs

@@ -1,4 +1,3 @@
-#![allow(clippy::missing_const_for_fn, clippy::pattern_type_mismatch)]
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
@@ -265,61 +264,59 @@ pub enum Expr {
 }
 
 impl Expr {
-    pub fn span(&self) -> &Span {
-        use Expr::*;
-        match self {
-            String { span, .. }
-            | RawString { span, .. }
-            | Number { span, .. }
-            | Bool { span, .. }
-            | Null { span, .. }
-            | Var { span, .. }
-            | Array { span, .. }
-            | Set { span, .. }
-            | Object { span, .. }
-            | ArrayCompr { span, .. }
-            | SetCompr { span, .. }
-            | ObjectCompr { span, .. }
-            | Call { span, .. }
-            | UnaryExpr { span, .. }
-            | RefDot { span, .. }
-            | RefBrack { span, .. }
-            | BinExpr { span, .. }
-            | BoolExpr { span, .. }
-            | ArithExpr { span, .. }
-            | AssignExpr { span, .. }
-            | Membership { span, .. } => span,
+    pub const fn span(&self) -> &Span {
+        match *self {
+            Self::String { ref span, .. }
+            | Self::RawString { ref span, .. }
+            | Self::Number { ref span, .. }
+            | Self::Bool { ref span, .. }
+            | Self::Null { ref span, .. }
+            | Self::Var { ref span, .. }
+            | Self::Array { ref span, .. }
+            | Self::Set { ref span, .. }
+            | Self::Object { ref span, .. }
+            | Self::ArrayCompr { ref span, .. }
+            | Self::SetCompr { ref span, .. }
+            | Self::ObjectCompr { ref span, .. }
+            | Self::Call { ref span, .. }
+            | Self::UnaryExpr { ref span, .. }
+            | Self::RefDot { ref span, .. }
+            | Self::RefBrack { ref span, .. }
+            | Self::BinExpr { ref span, .. }
+            | Self::BoolExpr { ref span, .. }
+            | Self::ArithExpr { ref span, .. }
+            | Self::AssignExpr { ref span, .. }
+            | Self::Membership { ref span, .. } => span,
             #[cfg(feature = "rego-extensions")]
-            OrExpr { span, .. } => span,
+            Self::OrExpr { ref span, .. } => span,
         }
     }
 
-    pub fn eidx(&self) -> u32 {
-        use Expr::*;
-        match self {
-            String { eidx, .. }
-            | RawString { eidx, .. }
-            | Number { eidx, .. }
-            | Bool { eidx, .. }
-            | Null { eidx, .. }
-            | Var { eidx, .. }
-            | Array { eidx, .. }
-            | Set { eidx, .. }
-            | Object { eidx, .. }
-            | ArrayCompr { eidx, .. }
-            | SetCompr { eidx, .. }
-            | ObjectCompr { eidx, .. }
-            | Call { eidx, .. }
-            | UnaryExpr { eidx, .. }
-            | RefDot { eidx, .. }
-            | RefBrack { eidx, .. }
-            | BinExpr { eidx, .. }
-            | BoolExpr { eidx, .. }
-            | ArithExpr { eidx, .. }
-            | AssignExpr { eidx, .. }
-            | Membership { eidx, .. } => *eidx,
+    pub const fn eidx(&self) -> u32 {
+        match *self {
+            Self::String { eidx, .. }
+            | Self::RawString { eidx, .. }
+            | Self::Number { eidx, .. }
+            | Self::Bool { eidx, .. }
+            | Self::Null { eidx, .. }
+            | Self::Var { eidx, .. }
+            | Self::Array { eidx, .. }
+            | Self::Set { eidx, .. }
+            | Self::Object { eidx, .. }
+            | Self::ArrayCompr { eidx, .. }
+            | Self::SetCompr { eidx, .. }
+            | Self::ObjectCompr { eidx, .. }
+            | Self::Call { eidx, .. }
+            | Self::UnaryExpr { eidx, .. }
+            | Self::RefDot { eidx, .. }
+            | Self::RefBrack { eidx, .. }
+            | Self::BinExpr { eidx, .. }
+            | Self::BoolExpr { eidx, .. }
+            | Self::ArithExpr { eidx, .. }
+            | Self::AssignExpr { eidx, .. }
+            | Self::Membership { eidx, .. } => eidx,
             #[cfg(feature = "rego-extensions")]
-            OrExpr { eidx, .. } => *eidx,
+            Self::OrExpr { eidx, .. } => eidx,
         }
     }
 }
@@ -435,9 +432,9 @@ pub enum Rule {
 }
 
 impl Rule {
-    pub fn span(&self) -> &Span {
-        match self {
-            Self::Spec { span, .. } | Self::Default { span, .. } => span,
+    pub const fn span(&self) -> &Span {
+        match *self {
+            Self::Spec { ref span, .. } | Self::Default { ref span, .. } => span,
         }
     }
 }

src/compiler.rs

@@ -1,14 +1,6 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
-#![allow(
-    clippy::missing_const_for_fn,
-    clippy::option_if_let_else,
-    clippy::if_then_some_else_none,
-    clippy::unused_self,
-    clippy::semicolon_if_nothing_returned,
-    clippy::useless_let_if_seq
-)]
 //! Compiler-related functionality for Regorus.
 //!
 //! This module contains utilities and data structures used during

src/compiler/context.rs

@@ -65,7 +65,7 @@ pub struct ScopeContext {
 
 impl ScopeContext {
     /// Create a new context with Query type (default, no output expressions)
-    pub fn new() -> Self {
+    pub const fn new() -> Self {
         Self {
             context_type: ContextType::Query,
             bound_vars: BTreeSet::new(),
@@ -81,7 +81,7 @@ impl ScopeContext {
 
     /// Create a new context with a specific context type
     #[allow(dead_code)]
-    pub fn with_context_type(context_type: ContextType) -> Self {
+    pub const fn with_context_type(context_type: ContextType) -> Self {
         Self {
             context_type,
             bound_vars: BTreeSet::new(),
@@ -97,7 +97,7 @@ impl ScopeContext {
 
     /// Create a new context with output expressions (for rules and comprehensions)
     #[allow(dead_code)]
-    pub fn with_output_exprs(
+    pub const fn with_output_exprs(
         context_type: ContextType,
         key_expr: Option<ExprRef>,
         value_expr: Option<ExprRef>,

src/compiler/destructuring_planner/utils.rs

@@ -149,10 +149,9 @@ pub(crate) fn check_literal_structure(
 }
 
 pub(crate) fn ensure_literal_match(plan: &DestructuringPlan, expr: &ExprRef) -> Result<()> {
-    match check_literal_structure(plan, expr).into_error() {
-        Some(err) => Err(err),
-        None => Ok(()),
-    }
+    check_literal_structure(plan, expr)
+        .into_error()
+        .map_or(Ok(()), Err)
 }
 
 pub(crate) fn collect_pattern_var_spans(expr: &ExprRef, spans: &mut Vec<Span>) {
@@ -248,13 +247,7 @@ pub(crate) fn ensure_structural_compatibility(
 
 /// Helper that discards destructuring plans which do not bind any variables.
 pub(crate) fn plan_only_if_binds(plan: Option<DestructuringPlan>) -> Option<DestructuringPlan> {
-    plan.and_then(|plan| {
-        if plan.introduces_binding() || plan.contains_wildcards() {
-            Some(plan)
-        } else {
-            None
-        }
-    })
+    plan.and_then(|plan| (plan.introduces_binding() || plan.contains_wildcards()).then_some(plan))
 }
 
 pub(crate) fn extract_literal_key(expr: &ExprRef) -> Option<Value> {

src/compiler/hoist.rs

@@ -576,20 +576,20 @@ impl LoopHoister {
 
         // Get the scheduled order if available
         let stmt_order: Vec<usize> = if let Some(ref schedule) = self.schedule {
-            if let Some(query_schedule) = schedule
+            schedule
                 .queries
                 .get_checked(module_idx, query.qidx)
                 .map_err(|err| anyhow!("schedule out of bounds: {err}"))?
-            {
-                query_schedule
-                    .order
-                    .iter()
-                    .map(|&idx| idx as usize)
-                    .collect()
-            } else {
-                // No schedule for this query, use source order
-                (0..query.stmts.len()).collect()
-            }
+                .map_or_else(
+                    || (0..query.stmts.len()).collect(),
+                    |query_schedule| {
+                        query_schedule
+                            .order
+                            .iter()
+                            .map(|&idx| idx as usize)
+                            .collect()
+                    },
+                )
         } else {
             // No schedule available, use source order
             (0..query.stmts.len()).collect()

src/engine.rs

@@ -1,13 +1,6 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
-#![allow(
-    clippy::indexing_slicing,
-    clippy::missing_const_for_fn,
-    clippy::semicolon_if_nothing_returned,
-    clippy::print_stderr,
-    clippy::as_conversions,
-    clippy::pattern_type_mismatch
-)]
+#![allow(clippy::print_stderr)]
 
 use crate::ast::*;
 use crate::compiled_policy::CompiledPolicy;
@@ -20,7 +13,7 @@ use crate::value::*;
 use crate::*;
 use crate::{Extension, QueryResults};
 
-use anyhow::{bail, Result};
+use anyhow::{anyhow, bail, Result};
 
 /// The Rego evaluation engine.
 ///
@@ -104,7 +97,7 @@ impl Engine {
     /// # }
     /// ```
     ///
-    pub fn set_rego_v0(&mut self, rego_v0: bool) {
+    pub const fn set_rego_v0(&mut self, rego_v0: bool) {
         self.rego_v1 = !rego_v0;
     }
 
@@ -398,7 +391,7 @@ impl Engine {
     /// **_NOTE:_** Currently not all builtins honor this flag and will always strictly raise errors.
     /// ----
     pub fn set_strict_builtin_errors(&mut self, b: bool) {
-        self.interpreter.set_strict_builtin_errors(b)
+        self.interpreter.set_strict_builtin_errors(b);
     }
 
     #[doc(hidden)]
@@ -782,13 +775,23 @@ impl Engine {
     /// ```
     pub fn eval_bool_query(&mut self, query: String, enable_tracing: bool) -> Result<bool> {
         let results = self.eval_query(query, enable_tracing)?;
-        match results.result.len() {
-            0 => bail!("query did not produce any values"),
-            1 if results.result[0].expressions.len() == 1 => {
-                results.result[0].expressions[0].value.as_bool().copied()
-            }
-            _ => bail!("query produced more than one value"),
+        let entries = results.result.as_slice();
+        let entry = entries
+            .first()
+            .ok_or_else(|| anyhow!("query did not produce any values"))?;
+        if entries.len() > 1 {
+            bail!("query produced more than one value");
         }
+
+        let expressions = entry.expressions.as_slice();
+        let expr = expressions
+            .first()
+            .ok_or_else(|| anyhow!("query result missing expression"))?;
+        if expressions.len() > 1 {
+            bail!("query produced more than one value");
+        }
+
+        expr.value.as_bool().copied()
     }
 
     /// Evaluate an `allow` query.
@@ -804,12 +807,12 @@ impl Engine {
     /// let enable_tracing = false;
     /// assert_eq!(engine.eval_allow_query("1 > 2".to_string(), enable_tracing), false);
     /// assert_eq!(engine.eval_allow_query("1 < 2".to_string(), enable_tracing), true);
-    ///
     /// assert_eq!(engine.eval_allow_query("1+1".to_string(), enable_tracing), false);
     /// assert_eq!(engine.eval_allow_query("true; true".to_string(), enable_tracing), false);
     /// assert_eq!(engine.eval_allow_query("true; false; true".to_string(), enable_tracing), false);
     /// # Ok(())
     /// # }
+    /// ```
     pub fn eval_allow_query(&mut self, query: String, enable_tracing: bool) -> bool {
         matches!(self.eval_bool_query(query, enable_tracing), Ok(true))
     }
@@ -832,6 +835,7 @@ impl Engine {
     /// assert_eq!(engine.eval_deny_query("true; false; true".to_string(), enable_tracing), true);
     /// # Ok(())
     /// # }
+    /// ```
     pub fn eval_deny_query(&mut self, query: String, enable_tracing: bool) -> bool {
         !matches!(self.eval_bool_query(query, enable_tracing), Ok(false))
     }
@@ -857,7 +861,8 @@ impl Engine {
         // Populate loop hoisting for the query snippet
         // Query snippets are treated as if they're in a module appended at the end (same as analyzer)
         // The loop hoisting table already has capacity for this (ensured in prepare_for_eval)
-        let module_idx = self.modules.len() as u32;
+        let module_idx = u32::try_from(self.modules.len())
+            .map_err(|_| anyhow!("module count exceeds u32::MAX"))?;
 
         use crate::compiler::hoist::LoopHoister;
 
@@ -1192,14 +1197,14 @@ impl Engine {
     /// If `enable` is different from the current value, then any existing coverage
     /// information will be cleared.
     pub fn set_enable_coverage(&mut self, enable: bool) {
-        self.interpreter.set_enable_coverage(enable)
+        self.interpreter.set_enable_coverage(enable);
     }
 
     #[cfg(feature = "coverage")]
     #[cfg_attr(docsrs, doc(cfg(feature = "coverage")))]
     /// Clear the gathered policy coverage data.
     pub fn clear_coverage_data(&mut self) {
-        self.interpreter.clear_coverage_data()
+        self.interpreter.clear_coverage_data();
     }
 
     /// Gather output from print statements instead of emiting to stderr.
@@ -1339,39 +1344,46 @@ impl Engine {
             let mut modifiers = vec![];
 
             for rule in &m.policy {
-                // Extract parameter definitions from the policy rule
-                // e.g. default parameters.a = 5
-                if let Rule::Default { refr, .. } = rule.as_ref() {
-                    let path = Parser::get_path_ref_components(refr)?;
-                    let paths: Vec<&str> = path.iter().map(|s| s.text()).collect();
+                match *rule.as_ref() {
+                    // Extract parameter definitions from the policy rule
+                    // e.g. default parameters.a = 5
+                    Rule::Default { ref refr, .. } => {
+                        let path = Parser::get_path_ref_components(refr)?;
+                        let paths: Vec<&str> = path.iter().map(|s| s.text()).collect();
 
-                    if paths.len() == 2 && paths[0] == "parameters" {
-                        // Todo: Fetch fields other than name from rego metadoc for the parameter
-                        parameters.push(PolicyParameter {
-                            name: paths[1].to_string(),
-                            modifiable: false,
-                            required: false,
-                        })
-                    }
-                }
-
-                // Extract modifiers to the parameters from the policy rule
-                // e.g. parameters.a = 5
-                if let Rule::Spec { head, .. } = rule.as_ref() {
-                    match head {
-                        RuleHead::Compr { refr, .. } => {
-                            let path = Parser::get_path_ref_components(refr)?;
-                            let paths: Vec<&str> = path.iter().map(|s| s.text()).collect();
-
-                            if paths.len() == 2 && paths[0] == "parameters" {
+                        if paths.len() == 2 && paths.first().is_some_and(|p| *p == "parameters") {
+                            if let Some(name) = paths.get(1) {
                                 // Todo: Fetch fields other than name from rego metadoc for the parameter
-                                modifiers.push(PolicyModifier {
-                                    name: paths[1].to_string(),
-                                })
+                                parameters.push(PolicyParameter {
+                                    name: (*name).to_string(),
+                                    modifiable: false,
+                                    required: false,
+                                });
                             }
                         }
-                        RuleHead::Func { .. } => {}
-                        RuleHead::Set { .. } => {}
+                    }
+                    // Extract modifiers to the parameters from the policy rule
+                    // e.g. parameters.a = 5
+                    Rule::Spec { ref head, .. } => {
+                        match *head {
+                            RuleHead::Compr { ref refr, .. } => {
+                                let path = Parser::get_path_ref_components(refr)?;
+                                let paths: Vec<&str> = path.iter().map(|s| s.text()).collect();
+
+                                if paths.len() == 2
+                                    && paths.first().is_some_and(|p| *p == "parameters")
+                                {
+                                    if let Some(name) = paths.get(1) {
+                                        // Todo: Fetch fields other than name from rego metadoc for the parameter
+                                        modifiers.push(PolicyModifier {
+                                            name: (*name).to_string(),
+                                        });
+                                    }
+                                }
+                            }
+                            RuleHead::Func { .. } => {}
+                            RuleHead::Set { .. } => {}
+                        }
                     }
                 }
             }