ronantakizawa/codereview-bench huggingface dataset:
Repository: diesel-rs/diesel
File: diesel_derives/src/sql_function.rs (Rust)

Code before:
    colon_token: Token![:],
    ty: syn::Type,
}

impl Parse for StrictFnArg {
    fn parse(input: ParseStream) -> parse::Result<Self> {
        let name = input.parse()?;
        let colon_token = input.parse()?;
        let ty = input.parse()?;
        Ok(Self {
            name,
            colon_token,
            ty,
        })
    }
}

impl ToTokens for StrictFnArg {
    fn to_tokens(&self, tokens: &mut TokenStream) {
        self.name.to_tokens(tokens);
        self.colon_token.to_tokens(tokens);
        self.name.to_tokens(tokens);
    }
}

fn is_sqlite_return_type(ty: &syn::Type) -> bool {
    [
        "BigInt",
        "Binary",
        "Bool",
        "Date",
        "Double",
        "Float",
        "Integer",
        "Numeric",
        "SmallInt",
        "Text",
        "Time",
        "Timestamp",
    ]
    .contains(&quote!(#ty).to_string().as_str())
}


Review comment (suggestion):
As we want to use that functions on sql functions with arguments to check the type of the argument too, I would prefer to just name it `is_sqlite_type`.

Code after:
    colon_token: Token![:],
    ty: syn::Type,
}

impl Parse for StrictFnArg {
    fn parse(input: ParseStream) -> parse::Result<Self> {
        let name = input.parse()?;
        let colon_token = input.parse()?;
        let ty = input.parse()?;
        Ok(Self {
            name,
            colon_token,
            ty,
        })
    }
}

impl ToTokens for StrictFnArg {
    fn to_tokens(&self, tokens: &mut TokenStream) {
        self.name.to_tokens(tokens);
        self.colon_token.to_tokens(tokens);
        self.name.to_tokens(tokens);
    }
}

fn is_sqlite_type(ty: &syn::Type) -> bool {
    [
        "BigInt",
        "Binary",
        "Bool",
        "Date",
        "Double",
        "Float",
        "Integer",
        "Numeric",
        "SmallInt",
        "Text",
        "Time",
        "Timestamp",
    ]
    .contains(&quote!(#ty).to_string().as_str())
}
Repository: diesel-rs/diesel
File: diesel_derives/src/sql_function.rs (Rust)

Code before:
    fn parse(input: ParseStream) -> parse::Result<Self> {
        let name = input.parse()?;
        let colon_token = input.parse()?;
        let ty = input.parse()?;
        Ok(Self {
            name,
            colon_token,
            ty,
        })
    }
}

impl ToTokens for StrictFnArg {
    fn to_tokens(&self, tokens: &mut TokenStream) {
        self.name.to_tokens(tokens);
        self.colon_token.to_tokens(tokens);
        self.name.to_tokens(tokens);
    }
}

fn is_sqlite_type<T: ToTokens>(ty: &T) -> bool {
    let ty = quote!(#ty).to_string();
    let ty_stripped = ty
        .strip_prefix("Nullable < ")
        .and_then(|t| t.strip_suffix(" >"))
        .unwrap_or(ty.as_str());
    [
        "BigInt",
        "Binary",
        "Bool",
        "Date",
        "Double",
        "Float",
        "Integer",
        "Numeric",
        "SmallInt",
        "Text",
        "Time",
        "Timestamp",
    ]
    .contains(&ty_stripped)
}


Review comment (suggestion):
Aggregations can have a nullable return type. If there's a way to handle that case without string manipulation then plz let know.

Code after:
    fn parse(input: ParseStream) -> parse::Result<Self> {
        let name = input.parse()?;
        let colon_token = input.parse()?;
        let ty = input.parse()?;
        Ok(Self {
            name,
            colon_token,
            ty,
        })
    }
}

impl ToTokens for StrictFnArg {
    fn to_tokens(&self, tokens: &mut TokenStream) {
        self.name.to_tokens(tokens);
        self.colon_token.to_tokens(tokens);
        self.name.to_tokens(tokens);
    }
}

fn is_sqlite_type(ty: &syn::Type) -> bool {
    let last_segment = if let syn::Type::Path(tp) = ty {
        if let Some(segment) = tp.path.segments.last() {
            segment
        } else {
            return false;
        }
    } else {
        return false;
    };

    let ident = last_segment.ident.to_string();
    if ident == "Nullable" {
        if let syn::PathArguments::AngleBracketed(ref ab) = last_segment.arguments {
            if let Some(syn::GenericArgument::Type(ty)) = ab.args.first() {
                return is_sqlite_type(&ty);
            }
        }
        return false;
    }

    [
        "BigInt",
        "Binary",
        "Bool",
        "Date",
        "Double",
        "Float",
        "Integer",
        "Numeric",
        "SmallInt",
Repository: diesel-rs/diesel
File: diesel/src/expression/assume_not_null.rs (Rust)

Code before:
    pub fn new(expr: T) -> Self {
        AssumeNotNull(expr)
    }
}

impl<T> Expression for AssumeNotNull<T>
where
    T: Expression,
    T::SqlType: IntoNotNullable,
    <T::SqlType as IntoNotNullable>::NotNullable: TypedExpressionType,
{
    type SqlType = <T::SqlType as IntoNotNullable>::NotNullable;
}

impl<T, DB> QueryFragment<DB> for AssumeNotNull<T>
where
    DB: Backend,
    T: QueryFragment<DB>,
{
    fn walk_ast(&self, pass: AstPass<DB>) -> QueryResult<()> {
        self.0.walk_ast(pass)
    }
}

/// AssumeNotNull can be used in where clauses everywhere, but can only be used in
/// select clauses for outer joins.
impl<T, QS> AppearsOnTable<QS> for AssumeNotNull<T>
where
    T: AppearsOnTable<QS>,
    AssumeNotNull<T>: Expression,
{
}

impl<T: QueryId> QueryId for AssumeNotNull<T> {
    type QueryId = T::QueryId;

    const HAS_STATIC_QUERY_ID: bool = T::HAS_STATIC_QUERY_ID;
}

impl<T, QS> SelectableExpression<QS> for AssumeNotNull<T>
where
    Self: AppearsOnTable<QS>,
    QS: ToInnerJoin,
    T: SelectableExpression<QS::InnerJoin>,
{
}

impl<T> SelectableExpression<()> for AssumeNotNull<T> where Self: AppearsOnTable<()> {}


Review comment (suggestion):
Is there any reason for this restriction? I mean it's totally fine and valid to have something like this:
```rust
some_table::table.filter(some_table::some_nullable_column.is_not_null()).select((some_table::some_nullable_column.assume_not_null()).load(…)?
```

Code after:
    pub fn new(expr: T) -> Self {
        AssumeNotNull(expr)
    }
}

impl<T> Expression for AssumeNotNull<T>
where
    T: Expression,
    T::SqlType: IntoNotNullable,
    <T::SqlType as IntoNotNullable>::NotNullable: TypedExpressionType,
{
    type SqlType = <T::SqlType as IntoNotNullable>::NotNullable;
}

impl<T, DB> QueryFragment<DB> for AssumeNotNull<T>
where
    DB: Backend,
    T: QueryFragment<DB>,
{
    fn walk_ast(&self, pass: AstPass<DB>) -> QueryResult<()> {
        self.0.walk_ast(pass)
    }
}

impl<T, QS> AppearsOnTable<QS> for AssumeNotNull<T>
where
    T: AppearsOnTable<QS>,
    AssumeNotNull<T>: Expression,
{
}

impl<T: QueryId> QueryId for AssumeNotNull<T> {
    type QueryId = T::QueryId;

    const HAS_STATIC_QUERY_ID: bool = T::HAS_STATIC_QUERY_ID;
}

impl<T, QS> SelectableExpression<QS> for AssumeNotNull<T>
where
    Self: AppearsOnTable<QS>,
    QS: ToInnerJoin,
    T: SelectableExpression<QS::InnerJoin>,
{
}

impl<T> SelectableExpression<()> for AssumeNotNull<T> where Self: AppearsOnTable<()> {}
Repository: diesel-rs/diesel
File: diesel_derives/tests/as_changeset.rs (Rust)

Code before:
        #[diesel(column_name = hair_color)]
        color_de_pelo: &'a str,
    }

    let connection = &mut connection_with_sean_and_tess_in_users_table();

    update(users::table.find(1))
        .set(&UserForm {
            nombre: "Jim",
            color_de_pelo: "blue",
        })
        .execute(connection)
        .unwrap();

    let expected = vec![
        (1, String::from("Jim"), Some(String::from("blue"))),
        (2, String::from("Tess"), Some(String::from("brown"))),
    ];
    let actual = users::table.order(users::id).load(connection);
    assert_eq!(Ok(expected), actual);
}

