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

Fix type resolution in rust-analyzer #3564

Merged
merged 1 commit into from
Mar 24, 2023

Conversation

weiznich
Copy link
Member

@weiznich weiznich commented Mar 24, 2023

It turns out that we relay on a type resolution hack for our derives that only work with rust 2015. That's not a problem with rustc, as it uses whatever edition a crate is using. So it will use rust-2015 for name resolution in the expansion of our proc macros, due to the fact that diesel_derives is a edition = 2015 crate. The problem is that rust-analyzer does not understand this, as they use basically the name resolution rules from the 2018 edition. (To be clear that's something that's not "standard" conform there, but it does not seem to be a priority to fix that).
As this affects our core type system (== all the SQL types, …), that has pretty serve effects on what users see in their IDE.

Fortunately the workaround is simple as we just need to use an alternative way to provide an diesel item in diesel itself to make our macros work there. It seems like both rustc and rust-analyzer agree on the extern crate self as diesel; solution.

After this basically all known cases of rust-analyzer issues are gone away (at least as far as I tested).

Before: image

After: image

It turns out that we relay on a type resolution hack for our derives
that only work with rust 2015. That's not a problem with rustc, as it
uses whatever edition a crate is using. So it will use rust-2015 for
name resolution in the expansion of our proc macros, due to the fact
that `diesel_derives` is a edition = 2015 crate. The problem is that
rust-analyzer does not understand this, as they use basically the name
resolution rules from the 2018 edition. (To be clear that's something
that's not "standard" conform there, but it does not seem to be a
priority to fix that).
As this affects our core type system (== all the SQL types, …), that has
pretty serve effects on what users see in their IDE.

Fortunately the workaround is simple as we just need to use an
alternative way to provide an diesel item in diesel itself to make our
macros work there. It seems like both rustc and rust-analyzer agree on
the `extern crate self as diesel;` solution.

After this basically all known cases of rust-analyzer issues are gone
away (at least as far as I tested).
@pksunkara pksunkara enabled auto-merge March 24, 2023 18:19
@pksunkara pksunkara added this pull request to the merge queue Mar 24, 2023
Merged via the queue into diesel-rs:master with commit 9ca35b1 Mar 24, 2023
@Ten0
Copy link
Member

Ten0 commented Mar 28, 2023

I can confirm this solves part of the issue but not all of the issues are resolved.
Essentially as soon as joins are involved it seems to lose track of the types:

#![allow(unreachable_code, unused)]

use diesel::{prelude::*, table};

table! {
	users {
		id -> Integer,
		name -> Text,
	}
}
table! {
	posts {
		id -> Integer,
		user_id -> Integer,
	}
}
joinable! { posts -> users (user_id) }
allow_tables_to_appear_in_same_query! { posts, users }

fn main() {
	let db: &mut diesel::pg::PgConnection = todo!();

	let p = posts::table.select(posts::id).load::<i32>(db).unwrap();

	let u = users::table
		.inner_join(posts::table)
		.select((users::name, posts::id))
		.load::<(String, i32)>(db)
		.unwrap();
}

gives (with in red the type inferences failing):

image

I suspect rust-lang/rust-analyzer#9881 may be the cause.

@weiznich
Copy link
Member Author

@Ten0 I fear we cannot do much about the inner join issue, if it's really related to that unimplemented chalk feature. That's then something that needs to be fixed in rust-analyzer/chalk.

@Ten0
Copy link
Member

Ten0 commented Mar 31, 2023

@Ten0 I fear we cannot do much about the inner join issue, if it's really related to that unimplemented chalk feature. That's then something that needs to be fixed in rust-analyzer/chalk.

Yup, was just mentioning it so that we don't spend more time investigating that thinking it was supposed to be fixed 🙂

After this basically all known cases of rust-analyzer issues are gone away (at least as far as I tested).

@ES1993
Copy link

ES1993 commented Dec 20, 2023

rustover ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants