query!() is evaluated incorrectly/inconsistently with rust-analyser because of static caching

[zkrising]

I have found these related issues/pull requests

Not related to any other issue on this repo - sort of related to #121, but not really

Description

This issue happens in workspaces with sqlx, and does not happen in cargo check, but does affect rust-analyser.

You need two crates in your workspace, and those crates need to use a different database. For example:

crates/
  postgres_red/
    .env (DATABASE_URL=postgres://blah/red_database)
  postgres_yellow/
    .env (DATABASE_URL=postgres://blah/yellow_database)

Rust-Analyser will incorrectly evaluate query!() executions against the wrong database, the right database, or both databases.

According to the rust-analyser devs (where this was originally reported), this is the issue:
rust-lang/rust-analyzer#18533 (comment)

Thanks for the repro, that made me reinvestigate and I just noticed the issue.

Well the lookup happens here launchbadge/sqlx@4590b9c/sqlx-macros-core/src/query/mod.rs#L109-L118
So I am stumped as to what's happening here as your cargo invocation isn't really special in that regard then

I completely missed this when looking into this the last time, this static caching is the culprit. rust-analyzer does not unload proc-macros unlike rustc, so that lazy static is going to be initialized with the first expansion and as such any other expansion in differing crates will observe the Metadata for the first crate.

So unfortunately that's a wontfix from our side, we are required to cache proc-macro expansions for implementation results (I am not going into detail here as we have this discussion before and there is no budging on that stance at this point) and we can't unload proc-macro dylibs either. That needs changing in sqlx, ideally by not using a lazy static that depends on the environment in the first place or replace it with a thread local as right now we spawn a new thread per expansion to clear such context.

Reproduction steps

This repository will reproduce the issue: https://github.com/zkrising/ra-sqlx-repro

A reproduction is available here: https://github.com/zkrising/ra-sqlx-repro

Note that cargo check et. al. report no issues, but rust-analyser will. You might have to edit and save the main.rs files just to get rust-analyser to trip up, but I've found doing that pretty reliably gives you an error that shouldn't exist.

The gist of the repro is that there's sqlite_red which queries red_things and sqlite_yellow which queries yellow_things. Despite the crates working and compiling, rust-analyser will seemingly manifest an error that the table does not exist.

SQLx version

0.8

Enabled SQLx features

runtime-tokio, sqlite, but any driver has this issue

Database server and version

sqlite 3.48.0, but any works

Operating system

Linux

Rust version

rustc 1.83.0-nightly (55a22d2a6 2024-10-06)

[abonander]

Changing to a thread_local seems like a reasonable workaround. However, since they can be somewhat expensive to access, we should probably put all relevant state into a single context type and access that as early as possible.

[zkrising]

How much work do you think this would be? I'm utterly unfamilar with this part of the codebase and proc-macros in general but if this is as simple as replacing a global with a thread local I could probably do that this weekend.

This issue currently causes me a lot of grief so I'd be willing to step up and fix it.

[esmevane]

@abonander By collecting all the state do you mean turning METADATA into a hash map or something like that? If so, is this a bit like what you're thinking?

• Determine some convention to id each consumer (probably the name?)
• Load everything at once in the closure, keyed by above.
• Figure out a way to handle cases where MIGRATOR.get fails, that doesn't impact call sites too much?

[huuff]

Anyone has any workarounds they care to share for this? Currently, we're just pinning an old version of rust-analyzer, but that's becoming less and less convenient with time

[numfin]

if you accept any ideas - it would be nice to configure where value is coming from. For example:

lets call config file sqlx.config.toml

[macro]
connection_url = { source = "file", path = ".env", key = "DATABASE_URL" }
connection_url = { source = "cache", location = "path_to_cache_dir_or_folder_idk_specifics" }
connection_url = { source = "local", value = "postgres://..." }

[abonander]

@numfin #3383

[numfin]

@abonander oh wow. I haven't been developing in rust for half a year and look how sqlx have improved! <3
Thank you for linking the pr

[abonander]

Changing to a thread_local seems like a reasonable workaround. However, since they can be somewhat expensive to access, we should probably put all relevant state into a single context type and access that as early as possible.

Actually I don't think it has to be a thread_local, but perhaps a global hashmap keyed by CARGO_MANIFEST_DIR.

That also gives us future-proofing in the event macros start to be expanded in parallel. That doesn't seem to be in the cards for the current parallel frontend, but it seems like it'd be an easy perf win in macro-heavy code.

[huuff]

@abonander if I understand right.... it's possible that this can be worked around by sqlx.toml? We could just declare different DATABASE_URL env vars for different crates in the workspace and thus they won't conflict?

[abonander]

@huuff that would also be an option, yes.