ronantakizawa/codereview-bench huggingface dataset:
Repository: diesel-rs/diesel
File: diesel/src/sqlite/connection/statement_iterator.rs (Rust)

Code before:
                            }
                        }
                    } else {
                        // any other state than `PrivateSqliteRow::Direct` is invalid here
                        // and should not happen. If this ever happens this is a logic error
                        // in the code above
                        unreachable!(
                            "You've reached an impossible internal state. \
                             If you ever see this error message please open \
                             an issue at https://github.com/diesel-rs/diesel \
                             providing example code how to trigger this error."
                        )
                    }
                } else {
                    Self::handle_duplicated_row_case(
                        last_row,
                        &mut self.column_names,
                        self.field_count,
                    )
                }
            }
            NotStarted(_) => unreachable!(
                "You've reached an impossible internal state. \
                             If you ever see this error message please open \
                             an issue at https://github.com/diesel-rs/diesel \
                             providing example code how to trigger this error."
            ),
        }
    }
}


Review comment (suggestion):
This seems reasonable to me. Either we've already returned some error or we've already returned `None`. In either case, returning `None` forever after is good. 

Code after:
                            }
                        }
                    } else {
                        // any other state than `PrivateSqliteRow::Direct` is invalid here
                        // and should not happen. If this ever happens this is a logic error
                        // in the code above
                        unreachable!(
                            "You've reached an impossible internal state. \
                             If you ever see this error message please open \
                             an issue at https://github.com/diesel-rs/diesel \
                             providing example code how to trigger this error."
                        )
                    }
                } else {
                    Self::handle_duplicated_row_case(
                        last_row,
                        &mut self.column_names,
                        self.field_count,
                    )
                }
            }
            NotStarted(_s) => {
                // we likely got an error while executing the other
                // `NotStarted` branch above. In this case we just want to stop
                // iterating here
                None
            }
        }
    }
}
Repository: diesel-rs/diesel
File: diesel/src/connection/instrumentation.rs (Rust)

Code before:
    }
}
impl<I: Instrumentation> From<I> for DynInstrumentation {
    fn from(instrumentation: I) -> Self {
        Self {
            inner: Some(unpack_instrumentation(Box::new(instrumentation))),
            no_instrumentation: NoInstrumentation,
        }
    }
}

struct NoInstrumentation;
impl Instrumentation for NoInstrumentation {
    fn on_connection_event(&mut self, _: InstrumentationEvent<'_>) {}
}

/// Unwrap unnecessary boxing levels
fn unpack_instrumentation(
    mut instrumentation: Box<dyn Instrumentation>,
) -> Box<dyn Instrumentation> {
    loop {
        match instrumentation.downcast::<Box<dyn Instrumentation>>() {
            Ok(extra_boxed_instrumentation) => instrumentation = *extra_boxed_instrumentation,
            Err(not_extra_boxed_instrumentation) => {
                break not_extra_boxed_instrumentation;
            }
        }
    }
}


Review comment (suggestion):
```suggestion

impl Instrumentation for NoInstrumentation {
```

Code after:

    #[diesel_derives::__diesel_public_if(
        feature = "i-implement-a-third-party-backend-and-opt-into-breaking-changes"
    )]
    pub(crate) fn on_connection_event(&mut self, event: InstrumentationEvent<'_>) {
        // This implementation is not necessary to be able to call this method on this object
        // because of the already existing Deref impl.
        // However it allows avoiding the dynamic dispatch to the stub value
        if let Some(inner) = self.inner.as_deref_mut() {
            inner.on_connection_event(event)
        }
    }
}

impl<I: Instrumentation> From<I> for DynInstrumentation {
    fn from(instrumentation: I) -> Self {
        Self {
            inner: Some(unpack_instrumentation(Box::new(instrumentation))),
            no_instrumentation: NoInstrumentation,
        }
    }
}

struct NoInstrumentation;

impl Instrumentation for NoInstrumentation {
    fn on_connection_event(&mut self, _: InstrumentationEvent<'_>) {}
}

/// Unwrap unnecessary boxing levels
fn unpack_instrumentation(
    mut instrumentation: Box<dyn Instrumentation>,
) -> Box<dyn Instrumentation> {
    loop {
        match instrumentation.downcast::<Box<dyn Instrumentation>>() {
            Ok(extra_boxed_instrumentation) => instrumentation = *extra_boxed_instrumentation,
            Err(not_extra_boxed_instrumentation) => {
                break not_extra_boxed_instrumentation;
            }
        }
    }
}
Repository: diesel-rs/diesel
File: diesel/src/pg/expression/operators.rs (Rust)

Code before:
    }
}

#[derive(Debug, Clone, Copy, QueryId, ValidGrouping)]
#[doc(hidden)]
pub struct UnionRange<L, R> {
    pub(crate) left: L,
    pub(crate) right: R,
}

impl<L, R> UnionRange<L, R> {
    pub fn new(left: L, right: R) -> Self {
        Self {
            left,
            right,
        }
    }
}

impl<L, R, ST> crate::expression::Expression for UnionRange<L, R>
where
    L: crate::expression::Expression<SqlType = Range<ST>>,
    R: crate::expression::Expression<SqlType = Range<ST>>,
    ST: SqlType + TypedExpressionType,
{
    type SqlType = ST;
}

impl_selectable_expression!(UnionRange<L, R>);

impl<L, R> QueryFragment<Pg> for UnionRange<L, R>
where
    L: QueryFragment<Pg>,
    R: QueryFragment<Pg>,
{
    fn walk_ast<'b>(
        &'b self,
        mut out: crate::query_builder::AstPass<'_, 'b, Pg>,
    ) -> crate::result::QueryResult<()> {
        out.push_sql("(");
        self.left.walk_ast(out.reborrow())?;
        out.push_sql(" + ");
        self.right.walk_ast(out.reborrow())?;
        out.push_sql(")");
        Ok(())
    }
}



Review comment (suggestion):
That's the problematic line that's causing the error.

You specify here that `range_a.union_range(range_b)` return the inner type of the range instead. You want `type SqlType = Range<ST>` instead, which just says that the operator returns a range as well.

That written: You should likely use the `infix_operator!()` macro instead to generate all this code for you. It should be much easier that way.

Code after:
where
    L: Column,
{
    type Table = <L as Column>::Table;
    type QueryAstNode = ArrayIndex<UncorrelatedColumn<L>, R>;

    fn into_target(self) -> Self::QueryAstNode {
        ArrayIndex::new(UncorrelatedColumn(self.array_expr), self.index_expr)
    }
}

/// Signifies that this column should be rendered without its 'correlation'
/// (i.e. table name prefix). For update statements, fully qualified column
/// names aren't allowed.
#[derive(Debug)]
pub struct UncorrelatedColumn<C>(C);

impl<C, DB> QueryFragment<DB> for UncorrelatedColumn<C>
where
    C: Column,
    DB: Backend + DieselReserveSpecialization,
{
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, DB>) -> QueryResult<()> {
        out.push_identifier(C::NAME)
    }
}
Repository: diesel-rs/diesel
File: diesel/src/pg/expression/functions.rs (Rust)

Code before:
    #[sql_name = "array_to_string"]
    /// Converts each array element to its text representation, and concatenates those separated by the delimiter string.
    /// This variant omits the `null_str` argument.
    /// # Example
    ///
    /// ```rust
    /// # include!("../../doctest_setup.rs");
    /// #
    /// # fn main() {
    /// #     run_test().unwrap();
    /// # }
    /// #
    /// # fn run_test() -> QueryResult<()> {
    /// #     use diesel::dsl::array_to_string;
    /// #     let conn = &mut establish_connection();
    ///
    /// let result = diesel::select(array_to_string::<Array<Text>, Text>(
    ///     vec!["a".to_string(), "b".to_string(), "c".to_string()],
    ///     ", ".to_string()
    /// )).get_result::<String>(conn)?;
    /// assert_eq!("a, b, c".to_string(), result);
    ///
    /// #     Ok(())
    /// # }
    /// ```
    fn array_to_string<Arr: ArrayOrNullableArray<Inner=Elem> + SingleValue, Elem: SingleValue>(
        arr: Arr,
        delim: Text
    ) -> Text;
}


