fix(key): key env vars that steer proc-macro expansion (#635) - #639
Merged
Conversation
rustc reports an env var in dep-info only when a crate reads it through env!/option_env!. A proc macro calling std::env::var while expanding leaves no trace: the rustc command line, the source hashes and the --extern set are byte-identical whether or not the var is set, while the emitted artifact is not. Both compiles key the same and the second one restores the first one's expansion. Reported against boltffi, whose #[export] macro strips itself from dependency crates under BOLTFFI_BINDING_EXPANSION. The dependency crate's invocation is identical in both phases, so a normal build restored the stripped 264 KiB artifact instead of building the 620 KiB one and failed with a missing trait impl. The same mechanism can produce a wrong binary that builds cleanly. Add `[cache] key_env_vars` / KACHE_KEY_ENV_VARS: exact names or a trailing-* prefix glob, folded into every rustc and cc key. Both the declared patterns and the matched NAME=VALUE pairs are folded. Folding the patterns is load-bearing, not decorative: without it the build that leaves the vars unset would keep its old key and land straight back on the poisoned entry that motivated the declaration. Values run through the PathNormalizer first, so a declared var holding a checkout path does not pin the entry to one machine. An empty list leaves keys byte-identical to the undeclared case, so no CACHE_KEY_VERSION bump. The fold is union-only: a misdeclared pattern costs a miss, never a wrong restore. tests/proc_macro_env_key_test.rs builds a real proc-macro dylib whose expansion depends on an env var and reads the marker back out of the compiled rlib. Without the declaration it fails with the reported bug - the normal build restores the expansion-mode artifact.
Cross-model review (codex) found a soundness hole in the first pass: running a declared variable's value through the PathNormalizer can merge two builds that produce different artifacts. A proc macro is free to paste the value straight into the code it emits, so BOLTFFI_ROOT= /home/alice/proj and /srv/build/bob/proj can collapse to the same <BASE_DIR> sentinel while the artifacts differ. That reintroduces exactly the wrong-hit this feature exists to prevent, and it contradicts the policy env_dep_is_safe_to_normalize already applies to reported env! deps: normalize only where a value is proven to be nothing but a locator, which for a proc-macro read can never be proven. Fold the exact value instead. The cost is real and is the right way round: a declared variable holding a machine-local path makes that crate's key machine-specific. Documented, with a steer toward declaring the switch a macro branches on rather than a glob that sweeps in path variables. Also from the same review: - Fold raw OS bytes rather than to_string_lossy. Distinct non-UTF-8 values both map to U+FFFD, and a macro reading var_os can still tell them apart. - Case-fold env var NAMES on Windows only, where PATH and Path are one variable and folding whichever casing the OS reported would split the cache between two machines describing the same environment. Unix keeps the case: there they are different variables. - Upper-case the declared patterns in normalize_key_env_vars. Matching is case-insensitive, so BOLTFFI_* and boltffi_* select identically and must not fold differently. - Keep environ order when a name appears twice. Sorting erases an order that getenv makes observable. Only reachable via a hand-built envp, since the set_var APIs replace, but it is a wrong-hit path. - Correct the interior-`*` warning: A*B matches a variable literally named A*B, which is legal on Unix. It does not "never match". Tests: distinct path values stay distinct (the inverse of the test this replaces), non-UTF-8 values stay distinct on unix, set-to-empty differs from unset, equivalent pattern spellings fold identically, and the TUI save path preserves key_env_vars (it has no form field, so a dropped field would silently disable the setting). Also pin the proc-macro dylib extension in the e2e test: a substring match on the crate name would pick up pmenv.pdb or pmenv.dll.lib on Windows.
Two leftovers from the cross-model audit of the previous commit. Windows compares env var names by an uppercase mapping that is not ASCII-only, so lowercasing individual UTF-16 units left non-ASCII names splitting the cache between machines describing the same environment. Fold through `str::to_uppercase` instead, which is what Windows' own comparison approximates. The unpaired-surrogate fallback keeps the raw units; both arms emit UTF-16LE so the encodings cannot be confused. Only ever a spurious miss either way, never a wrong hit. The interior-`*` warning was still inaccurate for `A*B*`: the trailing `*` does make it a prefix glob over the literal bytes `A*B`, so the message claiming it matches one exact name was wrong for that shape. Reword to describe what actually happens - the earlier `*` is matched literally - which is true for both `A*B` and `A*B*`.
CI's mutation gate found 6 survivors in the diff. Five were genuine coverage gaps in the new code, all in logic that has no observable effect unless a test controls the shape of the environment: - Deleting either `!` in the duplicate-name/sort decision survived, because nothing asserted that environ ORDER does not change the digest (or that it does, once a name repeats). That is exactly the ordering logic the review flagged, so it should not have been untested. - Replacing `env_name_key_bytes` with a constant survived, because every test folded a single variable, so a name that collapsed to a constant still produced distinct keys via the value. Extract `key_env_digest`, which takes the matched `(name, value)` byte pairs, so the ordering rules are testable without rewriting the process environment. Tests: permuted pairs digest identically, a repeated name keeps environ order, and swapping which name holds which value is a different digest. Plus direct tests that `env_name_key_bytes` and `env_os_key_bytes` distinguish their inputs and that the Windows-only case folding follows the platform rule. The sixth was `run_config_editor` -> Ok(()). It survives because the function sets up raw mode, an alternate screen and an event loop, none of which a test can drive; it only entered the diff because the change lives inside it. Extract `initial_editor_state`, which is the half that actually matters here - it decides which form-less settings survive a save - and test that `key_env_vars` and `path_only_env_vars` make it from a loaded config through to a written one. The terminal plumbing left in `run_config_editor` is no longer part of the change. Also rename a local `display` binding: it shadowed `tracing::field::display` inside the trace macro.
`run_config_editor` is the only mutant left surviving in this PR's diff. It enables raw mode, takes over the alternate screen, and runs an event loop until the user quits, so a replaced body is unobservable: CI has no TTY at all, and a test that did get one would seize the developer's terminal and then block on the loop. It survives by construction, not because of a coverage gap. Scoped to that one function rather than the file or the module: the parts of the editor that hold real logic - initial_editor_state, build_fields, fields_to_file_config, do_save_to - stay in scope and are mutation-tested normally. The extraction in the previous commit is what makes that division real; before it, the state assembly was inside the excluded function. Verified locally with the same invocation CI uses: 34 mutants, 1 missed (this one) before the exclusion; the mutant no longer appears in `--list` after it, leaving the other 33 caught or unviable.
Clippy's needless_return fires only on Windows, where the cfg(not(windows)) arm is compiled out and this block becomes the whole function body, so the return has nothing left to return early from. Under -D warnings that failed Test (Windows) while every other arm was green. Verified by running clippy-driver against the Windows target on an isolated copy of the function: the lint reproduces before the change and is gone after it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #635.
The problem
rustc records an env var in dep-info only when a crate reads it through
env!/option_env!. A proc macro callingstd::env::varwhile expanding leaves no trace at all: the rustc command line, the source hashes and the--externset are byte-identical whether or not the var is set, while the emitted artifact is not. Both compiles key the same, and the second one restores the first one's expansion.In the reported case a two-phase build ran the same crate graph twice, once with
BOLTFFI_BINDING_EXPANSION=1(where the#[export]macro strips itself from dependency crates and emits no trait impls) and once without. The dependency crate's invocation was identical in both phases, so the normal build restored the stripped 264 KiB artifact instead of building the 620 KiB one and failed with a missing trait impl. The same mechanism can just as easily produce a wrong binary that builds cleanly.proc_macro::tracked_envwould let a macro register these reads with the compiler and make this unnecessary, but it is still unstable, so the vars have to be declared.The fix
[cache] key_env_vars/KACHE_KEY_ENV_VARS: a list of env var names to fold into every rustc and cc cache key. Entries are exact names or a trailing-*prefix glob, matched ASCII case-insensitively so a Windows environment behaves like a Unix one.Two things are folded, and both are load-bearing:
NAME=VALUEpairs, sorted by name, which separates the two modes going forward.Values are folded exactly, as their raw OS bytes — deliberately not through the
PathNormalizerand not via a lossy UTF-8 conversion. A declared variable is an opaque semantic input: a macro may paste its value straight into the code it emits, so two checkout paths that collapse to the same<BASE_DIR>sentinel can still produce different artifacts. Normalizing them would reintroduce the wrong-hit this setting exists to prevent. This matches the policyenv_dep_is_safe_to_normalizealready applies to reportedenv!deps: normalize only where a value is proven to be nothing but a locator, which for a proc-macro read can never be proven. The cost is that a declared variable holding a machine-local path makes that crate's key machine-specific, so the docs steer toward declaring the switch a macro branches on rather than a glob that sweeps in path variables.Properties:
CACHE_KEY_VERSIONbump and no cold cache for anyone who does not use it.BOLTFFI_*andboltffi_*in either order share a cache. Matched pairs are sorted too, sovars_osorder cannot leak into the key. The one exception is a process carrying the same name twice (only constructible via a hand-builtenvp):getenvreturns the first occurrence, which makes the order semantically observable, so environ order is preserved in that case.PATHandPathare one variable. On Unix they are genuinely different variables and the case is kept.Tests
tests/proc_macro_env_key_test.rsbuilds a real proc-macro dylib whose expansion depends on an env var, compiles a consumer through kache four times, and reads the marker back out of the compiled rlib. Hit/miss counts prove the key moved; reading the artifact proves kache stopped serving the wrong expansion.Confirmed non-vacuous — with the declaration removed the test fails exactly as reported:
Plus unit coverage for the matcher (exact/prefix/case/interior-
*), the identity property when unconfigured, set-vs-unset-vs-empty separation, equivalent pattern spellings folding identically, distinct path values staying distinct, non-UTF-8 values staying distinct on unix, config env-vs-file precedence including theignore_envlockdown, and the TUI save path preservingkey_env_vars(it has no form field, so a dropped field would silently disable the setting).Review notes
The first pass path-normalized declared values to keep them portable across machines. A cross-model review flagged that as unsound for the reason above, and the second commit replaces it with exact folding. The same review turned up the lossy-
OsStringcollision, the Windows name-casing split, the pattern-case split, the duplicate-name ordering hole, and an interior-*warning that claimed such patterns "can never match" (they match a variable literally namedA*B, which is legal on Unix). All are fixed in the second commit.Docs
New Env vars that steer expansion section with the semantics and the sharp edges, a settings-table row, and cross-references from the cache-key page. The doc also points at
cache.excludefor anyone who would rather not cache the affected crate at all.What this does not do
The report also suggested a per-project crate denylist and a strict mode hashing the whole environment. The denylist is already covered by
cache.exclude(source-path globs, documented in the new section). A strict whole-environment mode is deliberately left out: it would foldPWD, job ids and timestamps into every key and disable caching wholesale.