Skip to content

Allow lone-surrogate keyword keys in f(**d) #8409

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 2 commits into from
Jul 30, 2026
Merged

Allow lone-surrogate keyword keys in f(**d) #8409

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daf72d7
Allow lone-surrogate keyword keys in f(**d)
HyoJongPark Jul 28, 2026
de4f2d1
Avoid cloning kwargs keys in _ast, borrow via as_ref instead
HyoJongPark Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Allow lone-surrogate keyword keys in f(**d) 
`f(**{'\udc81': 2})` raised `TypeError: keywords must be strings` even
though the key is a valid `str`, because `KwArgs` stored keys as Rust
`String` (strict UTF-8) and `collect_ex_args` narrowed each key to
`PyUtf8Str` (valid UTF-8 required). CPython only checks that the key is a
`str` (`Py_TPFLAGS_UNICODE_SUBCLASS`), never encoding validity.

Change `KwArgs<T>`'s key type from `String` to `Wtf8Buf` (the WTF-8
storage `PyStr` already uses) and relax the keyword-key downcast from
`PyUtf8Str` to `PyStr` in `collect_ex_args`, `from_vectorcall`/
`from_vectorcall_owned` (which previously panicked on surrogate keys),
the `functools.partial` keyword merge, and the C-API `dict_to_kwargs`.

`Wtf8Buf` borrows only as `Wtf8`, so inherent `get`/`contains_key`/
`swap_remove`/`shift_remove(&str)` on `KwArgs` restore the `&str` lookup
interface `String: Borrow<str>` used to provide (via the zero-cost
`Wtf8::new` cast), and a generic `FromIterator<(K: Into<Wtf8Buf>, T)>`
keeps construction sites unchanged. WTF-8 awareness stays localized to
`function/argument.rs`.

Fixes #8228

Assisted-by: Claude Code:claude-opus-4-8
  • Loading branch information
