Update email library + test.support v3.13.11 #6642
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughTwo files are modified: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/vm/src/stdlib/time.rs (1)
200-206: Hardcoded 3600-second DST offset may be incorrect for some timezones.The calculation
c_timezone - 3600assumes DST offset is always 1 hour, but some regions use different DST offsets (e.g., Lord Howe Island uses 30 minutes). While the FIXME acknowledges this limitation, consider documenting which timezones may return incorrect values or adding a more prominent warning.The implementation follows the existing pattern in this module and is acceptable as a placeholder until proper C
altzonesupport is added.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (93)
Lib/email/_header_value_parser.pyis excluded by!Lib/**Lib/email/_parseaddr.pyis excluded by!Lib/**Lib/email/_policybase.pyis excluded by!Lib/**Lib/email/contentmanager.pyis excluded by!Lib/**Lib/email/feedparser.pyis excluded by!Lib/**Lib/email/generator.pyis excluded by!Lib/**Lib/email/header.pyis excluded by!Lib/**Lib/email/message.pyis excluded by!Lib/**Lib/email/parser.pyis excluded by!Lib/**Lib/email/utils.pyis excluded by!Lib/**Lib/test/support/testresult.pyis excluded by!Lib/**Lib/test/test_email/__init__.pyis excluded by!Lib/**Lib/test/test_email/__main__.pyis excluded by!Lib/**Lib/test/test_email/data/msg_01.txtis excluded by!Lib/**Lib/test/test_email/data/msg_02.txtis excluded by!Lib/**Lib/test/test_email/data/msg_03.txtis excluded by!Lib/**Lib/test/test_email/data/msg_04.txtis excluded by!Lib/**Lib/test/test_email/data/msg_05.txtis excluded by!Lib/**Lib/test/test_email/data/msg_06.txtis excluded by!Lib/**Lib/test/test_email/data/msg_07.txtis excluded by!Lib/**Lib/test/test_email/data/msg_08.txtis excluded by!Lib/**Lib/test/test_email/data/msg_09.txtis excluded by!Lib/**Lib/test/test_email/data/msg_10.txtis excluded by!Lib/**Lib/test/test_email/data/msg_11.txtis excluded by!Lib/**Lib/test/test_email/data/msg_12.txtis excluded by!Lib/**Lib/test/test_email/data/msg_12a.txtis excluded by!Lib/**Lib/test/test_email/data/msg_13.txtis excluded by!Lib/**Lib/test/test_email/data/msg_14.txtis excluded by!Lib/**Lib/test/test_email/data/msg_15.txtis excluded by!Lib/**Lib/test/test_email/data/msg_16.txtis excluded by!Lib/**Lib/test/test_email/data/msg_17.txtis excluded by!Lib/**Lib/test/test_email/data/msg_18.txtis excluded by!Lib/**Lib/test/test_email/data/msg_19.txtis excluded by!Lib/**Lib/test/test_email/data/msg_20.txtis excluded by!Lib/**Lib/test/test_email/data/msg_21.txtis excluded by!Lib/**Lib/test/test_email/data/msg_22.txtis excluded by!Lib/**Lib/test/test_email/data/msg_23.txtis excluded by!Lib/**Lib/test/test_email/data/msg_24.txtis excluded by!Lib/**Lib/test/test_email/data/msg_25.txtis excluded by!Lib/**Lib/test/test_email/data/msg_26.txtis excluded by!Lib/**Lib/test/test_email/data/msg_27.txtis excluded by!Lib/**Lib/test/test_email/data/msg_28.txtis excluded by!Lib/**Lib/test/test_email/data/msg_29.txtis excluded by!Lib/**Lib/test/test_email/data/msg_30.txtis excluded by!Lib/**Lib/test/test_email/data/msg_31.txtis excluded by!Lib/**Lib/test/test_email/data/msg_32.txtis excluded by!Lib/**Lib/test/test_email/data/msg_33.txtis excluded by!Lib/**Lib/test/test_email/data/msg_34.txtis excluded by!Lib/**Lib/test/test_email/data/msg_35.txtis excluded by!Lib/**Lib/test/test_email/data/msg_36.txtis excluded by!Lib/**Lib/test/test_email/data/msg_37.txtis excluded by!Lib/**Lib/test/test_email/data/msg_38.txtis excluded by!Lib/**Lib/test/test_email/data/msg_39.txtis excluded by!Lib/**Lib/test/test_email/data/msg_40.txtis excluded by!Lib/**Lib/test/test_email/data/msg_41.txtis excluded by!Lib/**Lib/test/test_email/data/msg_42.txtis excluded by!Lib/**Lib/test/test_email/data/msg_43.txtis excluded by!Lib/**Lib/test/test_email/data/msg_44.txtis excluded by!Lib/**Lib/test/test_email/data/msg_45.txtis excluded by!Lib/**Lib/test/test_email/data/msg_46.txtis excluded by!Lib/**Lib/test/test_email/data/msg_47.txtis excluded by!Lib/**Lib/test/test_email/data/python.bmpis excluded by!**/*.bmp,!Lib/**Lib/test/test_email/data/python.exris excluded by!Lib/**Lib/test/test_email/data/python.gifis excluded by!**/*.gif,!Lib/**Lib/test/test_email/data/python.jpgis excluded by!**/*.jpg,!Lib/**Lib/test/test_email/data/python.pbmis excluded by!Lib/**Lib/test/test_email/data/python.pgmis excluded by!Lib/**Lib/test/test_email/data/python.pngis excluded by!**/*.png,!Lib/**Lib/test/test_email/data/python.ppmis excluded by!Lib/**Lib/test/test_email/data/python.rasis excluded by!Lib/**Lib/test/test_email/data/python.sgiis excluded by!Lib/**Lib/test/test_email/data/python.tiffis excluded by!**/*.tiff,!Lib/**Lib/test/test_email/data/python.webpis excluded by!Lib/**Lib/test/test_email/data/python.xbmis excluded by!Lib/**Lib/test/test_email/data/sndhdr.aifcis excluded by!Lib/**Lib/test/test_email/data/sndhdr.aiffis excluded by!Lib/**Lib/test/test_email/data/sndhdr.auis excluded by!Lib/**Lib/test/test_email/data/sndhdr.wavis excluded by!**/*.wav,!Lib/**Lib/test/test_email/test__encoded_words.pyis excluded by!Lib/**Lib/test/test_email/test__header_value_parser.pyis excluded by!Lib/**Lib/test/test_email/test_asian_codecs.pyis excluded by!Lib/**Lib/test/test_email/test_contentmanager.pyis excluded by!Lib/**Lib/test/test_email/test_defect_handling.pyis excluded by!Lib/**Lib/test/test_email/test_email.pyis excluded by!Lib/**Lib/test/test_email/test_generator.pyis excluded by!Lib/**Lib/test/test_email/test_headerregistry.pyis excluded by!Lib/**Lib/test/test_email/test_inversion.pyis excluded by!Lib/**Lib/test/test_email/test_message.pyis excluded by!Lib/**Lib/test/test_email/test_parser.pyis excluded by!Lib/**Lib/test/test_email/test_pickleable.pyis excluded by!Lib/**Lib/test/test_email/test_policy.pyis excluded by!Lib/**Lib/test/test_email/test_utils.pyis excluded by!Lib/**Lib/test/test_email/torture_test.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/vm/src/bytes_inner.rscrates/vm/src/stdlib/time.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/stdlib/time.rscrates/vm/src/bytes_inner.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (1)
crates/vm/src/bytes_inner.rs (1)
1091-1097: LGTM!Adding the
Debugderive toDecodeArgsimproves debuggability. The struct definition is clean and follows the existing patterns in this file.
crates/vm/src/bytes_inner.rs
Outdated
| let encoding = match encoding.as_ref() { | ||
| Some(s) => match s.to_str() { | ||
| Some(valid_str) => valid_str, | ||
| None => { | ||
| let Some(errors) = &errors else { | ||
| return Err(vm.new_unicode_encode_error(format!( | ||
| "'{}' codec can't encode characters: surrogates not allowed", | ||
| s | ||
| ))); | ||
| }; | ||
|
|
||
| match errors.as_str() { | ||
| "strict" => { | ||
| return Err( | ||
| vm.new_unicode_encode_error("Struct format must be a UTF-8 string") | ||
| ); | ||
| } | ||
| "ignore" => todo!("TODO"), | ||
| "replace" => todo!("TODO"), | ||
| "backslashreplace" => todo!("TODO"), | ||
| "surrogateescape" => &s.to_string_lossy(), | ||
| _ => { | ||
| return Err(vm.new_unicode_encode_error(format!( | ||
| "'{}' codec can't encode characters: surrogates not allowed", | ||
| s | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| None => crate::codecs::DEFAULT_ENCODING, | ||
| }; |
There was a problem hiding this comment.
Critical: Dangling reference and incomplete implementation.
Two significant issues:
-
Dangling reference (Line 1126):
&s.to_string_lossy()creates a temporaryCow<str>that is dropped at the end of the expression. The resulting&strreference would be invalid when used later. This should either fail to compile or cause undefined behavior. -
Unimplemented paths will panic (Lines 1123-1125): The
todo!()macros for "ignore", "replace", and "backslashreplace" will panic at runtime if these error handlers are used.
🔎 Proposed fix
To fix the lifetime issue, you need to own the string. One approach:
pub fn bytes_decode(
zelf: PyObjectRef,
args: DecodeArgs,
vm: &VirtualMachine,
) -> PyResult<PyStrRef> {
let DecodeArgs { encoding, errors } = args;
- let encoding = match encoding.as_ref() {
- Some(s) => match s.to_str() {
- Some(valid_str) => valid_str,
+ let encoding_owned: Option<String>;
+ let encoding: &str = match encoding.as_ref() {
+ Some(s) => match s.to_str() {
+ Some(valid_str) => valid_str,
None => {
let Some(errors) = &errors else {
return Err(vm.new_unicode_encode_error(format!(
"'{}' codec can't encode characters: surrogates not allowed",
s
)));
};
match errors.as_str() {
"strict" => {
return Err(
- vm.new_unicode_encode_error("Struct format must be a UTF-8 string")
+ vm.new_unicode_encode_error("encoding must be a valid UTF-8 string")
);
}
- "ignore" => todo!("TODO"),
- "replace" => todo!("TODO"),
- "backslashreplace" => todo!("TODO"),
- "surrogateescape" => &s.to_string_lossy(),
+ "ignore" | "replace" | "backslashreplace" => {
+ return Err(vm.new_not_implemented_error(
+ format!("error handler '{}' not yet implemented for encoding with surrogates", errors.as_str())
+ ));
+ }
+ "surrogateescape" => {
+ encoding_owned = Some(s.to_string_lossy().into_owned());
+ encoding_owned.as_ref().unwrap()
+ }
_ => {
return Err(vm.new_unicode_encode_error(format!(
"'{}' codec can't encode characters: surrogates not allowed",
s
)));
}
}
}
},
None => crate::codecs::DEFAULT_ENCODING,
};Committable suggestion skipped: line range outside the PR's diff.
|
@terryluan12 was there problem in this PR? |
|
I don't remember closing this, whoops. Yes, there are many lol. Please don't merge. I'm planning to keep the PR open while I work on it, if that's alright |
bf6340e to
be9a494
Compare
…r parameter in bytes_decode function in `bytes_inner.rs`
be9a494 to
af6bf40
Compare
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin update_email |
bdc4810 to
f6ab03c
Compare
Summary by CodeRabbit
errorsparameter supporting multiple error-handling strategies: strict, ignore, replace, backslashreplace, and surrogateescape modes.time.altzone()function to retrieve alternative timezone offset information.✏️ Tip: You can customize this high-level summary in your review settings.