Problem differentiating between built-in PartialEq trait and Diesel ExpressionMethods trait

[brijorn]

extern crate diesel;

use diesel::prelude::*;
use crate::{database, schema::recurring_log::dsl::*};
use database::models::*;
pub fn get_post() {

    let connection = database::connect_to_db();

    let results = recurring_log.filter(finished.eq(true))
    .limit(5)
    .load::<RecurringLog>(&connection)
    .expect("Error loading posts");
}

rust-analyzer says that eq() is not found but rustc does not show the same error.

[lnicola]

Can you post a full test case if it's not too much of a hassle?

[weiznich]

#[macro_use]
extern crate diesel; // 1.4.5

use diesel::prelude::*;

table! {
    recurring_log {
        id -> Integer,
        finished -> Bool,
    }
}

pub fn get_post() {
    use self::recurring_log::dsl::*;

    let _ = recurring_log.filter(finished.eq(true));
}

should be sufficient as minimal reproducing example.

[edwin0cheng]

@weiznich Can't reproduce it in master 0ac4a8f

1. Make sure you enabled :

{
    "rust-analyzer.cargo.loadOutDirsFromCheck": true,
    "rust-analyzer.procMacro.enable": true,
}

2. Local use item support is just implemented in recent commits, maybe you need to try it next monday if you are using stable channel.

[weiznich]

@edwin0cheng I've haven't tried to reproduce that by my self yet, we only got that report in our gitter channel, so I've forwarded it to the in my opinion right place to report it.

[lnicola]

Can reproduce at 92a6dcc36 and 0ac4a8f.

I spotted a couple of problems:

• there are some local imports, but they don't seem to matter
• we don't expand table! correctly
• we don't understand associated type projections
• "expand glob import" doesn't import traits?
• even after fixing the above, we still don't resolve eq

Somewhat minimized test case

use diesel::prelude::ExpressionMethods;
use diesel::query_builder::nodes::Identifier;
use diesel::query_builder::{AsQuery, QueryId, SelectStatement};
use diesel::query_source::{AppearsInFromClause, Never, Once};
use diesel::sql_types::Integer;
use diesel::{QuerySource, Table};

/// The actual table struct
///
/// This is the type which provides the base methods of the query
/// builder, such as `.select` and `.filter`.
pub struct table;

impl QueryId for table {
    type QueryId = table;
    const HAS_STATIC_QUERY_ID: bool = true;
}
/// The SQL type of all of the columns on this table
pub type SqlType = (Integer,);
impl QuerySource for table {
    type FromClause = Identifier<'static>;
    type DefaultSelection = (id,);
    fn from_clause(&self) -> Identifier<'static> {
        Identifier("recurring_log")
    }
    fn default_selection(&self) -> (id,) {
        Self::all_columns()
    }
}
impl AsQuery for table {
    type SqlType = SqlType;
    type Query = SelectStatement<Self>;
    fn as_query(self) -> SelectStatement<Self> {
        SelectStatement::simple(self)
    }
}
impl Table for table {
    type PrimaryKey = id;
    type AllColumns = (id,);
    fn primary_key(&self) -> id {
        id
    }
    fn all_columns() -> (id,) {
        (id,)
    }
}
impl AppearsInFromClause<table> for table {
    type Count = Once;
}
impl AppearsInFromClause<table> for () {
    type Count = Never;
}
/// Contains all of the columns of this table
pub mod columns {
    use super::table;
    use diesel::expression::{Expression, NonAggregate};
    use diesel::query_builder::QueryId;
    use diesel::query_source::Column;
    use diesel::sql_types::Integer;
    use diesel::{AppearsOnTable, SelectableExpression};

    pub struct id;

    impl QueryId for id {
        type QueryId = id;
        const HAS_STATIC_QUERY_ID: bool = true;
    }

    impl Expression for id {
        type SqlType = Integer;
    }

    impl SelectableExpression<table> for id {}
    impl<QS> AppearsOnTable<QS> for id {}
    impl NonAggregate for id {}
    impl Column for id {
        type Table = table;
        const NAME: &'static str = "id";
    }
}

use self::columns::id;

pub fn main() {
    id.eq(42);
}

[detrumi]

Note that this has nothing to do with PartialEq, it happens with all/most of diesel's ExpressionMethods.