#[test]
fn with_serialize_as() {
    #[derive(Debug, FromSqlRow, AsExpression)]
    #[sql_type = "sql_types::Text"]
    struct UppercaseString(pub String);

    impl Into<UppercaseString> for String {
        fn into(self) -> UppercaseString {
            UppercaseString(self.to_uppercase())
        }
    }

    impl<DB> serialize::ToSql<sql_types::Text, DB> for UppercaseString
    where
        DB: backend::Backend,
        String: serialize::ToSql<sql_types::Text, DB>,
    {
        fn to_sql<W: std::io::Write>(
            &self,
            out: &mut serialize::Output<W, DB>,
        ) -> serialize::Result {
            self.0.to_sql(out)
        }
    }

    #[derive(AsChangeset)]
    struct User {
        #[diesel(serialize_as = UppercaseString)]
        name: String,

Review comment (suggestion):
```suggestion
    #[diesel(sql_type = sql_types::Text)]
```

Code after:
        #[diesel(column_name = hair_color)]
        color_de_pelo: &'a str,
    }

    let connection = &mut connection_with_sean_and_tess_in_users_table();

    update(users::table.find(1))
        .set(&UserForm {
            nombre: "Jim",
            color_de_pelo: "blue",
        })
        .execute(connection)
        .unwrap();

    let expected = vec![
        (1, String::from("Jim"), Some(String::from("blue"))),
        (2, String::from("Tess"), Some(String::from("brown"))),
    ];
    let actual = users::table.order(users::id).load(connection);
    assert_eq!(Ok(expected), actual);
}

