{{ message }}
Merged
Conversation
Contributor
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin stdlib_dir |
youknowone
force-pushed
the
stdlib_dir
branch
from
f42eaaf to
22569fa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@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.
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}"); |
Contributor
There was a problem hiding this comment.
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.
🔧 Proposed safer emission
- 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);
+ }🤖 Prompt for AI Agents
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.
7371d30 to
b212d9a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@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).
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
962-962: Inconsistent downcast pattern withDictUpdate.
DictUpdate(line 934) uses a safe downcast with.expect():let dict = dict.downcast_ref::<PyDict>().expect("exact dict expected");But
DictMergeuses an unsafe downcast without aSAFETYcomment explaining the invariant. Consider using the same safe pattern asDictUpdatefor consistency and defensive coding, or at minimum add aSAFETYcomment 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");
Comment on lines
+6983
to
+6986
Contributor
There was a problem hiding this comment.
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.
🔧 Possible fix (merge into a fresh accumulator)
- 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 });
+ }
+ }🤖 Prompt for AI Agents
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).
c9e25ea to
3867a0a
Compare
youknowone
force-pushed
the
stdlib_dir
branch
from
3867a0a to
99f3e3e
Compare
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
1 participant
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.