Improving our panic audit (#340) (#344)

.cargo/config.toml

@@ -5,4 +5,5 @@ rustflags = [
     "-Dclippy::fallible_impl_from",
     "-Dclippy::unreachable",
     "-Dclippy::indexing_slicing",
+    "-Dclippy::panic",
 ]

.github/workflows/ci.yml

@@ -22,7 +22,7 @@ jobs:
       - run: cargo test --verbose --features "experimental"
       - run: cargo test --verbose
       - run: cargo doc --all-features
-      - run: cargo clippy
+      - run: cargo clippy --all-features
       - run: ./panic_safety.sh
       - run: cargo test --verbose -- --ignored
       - run: cargo bench --no-run

cedar-policy-core/src/ast/policy.rs

@@ -1527,6 +1527,10 @@ pub mod test_generators {
 }
 
 #[cfg(test)]
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::indexing_slicing)]
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 mod test {
     use std::collections::HashSet;
 

cedar-policy-core/src/ast/policy_set.rs

@@ -263,6 +263,8 @@ impl std::fmt::Display for PolicySet {
     }
 }
 
+// PANIC SAFETY tests
+#[allow(clippy::panic)]
 // PANIC SAFETY tests
 #[allow(clippy::indexing_slicing)]
 #[cfg(test)]

cedar-policy-core/src/ast/request.rs