#[test]
fn with_serialize_as() {
    #[derive(Debug, FromSqlRow, AsExpression)]
    #[diesel(sql_type = sql_types::Text)]
    struct UppercaseString(pub String);

    impl Into<UppercaseString> for String {
        fn into(self) -> UppercaseString {
            UppercaseString(self.to_uppercase())
        }
    }

    impl<DB> serialize::ToSql<sql_types::Text, DB> for UppercaseString
    where
        DB: backend::Backend,
        String: serialize::ToSql<sql_types::Text, DB>,
    {
        fn to_sql<W: std::io::Write>(
            &self,
            out: &mut serialize::Output<W, DB>,
        ) -> serialize::Result {
            self.0.to_sql(out)
        }
    }

    #[derive(AsChangeset)]
    struct User {
        #[diesel(serialize_as = UppercaseString)]
        name: String,
Repository: diesel-rs/diesel
File: diesel_cli/src/infer_schema_internals/sqlite.rs (Rust)

Code before:
use std::error::Error;

use diesel::dsl::sql;
use diesel::*;

use super::data_structures::*;
use super::table_data::TableName;
use crate::print_schema::ColumnSorting;

table! {
    sqlite_master (name) {
        name -> VarChar,
    }
}

table! {
    pragma_table_info (cid) {
        cid ->Integer,
        name -> VarChar,
        type_name -> VarChar,
        notnull -> Bool,
        dflt_value -> Nullable<VarChar>,
        pk -> Bool,
        hidden -> Bool,
    }
}

table! {
    pragma_foreign_key_list {
        id -> Integer,
        seq -> Integer,
        _table -> VarChar,
        from -> VarChar,
        to -> VarChar,
        on_update -> VarChar,
        on_delete -> VarChar,
        _match -> VarChar,
    }
}

pub fn load_table_names(
    connection: &mut SqliteConnection,
    schema_name: Option<&str>,
) -> Result<Vec<TableName>, Box<dyn Error + Send + Sync + 'static>> {
    use self::sqlite_master::dsl::*;

    if schema_name.is_some() {
        return Err("sqlite cannot infer schema for databases other than the \
                    main database"

Review comment (suggestion):
I think this should be an `Integer` as this could be 0,1,2 or 3. This will also change any code referring to this column.

Code after:
use std::error::Error;

use diesel::dsl::sql;
use diesel::*;

use super::data_structures::*;
use super::table_data::TableName;
use crate::print_schema::ColumnSorting;

table! {
    sqlite_master (name) {
        name -> VarChar,
    }
}

table! {
    pragma_table_info (cid) {
        cid ->Integer,
        name -> VarChar,
        type_name -> VarChar,
        notnull -> Bool,
        dflt_value -> Nullable<VarChar>,
        pk -> Bool,
        hidden -> Integer,
    }
}

table! {
    pragma_foreign_key_list {
        id -> Integer,
        seq -> Integer,
        _table -> VarChar,
        from -> VarChar,
        to -> VarChar,
        on_update -> VarChar,
        on_delete -> VarChar,
        _match -> VarChar,
    }
}

pub fn load_table_names(
    connection: &mut SqliteConnection,
    schema_name: Option<&str>,
) -> Result<Vec<TableName>, Box<dyn Error + Send + Sync + 'static>> {
    use self::sqlite_master::dsl::*;

    if schema_name.is_some() {
        return Err("sqlite cannot infer schema for databases other than the \
                    main database"
Repository: diesel-rs/diesel
File: diesel/src/sqlite/connection/bind_collector.rs (Rust)

Code before:
use crate::query_builder::BindCollector;
use crate::serialize::{IsNull, Output};
use crate::sql_types::HasSqlType;
use crate::sqlite::{Sqlite, SqliteType};
use crate::QueryResult;

#[derive(Debug)]
pub struct SqliteBindCollector<'a> {
    pub(in crate::sqlite) binds: Vec<(InternalSqliteBindValue<'a>, SqliteType)>,
}

impl SqliteBindCollector<'_> {
    pub(in crate::sqlite) fn new() -> Self {
        Self { binds: Vec::new() }
    }
}

/// This type represents a value bound to
/// an sqlite prepared statement
///
/// It can be constructed via the various `From<T>` implemenations
#[derive(Debug)]
pub struct SqliteBindValue<'a> {
    pub(in crate::sqlite) inner: InternalSqliteBindValue<'a>,
}

impl<'a> From<i32> for SqliteBindValue<'a> {
    fn from(i: i32) -> Self {
        Self {
            inner: InternalSqliteBindValue::I32(i),
        }
    }
}

impl<'a> From<i64> for SqliteBindValue<'a> {
    fn from(i: i64) -> Self {
        Self {
            inner: InternalSqliteBindValue::I64(i),
        }
    }
}

impl<'a> From<f64> for SqliteBindValue<'a> {
    fn from(f: f64) -> Self {
        Self {
            inner: InternalSqliteBindValue::F64(f),

Review comment (suggestion):
```suggestion
/// It can be constructed via the various `From<T>` implementations
```

Code after:
use crate::query_builder::BindCollector;
use crate::serialize::{IsNull, Output};
use crate::sql_types::HasSqlType;
use crate::sqlite::{Sqlite, SqliteType};
use crate::QueryResult;

#[derive(Debug)]
pub struct SqliteBindCollector<'a> {
    pub(in crate::sqlite) binds: Vec<(InternalSqliteBindValue<'a>, SqliteType)>,
}

impl SqliteBindCollector<'_> {
    pub(in crate::sqlite) fn new() -> Self {
        Self { binds: Vec::new() }
    }
}

/// This type represents a value bound to
/// an sqlite prepared statement
///
/// It can be constructed via the various `From<T>` implementations
#[derive(Debug)]
pub struct SqliteBindValue<'a> {
    pub(in crate::sqlite) inner: InternalSqliteBindValue<'a>,
}

impl<'a> From<i32> for SqliteBindValue<'a> {
    fn from(i: i32) -> Self {
        Self {
            inner: InternalSqliteBindValue::I32(i),
        }
    }
}

impl<'a> From<i64> for SqliteBindValue<'a> {
    fn from(i: i64) -> Self {
        Self {
            inner: InternalSqliteBindValue::I64(i),
        }
    }
}

impl<'a> From<f64> for SqliteBindValue<'a> {
    fn from(f: f64) -> Self {
        Self {
            inner: InternalSqliteBindValue::F64(f),
Repository: diesel-rs/diesel
File: diesel/src/pg/expression/expression_methods.rs (Rust)

Code before:
    /// #     Ok(())
    /// # }
    /// # #[cfg(not(feature = "serde_json"))]
    /// # fn run_test() -> QueryResult<()> {
    /// #     Ok(())
    /// # }
    /// ```
    fn concat<T>(self, other: T) -> dsl::ConcatJsonb<Self, T>
    where
        T: AsExpression<Jsonb>,
    {
        Grouped(ConcatJsonb::new(self, other.as_expression()))
    }
}

impl<T> PgJsonbExpressionMethods for T
where
    T: Expression,
    T::SqlType: JsonB,
{
}
#[doc(hidden)]
/// Marker trait used to implement `PgJsonbExpressionMethods` on the appropriate types.
pub trait JsonB {}

impl JsonB for Jsonb {}


Review comment (suggestion):
We should allow nullable `jsonb` values here too. That's possible by the following change:

```suggestion
   T::SqlType: JsonbOrNullableJsonb,
{
}
#[doc(hidden)]
/// Marker trait used to implement `PgJsonbExpressionMethods` on the appropriate types.
pub trait JsonbOrNullableJsonb {}

impl JsonbOrNullableJsonb for Jsonb {}
impl JsonbOrNullableJsonb for Nullable<Jsonb> {}
```

Code after:
    /// #     Ok(())
    /// # }
    /// # #[cfg(not(feature = "serde_json"))]
    /// # fn run_test() -> QueryResult<()> {
    /// #     Ok(())
    /// # }
    /// ```
    fn concat<T>(self, other: T) -> dsl::ConcatJsonb<Self, T>
    where
        T: AsExpression<Jsonb>,
    {
        Grouped(ConcatJsonb::new(self, other.as_expression()))
    }
}

impl<T> PgJsonbExpressionMethods for T
where
    T: Expression,
    T::SqlType: JsonbOrNullableJsonb,
{
}
#[doc(hidden)]
/// Marker trait used to implement `PgJsonbExpressionMethods` on the appropriate types.
pub trait JsonbOrNullableJsonb {}
impl JsonbOrNullableJsonb for Jsonb {}
impl JsonbOrNullableJsonb for Nullable<Jsonb> {}
Repository: diesel-rs/diesel
File: diesel/src/type_impls/date_and_time.rs (Rust)

Code before:
    struct NaiveDateProxy(NaiveDate);

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Time)]
    struct NaiveTimeProxy(NaiveTime);

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Timestamp)]
    #[cfg_attr(
        feature = "postgres_backend",
        diesel(sql_type = crate::sql_types::Timestamptz)
    )]
    #[cfg_attr(feature = "mysql_backend", diesel(sql_type = crate::sql_types::Datetime))]
    // TODO: Add TimestamptzSqlite stuff here??
    struct NaiveDateTimeProxy(NaiveDateTime);

    #[derive(FromSqlRow)]
    #[diesel(foreign_derive)]
    #[cfg_attr(
        feature = "postgres_backend",
        derive(AsExpression),
        diesel(sql_type = crate::sql_types::Timestamptz)
    )]
    #[cfg_attr(feature = "sqlite", derive(AsExpression), diesel(sql_type = crate::sql_types::TimestamptzSqlite))]
    struct DateTimeProxy<Tz: TimeZone>(DateTime<Tz>);
}


