Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some diesel trait resolution remains broken #14607

Open
weiznich opened this issue Apr 18, 2023 · 15 comments
Open

Some diesel trait resolution remains broken #14607

weiznich opened this issue Apr 18, 2023 · 15 comments
Labels
A-ty type system / type inference / traits / method resolution C-bug Category: bug S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work.

Comments

@weiznich
Copy link
Contributor

rust-analyzer version: rust-analyzer 0.3.1481-standalone (I've downloaded this release)

rustc version: rustc 1.68.0 (2c8cc3432 2023-03-06)

relevant settings: None non-standard settings

Rust analyzer fails to infer some diesel trait bounds even after applying a workaround for the previous name resolution bug. It seems to correctly pick up some of the diesel provided traits, but fails on others. It's unclear why some work, while others remain broken even if they use similar implementation patterns.
For example this impl in diesel works, while this won't work. I'm happy to provide more information here if someone points out what might be helpful.

This results in no type hints being shown and no completions being suggested after one of the failing trait methods is used.

Code to reproduce the issue:

[dependencies]
# this version already contains the workaround for the rust-analyzer name resolution "bug"
diesel = { version = "=2.0.4", features = ["sqlite"], default-features = false }
use diesel::prelude::*;

table! {
    users {
        id -> Integer,
        name -> Text,
    }
}

table! {
    posts {
        id -> Integer,
        user_id -> Integer,
    }
}

allow_tables_to_appear_in_same_query!(users, posts);
joinable!(posts -> users (user_id));

#[derive(Selectable, Queryable)]
struct User {
    id: i32,
}

fn main() {
    let conn = &mut SqliteConnection::establish("_").unwrap();

    // these QueryDsl methods work
    let u = users::table
        .limit(10)
        .offset(15)
        .distinct()
        .order_by(users::id)
        .group_by(users::id)
        .then_order_by(users::name)
        .having(users::id.eq(42))
        .select(users::id)
        .load::<i32>(conn);

    // these queries don't work
    let u = users::table
        .inner_join(posts::table)
        .load::<((i32, String), (i32, i32))>(conn);

    let u = users::table
        .left_join(posts::table)
        .load::<((i32, String), Option<(i32, i32)>)>(conn);

    let u = users::table
        .filter(users::id.eq(42))
        .load::<(i32, String)>(conn);

    let u = users::table
        .find(42)
        .load::<(i32, String)>(conn);

    let u = users::table.select(User::as_select()).load(conn);

}

Inferred Types:
image

@weiznich weiznich added the C-bug Category: bug label Apr 18, 2023
@lowr lowr added the A-ty type system / type inference / traits / method resolution label Apr 20, 2023
bors added a commit that referenced this issue Apr 22, 2023
fix: Resolve `$crate` in derive paths

Paths in derive meta item list may contain any kind of paths, including those that start with `$crate` generated by macros. We need to take hygiene into account when we lower paths in the list.

This issue was identified while investigating #14607, though this patch doesn't fix the broken trait resolution.
@lowr
Copy link
Contributor

lowr commented Apr 22, 2023

One of the ultimate causes of this bug is same as #14369, for which I submitted rust-lang/chalk#792. Because of this, chalk fails to normalize <posts::table as diesel::JoinTo<users::table>>::OnClause and other similar projections that are generated by the joinable! macro.

Note that rust-lang/chalk#792 only fixes some issues and introduces a few new type mismatches (see the image below taken with my custom-built rust-analyzer with the chalk patch applied). I had analyzed this regression before submitting the patch, and it's not because the patch is wrong, I believe, but because it uncovers another consequence of rust-lang/chalk#584. I'd rather not write in details on this as it's complicated, though I can provide more info if necessary.

It's really not human friendly, but here's a gist with chalk's debug log I analyzed if anyone's interested.

230422-diesel-trait-res

@boehs
Copy link

boehs commented Apr 23, 2023

Is there a version of rust analyzer where this works?

@AJTJ
Copy link

AJTJ commented Jun 21, 2023

I would like to bump this, as having to estimate types in Diesel is rather frustrating.

@AJTJ
Copy link

AJTJ commented Jun 21, 2023

@weiznich I see that you say this current bug is rust-analyzer's issue here:
diesel-rs/diesel@74f301b
And you mention that this is because of rust-analyzer lacking compatibility with the 2015 name style resolution here:
#12589 (comment)

If you are confident this is the case, can we link to that issue/missing feature here?

@Veykril
Copy link
Member

Veykril commented Jun 21, 2023

The remaining resolution errors here are not due to the 2015 name resolution being missing in r-a as diesel no longer uses that. There are some other type inference problems in r-a here that cause the remaining ones.

@AJTJ
Copy link

AJTJ commented Jun 24, 2023

Welp, I don't think I'm doing this wrong, but there are times when I'm getting absolutely no help.

@Veykril do we have an idea what's wrong? Can I help?

Screenshot 2023-06-23 at 7 10 16 PM

@Veykril
Copy link
Member

Veykril commented Jun 24, 2023

lowr pointed some of the issues out #14607 (comment)

@Ddystopia
Copy link
Contributor

Method run_pending_migrations is also not working:

image

@aminya
Copy link

aminya commented Mar 13, 2024

The find method also doesn't seem to work. rust-analyzer complains that I can't call .execute on the Output of find, while it works with the Rust compiler.

@Veykril Veykril added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. label Mar 14, 2024
@JasonBoyett
Copy link

For some reason my rust analyzer works fine with diesel on my MacBook but I run into issues when I try to use it on pop os.

@opsnull

This comment was marked as off-topic.

@AJTJ
Copy link

AJTJ commented Aug 26, 2024

@weiznich any updates on this? Anywhere I can help?

@Veykril
Copy link
Member

Veykril commented Aug 26, 2024

This is blocked on the new trait solver integration, there isn't anything we can do right now

@rust-lang rust-lang locked and limited conversation to collaborators Aug 26, 2024
@rust-lang rust-lang unlocked this conversation Oct 13, 2024
@weiznich
Copy link
Contributor Author

From diesel's point of view there are two QueryDSL methods broken here: filter/find and left_join/inner_join.

At least for the filter/find methods there exists the following workaround:

Instead of this:

let result = users::table.filter(users::id.eq(42)).load::<User>(conn)?;

write this:

let mut user_query = users::table.into_boxed();

user_query = user_query.filter(users::id.eq(42));

let result = user_query.load::<User>(conn)?;

The important parts here are:

  • Boxing the query via .into_boxed()
  • Storing the boxed query in a variable
  • Reassign the variable after applying filter or find

That workarounds the above issue as rust-analyzer is able to infer the type of the boxed query at the first assign. Afterwards it can assume that the type stays the same even if it cannot infer the correct type of the filter/find expression.

I'm not aware of any workaround for the left_join/inner_join methods yet.

I would assume that if someone wants to help resolving both cases for real the best way forward is to build a minimal example that triggers the same behavior. That would mean you would start with the latest diesel version and strip out as much stuff as possible so that the issue still reproduces. See the following blog post for general strategies how to do that.

@realtica
Copy link

rust-analyzer version: v0.3.2154 (latest)
rustc version: 1.80.1
editor or extension: nvim v0.10

The latest version the inference is ok for tables.., but it returns a let site: {unknown} , and what about the ORM functions like Comment::belonging_to() ? (inference for filter fails)
Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ty type system / type inference / traits / method resolution C-bug Category: bug S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Development

No branches or pull requests

10 participants