Review comment (suggestion):
This bound can be simplified to
```suggestion
    fn array_to_string<Arr: ArrayOrNullableArray>(
```

We don't use `Elem` anywhere.

Code after:

#[cfg(feature = "postgres_backend")]
define_sql_function! {
    /// Prepends an element to the beginning of an array
    ///
    /// # Example
    ///
    /// ```rust
    /// # include!("../../doctest_setup.rs");
    /// #
    /// # fn main() {
    /// #     run_test().unwrap();
    /// # }
    /// #
    /// # fn run_test() -> QueryResult<()> {
    /// #     use diesel::dsl::array_prepend;
    /// #     use diesel::sql_types::{Nullable, Integer, Array};
    /// #     let connection = &mut establish_connection();
    /// let ints = diesel::select(array_prepend::<Integer, Array<_>, _, _>(3, vec![1, 2]))
    ///     .get_result::<Vec<i32>>(connection)?;
    /// assert_eq!(vec![3, 1, 2], ints);
    ///
    /// let ints = diesel::select(array_prepend::<Nullable<Integer>, Array<_>, _, _>(None::<i32>, vec![Some(1), Some(2)]))
    ///     .get_result::<Vec<Option<i32>>>(connection)?;
    /// assert_eq!(vec![None, Some(1), Some(2)], ints);
    ///
    /// let ints = diesel::select(array_prepend::<Integer, Nullable<Array<_>>, _, _>(3, None::<Vec<i32>>))
    ///     .get_result::<Vec<i32>>(connection)?;
    /// assert_eq!(vec![3], ints);
    ///
    /// let ints = diesel::select(array_prepend::<Nullable<Integer>, Nullable<Array<_>>, _, _>(None::<i32>, None::<Vec<i32>>))
    ///     .get_result::<Vec<Option<i32>>>(connection)?;
    /// assert_eq!(vec![None], ints);
    /// #     Ok(())
    /// # }
    /// ```
    fn array_prepend<T: SingleValue, Arr: ArrayOrNullableArray<Inner=T> + SingleValue>(e: T, a: Arr) -> Array<T>;
}