Review comment (refactor):
This should be handled in a similar manner than the `AsExpression` derive for `NaiveDateTimeProxy` above. That means the `derive` itself should not be behind a `#[cfg()]` flag as this cases duplicated impls for cases where both backends are enabled due to the fact that rustc then sees the same derive two times.

Code after:
    struct NaiveDateProxy(NaiveDate);

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Time)]
    struct NaiveTimeProxy(NaiveTime);

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Timestamp)]
    #[cfg_attr(
        feature = "postgres_backend",
        diesel(sql_type = crate::sql_types::Timestamptz)
    )]
    #[cfg_attr(feature = "mysql_backend", diesel(sql_type = crate::sql_types::Datetime))]
    struct NaiveDateTimeProxy(NaiveDateTime);

    #[derive(FromSqlRow)]
    #[diesel(foreign_derive)]
    #[cfg_attr(
        any(feature = "postgres_backend", feature = "sqlite"),
        derive(AsExpression)
    )]
    #[cfg_attr(
        feature = "postgres_backend",
        diesel(sql_type = crate::sql_types::Timestamptz)
    )]
    #[cfg_attr(feature = "sqlite", diesel(sql_type = crate::sql_types::TimestamptzSqlite))]
    struct DateTimeProxy<Tz: TimeZone>(DateTime<Tz>);
}
Repository: diesel-rs/diesel
File: diesel/src/type_impls/date_and_time.rs (Rust)

