Add sys._stdlib_dir - #6798
Merged
youknowone merged 1 commit intoJan 19, 2026
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
📝 Walkthrough
WalkthroughIntroduces a new stdlib_dir field to the path configuration system, computed via platform-specific logic in getpath.rs, exposed through the sys module as _stdlib_dir, and wired during interpreter initialization to store the standard library location derived from either frozen or dynamic paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 3 ✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin stdlib_dir |
Sorry, something went wrong.
youknowone
force-pushed
the
stdlib_dir
branch
from
f42eaaf to
22569fa
Compare
January 19, 2026 14:03
youknowone
marked this pull request as ready for review
January 19, 2026 14:44
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agentsIn `@crates/pylib/build.rs`:
- Around line 14-31: The build script currently calls
canonicalize(...).to_str().unwrap(), which can panic on non‑UTF‑8 Windows paths;
change the emission to use a lossy conversion from the canonicalized Path (e.g.,
Path::to_string_lossy on the value returned by std::fs::canonicalize(&lib_path))
and print that into the cargo:rustc-env=win_lib_path value, and if
std::fs::canonicalize(&lib_path) returns an Err emit a visible build warning via
println!("cargo:warning=...") instead of silently skipping so missing
win_lib_path is obvious; update the code paths referencing canonicalized_path
and to_str().unwrap() accordingly.
Sorry, something went wrong.
Comment thread
crates/pylib/build.rs
Comment on lines
+14
to
+31
| if cfg!(windows) { | ||
| // On Windows, the Lib entry can be either: | ||
| // 1. A text file containing the relative path (git without symlink support) | ||
| // 2. A proper symlink (git with symlink support) | ||
| // We handle both cases to resolve to the actual Lib directory. | ||
| let lib_path = if let Ok(real_path) = std::fs::read_to_string("Lib") { | ||
| // Case 1: Text file containing relative path | ||
| std::path::PathBuf::from(real_path.trim()) | ||
| } else { | ||
| // Case 2: Symlink or directory - canonicalize directly | ||
| std::path::PathBuf::from("Lib") | ||
| }; | ||
|
|
||
| if let Ok(canonicalized_path) = std::fs::canonicalize(&lib_path) { | ||
| // Strip the extended path prefix (\\?\) that canonicalize adds on Windows | ||
| let path_str = canonicalized_path.to_str().unwrap(); | ||
| let path_str = path_str.strip_prefix(r"\\?\").unwrap_or(path_str); | ||
| println!("cargo:rustc-env=win_lib_path={path_str}"); |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Avoid panic on non‑UTF‑8 Windows paths.
to_str().unwrap() can panic on valid Windows paths containing non‑UTF‑8 sequences, breaking the build. Prefer a lossy conversion and consider warning when canonicalization fails so missing win_lib_path isn’t silent.
- if let Ok(canonicalized_path) = std::fs::canonicalize(&lib_path) {
- // Strip the extended path prefix (\\?\) that canonicalize adds on Windows
- let path_str = canonicalized_path.to_str().unwrap();
- let path_str = path_str.strip_prefix(r"\\?\").unwrap_or(path_str);
- println!("cargo:rustc-env=win_lib_path={path_str}");
- }
+ if let Ok(canonicalized_path) = std::fs::canonicalize(&lib_path) {
+ // Strip the extended path prefix (\\?\) that canonicalize adds on Windows
+ let path_str = canonicalized_path.to_string_lossy();
+ let path_str = path_str.strip_prefix(r"\\?\").unwrap_or(&path_str);
+ println!("cargo:rustc-env=win_lib_path={path_str}");
+ } else {
+ println!("cargo:warning=failed to canonicalize Lib path: {:?}", lib_path);
+ }In `@crates/pylib/build.rs` around lines 14 - 31, The build script currently calls
canonicalize(...).to_str().unwrap(), which can panic on non‑UTF‑8 Windows paths;
change the emission to use a lossy conversion from the canonicalized Path (e.g.,
Path::to_string_lossy on the value returned by std::fs::canonicalize(&lib_path))
and print that into the cargo:rustc-env=win_lib_path value, and if
std::fs::canonicalize(&lib_path) returns an Err emit a visible build warning via
println!("cargo:warning=...") instead of silently skipping so missing
win_lib_path is obvious; update the code paths referencing canonicalized_path
and to_str().unwrap() accordingly.
Sorry, something went wrong.
youknowone
force-pushed
the
stdlib_dir
branch
from
7371d30 to
b212d9a
Compare
January 19, 2026 15:32
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agentsIn `@crates/codegen/src/compile.rs`:
- Around line 6983-6986: The DictMerge path currently uses the first mapping as
the accumulator which can mutate a caller-provided **kwargs dict; change
compile.rs so that when merging multiple mappings (the loop around emit!(self,
Instruction::DictMerge { index: 1 })), you first allocate a fresh empty
accumulator and merge each mapping into that fresh map instead of using the
first input as the target; specifically, before the for-loop that emits
Instruction::DictMerge, emit an instruction to create a new empty dict (so the
accumulator is new) and then merge all incoming `**` groups into that fresh
accumulator (update the code that emits Instruction::DictMerge and the
surrounding logic in the same block to use the new accumulator).
crates/vm/src/frame.rs (1)962-962: Inconsistent downcast pattern with DictUpdate.
DictUpdate (line 934) uses a safe downcast with .expect():
let dict = dict.downcast_ref::<PyDict>().expect("exact dict expected");But DictMerge uses an unsafe downcast without a SAFETY comment explaining the invariant. Consider using the same safe pattern as DictUpdate for consistency and defensive coding, or at minimum add a SAFETY comment like the other unsafe downcasts in this file (e.g., ListAppend, ListExtend).
♻️ Suggested fix for consistency- let dict: &Py<PyDict> = unsafe { dict_ref.downcast_unchecked_ref() }; + let dict: &Py<PyDict> = unsafe { + // SAFETY: compiler guarantees correct type + dict_ref.downcast_unchecked_ref() + };Or use the safe pattern:
- let dict: &Py<PyDict> = unsafe { dict_ref.downcast_unchecked_ref() }; + let dict = dict_ref.downcast_ref::<PyDict>().expect("exact dict expected");
Sorry, something went wrong.
Comment thread
crates/codegen/src/compile.rs
Comment on lines
+6983
to
+6986
| // Merge all dicts: first dict is accumulator, merge rest into it | ||
| for _ in 1..size { | ||
| emit!(self, Instruction::DictMerge { index: 1 }); | ||
| } |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Avoid mutating user-provided **kwargs when merging.
DictMerge uses the first mapping as the accumulator. If the first group is **kwargs, this mutates the caller’s dict when multiple ** groups exist (observable vs CPython, which builds a fresh dict). Please ensure the accumulator is always a new map before merging any ** inputs.
- let mut size = 0;
- let groupby = keywords.iter().chunk_by(|e| e.arg.is_none());
- for (is_unpacking, sub_keywords) in &groupby {
- if is_unpacking {
- for keyword in sub_keywords {
- self.compile_expression(&keyword.value)?;
- size += 1;
- }
- } else {
- let mut sub_size = 0;
- for keyword in sub_keywords {
- if let Some(name) = &keyword.arg {
- self.emit_load_const(ConstantData::Str {
- value: name.as_str().into(),
- });
- self.compile_expression(&keyword.value)?;
- sub_size += 1;
- }
- }
- emit!(self, Instruction::BuildMap { size: sub_size });
- size += 1;
- }
- }
- if size > 1 {
- // Merge all dicts: first dict is accumulator, merge rest into it
- for _ in 1..size {
- emit!(self, Instruction::DictMerge { index: 1 });
- }
- }
+ let mut have_accumulator = false;
+ let groupby = keywords.iter().chunk_by(|e| e.arg.is_none());
+ for (is_unpacking, sub_keywords) in &groupby {
+ if !have_accumulator {
+ emit!(self, Instruction::BuildMap { size: 0 }); // fresh accumulator
+ have_accumulator = true;
+ }
+ if is_unpacking {
+ for keyword in sub_keywords {
+ self.compile_expression(&keyword.value)?;
+ emit!(self, Instruction::DictMerge { index: 1 });
+ }
+ } else {
+ let mut sub_size = 0;
+ for keyword in sub_keywords {
+ if let Some(name) = &keyword.arg {
+ self.emit_load_const(ConstantData::Str {
+ value: name.as_str().into(),
+ });
+ self.compile_expression(&keyword.value)?;
+ sub_size += 1;
+ }
+ }
+ emit!(self, Instruction::BuildMap { size: sub_size });
+ emit!(self, Instruction::DictMerge { index: 1 });
+ }
+ }In `@crates/codegen/src/compile.rs` around lines 6983 - 6986, The DictMerge path
currently uses the first mapping as the accumulator which can mutate a
caller-provided **kwargs dict; change compile.rs so that when merging multiple
mappings (the loop around emit!(self, Instruction::DictMerge { index: 1 })), you
first allocate a fresh empty accumulator and merge each mapping into that fresh
map instead of using the first input as the target; specifically, before the
for-loop that emits Instruction::DictMerge, emit an instruction to create a new
empty dict (so the accumulator is new) and then merge all incoming `**` groups
into that fresh accumulator (update the code that emits Instruction::DictMerge
and the surrounding logic in the same block to use the new accumulator).
Sorry, something went wrong.
youknowone
force-pushed
the
stdlib_dir
branch
from
c9e25ea to
3867a0a
Compare
January 19, 2026 17:12
youknowone
force-pushed
the
stdlib_dir
branch
from
3867a0a to
99f3e3e
Compare
January 19, 2026 17:32
Merged
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.
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.