#[cfg(feature = "postgres_backend")]
define_sql_function! {
    /// Removes all elements equal to the given value from the array
    ///
    /// # Example
    ///
    /// ```rust
    /// # include!("../../doctest_setup.rs");
    /// #
    /// # fn main() {
    /// #     run_test().unwrap();
    /// # }
Repository: diesel-rs/diesel
File: diesel/src/query_builder/combination_clause.rs (Rust)

Code before:

impl<Combinator, Rule, Source, Rhs> CombinationClause<Combinator, Rule, Source, Rhs> {
    /// Create a new combination
    pub(crate) fn new(
        combinator: Combinator,
        duplicate_rule: Rule,
        source: Source,
        rhs: Rhs,
    ) -> Self {
        CombinationClause {
            combinator,
            duplicate_rule,
            source: ParenthesisWrapper(source),
            rhs: ParenthesisWrapper(rhs),
            limit_offset: LimitOffsetClause {
                limit_clause: NoLimitClause,
                offset_clause: NoOffsetClause,
            },
        }
    }
}

impl<Combinator, Rule, Source, Rhs, LOf>
    CombinationClause<Combinator, Rule, Source, Rhs, LOf>
{
    pub(crate) fn new_full(
        combinator: Combinator,
        duplicate_rule: Rule,
        source: Source,
        rhs: Rhs,
        limit_offset: LOf,
    ) -> Self {
        CombinationClause {
            combinator,
            duplicate_rule,
            source: ParenthesisWrapper(source),
            rhs: ParenthesisWrapper(rhs),
            limit_offset,
        }
    }
}

impl<Combinator, Rule, Source, Rhs, LOf> QueryDsl
    for CombinationClause<Combinator, Rule, Source, Rhs, LOf> {}

impl<Combinator, Rule, Source, Rhs, LOf> QuerySource
    for CombinationClause<Combinator, Rule, Source, Rhs, LOf>
where
    Combinator: AsQuerySource,
    <Combinator::QuerySource as QuerySource>::DefaultSelection: SelectableExpression<Self>,
{

Review comment (refactor):
I could remove this method and change `new` instead, but then I would need to propagate the LimitOffsetClause declaration to each call site, which happens in several files, so I'm not entirely sure about that and I'm looking for feedback on it.

The method name is subpar :/

Code after:
    pub(crate) fn new(
        combinator: Combinator,
        duplicate_rule: Rule,
        source: Source,
        rhs: Rhs,
    ) -> Self {
        CombinationClause {
            combinator,
            duplicate_rule,
            source: ParenthesisWrapper(source),
            rhs: ParenthesisWrapper(rhs),
            limit_offset: LimitOffsetClause {
                limit_clause: NoLimitClause,
                offset_clause: NoOffsetClause,
            },
        }
    }
}

impl<Combinator, Rule, Source, Rhs, LOf> QueryDsl
    for CombinationClause<Combinator, Rule, Source, Rhs, LOf>
{
}

impl<Combinator, Rule, Source, Rhs, LOf> Query
    for CombinationClause<Combinator, Rule, Source, Rhs, LOf>
where
    Source: Query,
    Rhs: Query<SqlType = Source::SqlType>,
{
    type SqlType = Source::SqlType;
}

impl<Combinator, Rule, Source, Rhs, LOf> SelectQuery
    for CombinationClause<Combinator, Rule, Source, Rhs, LOf>
where
    Source: SelectQuery,
    Rhs: SelectQuery<SqlType = Source::SqlType>,
{
    type SqlType = Source::SqlType;
}

impl<Combinator, Rule, Source, Rhs, LOf, QS> ValidSubselect<QS>
    for CombinationClause<Combinator, Rule, Source, Rhs, LOf>
where
    Source: ValidSubselect<QS>,
    Rhs: ValidSubselect<QS>,
{
}

impl<Combinator, Rule, Source, Rhs, LOf, Conn> RunQueryDsl<Conn>
Repository: diesel-rs/diesel
File: diesel/src/pg/expression/functions.rs (Rust)

Code before:
    /// assert_eq!(r#""abc""#, result);
    ///
    /// let result = diesel::select(jsonb_pretty::<Jsonb, _>(json!(22)))
    ///     .get_result::<String>(connection)?;
    ///
    /// assert_eq!(r#"22"#, result);
    ///
    /// let result = diesel::select(jsonb_pretty::<Jsonb, _>(json!(false)))
    ///     .get_result::<String>(connection)?;
    ///
    /// assert_eq!(r#"false"#, result);
    ///
    /// let result = diesel::select(jsonb_pretty::<Jsonb, _>(json!(null)))
    ///     .get_result::<String>(connection)?;
    ///
    /// assert_eq!(r#"null"#, result);
    ///
    /// let result = diesel::select(jsonb_pretty::<Jsonb, _>(json!({})))
    ///     .get_result::<String>(connection)?;
    ///
    /// assert_eq!(r#"{
    /// }"#, result);
    /// #     Ok(())
    /// # }
    /// ```
    fn jsonb_pretty<E: JsonbOrNullableJsonb + SingleValue>(e: E) -> Text;
}


Review comment (suggestion):
```suggestion
    fn jsonb_pretty<E: JsonbOrNullableJsonb + MaybeNullableValue<Text> + SingleValue>(e: E) -> E::Out;
```

to correctly handle null value inputs.