@@ -153,6 +153,7 @@ impl std::fmt::Display for Request {
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct Context {
     /// an `Expr::Record` that qualifies as a "restricted expression"
+    // INVARIANT: `context` must be a `Record`
     #[serde(flatten)]
     context: RestrictedExpr,
 }
@@ -164,13 +165,15 @@ impl Context {
     }
 
     /// Create a `Context` from a `RestrictedExpr`, which must be a `Record`
+    /// INVARIANT: `from_expr` must only be called with an `expr` that is a `Record`
     pub fn from_expr(expr: RestrictedExpr) -> Self {
         debug_assert!(matches!(expr.expr_kind(), ExprKind::Record { .. }));
         Self { context: expr }
     }
 
     /// Create a `Context` from a map of key to `RestrictedExpr`, or a Vec of
     /// `(key, RestrictedExpr)` pairs, or any other iterator of `(key, RestrictedExpr)` pairs
+    // INVARIANT: always constructs a record
     pub fn from_pairs(pairs: impl IntoIterator<Item = (SmolStr, RestrictedExpr)>) -> Self {
         Self {
             context: RestrictedExpr::record(pairs),
@@ -184,6 +187,7 @@ impl Context {
     ///
     /// For schema-based parsing, use `ContextJsonParser`.
     pub fn from_json_str(json: &str) -> Result<Self, JsonDeserializationError> {
+        // INVARIANT `.from_json_str` always produces an expression of variant `Record`
         ContextJsonParser::new(None::<&NullContextSchema>, Extensions::all_available())
             .from_json_str(json)
     }
@@ -195,6 +199,7 @@ impl Context {
     ///
     /// For schema-based parsing, use `ContextJsonParser`.
     pub fn from_json_value(json: serde_json::Value) -> Result<Self, JsonDeserializationError> {
+        // INVARIANT `.from_json_value` always produces an expression of variant `Record`
         ContextJsonParser::new(None::<&NullContextSchema>, Extensions::all_available())
             .from_json_value(json)
     }
@@ -206,12 +211,15 @@ impl Context {
     ///
     /// For schema-based parsing, use `ContextJsonParser`.
     pub fn from_json_file(json: impl std::io::Read) -> Result<Self, JsonDeserializationError> {
+        // INVARIANT `.from_json_file` always produces an expression of variant `Record`
         ContextJsonParser::new(None::<&NullContextSchema>, Extensions::all_available())
             .from_json_file(json)
     }
 
     /// Iterate over the (key, value) pairs in the `Context`
     pub fn iter(&self) -> impl Iterator<Item = (&str, BorrowedRestrictedExpr<'_>)> {
+        // PANIC SAFETY invariant on `self.context` ensures that it is a Record
+        #[allow(clippy::panic)]
         match self.context.as_ref().expr_kind() {
             ExprKind::Record { pairs } => pairs
                 .iter()
@@ -238,3 +246,13 @@ impl std::fmt::Display for Context {
         write!(f, "{}", self.context)
     }
 }
+
+#[cfg(test)]
+mod test {
+
+    #[test]
+    fn test_json_from_str_non_record() {
+        let src = "1";
+        assert!(super::Context::from_json_str(src).is_err());
+    }
+}

cedar-policy-core/src/ast/value.rs

@@ -454,6 +454,8 @@ impl Value {
     }
 }
 
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 mod test {
     use super::*;

cedar-policy-core/src/authorizer.rs

@@ -393,6 +393,8 @@ impl std::fmt::Debug for Authorizer {
     }
 }
 
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 mod test {
     use std::collections::BTreeMap;

cedar-policy-core/src/entities.rs

@@ -265,6 +265,8 @@ where
     /// # Panics
     ///
     /// Panics if the self value is not `Data`.
+    // PANIC SAFETY: This function is intended to panic, and says so in the documentation
+    #[allow(clippy::panic)]
     pub fn unwrap(self) -> &'a T {
         match self {
             Self::Data(e) => e,
@@ -281,6 +283,8 @@ where
     /// # Panics
     ///
     /// Panics if the self value is not `Data`.
+    // PANIC SAFETY: This function is intended to panic, and says so in the documentation
+    #[allow(clippy::panic)]
     pub fn expect(self, msg: &str) -> &'a T {
         match self {
             Self::Data(e) => e,
@@ -320,6 +324,8 @@ pub enum TCComputation {
     ComputeNow,
 }
 
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 mod json_parsing_tests {
     use super::*;
@@ -822,6 +828,8 @@ mod json_parsing_tests {
     }
 }
 
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 mod entities_tests {
     use super::*;
@@ -894,6 +902,8 @@ mod entities_tests {
     }
 }
 
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 mod schema_based_parsing_tests {
     use super::*;

cedar-policy-core/src/entities/json/context.rs

@@ -80,6 +80,8 @@ impl<'e, 's, S: ContextSchema> ContextJsonParser<'e, 's, S> {
         let rexpr = vparser.val_into_rexpr(json, expected_ty.as_ref(), || {
             JsonDeserializationErrorContext::Context
         })?;
+        // INVARIANT `Context::from_exprs` requires that `rexpr` is a `Record`.
+        // This is checked by the `match` expression
         match rexpr.expr_kind() {
             ExprKind::Record { .. } => Ok(Context::from_expr(rexpr)),
             _ => Err(JsonDeserializationError::ExpectedContextToBeRecord {

cedar-policy-core/src/est.rs

@@ -278,6 +278,8 @@ impl From<ast::Expr> for Clause {
     }
 }
 
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 mod test {
     use super::*;

cedar-policy-core/src/evaluator.rs

@@ -857,6 +857,8 @@ fn stack_size_check() -> Result<()> {
     Ok(())
 }
 
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 pub mod test {
     use std::str::FromStr;

cedar-policy-core/src/extensions/decimal.rs

@@ -279,6 +279,8 @@ pub fn extension() -> Extension {
 }
 
 #[cfg(test)]
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 mod tests {
     use super::*;
     use crate::ast::{Expr, Type, Value};

cedar-policy-core/src/extensions/ipaddr.rs

@@ -380,6 +380,8 @@ pub fn extension() -> Extension {
     )
 }
 
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 mod tests {
     use super::*;

cedar-policy-core/src/parser.rs

@@ -336,6 +336,8 @@ pub(crate) fn parse_ident(id: &str) -> Result<ast::Id, err::ParseErrors> {
     }
 }
 
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 mod test {
     use super::*;

cedar-policy-core/src/parser/cst_to_ast.rs

@@ -2152,6 +2152,8 @@ fn construct_expr_record(kvs: Vec<(SmolStr, ast::Expr)>, l: SourceInfo) -> ast::
     ast::ExprBuilder::new().with_source_info(l).record(kvs)
 }
 
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 mod tests {
     use super::*;

cedar-policy-core/src/parser/text_to_cst.rs

@@ -25,6 +25,8 @@ lalrpop_mod!(
     #[allow(clippy::indexing_slicing)]
     //PANIC SAFETY: lalrpop uses unreachable, and we are trusting lalrpop to generate correct code
     #[allow(clippy::unreachable)]
+    //PANIC SAFETY: lalrpop uses panic, and we are trusting lalrpop to generate correct code
+    #[allow(clippy::panic)]
     pub grammar,
     "/src/parser/grammar.rs"
 );

cedar-policy-core/src/transitive_closure.rs

@@ -185,6 +185,8 @@ where
 
 // PANIC SAFETY test cases
 #[allow(clippy::indexing_slicing)]
+// PANIC SAFETY: Unit Test Code
+#[allow(clippy::panic)]
 #[cfg(test)]
 mod tests {
     use crate::ast::{Entity, EntityUID};

cedar-policy-validator/src/extensions/decimal.rs

@@ -13,6 +13,10 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+//! Note on panic safety
+//! If any of the panics in this file are triggered, that means that this file has become
+//! out-of-date with the decimal extension definition in CedarCore.
+//! This is tested by the `extension_schema_correctness()` test
 
 use crate::extension_schema::{ArgumentCheckFn, ExtensionFunctionType, ExtensionSchema};
 use crate::types::{self, Type};
@@ -21,9 +25,8 @@ use cedar_policy_core::evaluator::RestrictedEvaluator;
 use cedar_policy_core::extensions::{decimal, Extensions};
 use std::str::FromStr;
 
-/// If any of the panics in this file are triggered, that means that this file has become
-/// out-of-date with the decimal extension definition in CedarCore.
-
+// PANIC SAFETY see note on panic safety above
+#[allow(clippy::panic)]
 fn get_argument_types(fname: &str, decimal_ty: &Type) -> Vec<types::Type> {
     match fname {
         "decimal" => vec![Type::primitive_string()],
@@ -34,6 +37,8 @@ fn get_argument_types(fname: &str, decimal_ty: &Type) -> Vec<types::Type> {
     }
 }
 
+// PANIC SAFETY see note on panic safety above
+#[allow(clippy::panic)]
 fn get_return_type(fname: &str, decimal_ty: &Type) -> Type {
     match fname {
         "decimal" => decimal_ty.clone(),
@@ -44,6 +49,8 @@ fn get_return_type(fname: &str, decimal_ty: &Type) -> Type {
     }
 }
 
+// PANIC SAFETY see note on panic safety above
+#[allow(clippy::panic)]
 fn get_argument_check(fname: &str) -> Option<ArgumentCheckFn> {
     match fname {
         "decimal" => Some(Box::new(validate_decimal_string)),
@@ -96,3 +103,14 @@ fn validate_decimal_string(exprs: &[Expr]) -> Result<(), String> {
         _ => Ok(()),
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    // Ensures that `extension_schema()` does not panic
+    #[test]
+    fn extension_schema_correctness() {
+        let _ = extension_schema();
+    }
+}

cedar-policy-validator/src/extensions/ipaddr.rs

@@ -13,6 +13,10 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+//! Note on panic safety
+//! If any of the panics in this file are triggered, that means that this file has become
+//! out-of-date with the decimal extension definition in CedarCore.
+//! This is tested by the `extension_schema_correctness()` test
 
 use crate::extension_schema::{ArgumentCheckFn, ExtensionFunctionType, ExtensionSchema};
 use crate::types::{self, Type};
@@ -21,9 +25,8 @@ use cedar_policy_core::evaluator::RestrictedEvaluator;
 use cedar_policy_core::extensions::{ipaddr, Extensions};
 use std::str::FromStr;
 
-/// If any of the panics in this file are triggered, that means that this file has become
-/// out-of-date with the ipaddr extension definition in CedarCore.
-
+// PANIC SAFETY see note on panic safety above
+#[allow(clippy::panic)]
 fn get_argument_types(fname: &str, ipaddr_ty: &Type) -> Vec<types::Type> {
     match fname {
         "ip" => vec![Type::primitive_string()],
@@ -33,6 +36,8 @@ fn get_argument_types(fname: &str, ipaddr_ty: &Type) -> Vec<types::Type> {
     }
 }
 
+// PANIC SAFETY see note on panic safety above
+#[allow(clippy::panic)]
 fn get_return_type(fname: &str, ipaddr_ty: &Type) -> Type {
     match fname {
         "ip" => ipaddr_ty.clone(),
@@ -43,6 +48,8 @@ fn get_return_type(fname: &str, ipaddr_ty: &Type) -> Type {
     }
 }
 
+// PANIC SAFETY see note on panic safety above
+#[allow(clippy::panic)]
 fn get_argument_check(fname: &str) -> Option<ArgumentCheckFn> {
     match fname {
         "ip" => Some(Box::new(validate_ip_string)),
@@ -96,3 +103,14 @@ fn validate_ip_string(exprs: &[Expr]) -> Result<(), String> {
         _ => Ok(()),
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    // Ensures that `extension_schema()` does not panic
+    #[test]
+    fn extension_schema_correctness() {
+        let _ = extension_schema();
+    }
+}