@HyoJongPark
HyoJongPark committed Jul 29, 2026
commit daf72d7d55a1d402703f5334023096b5ae15f49a
4 changes: 3 additions & 1 deletion crates/capi/src/abstract_.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@ fn dict_to_kwargs(vm: &VirtualMachine, dict: &Py<PyDict>) -> PyResult<KwArgs> {
dict.items_vec()
.into_iter()
.map(|(key, value)| {
// `to_string()` would replace lone surrogates with U+FFFD; keep the
// raw WTF-8 so surrogate keys round-trip (issue #8228).
let key = key
.downcast_ref::<PyStr>()
.map(|s| s.to_string())
.map(|s| s.as_wtf8().to_owned())
.ok_or_else(|| vm.new_type_error("keywords must be strings"))?;
Ok((key, value))
})
Expand Down
8 changes: 4 additions & 4 deletions crates/stdlib/src/_asyncio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ pub(crate) mod _asyncio {
let args = if let Some(ctx) = context {
FuncArgs::new(
vec![callback, future_arg],
KwArgs::new(core::iter::once(("context".to_owned(), ctx)).collect()),
KwArgs::new(core::iter::once((Wtf8Buf::from("context"), ctx)).collect()),
)
} else {
FuncArgs::new(vec![callback, future_arg], KwArgs::default())
Expand Down Expand Up @@ -1498,7 +1498,7 @@ pub(crate) mod _asyncio {
let args = if let Some(ctx) = context {
FuncArgs::new(
vec![callback, task_arg],
KwArgs::new(core::iter::once(("context".to_owned(), ctx)).collect()),
KwArgs::new(core::iter::once((Wtf8Buf::from("context"), ctx)).collect()),
)
} else {
FuncArgs::new(vec![callback, task_arg], KwArgs::default())
Expand Down Expand Up @@ -1527,7 +1527,7 @@ pub(crate) mod _asyncio {
let cancel_args = if let Some(ref m) = msg_value {
FuncArgs::new(
vec![],
KwArgs::new(core::iter::once(("msg".to_owned(), m.clone())).collect()),
KwArgs::new(core::iter::once((Wtf8Buf::from("msg"), m.clone())).collect()),
)
} else {
FuncArgs::new(vec![], KwArgs::default())
Expand Down Expand Up @@ -2213,7 +2213,7 @@ pub(crate) mod _asyncio {
let cancel_args = if let Some(ref m) = cancel_msg {
FuncArgs::new(
vec![],
KwArgs::new(core::iter::once(("msg".to_owned(), m.clone())).collect()),
KwArgs::new(core::iter::once((Wtf8Buf::from("msg"), m.clone())).collect()),
)
} else {
FuncArgs::new(vec![], KwArgs::default())
Expand Down
11 changes: 9 additions & 2 deletions crates/vm/src/builtins/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,8 +337,13 @@ impl PyFunction {
let mut posonly_passed_as_kwarg = Vec::new();
// Handle keyword arguments
for (name, value) in func_args.kwargs {
// Parameter names are plain identifiers, so a non-UTF-8 (surrogate) key
// can never match one and just falls through to **kwargs / the error path.
let name_str = name.as_str().ok();
// Check if we have a parameter with this name:
if let Some(pos) = arg_pos(code.posonlyarg_count as usize..total_args, &name) {
if let Some(pos) =
name_str.and_then(|s| arg_pos(code.posonlyarg_count as usize..total_args, s))
{
let slot = &mut fastlocals[pos];
if slot.is_some() {
return Err(vm.new_type_error(format!(
Expand All @@ -350,7 +355,9 @@ impl PyFunction {
*slot = Some(value);
} else if let Some(kwargs) = kwargs.as_ref() {
kwargs.set_item(&name, value, vm)?;
} else if arg_pos(0..code.posonlyarg_count as usize, &name).is_some() {
} else if name_str
.is_some_and(|s| arg_pos(0..code.posonlyarg_count as usize, s).is_some())
{
posonly_passed_as_kwarg.push(name);
} else {
return Err(vm.new_type_error(format!(
Expand Down
3 changes: 3 additions & 0 deletions crates/vm/src/builtins/function/jit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ pub(crate) fn get_jit_args<'a>(
for (name, value) in &func_args.kwargs {
let arg_pos =
|args: &[&PyStrInterned], name: &str| args.iter().position(|arg| arg.as_str() == name);
// Parameter names are plain identifiers, so a non-UTF-8 (surrogate) key
// can never match one.
let name = name.as_str().map_err(|_| ArgsError::NotAKeywordArg)?;
if let Some(arg_idx) = arg_pos(arg_names.args, name) {
if jit_args.is_set(arg_idx) {
return Err(ArgsError::ArgPassedMultipleTimes);
Expand Down
6 changes: 4 additions & 2 deletions crates/vm/src/frame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7112,11 +7112,13 @@ impl ExecutingFrame<'_> {
let func_str = Self::object_function_str(callable, vm);

Self::iterate_mapping_keys(vm, &kw_obj, &func_str, |key| {
// `PyStr`, not `PyUtf8Str`: CPython only checks that the key is a
// `str`, not that it is valid UTF-8, so surrogate keys are accepted.
let key_str = key
.downcast_ref::<PyUtf8Str>()
.downcast_ref::<PyStr>()
.ok_or_else(|| vm.new_type_error("keywords must be strings"))?;
let value = kw_obj.get_item(&*key, vm)?;
kwargs.insert(key_str.as_str().to_owned(), value);
kwargs.insert(key_str.as_wtf8().to_owned(), value);
Ok(())
})?
};
Expand Down
60 changes: 45 additions & 15 deletions crates/vm/src/function/argument.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::{
AsObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject, VirtualMachine,
builtins::{PyBaseExceptionRef, PyTupleRef, PyTypeRef},
common::wtf8::{Wtf8, Wtf8Buf},
convert::ToPyObject,
object::{Traverse, TraverseFn},
};
Expand Down Expand Up @@ -155,10 +156,12 @@ impl FuncArgs {
.iter()
.zip(&args[nargs..nargs + names.len()])
.map(|(name, val)| {
// `PyStr`, not `PyUtf8Str`: a surrogate key is a valid str and
// must survive as WTF-8 rather than panic.
let key = name
.downcast_ref::<crate::builtins::PyUtf8Str>()
.downcast_ref::<crate::builtins::PyStr>()
.expect("kwnames must be strings")
.as_str()
.as_wtf8()
.to_owned();
(key, val.clone())
})
Expand Down Expand Up @@ -187,9 +190,9 @@ impl FuncArgs {
.zip(args.drain(nargs..nargs + kw_count))
.map(|(name, val)| {
let key = name
.downcast_ref::<crate::builtins::PyUtf8Str>()
.downcast_ref::<crate::builtins::PyStr>()
.expect("kwnames must be strings")
.as_str()
.as_wtf8()
.to_owned();
(key, val)
})
Expand Down Expand Up @@ -268,7 +271,7 @@ impl FuncArgs {
self.kwargs.swap_remove(name)
}

pub fn remaining_keywords(&mut self) -> impl Iterator<Item = (String, PyObjectRef)> + '_ {
pub fn remaining_keywords(&mut self) -> impl Iterator<Item = (Wtf8Buf, PyObjectRef)> + '_ {
self.kwargs.drain(..)
}

Expand Down Expand Up @@ -406,8 +409,12 @@ impl<T: TryFromObject> FromArgOptional for T {
/// KwArgs is only for functions that accept arbitrary keyword arguments. For
/// functions that accept only *specific* named arguments, a rust struct with
/// an appropriate FromArgs implementation must be created.
// Keys are stored as `Wtf8Buf`, not `String`, so that a lone-surrogate keyword
// name coming through `f(**d)` is preserved instead of being rejected (see
// issue #8228). `PyStr` is WTF-8 backed, and CPython only requires that a
// keyword key be a `str`, not that it be valid UTF-8.
#[derive(Clone, Debug)]
pub struct KwArgs<T = PyObjectRef>(IndexMap<String, T>);
pub struct KwArgs<T = PyObjectRef>(IndexMap<Wtf8Buf, T>);

impl<T> Default for KwArgs<T> {
fn default() -> Self {
Expand All @@ -416,7 +423,7 @@ impl<T> Default for KwArgs<T> {
}

impl<T> Deref for KwArgs<T> {
type class="x x-first x-last">String, T>;
type class="x x-first x-last">Wtf8Buf, T>;

fn deref(&self) -> &Self::Target {
&self.0
Expand All @@ -440,33 +447,56 @@ where

impl<T> KwArgs<T> {
#[must_use]
pub const fn new(map: IndexMap<String, T>) -> Self {
pub const fn new(map: IndexMap<Wtf8Buf, T>) -> Self {
Self(map)
}

// `String` keys accepted `&str` lookups for free via `Borrow<str>`; `Wtf8Buf`
// borrows only as `Wtf8`, so these inherent methods restore the `&str` interface
// via the zero-cost `Wtf8::new` cast, keeping every call site unchanged.
#[must_use]
pub fn get(&self, name: &str) -> Option<&T> {
self.0.get(Wtf8::new(name))
}

#[must_use]
pub fn contains_key(&self, name: &str) -> bool {
self.0.contains_key(Wtf8::new(name))
}

pub fn swap_remove(&mut self, name: &str) -> Option<T> {
self.0.swap_remove(Wtf8::new(name))
}

pub fn shift_remove(&mut self, name: &str) -> Option<T> {
self.0.shift_remove(Wtf8::new(name))
}

pub fn pop_kwarg(&mut self, name: &str) -> Option<T> {
self.swap_remove(name)
}
}

impl<T> FromIterator<(String, T)> for KwArgs<T> {
fn from_iter<I: IntoIterator<Item = (String, T)>>(iter: I) -> Self {
Self(iter.into_iter().collect())
// Accept any key that converts into `Wtf8Buf` (notably `String`), so existing
// call sites that build kwargs from string literals keep compiling unchanged.
impl<K: Into<Wtf8Buf>, T> FromIterator<(K, T)> for KwArgs<T> {
fn from_iter<I: IntoIterator<Item = (K, T)>>(iter: I) -> Self {
Self(iter.into_iter().map(|(k, v)| (k.into(), v)).collect())
}
}

impl<'a, T> IntoIterator for &'a KwArgs<T> {
type Item = (&'a String, &'a T);
type IntoIter = indexmap::map::Iter<'a, String, T>;
type Item = (&'a Wtf8Buf, &'a T);
type IntoIter = indexmap::map::Iter<'a, Wtf8Buf, T>;

fn into_iter(self) -> Self::IntoIter {
self.0.iter()
}
}

impl<T> IntoIterator for KwArgs<T> {
type Item = (String, T);
type IntoIter = indexmap::map::IntoIter<String, T>;
type Item = (Wtf8Buf, T);
type IntoIter = indexmap::map::IntoIter<Wtf8Buf, T>;

fn into_iter(self) -> Self::IntoIter {
self.0.into_iter()
Expand Down
13 changes: 7 additions & 6 deletions crates/vm/src/stdlib/_ast/python.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ use super::{
pub(crate) mod _ast {
use crate::{
AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine,
builtins::{PyDictRef, PySet, PyStr, PyTupleRef, PyType, PyTypeRef, PyUtf8Str},
builtins::{PyDictRef, PySet, PyStr, PyTupleRef, PyType, PyTypeRef},
class::{PyClassImpl, StaticType},
common::wtf8::Wtf8Buf,
function::{ArgIterable, FuncArgs, KwArgs, PyMethodDef, PyMethodFlags},
stdlib::_ast::repr,
types::{Constructor, Initializer},
Expand Down Expand Up @@ -229,7 +230,7 @@ pub(crate) mod _ast {
ast_replace_set_update(&expecting, attributes.as_ref(), vm)?;

for (key, _value) in &args.kwargs {
let key_obj: PyObjectRef = vm.ctx.new_str(key.as_str()).into();
let key_obj: PyObjectRef = vm.ctx.new_str(key.clone()).into();

@youknowone youknowone Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there must be a way not to use clone here. could you check? I will find out one if there isn't any way to do it yet.

@HyoJongPark HyoJongPark Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

         for (key, _value) in &args.kwargs {
-            let key_obj: PyObjectRef = vm.ctx.new_str(key.clone()).into();
+            let key_obj: PyObjectRef = vm.ctx.new_str(key.as_ref()).into();

Replaced the clone with as_ref() to avoid an unnecessary copy.
new_str only needs a &Wtf8, so instead of cloning the key into an owned Wtf8Buf, I pass key.as_ref() to borrow it instead.

Does this match what you had in mind?

if !ast_replace_set_discard(&expecting, &key_obj, vm)? {
return Err(vm.new_type_error(format!(
"{}.__replace__ got an unexpected keyword argument '{}'.",
Expand Down Expand Up @@ -290,11 +291,11 @@ pub(crate) mod _ast {
.into_iter()
.map(|(key, value)| {
let key = key
.downcast::<PyUtf8Str>()
.downcast::<PyStr>()
.map_err(|_| vm.new_type_error("keywords must be strings"))?;
Ok((key.as_str().to_owned(), value))
Ok((key.as_wtf8().to_owned(), value))
})
.collect::<PyResult<IndexMap<String, PyObjectRef>>>()?;
.collect::<PyResult<IndexMap<Wtf8Buf, PyObjectRef>>>()?;
let result = type_obj.call(FuncArgs::new(vec![], KwArgs::new(kwargs)), vm)?;
Ok(result)
}
Expand Down Expand Up @@ -418,7 +419,7 @@ pub(crate) mod _ast {
ast_replace_set_discard(&remaining_fields, &name, vm)?;
}
for (key, value) in args.kwargs {
let key_obj: PyObjectRef = vm.ctx.new_str(key.as_str()).into();
let key_obj: PyObjectRef = vm.ctx.new_str(key.clone()).into();

@youknowone youknowone Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another clone here

let contains = fields_seq.contains(&key_obj, vm)?;
if contains {
if !ast_replace_set_discard(&remaining_fields, &key_obj, vm)? {
Expand Down
9 changes: 5 additions & 4 deletions crates/vm/src/stdlib/_ctypes/structure.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::base::{CDATA_BUFFER_METHODS, PyCData, PyCField, StgInfo, StgInfoFlags};
use crate::builtins::{PyList, PyStr, PyTuple, PyType, PyTypeRef, PyUtf8Str};
use crate::common::wtf8::Wtf8Buf;
use crate::convert::ToPyObject;
use crate::function::{FuncArgs, OptionalArg, PySetterValue};
use crate::protocol::{BufferDescriptor, PyBuffer, PyNumberMethods};
Expand Down Expand Up @@ -712,7 +713,7 @@ impl PyCStructure {
self_obj: &Py<Self>,
type_obj: &Py<PyType>,
args: &[PyObjectRef],
kwargs: &indexmap::IndexMap<String, PyObjectRef>,
kwargs: &indexmap::IndexMap<Wtf8Buf, PyObjectRef>,
index: usize,
vm: &VirtualMachine,
) -> PyResult<usize> {
Expand Down Expand Up @@ -746,7 +747,7 @@ impl PyCStructure {
&& let Some(name) = tuple.first()
&& let Some(name_str) = name.downcast_ref::<PyUtf8Str>()
{
let field_name = name_str.as_str().to_owned();
let field_name = name_str.as_wtf8().to_owned();
// Check for duplicate in kwargs
if kwargs.contains_key(&field_name) {
return Err(
Expand Down Expand Up @@ -784,9 +785,9 @@ impl Initializer for PyCStructure {
}

// 2. Process keyword arguments
for (key, value) in &args.kwargs {
for (key, value) in args.kwargs {
zelf.as_object()
.set_attr(vm.ctx.intern_str(key.as_str()), value.clone(), vm)?;
.set_attr(vm.ctx.intern_str(key), value, vm)?;
}

Ok(())
Expand Down
9 changes: 5 additions & 4 deletions crates/vm/src/stdlib/_ctypes/union.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::base::{CDATA_BUFFER_METHODS, StgInfoFlags};
use super::{PyCData, PyCField, StgInfo};
use crate::builtins::{PyList, PyStr, PyTuple, PyType, PyTypeRef, PyUtf8Str};
use crate::common::wtf8::Wtf8Buf;
use crate::convert::ToPyObject;
use crate::function::{ArgBytesLike, FuncArgs, OptionalArg, PySetterValue};
use crate::protocol::{BufferDescriptor, PyBuffer};
Expand Down Expand Up @@ -581,7 +582,7 @@ impl PyCUnion {
self_obj: &Py<Self>,
type_obj: &Py<PyType>,
args: &[PyObjectRef],
kwargs: &indexmap::IndexMap<String, PyObjectRef>,
kwargs: &indexmap::IndexMap<Wtf8Buf, PyObjectRef>,
index: usize,
vm: &VirtualMachine,
) -> PyResult<usize> {
Expand Down Expand Up @@ -617,7 +618,7 @@ impl PyCUnion {
&& let Some(name) = tuple.first()
&& let Some(name_str) = name.downcast_ref::<PyUtf8Str>()
{
let field_name = name_str.as_str().to_owned();
let field_name = name_str.as_wtf8().to_owned();
// Check for duplicate in kwargs
if kwargs.contains_key(&field_name) {
return Err(
Expand Down Expand Up @@ -655,9 +656,9 @@ impl Initializer for PyCUnion {
}

// 2. Process keyword arguments
for (key, value) in &args.kwargs {
for (key, value) in args.kwargs {
zelf.as_object()
.set_attr(vm.ctx.intern_str(key.as_str()), value.clone(), vm)?;
.set_attr(vm.ctx.intern_str(key), value, vm)?;
}

Ok(())
Expand Down
5 changes: 3 additions & 2 deletions crates/vm/src/stdlib/_functools.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ mod _functools {

// Add new keywords
for (key, value) in args.kwargs {
final_keywords.set_item(vm.ctx.intern_str(key.as_str()), value, vm)?;
final_keywords.set_item(vm.ctx.intern_str(key), value, vm)?;
}

Ok(Self {
Expand Down Expand Up @@ -436,10 +436,11 @@ mod _functools {

// Add keywords from self.keywords
for (key, value) in &*keywords {
// `expect_str()` would panic on surrogate keys; keep them as WTF-8.
let key_str = key
.downcast_ref::<crate::builtins::PyStr>()
.ok_or_else(|| vm.new_type_error("keywords must be strings"))?;
final_kwargs.insert(key_str.expect_str().to_owned(), value);
final_kwargs.insert(key_str.as_wtf8().to_owned(), value);
}

// Add keywords from args.kwargs (these override self.keywords)
Expand Down
2 changes: 1 addition & 1 deletion crates/vm/src/stdlib/_operator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ mod _operator {
}
for (key, value) in kwargs {
result.push_str(", ");
result.push_str(key);
result.push_wtf8(key);
result.push_char('=');
result.push_wtf8(value.repr(vm)?.as_wtf8());
}
Expand Down
4 changes: 2 additions & 2 deletions crates/vm/src/stdlib/_typing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,9 +353,9 @@ pub(crate) mod decl {
// typealias(name, value, *, type_params=())
// name and value are positional-or-keyword; type_params is keyword-only.

// Reject unexpected keyword arguments
// Reject unexpected keyword arguments.
for key in args.kwargs.keys() {
if key != "name" && key != "value" && key != "type_params" {
if !matches!(key.as_str(), Ok("name" | "value" | "type_params")) {
return Err(vm.new_type_error(format!(
"typealias() got an unexpected keyword argument '{key}'"
)));
Expand Down
Loading
Loading