See the longer explanation [here](https://github.com/diesel-rs/diesel/pull/4229) how this works. (You are not expected to know this on your own, I just don't want to copy this comment everywhere)

Code after:
    /// assert_eq!("boolean".to_string(), result);
    ///
    /// let result = diesel::select(json_typeof::<Json, _>(json!(null)))
    ///     .get_result::<String>(connection)?;
    ///
    /// assert_eq!("null".to_string(), result);
    ///
    /// let result = diesel::select(json_typeof::<Nullable<Json>, _>(None::<Value>))
    ///     .get_result::<Option<String>>(connection)?;
    ///
    /// assert!(result.is_none());
    /// #     Ok(())
    /// # }
    /// ```
    fn json_typeof<E: JsonOrNullableJson + SingleValue + MaybeNullableValue<Text>>(e: E) -> E::Out;
}

#[cfg(feature = "postgres_backend")]
define_sql_function! {
    /// Returns the type of the top-level jsonb value as a text-string
    ///
    /// # Example
    ///
    /// ```rust
    /// # include!("../../doctest_setup.rs");
    /// #
    /// # fn main() {
    /// #     #[cfg(feature = "serde_json")]
    /// #     run_test().unwrap();
    /// # }
    /// #
    /// # #[cfg(feature = "serde_json")]
    /// # fn run_test() -> QueryResult<()> {
    /// #     use diesel::dsl::jsonb_typeof;
    /// #     use serde_json::{json, Value};
    /// #     use diesel::sql_types::{Jsonb, Nullable};
    /// #     let connection = &mut establish_connection();
    /// let result = diesel::select(jsonb_typeof::<Jsonb, _>(json!({"a": "b", "c": 1})))
    ///     .get_result::<String>(connection)?;
    ///
    /// assert_eq!("object".to_string(), result);
    ///
    /// let result = diesel::select(jsonb_typeof::<Jsonb, _>(json!([1,2,3])))
    ///     .get_result::<String>(connection)?;
    ///
    /// assert_eq!("array".to_string(), result);
    ///
    /// let result = diesel::select(jsonb_typeof::<Jsonb, _>(json!("abc")))
    ///     .get_result::<String>(connection)?;
    ///
    /// assert_eq!("string".to_string(), result);
Repository: diesel-rs/diesel
File: diesel/src/deserialize.rs (Rust)

Code before:
    DB: Backend,
    T: FromStaticSqlRow<ST, DB>,
    ST: SingleValue,
{
    type Row = Self;

    fn build(row: Self::Row) -> Self {
        row
    }
}*/

impl<T, ST, DB> StaticallySizedRow<ST, DB> for T
where
    ST: SqlTypeOrSelectable + crate::util::TupleSize,
    T: Queryable<ST, DB>,
    DB: Backend,
{
    const FIELD_COUNT: usize = <ST as crate::util::TupleSize>::SIZE;
}

/// A helper trait for giving a type a useful default value.
///
/// This is needed for types that can be used as range to represent the empty range as
/// (Bound::Excluded(DEFAULT), Bound::Excluded(DEFAULT)).
/// When possible, implementations of this trait should fall back to using std::default::Default.
pub trait Defaultable {
    /// Returns the "default value" for a type.
    fn default_value() -> Self;
}

// We cannot have this impl because rustc
// then complains in third party crates that
// diesel may implement `Default`in the future.
// If we get negative trait impls at some point in time
// it should be possible to make this work.
//// Defaultable for types that has Default
//impl<T: Default> Defaultable for T {
//    fn default_value() -> Self {
//        T::default()
//    }
//}


Review comment (suggestion):
We do not want to make this a public trait for now, as this is just a workaround. I would rather prefer to go the way to keep this trait private and have the wild card impl below instead. In that way other crates can still support it by implementing `Default`.

Code after:
    DB: Backend,
    T: FromStaticSqlRow<ST, DB>,
    ST: SingleValue,
{
    type Row = Self;

    fn build(row: Self::Row) -> Self {
        row
    }
}*/

impl<T, ST, DB> StaticallySizedRow<ST, DB> for T
where
    ST: SqlTypeOrSelectable + crate::util::TupleSize,
    T: Queryable<ST, DB>,
    DB: Backend,
{
    const FIELD_COUNT: usize = <ST as crate::util::TupleSize>::SIZE;
}

/// A helper trait for giving a type a useful default value.
///
/// This is needed for types that can be used as range to represent the empty range as
/// (Bound::Excluded(DEFAULT), Bound::Excluded(DEFAULT)).
/// When possible, implementations of this trait should fall back to using std::default::Default.
#[allow(dead_code)]
pub(crate) trait Defaultable {
    /// Returns the "default value" for a type.
    fn default_value() -> Self;
}

// We cannot have this impl because rustc
// then complains in third party crates that
// diesel may implement `Default`in the future.
// If we get negative trait impls at some point in time
// it should be possible to make this work.
//// Defaultable for types that has Default
//impl<T: Default> Defaultable for T {
//    fn default_value() -> Self {
//        T::default()
//    }
//}
Repository: diesel-rs/diesel
File: diesel/src/pg/expression/functions.rs (Rust)

Code before:
    ///     // Example SQL query selecting a custom record type
    ///     let query = diesel::select(sql::<Record<(Text, Integer)>>("ROW('John', 30)::record"));
    ///
    ///     // Running the query and fetching the result as JSON
    ///     let (name, age): (String, i32) = query.get_result(connection)?;
    ///
    ///     // Create a Json Value
    ///     let json_value: Value = serde_json::json!({
    ///         "name": name,
    ///         "age": age
    ///     });
    ///
    ///     // Expected JSON output from the Record type
    ///     let expected: Value = serde_json::json!({
    ///         "name": "John",
    ///         "age": 30
    ///     });
    ///
    ///     // Asserting the result
    ///     assert_eq!(expected, json_value);
    ///
    ///     Ok(())
    /// # }
    /// ```
    #[sql_name = "jsonb_object"]
    fn row_to_json<R: RecordOrNullableRecord + SingleValue + CombinedNullableValue<Record<Text>, Jsonb>>(record: R) -> R::Out;
}