The minimized example can't find the Eq type at all. That's probably because it's defined inside a macro.

[lnicola]

Next step would be to extract those macros and try to minimize them.

[weiznich]

Is there anything I can provide from diesels side here?

[edwin0cheng]

Can reproduce at 92a6dcc36 and 0ac4a8f.

It is weird that in my machine I dont' see any error :

[Update] Oh, I can reproduce now. Will take a look for the table! macro. It seem to able to expand but something is wrong.

[Update] Actually the table! macro expand correctly, but that eq trait is still not found.

[edwin0cheng]

Hm , I finally found what's happening. Here is a more compact example:

use diesel:prelude::ExpressionMethods;
use diesel::SqlType;

#[derive(SqlType)]
pub struct Kool {}

impl diesel::expression::Expression for Kool {
    type SqlType = Self;
}

pub struct Finished;
impl diesel::expression::Expression for Finished {
    type SqlType = Kool;
}

fn main() {    
    let _a = Finished.eq(Kool{});
}

The problem here is the derive(SqlType) , which expanded to :

pub struct Kool {}
#[allow(non_snake_case, unused_extern_crates, unused_imports)]
fn _impl_sql_type_for_kool() {
    extern crate std;
    use diesel;
    impl diesel::sql_types::NotNull for Kool {}
    impl diesel::sql_types::SingleValue for Kool {}
}
impl diesel::expression::Expression for Kool {
    type SqlType = Self;
}

Note that these impl are local. And we do not handle local impls such that we can't infer that eq to ExpressionMethods::eq (which assumed Kool has to implement SingleValue trait).

It seem like a lot of derive macros use this trick (at least serde do it too) to bypass hygiene limitation of proc-macro. But local trait impls for non-local types is very problematic for IDE perspective (see https://rust-analyzer.github.io/blog/2020/05/18/next-few-years.html#language-design-for-locality )
Maybe we should do something specific about this case ?

[weiznich]

@edwin0cheng I'm open to change this in diesel, as long as we have another way that allows us to import things without polluting the namespace where the derive is applied. This solution is just one that seems to work (at least for the last few years since the initial release of derive macros)

[edwin0cheng]

I'm open to change this in diesel, as long as we have another way that allows us to import things without polluting the namespace where the derive is applied.

@weiznich

1. Is it possible to change to use mod instead of fn :

pub struct Kool {}
#[allow(non_snake_case, unused_extern_crates, unused_imports)]
mod _impl_sql_type_for_kool {
    use super::*;
    extern crate std;
    use diesel;
    impl diesel::sql_types::NotNull for Kool {}
    impl diesel::sql_types::SingleValue for Kool {}
}
impl diesel::expression::Expression for Kool {
    type SqlType = Self;
}

2. I just checked that in the git master of diesel , it use an unnamed const for wrapping these impls:

pub fn wrap_in_dummy_mod(item: TokenStream) -> TokenStream {
    quote! {
        #[allow(unused_imports)]
        const _: () = {
            // This import is not actually redundant. When using diesel_derives
            // inside of diesel, `diesel` doesn't exist as an extern crate, and
            // to work around that it contains a private
            // `mod diesel { pub use super::*; }` that this import will then
            // refer to. In all other cases, this imports refers to the extern
            // crate diesel.
            use diesel;

            #item
        };
    }
}

I opened #7550 for discussion to support this usage as escape hatch, as workaround for lacking of proc-macro hygiene. Maybe you would interest to follow.

[lnicola]

Triage: this still happens with my test case from #7484 (comment).

[edwin0cheng]

Triage: this still happens with my test case from #7484 (comment).

Yeah, that's need to change it in their side (change to use mod) or implement #7550 to fix.

In your test case :

impl Expression for id {
        type SqlType = Integer;   <--- Integer itself is using SqlType macro to define
    }

[weiznich]

Now that diesel released a fix for the previously outlined issue (for quite some time already) I re-checked this issue and it continues to happen. I also spend some time trying to build a minimal reproducible example. Everyting below is with diesel = "=2.0.3"

I've used this code for testing (that's a heavily minimized version of what diesel::table! generates + a striped down version of some diesel parts):

mod mini_diesel {
    pub struct Integer;

