Skip to content

Commit

Permalink
Add redundant-clones lint rule, remove stuff it finds (#164)
Browse files Browse the repository at this point in the history
<!-- The PR description should answer 2 (maybe 3) important questions:
-->

### What

I find this lint rule helpful, popped it on.

### How

<!-- How is it trying to accomplish it (what are the implementation
steps)? -->
  • Loading branch information
danieljharvey authored Dec 11, 2024
1 parent 18929d2 commit b49e56b
Show file tree
Hide file tree
Showing 8 changed files with 26 additions and 30 deletions.
2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ members = [
[workspace.lints.clippy]
all = { level = "warn", priority = -1 }
pedantic = { level = "warn", priority = -1 }
# extra pedanticness
redundant_clone = "warn"
# disable certain pedantic warnings
doc_markdown = "allow"
missing_errors_doc = "allow"
Expand Down
2 changes: 1 addition & 1 deletion crates/ndc-sqlserver/src/error/convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ pub fn execution_error_to_response(error: query_engine_execution::error::Error)
use query_engine_execution::error::*;
match error {
Error::Query(query_error) => {
connector::QueryError::new_invalid_request(&query_error.to_string()).into()
connector::QueryError::new_invalid_request(&query_error).into()
}
Error::Mutation(mutation_error) => {
connector::MutationError::new_invalid_request(&mutation_error.to_string()).into()
Expand Down
6 changes: 3 additions & 3 deletions crates/query-engine/sql/src/sql/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ pub fn select_rowset(
let mut final_row_select = simple_select(rows_row);

final_row_select.from = Some(From::Select {
alias: row_table_alias.clone(),
alias: row_table_alias,
select: Box::new(row_select),
alias_path: AliasPath {
elements: vec![row_column_alias],
Expand Down Expand Up @@ -264,7 +264,7 @@ pub fn select_rowset(

final_select.joins = vec![Join::CrossJoin(CrossJoin {
select: Box::new(aggregate_select_star),
alias: aggregate_table_alias.clone(),
alias: aggregate_table_alias,
alias_path: AliasPath {
elements: vec![aggregate_column_alias],
},
Expand Down Expand Up @@ -478,7 +478,7 @@ pub fn select_mutation_rowset(

final_select.joins = vec![Join::CrossJoin(CrossJoin {
select: Box::new(aggregate_select_star),
alias: aggregate_table_alias.clone(),
alias: aggregate_table_alias,
alias_path: AliasPath {
elements: vec![make_column_alias("aggregates".to_string())],
},
Expand Down
12 changes: 6 additions & 6 deletions crates/query-engine/sql/src/sql/rewrites/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,31 +216,31 @@ mod tests {
fn true_and_true_is_true() {
let left_side = expr_true();
let right_side = expr_true();
let expr = expr_and(left_side, right_side.clone());
let expr = expr_and(left_side, right_side);
assert_eq!(normalize_expr(expr), expr_true());
}

#[test]
fn false_or_false_is_false() {
let left_side = expr_false();
let right_side = expr_false();
let expr = expr_or(left_side, right_side.clone());
let expr = expr_or(left_side, right_side);
assert_eq!(normalize_expr(expr), expr_false());
}

#[test]
fn true_and_false_is_false() {
let left_side = expr_true();
let right_side = expr_false();
let expr = expr_and(left_side, right_side.clone());
let expr = expr_and(left_side, right_side);
assert_eq!(normalize_expr(expr), expr_false());
}

#[test]
fn false_or_true_is_true() {
let left_side = expr_false();
let right_side = expr_true();
let expr = expr_or(left_side, right_side.clone());
let expr = expr_or(left_side, right_side);
assert_eq!(normalize_expr(expr), expr_true());
}

Expand Down Expand Up @@ -273,8 +273,8 @@ mod tests {
fn eq_expr_is_not_removed() {
let eq_expr = expr_eq(expr_seven(), expr_seven());
let left_side = expr_seven();
let right_side = expr_and(eq_expr.clone(), eq_expr.clone());
let expr = expr_and(left_side, right_side.clone());
let right_side = expr_and(eq_expr.clone(), eq_expr);
let expr = expr_and(left_side, right_side);
assert_eq!(normalize_expr(expr.clone()), expr);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,7 @@ fn translate_mutation_operation(
})
}
ProcedureInfo::StoredProcedure { name, info } => {
let stored_procedure_info = super::helpers::StoredProcedureInfo {
name: name.to_string(),
info,
};
let stored_procedure_info = super::helpers::StoredProcedureInfo { name, info };
MutationOperationKind::StoredProcedure(stored_procedure_info)
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ fn translate_comparison_target(

Ok((
sql::ast::Expression::ColumnReference(sql::ast::ColumnReference::TableColumn {
table: table_ref.reference.clone(),
table: table_ref.reference,
name,
}),
joins,
Expand Down Expand Up @@ -443,7 +443,7 @@ pub fn translate_exists_in_collection(
let column_alias = sql::helpers::make_column_alias("one".to_string());

let select_cols = vec![(
column_alias.clone(),
column_alias,
sql::ast::Expression::Value(sql::ast::Value::Int8(1)),
)];

Expand All @@ -455,7 +455,7 @@ pub fn translate_exists_in_collection(
root_table: root_and_current_tables.root_table.clone(),
current_table: TableNameAndReference {
reference: table.reference.clone(),
name: table.name.clone(),
name: table.name,
},
};

Expand Down Expand Up @@ -502,7 +502,7 @@ pub fn translate_exists_in_collection(
let column_alias = sql::helpers::make_column_alias("one".to_string());

let select_cols = vec![(
column_alias.clone(),
column_alias,
sql::ast::Expression::Value(sql::ast::Value::Int8(1)),
)];

Expand Down
6 changes: 3 additions & 3 deletions crates/query-engine/translation/src/translation/query/root.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ pub fn translate_rows_query(
let column_info = collection_info.lookup_column(&column)?;
Ok(sql::helpers::make_column(
current_table.reference.clone(),
column_info.name.clone(),
column_info.name,
sql::helpers::make_column_alias(alias.to_string()),
))
}
Expand All @@ -202,7 +202,7 @@ pub fn translate_rows_query(
let json_column_alias = sql::helpers::make_json_column_alias();
let column_name = sql::ast::ColumnReference::AliasedColumn {
table: sql::ast::TableReference::AliasedTable(table_alias.clone()),
column: json_column_alias.clone(),
column: json_column_alias,
};
join_fields.push(relationships::JoinFieldInfo {
table_alias,
Expand Down Expand Up @@ -362,7 +362,7 @@ pub fn make_from_clause_and_reference(

let current_table = TableNameAndReference {
name: collection_name.to_string(),
reference: collection_alias_name.clone(),
reference: collection_alias_name,
};
Ok((current_table, from_clause))
}
Expand Down
15 changes: 6 additions & 9 deletions crates/query-engine/translation/src/translation/query/sorting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ fn translate_order_by_target_for_column(
table: root_and_current_tables.current_table.reference.clone(),
// we are going to deliberately use the table column name and not an alias we get from
// the query request because this is internal to the sorting mechanism.
column: sql::helpers::make_column_alias(selected_column.name.0.clone()),
column: sql::helpers::make_column_alias(selected_column.name.0),
};
Ok(ColumnOrSelect::Column(selected_column_name))
}
Expand All @@ -367,18 +367,16 @@ fn translate_order_by_target_for_column(
table: last_table.reference,
// we are going to deliberately use the table column name and not an alias we get from
// the query request because this is internal to the sorting mechanism.
column: sql::helpers::make_column_alias(selected_column.name.0.clone()),
column: sql::helpers::make_column_alias(selected_column.name.0),
};

// if we got a function, we wrap the required column with
// a function call.
let selected_column_expr = match function {
None => sql::ast::Expression::ColumnReference(selected_column_name.clone()),
None => sql::ast::Expression::ColumnReference(selected_column_name),
Some(func) => sql::ast::Expression::FunctionCall {
function: sql::ast::Function::Unknown(func.to_string()),
args: vec![sql::ast::Expression::ColumnReference(
selected_column_name.clone(),
)],
args: vec![sql::ast::Expression::ColumnReference(selected_column_name)],
},
};

Expand Down Expand Up @@ -467,7 +465,7 @@ fn process_path_element_for_order_by_target_for_column(
// we are going to deliberately use the table column name and not an alias we get from
// the query request because this is internal to the sorting mechanism.
let selected_column_alias =
sql::helpers::make_column_alias(selected_column.name.0.clone());
sql::helpers::make_column_alias(selected_column.name.0);
// we use the real name of the column as an alias as well.
Ok((
selected_column_alias.clone(),
Expand All @@ -486,8 +484,7 @@ fn process_path_element_for_order_by_target_for_column(
let selected_column = target_collection.lookup_column(&target_column_name.into())?;
// we are going to deliberately use the table column name and not an alias we get from
// the query request because this is internal to the sorting mechanism.
let selected_column_alias =
sql::helpers::make_column_alias(selected_column.name.0.clone());
let selected_column_alias = sql::helpers::make_column_alias(selected_column.name.0);
// we use the real name of the column as an alias as well.
Ok(vec![(
selected_column_alias.clone(),
Expand Down

0 comments on commit b49e56b

Please sign in to comment.