Code before:
    use crate::sql_types::{Date, Time, Timestamp};

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Date)]
    struct NaiveDateProxy(NaiveDate);

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Time)]
    struct NaiveTimeProxy(NaiveTime);

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Timestamp)]
    #[cfg_attr(
        feature = "postgres_backend",
        diesel(sql_type = crate::sql_types::Timestamptz)
    )]
    #[cfg_attr(feature = "mysql_backend", diesel(sql_type = crate::sql_types::Datetime))]
    #[cfg_attr(feature = "sqlite", diesel(sql_type = crate::sql_types::TimestamptzSqlite))]
    struct NaiveDateTimeProxy(NaiveDateTime);

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Timestamp)]
    #[cfg_attr(
        feature = "postgres_backend",
        diesel(sql_type = crate::sql_types::Timestamptz)
    )]
    #[cfg_attr(feature = "sqlite", diesel(sql_type = crate::sql_types::TimestamptzSqlite))]
    struct DateTimeProxy<Tz: TimeZone>(DateTime<Tz>);
}


Review comment (suggestion):
We should not allow converting a chrono value with timezone to a `Timestamp` sql value without timestamp.  (Just remove that line)

Code after:
    use crate::sql_types::{Date, Time, Timestamp};

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Date)]
    struct NaiveDateProxy(NaiveDate);

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Time)]
    struct NaiveTimeProxy(NaiveTime);

    #[derive(AsExpression, FromSqlRow)]
    #[diesel(foreign_derive)]
    #[diesel(sql_type = Timestamp)]
    #[cfg_attr(
        feature = "postgres_backend",
        diesel(sql_type = crate::sql_types::Timestamptz)
    )]
    #[cfg_attr(feature = "mysql_backend", diesel(sql_type = crate::sql_types::Datetime))]
    struct NaiveDateTimeProxy(NaiveDateTime);

    #[derive(FromSqlRow)]
    #[diesel(foreign_derive)]
    #[cfg_attr(
        any(feature = "postgres_backend", feature = "sqlite"),
        derive(AsExpression)
    )]
    #[cfg_attr(
        feature = "postgres_backend",
        diesel(sql_type = crate::sql_types::Timestamptz)
    )]
    #[cfg_attr(feature = "sqlite", diesel(sql_type = crate::sql_types::TimestamptzSqlite))]
    struct DateTimeProxy<Tz: TimeZone>(DateTime<Tz>);
}