    pub trait Expression {
        type SqlType;
    }

    impl diesel::sql_types::SqlType for Integer {
        type IsNull = diesel::sql_types::is_nullable::IsNullable;
    }
    impl diesel::sql_types::SingleValue for Integer {}

    pub type Eq<S, T> = (S, T);

    pub trait ExpressionMethods: Expression + Sized {
        fn eq<T>(self, _other: T) -> Eq<Self, T> {
            todo!()
        }
    }

    impl<T> ExpressionMethods for T
    where
        T: Expression,
        T::SqlType: diesel::sql_types::SingleValue,
    {
    }
}

use self::mini_diesel::*;

pub fn main() {
    use users::{works, broken};
    let _t = works.eq(42);
    let _t2 = ExpressionMethods::eq(works, 42);

    let _t3 = broken.eq(42);
    let _t4 = ExpressionMethods::eq(broken, 42);
}



pub mod users {
    pub use self::columns::{works, broken};


    #[allow(non_camel_case_types)]
    pub mod columns {
        pub struct works;

        impl crate::mini_diesel::Expression for works {
            type SqlType = crate::mini_diesel::Integer;
        }

        pub struct broken;

        impl crate::mini_diesel::Expression for broken {
            type SqlType = diesel::sql_types::Integer;
        }
    }
}

This code compiles using rustc. Rust-analyzer shows the correct types for _t, _t2, _t4. Personally I find it surprising that it shows the correct type for _t1 but not for _t3, as the mini_diesel Integer has exactly the same trait impls (for the relevant traits) as that one provided by diesel.

I've tried to completely remove the diesel dependency from this example, but that fails as soon as you try to extract the SingleValue trait including all possible relevant impls (there are only one generic impl + one for each sql type in diesel). Again that's rather surprising, as that's not a complicated pattern yet. In fact, removing the SingleValue bound from the ExpressionMethods impl results in rust-analyzer showing the correct type for _t3 as well.

I hope that brings this issue up to the table again, because we as diesel team get rather many reports that rust-analyzer does not show the correct types for some/most of the diesel types. It would be great to have a fix for that, or at least an indication how that situation can be improved. I specifically tried to look into this issue, because there is no complicated generic code involved here. All trait impls are rather simple and straight forward.

[Veykril]

The problem is that the expansion turns into:

#[allow(unused_imports)]
const _: () = {
    use diesel;
    impl diesel::sql_types::SqlType for Integer {
        type IsNull = diesel::sql_types::is_nullable::NotNull;
    }
    impl diesel::sql_types::SingleValue for Integer {}
};

yet diesel is not a thing sql-types/mod.rs (the module is defined in the crate root but not imported in sql-types/mod.rs) so I am surprised this even compiles, not sure what I am not seeing here

[weiznich]

That would explain why this works even if I just copy the #[derive(SqlType)] struct Integer; part into mini_diesel impl.

I think I can explain how the name resolution works there, because I broke it by accident today 🙈

So there is this reexport in diesel itself: https://github.com/diesel-rs/diesel/blob/dba995346b9b38984ea6f1129c2ea7193abfa0bc/diesel/src/lib.rs#L600
That by itself does not resolve probably the use diesel;, as that's not a valid name resolution in rust 2021 and rust 2018. But it is in rust 2015. Now diesel itself currently uses the 2018 edition, but diesel_derive uses the 2015 edition (or better it does not specify an edition at all, which means 2015 by default). I think these derive macros then just use the edition of the macro crate to determine the edition of the expanded code, which gives that code rust 2015 name resolution rules, which in turn resolves that correctly to the linked submodule.

Now I'm happy to change that, if that's the issue. Unfortunately I'm not aware of any other solution to use the same proc macros in both, the main crate and in third party crates due to the fact that there is just no $crate meta variable there.

[Veykril]

Oh that would explain it, r-a just assumes 2018 name resolution. I think you can do extern crate self as diesel in the diesel crate through

[weiznich]

That's a great idea. In fact using extern crate self as diesel; works and removes the issue. I've filled diesel-rs/diesel#3564 in diesel to resolve that. Thanks for the pointer 🙏

[Veykril]

I take it we can close the issue as solved then?

[weiznich]

Yes, it can be closed. The fix on diesel side will be released with the 2.1 feature release.