Skip to content

Commit

Permalink
refactor: initialised clippy::missing_panics_doc (#188)
Browse files Browse the repository at this point in the history
# Rationale for this change

In order to improve documentation and identify possible panics, we should enable clippy lints that warn when a function that panics but does not document this behavior.

# What changes are included in this PR?

I have added all the panic doc comments in all the `unwrap()` statements
over the codebase
  • Loading branch information
Gmin2 authored Oct 5, 2024
1 parent d05636a commit b70c212
Show file tree
Hide file tree
Showing 77 changed files with 663 additions and 31 deletions.
3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,6 @@ zerocopy = { version = "0.7.34" }

[workspace.lints.rust]
missing_docs = "warn"

[workspace.lints.clippy]
missing_panics_doc = "warn"
1 change: 1 addition & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
missing-docs-in-crate-items = true
check-private-items = true
doc-valid-idents = ["DeFi"]
5 changes: 5 additions & 0 deletions crates/proof-of-sql-parser/src/identifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ impl Identifier {
/// This is necessary to guarantee that no one outside the crate
/// can create Names, thus securing that [`ResourceId`]s and [`Identifier`]s
/// are always valid postgresql identifiers.
///
/// # Panics
///
/// This function will panic if:
/// - The provided string is too long to fit into the internal `ArrayString`.
pub(crate) fn new<S: AsRef<str>>(string: S) -> Self {
Self {
name: ArrayString::from(&string.as_ref().to_lowercase()).expect("Identifier too long"),
Expand Down
4 changes: 4 additions & 0 deletions crates/proof-of-sql-parser/src/intermediate_ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,10 @@ impl Expression {
})
}
/// Create an `AliasedResultExpr` from an `Expression` using the provided alias.
/// # Panics
///
/// This function will panic if the provided `alias` cannot be parsed into an `Identifier`.
/// It will also panic if `self` cannot be boxed.
#[must_use]
pub fn alias(self, alias: &str) -> AliasedResultExpr {
AliasedResultExpr {
Expand Down
4 changes: 2 additions & 2 deletions crates/proof-of-sql-parser/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![doc = include_str!("../README.md")]
#![no_std]
#![allow(clippy::missing_panics_doc)] // Fixed in Issue #163
#![cfg_attr(test, allow(clippy::missing_panics_doc))]
extern crate alloc;

/// Module for handling an intermediate decimal type received from the lexer.
Expand Down Expand Up @@ -35,7 +35,7 @@ pub mod resource_id;
pub use resource_id::ResourceId;

// lalrpop-generated code is not clippy-compliant
lalrpop_mod!(#[allow(clippy::all, missing_docs, clippy::missing_docs_in_private_items, clippy::pedantic)] pub sql);
lalrpop_mod!(#[allow(clippy::all, missing_docs, clippy::missing_docs_in_private_items, clippy::pedantic, clippy::missing_panics_doc)] pub sql);

/// Implement [`Deserialize`](serde::Deserialize) through [`FromStr`](core::str::FromStr) to avoid invalid identifiers.
#[macro_export]
Expand Down
2 changes: 1 addition & 1 deletion crates/proof-of-sql-parser/src/posql_time/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ impl fmt::Display for PoSQLTimeUnit {
// allow(deprecated) for the sole purpose of testing that
// timestamp precision is parsed correctly.
#[cfg(test)]
#[allow(deprecated)]
#[allow(deprecated, clippy::missing_panics_doc)]
mod time_unit_tests {
use super::*;
use crate::posql_time::{PoSQLTimestamp, PoSQLTimestampError};
Expand Down
5 changes: 5 additions & 0 deletions crates/proof-of-sql-parser/src/select_statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ impl FromStr for SelectStatement {
}
}

/// # Panics
///
/// This function will panic in the following cases:
/// - If `ResourceId::try_new` fails to create a valid `ResourceId`,
/// the `.unwrap()` call will cause a panic.
fn convert_table_expr_to_resource_id_vector(
table_expressions: &[Box<TableExpression>],
default_schema: Identifier,
Expand Down
53 changes: 53 additions & 0 deletions crates/proof-of-sql-parser/src/utility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ use crate::{
};
use alloc::{boxed::Box, vec, vec::Vec};

///
/// # Panics
///
/// This function will panic if`name`(if provided) cannot be parsed.
/// Construct an identifier from a str
#[must_use]
pub fn ident(name: &str) -> Identifier {
Expand Down Expand Up @@ -115,6 +119,9 @@ pub fn div(left: Box<Expression>, right: Box<Expression>) -> Box<Expression> {
/// Get table from schema and name.
///
/// If the schema is `None`, the table is assumed to be in the default schema.
/// # Panics
///
/// This function will panic if either the `name` or the `schema` (if provided) cannot be parsed as valid [Identifier]s.
#[must_use]
pub fn tab(schema: Option<&str>, name: &str) -> Box<TableExpression> {
Box::new(TableExpression::Named {
Expand All @@ -124,6 +131,10 @@ pub fn tab(schema: Option<&str>, name: &str) -> Box<TableExpression> {
}

/// Get column from name
///
/// # Panics
///
/// This function will panic if the `name` cannot be parsed into a valid column expression as valid [Identifier]s.
#[must_use]
pub fn col(name: &str) -> Box<Expression> {
Box::new(Expression::Column(name.parse().unwrap()))
Expand Down Expand Up @@ -177,6 +188,10 @@ pub fn count_all() -> Box<Expression> {
}

/// An expression with an alias i.e. EXPR AS ALIAS
///
/// # Panics
///
/// This function will panic if the `alias` cannot be parsed as valid [Identifier]s.
#[must_use]
pub fn aliased_expr(expr: Box<Expression>, alias: &str) -> AliasedResultExpr {
AliasedResultExpr {
Expand All @@ -192,6 +207,10 @@ pub fn col_res_all() -> SelectResultExpr {
}

/// Select one column from a table and give it an alias i.e. SELECT COL AS ALIAS
///
/// # Panics
///
/// This function will panic if the `alias` cannot be parsed as valid [Identifier]s.
#[must_use]
pub fn col_res(col_val: Box<Expression>, alias: &str) -> SelectResultExpr {
SelectResultExpr::AliasedResultExpr(AliasedResultExpr {
Expand All @@ -207,6 +226,10 @@ pub fn cols_res(names: &[&str]) -> Vec<SelectResultExpr> {
}

/// Compute the minimum of an expression and give it an alias i.e. SELECT MIN(EXPR) AS ALIAS
///
/// # Panics
///
/// This function will panic if the `alias` cannot be parsed.
#[must_use]
pub fn min_res(expr: Box<Expression>, alias: &str) -> SelectResultExpr {
SelectResultExpr::AliasedResultExpr(AliasedResultExpr {
Expand All @@ -216,6 +239,10 @@ pub fn min_res(expr: Box<Expression>, alias: &str) -> SelectResultExpr {
}

/// Compute the maximum of an expression and give it an alias i.e. SELECT MAX(EXPR) AS ALIAS
///
/// # Panics
///
/// This function will panic if the `alias` cannot be parsed.
#[must_use]
pub fn max_res(expr: Box<Expression>, alias: &str) -> SelectResultExpr {
SelectResultExpr::AliasedResultExpr(AliasedResultExpr {
Expand All @@ -225,6 +252,10 @@ pub fn max_res(expr: Box<Expression>, alias: &str) -> SelectResultExpr {
}

/// Compute the sum of an expression and give it an alias i.e. SELECT SUM(EXPR) AS ALIAS
///
/// # Panics
///
/// This function will panic if the `alias` cannot be parsed.
#[must_use]
pub fn sum_res(expr: Box<Expression>, alias: &str) -> SelectResultExpr {
SelectResultExpr::AliasedResultExpr(AliasedResultExpr {
Expand All @@ -234,6 +265,10 @@ pub fn sum_res(expr: Box<Expression>, alias: &str) -> SelectResultExpr {
}

/// Count the amount of non-null entries of expression and give it an alias i.e. SELECT COUNT(EXPR) AS ALIAS
///
/// # Panics
///
/// This function will panic if the `alias` cannot be parsed.
#[must_use]
pub fn count_res(expr: Box<Expression>, alias: &str) -> SelectResultExpr {
SelectResultExpr::AliasedResultExpr(AliasedResultExpr {
Expand All @@ -243,6 +278,10 @@ pub fn count_res(expr: Box<Expression>, alias: &str) -> SelectResultExpr {
}

/// Count rows and give the result an alias i.e. SELECT COUNT(*) AS ALIAS
///
/// # Panics
///
/// This function will panic if the `alias` cannot be parsed.
#[must_use]
pub fn count_all_res(alias: &str) -> SelectResultExpr {
SelectResultExpr::AliasedResultExpr(AliasedResultExpr {
Expand Down Expand Up @@ -305,6 +344,10 @@ pub fn select(
}

/// Order by one column i.e. ORDER BY ID [ASC|DESC]
///
/// # Panics
///
/// This function will panic if the `id` cannot be parsed into an identifier.
#[must_use]
pub fn order(id: &str, direction: OrderByDirection) -> Vec<OrderBy> {
vec![OrderBy {
Expand All @@ -314,6 +357,11 @@ pub fn order(id: &str, direction: OrderByDirection) -> Vec<OrderBy> {
}

/// Order by multiple columns i.e. ORDER BY ID0 [ASC|DESC], ID1 [ASC|DESC], ...
///
/// # Panics
///
/// This function will panic if any of the `ids` cannot be parsed
/// into an identifier.
#[must_use]
pub fn orders(ids: &[&str], directions: &[OrderByDirection]) -> Vec<OrderBy> {
ids.iter()
Expand All @@ -335,6 +383,11 @@ pub fn slice(number_rows: u64, offset_value: i64) -> Option<Slice> {
}

/// Group by clause with multiple columns i.e. GROUP BY ID0, ID1, ...
///
/// # Panics
///
/// This function will panic if any of the `ids` cannot be parsed
/// into an identifier.
#[must_use]
pub fn group_by(ids: &[&str]) -> Vec<Identifier> {
ids.iter().map(|id| id.parse().unwrap()).collect()
Expand Down
6 changes: 6 additions & 0 deletions crates/proof-of-sql/benches/bench_append_rows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ use std::ops::Deref;
/// append 10 rows to 10 cols in 100 tables = 1.1382 seconds
/// append 1000 rows to 10 cols in 1 table = 652ms
/// ```
///
/// # Panics
///
/// Will panic if the creation of the table commitment fails due to invalid column data or an incorrect prover setup.
///
/// Will panic if the row appending operation fails due to invalid data or if the local commitment has reached an invalid state.
fn bench_append_rows(c: &mut Criterion, cols: usize, rows: usize) {
let public_parameters = PublicParameters::test_rand(10, &mut test_rng());
let prover_setup = ProverSetup::from(&public_parameters);
Expand Down
2 changes: 0 additions & 2 deletions crates/proof-of-sql/benches/jaeger_benches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,11 @@ use proof_of_sql::proof_primitive::dory::{
DoryEvaluationProof, DoryProverPublicSetup, DoryVerifierPublicSetup, ProverSetup,
PublicParameters, VerifierSetup,
};
/// TODO: add docs
mod scaffold;
use crate::scaffold::querys::QUERIES;
use scaffold::jaeger_scaffold;
use std::env;

/// TODO: add docs
const SIZE: usize = 1_000_000;

fn main() {
Expand Down
15 changes: 15 additions & 0 deletions crates/proof-of-sql/benches/scaffold/benchmark_accessor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ pub struct BenchmarkAccessor<'a, C: Commitment> {
}

impl<'a, C: Commitment> BenchmarkAccessor<'a, C> {
/// # Panics
///
/// Will panic if the length of the columns does not match after insertion or if the commitment computation fails.
pub fn insert_table(
&mut self,
table_ref: TableRef,
Expand Down Expand Up @@ -63,11 +66,17 @@ impl<'a, C: Commitment> BenchmarkAccessor<'a, C> {
}

impl<C: Commitment> DataAccessor<C::Scalar> for BenchmarkAccessor<'_, C> {
/// # Panics
///
/// Will panic if the column reference does not exist in the accessor.
fn get_column(&self, column: ColumnRef) -> Column<C::Scalar> {
*self.columns.get(&column).unwrap()
}
}
impl<C: Commitment> MetadataAccessor for BenchmarkAccessor<'_, C> {
/// # Panics
///
/// Will panic if the table reference does not exist in the lengths map.
fn get_length(&self, table_ref: TableRef) -> usize {
*self.lengths.get(&table_ref).unwrap()
}
Expand All @@ -76,6 +85,9 @@ impl<C: Commitment> MetadataAccessor for BenchmarkAccessor<'_, C> {
}
}
impl<C: Commitment> CommitmentAccessor<C> for BenchmarkAccessor<'_, C> {
/// # Panics
///
/// Will panic if the column reference does not exist in the commitments map.
fn get_commitment(&self, column: ColumnRef) -> C {
self.commitments.get(&column).unwrap().clone()
}
Expand All @@ -84,6 +96,9 @@ impl<C: Commitment> SchemaAccessor for BenchmarkAccessor<'_, C> {
fn lookup_column(&self, table_ref: TableRef, column_id: Identifier) -> Option<ColumnType> {
self.column_types.get(&(table_ref, column_id)).copied()
}
/// # Panics
///
/// Will panic if the table reference does not exist in the table schemas map.
fn lookup_schema(&self, table_ref: TableRef) -> Vec<(Identifier, ColumnType)> {
self.table_schemas.get(&table_ref).unwrap().clone()
}
Expand Down
12 changes: 12 additions & 0 deletions crates/proof-of-sql/benches/scaffold/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ pub mod querys;
mod random_util;
use random_util::{generate_random_columns, OptionalRandBound};

/// # Panics
///
/// Will panic if:
/// - The table reference cannot be parsed from the string.
/// - The columns generated from `generate_random_columns` lead to a failure in `insert_table`.
/// - The query string cannot be parsed into a `QueryExpr`.
/// - The creation of the `VerifiableQueryResult` fails due to invalid proof expressions.
fn scaffold<'a, CP: CommitmentEvaluationProof>(
query: &str,
columns: &[(&str, ColumnType, OptionalRandBound)],
Expand All @@ -36,6 +43,11 @@ fn scaffold<'a, CP: CommitmentEvaluationProof>(
level = "debug",
skip(query, columns, size, prover_setup, verifier_setup)
)]
/// # Panics
///
/// Will panic if:
/// - The call to `scaffold` results in a panic due to invalid inputs.
/// - The `verify` method of `VerifiableQueryResult` fails, indicating an invalid proof.
pub fn jaeger_scaffold<CP: CommitmentEvaluationProof>(
title: &str,
query: &str,
Expand Down
5 changes: 5 additions & 0 deletions crates/proof-of-sql/benches/scaffold/random_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ use proof_of_sql_parser::Identifier;
use rand::Rng;

pub type OptionalRandBound = Option<fn(usize) -> i64>;
/// # Panics
///
/// Will panic if:
/// - The provided identifier cannot be parsed into an `Identifier` type.
/// - An unsupported `ColumnType` is encountered, triggering a panic in the `todo!()` macro.
pub fn generate_random_columns<'a, S: Scalar>(
alloc: &'a Bump,
rng: &mut impl Rng,
Expand Down
17 changes: 15 additions & 2 deletions crates/proof-of-sql/examples/hello_world/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,30 @@ use std::{
time::Instant,
};

/// TODO: add docs
/// # Panics
///
/// Will panic if flushing the output fails, which can happen due to issues with the underlying output stream.
fn start_timer(message: &str) -> Instant {
print!("{}...", message);
stdout().flush().unwrap();
Instant::now()
}
/// TODO: add docs
/// # Panics
///
/// This function does not panic under normal circumstances but may panic if the internal printing fails due to issues with the output stream.
fn end_timer(instant: Instant) {
println!(" {:?}", instant.elapsed());
}

/// # Panics
///
/// - Will panic if the GPU initialization fails during `init_backend`.
/// - Will panic if the table reference cannot be parsed in `add_table`.
/// - Will panic if the offset provided to `add_table` is invalid.
/// - Will panic if the query string cannot be parsed in `QueryExpr::try_new`.
/// - Will panic if the table reference cannot be parsed in `QueryExpr::try_new`.
/// - Will panic if the query expression creation fails.
/// - Will panic if printing fails during error handling.
fn main() {
let timer = start_timer("Warming up GPU");
init_backend();
Expand Down
19 changes: 19 additions & 0 deletions crates/proof-of-sql/examples/posql_db/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ enum Commands {
},
}

/// # Panics
///
/// Will panic if the call to `stdout().flush()` fails, indicating that the
/// standard output stream could not be flushed
fn start_timer(message: &str) -> Instant {
print!("{}...", message);
stdout().flush().unwrap();
Expand All @@ -125,6 +129,21 @@ fn end_timer(instant: Instant) {
println!(" {:?}", instant.elapsed());
}

/// # Panics
///
/// This function can panic under the following circumstances:
///
/// - **GPU Initialization Failure**: The program will panic if the GPU backend initialization fails.
/// - **Commit Load Failure**: Panics if the commit cannot be loaded from the specified path.
/// - **Table Commitment Creation Failure**: Panics if the table commitment creation fails.
/// - **Commit Write Failure**: Panics if writing the commit to storage fails.
/// - **CSV Write Failure**: Panics if writing the table or batch data to the CSV accessor fails.
/// - **CSV Read Failure**: Panics if reading a CSV file into a record batch fails.
/// - **Query Parsing Failure**: Panics if parsing the query expression fails.
/// - **Proof Generation Failure**: Panics if generating the cryptographic proof fails.
/// - **Proof Verification Failure**: Panics if the proof verification process fails.
/// - **Serialization/Deserialization Failure**: Panics if the proof cannot be serialized or deserialized.
/// - **Record Batch Conversion Failure**: Panics if the query result cannot be converted into a `RecordBatch`.
fn main() {
let args = CliArgs::parse();
println!("Warming up GPU...");
Expand Down
Loading

0 comments on commit b70c212

Please sign in to comment.