fix(macros-core): more consistent env loading and reading logic #4018
Conversation
While giving the new `sqlx.toml` feature a try, I discovered that its `database_url_var` setting behaved inconsistently compared to other environment variables: it wasn't loaded from `.env` files, causing it to be effectively ignored in favor of the default `DATABASE_URL` variable. This is an inconvenient outcome for the multi-database workspaces that this feature is designed to support. To address this issue, I reworked the `.env` loading logic to account for this configuration and make it simpler to use to achieve consistent behavior. The new algorithm works as follows: - First, it generates lists of environment variables that can be loaded from `.env` files and candidate `.env` file paths. When applicable, the compiler is informed to track changes to their elements. - Next, it loads the set of potentially tracked environment variables using `set_var`, similar to how `dotenvy::dotenv()` operates. When a variable is defined in both the process environment and a `.env` file, the process environment takes precedence, as before. - Macro code can now use the `env` function freely to read environment variable values, abstracting itself away from their source, which results in simpler, less error-prone code. Trivially, this rework resolves the issue I encountered because the `database_url_var` value is now part of the list of loadable environment variables. Future code can easily make such additions as necessary.
AlexTMjugador
force-pushed
the
fix/consistent-macro-env-handling
branch
from
a8a9e0c to
89858d7
Compare
| static LOADED_ENV_VARS: Mutex<HashMap<String, String, BuildHasherDefault<DefaultHasher>>> = | ||
| Mutex::new(HashMap::with_hasher(BuildHasherDefault::new())); |
There was a problem hiding this comment.
We can't actually use a single global static here, because of Rust-Analzyer. RA will load the proc-macro dylib into its process and keep it resident for all compiler invocations, so we can't assume the state is reset between different crates.
This was the cause of #3738 and the solution was to store all relevant context per-crate keyed by CARGO_MANIFEST_DIR.
There was a problem hiding this comment.
For the same reason, we might actually want to store the modified time of the .env files we load and check for changes to that. If the file has been modified, we need to re-load it.
| "DATABASE_URL", | ||
| "SQLX_OFFLINE", | ||
| "SQLX_OFFLINE_DIR", | ||
| config.common.database_url_var(), |
There was a problem hiding this comment.
This defaults to DATABASE_URL if it's not overridden, so there's no need to specify that separately.
| let database_url = env(config.common.database_url_var()).ok().or(database_url); | ||
| let database_url = env(config.common.database_url_var()) | ||
| .ok() | ||
| .or_else(|| env("DATABASE_URL").ok()); |
There was a problem hiding this comment.
database_url_var defaults to DATABASE_URL if not overridden, so this is redundant.
| // Avoid tracking the same env var multiple times, which would undesirably modify | ||
| // build system state and thus behavior in case we change var values. | ||
| Some(proc_macro::tracked_env::var(name)) |
There was a problem hiding this comment.
I don't believe this tracking is necessary, as the back-end for this call inserts into an IndexMap: https://github.com/rust-lang/rust/blob/5eda692e73f37dcbe2437ce878db7bb71f323e74/compiler/rustc_expand/src/proc_macro_server.rs#L479
Redundant calls shouldn't have any observable side-effects if the value doesn't change.
If the value does change between invocations somehow, we probably do want to update that. Otherwise, the crate may not get re-compiled when the user wants it to.
Also, because of RA, this could actually be invoked in the context of a completely different crate in the same process, so we want to make sure the variable is added to that crate's depinfo as well.
Context, problem statement and description
While giving the new
sqlx.tomlfeature a try, I discovered that itsdatabase_url_varsetting behaved inconsistently compared to other environment variables: it wasn't loaded from.envfiles, causing it to be effectively ignored in favor of the defaultDATABASE_URLvariable. This is an inconvenient outcome for the multi-database workspaces that this feature is designed to support.To address this issue, I reworked the
.envloading logic to account for this configuration and make it simpler to use to achieve consistent behavior. The new algorithm works as follows:.envfiles and candidate.envfile paths. When applicable, the compiler is informed to track changes to their elements.set_envwas used, but I changed it as a response to the review comments below). When a variable is defined in both the process environment and a.envfile, the process environment takes precedence, as before.envfunction freely to read environment variable values, abstracting itself away from their source, which results in simpler, less error-prone code.Trivially, this rework resolves the issue I encountered because the
database_url_varvalue is now part of the list of loadable environment variables. Future code can easily make such additions as necessary.Does your PR solve an issue?
To my knowledge, this PR doesn't directly address any previously reported issue in this repository.
Is this a breaking change?
Technically yes when compared to the released
0.9.0alpha version, as environment variables likedatabase_url_varmay now be loaded from another source. However, I don't think this technically breaking change will cause significant inconvenience for most users, especially since the new behavior is more consistent and useful, and the affected variables are bound to not be widely used due to them being published as an alpha release so far.