Review comment (suggestion):
I fear this function is somewhat problematic to implement with the current diesel types setup. It doesn't accept anonymous record types (as represented by the diesel `Record` type), but only rows with names. The later is something that's not yet expressible with diesels type setup. 

Given that I suppose this won't work in that way :(

(I also do not have any real suggestion how to make it easily work)

Code after:
    /// assert_eq!(None::<Value>,jsonb);
    ///
    /// let empty: Vec<String> = Vec::new();
    /// let jsonb = diesel::select(jsonb_object_with_keys_and_values::<Array<Text>, Array<Text>, _, _>(
    ///             vec!["hello","John"],empty))
    ///             .get_result::<Value>(connection);
    /// assert!(jsonb.is_err());
    ///
    /// #     Ok(())
    /// # }
    /// ```
    #[sql_name = "jsonb_object"]
    fn jsonb_object_with_keys_and_values<
        Arr1: TextArrayOrNullableTextArray + SingleValue,
        Arr2: TextArrayOrNullableTextArray + CombinedNullableValue<Arr1, Jsonb>
    >(
        keys: Arr1,
        values: Arr2
    ) -> Arr2::Out;
}

#[cfg(feature = "postgres_backend")]
define_sql_function! {
    /// This function `row_to_json` takes a Record type as an input and converts it to JSON.
    ///
    /// # Example
    ///
    /// ```rust
    /// # include!("../../doctest_setup.rs");
    /// #
    /// # fn main() {
    /// #     #[cfg(feature = "serde_json")]
    /// #     run_test().unwrap();
    /// # }
    /// #
    /// # #[cfg(feature = "serde_json")]
    /// # fn run_test() -> QueryResult<()> {
    /// #     use diesel::dsl::row_to_json;
    /// #     use diesel::dsl::sql;
    /// #     use diesel::sql_types::{Record, Text, Integer};
    /// #     use serde_json::Value;
    /// #     let connection = &mut establish_connection();
    ///
    /// let json_value = diesel::select(row_to_json(sql::<Record<(Text, Integer)>>(
    ///     "ROW('John', 30)"
    /// )))
    /// .get_result::<Value>(connection)?;
    /// let expected: Value = serde_json::json!({
    ///     "f1": "John",
    ///     "f2": 30
    /// });