pyo3_benchmarks: benchmarks as separate Python example

examples/pyo3_benchmarks/Cargo.toml

@@ -0,0 +1,16 @@
+[package]
+authors = ["PyO3 Authors"]
+name = "pyo3-benchmarks"
+version = "0.1.0"
+description = "Python-based benchmarks for various PyO3 functionality"
+edition = "2018"
+
+[dependencies]
+
+[dependencies.pyo3]
+path = "../../"
+features = ["extension-module"]
+
+[lib]
+name = "_pyo3_benchmarks"
+crate-type = ["cdylib"]

examples/pyo3_benchmarks/MANIFEST.in

@@ -0,0 +1,2 @@
+include pyproject.toml Cargo.toml
+recursive-include src *

examples/pyo3_benchmarks/README.md

@@ -0,0 +1,17 @@
+# rustapi_module
+
+A simple extension module built using PyO3.
+
+## Build
+
+```shell
+python setup.py install
+```
+
+## Testing
+
+To test install tox globally and run
+
+```shell
+tox -e py
+```

examples/pyo3_benchmarks/pyo3_benchmarks/__init__.py

@@ -0,0 +1 @@
+from ._pyo3_benchmarks import *

examples/pyo3_benchmarks/requirements-dev.txt

@@ -0,0 +1,6 @@
+pip>=19.1
+hypothesis>=3.55
+pytest>=3.5.0
+setuptools-rust>=0.10.2
+psutil>=5.6
+pytest-benchmark~=3.2

examples/pyo3_benchmarks/setup.py

@@ -0,0 +1,44 @@
+import sys
+import platform
+
+from setuptools import setup
+from setuptools_rust import RustExtension
+
+
+def get_py_version_cfgs():
+    # For now each Cfg Py_3_X flag is interpreted as "at least 3.X"
+    version = sys.version_info[0:2]
+    py3_min = 6
+    out_cfg = []
+    for minor in range(py3_min, version[1] + 1):
+        out_cfg.append("--cfg=Py_3_%d" % minor)
+
+    if platform.python_implementation() == "PyPy":
+        out_cfg.append("--cfg=PyPy")
+
+    return out_cfg
+
+
+setup(
+    name="pyo3-benchmarks",
+    version="0.1.0",
+    classifiers=[
+        "License :: OSI Approved :: MIT License",
+        "Development Status :: 3 - Alpha",
+        "Intended Audience :: Developers",
+        "Programming Language :: Python",
+        "Programming Language :: Rust",
+        "Operating System :: POSIX",
+        "Operating System :: MacOS :: MacOS X",
+    ],
+    packages=["pyo3_benchmarks"],
+    rust_extensions=[
+        RustExtension(
+            "pyo3_benchmarks._pyo3_benchmarks",
+            rustc_flags=get_py_version_cfgs(),
+            debug=False,
+        ),
+    ],
+    include_package_data=True,
+    zip_safe=False,
+)

examples/pyo3_benchmarks/src/lib.rs

@@ -0,0 +1,33 @@
+use pyo3::prelude::*;
+use pyo3::types::{PyDict, PyTuple};
+use pyo3::wrap_pyfunction;
+
+#[pyfunction(args = "*", kwargs = "**")]
+fn args_and_kwargs<'a>(
+    args: &'a PyTuple,
+    kwargs: Option<&'a PyDict>,
+) -> (&'a PyTuple, Option<&'a PyDict>) {
+    (args, kwargs)
+}
+
+#[pyfunction(a, b = 2, args = "*", c = 4, kwargs = "**")]
+fn mixed_args<'a>(
+    a: i32,
+    b: i32,
+    args: &'a PyTuple,
+    c: i32,
+    kwargs: Option<&'a PyDict>,
+) -> (i32, i32, &'a PyTuple, i32, Option<&'a PyDict>) {
+    (a, b, args, c, kwargs)
+}
+
+#[pyfunction]
+fn no_args() {}
+
+#[pymodule]
+fn _pyo3_benchmarks(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
+    m.add_function(wrap_pyfunction!(args_and_kwargs, m)?)?;
+    m.add_function(wrap_pyfunction!(mixed_args, m)?)?;
+    m.add_function(wrap_pyfunction!(no_args, m)?)?;
+    Ok(())
+}

examples/pyo3_benchmarks/tests/test_benchmarks.py

@@ -0,0 +1,46 @@
+import pyo3_benchmarks
+
+
+def test_args_and_kwargs(benchmark):
+    benchmark(pyo3_benchmarks.args_and_kwargs, 1, 2, 3, a=4, foo=10)
+
+
+def args_and_kwargs_py(*args, **kwargs):
+    return (args, kwargs)
+
+
+def test_args_and_kwargs_py(benchmark):
+    rust = pyo3_benchmarks.args_and_kwargs(1, 2, 3, bar=4, foo=10)
+    py = args_and_kwargs_py(1, 2, 3, bar=4, foo=10)
+    assert rust == py
+    benchmark(args_and_kwargs_py, 1, 2, 3, bar=4, foo=10)
+
+
+def test_mixed_args(benchmark):
+    benchmark(pyo3_benchmarks.mixed_args, 1, 2, 3, bar=4, foo=10)
+
+
+def mixed_args_py(a, b=2, *args, c=4, **kwargs):
+    return (a, b, args, c, kwargs)
+
+
+def test_mixed_args_py(benchmark):
+    rust = pyo3_benchmarks.mixed_args(1, 2, 3, bar=4, foo=10)
+    py = mixed_args_py(1, 2, 3, bar=4, foo=10)
+    assert rust == py
+    benchmark(mixed_args_py, 1, 2, 3, bar=4, foo=10)
+
+
+def test_no_args(benchmark):
+    benchmark(pyo3_benchmarks.no_args)
+
+
+def no_args_py():
+    return None
+
+
+def test_no_args_py(benchmark):
+    rust = pyo3_benchmarks.no_args()
+    py = no_args_py()
+    assert rust == py
+    benchmark(no_args_py)

examples/pyo3_benchmarks/tox.ini

@@ -0,0 +1,10 @@
+[tox]
+# can't install from sdist because local pyo3 repo can't be included in the sdist
+skipsdist = true
+
+[testenv]
+description = Run the unit tests under {basepython}
+deps = -rrequirements-dev.txt
+commands =
+    python setup.py install
+    pytest {posargs}

pyo3-macros-backend/src/module.rs

@@ -8,7 +8,7 @@ use crate::pymethod::get_arg_names;
 use crate::utils;
 use proc_macro2::{Span, TokenStream};
 use quote::{format_ident, quote};
-use syn::{spanned::Spanned, Ident};
+use syn::{spanned::Spanned, Ident, Result};
 
 /// Generates the function that is called by the python interpreter to initialize the native
 /// module
@@ -197,7 +197,7 @@ pub fn add_fn_to_module(
 
     let name = &func.sig.ident;
     let wrapper_ident = format_ident!("__pyo3_raw_{}", name);
-    let wrapper = function_c_wrapper(name, &wrapper_ident, &spec, pyfn_attrs.pass_module);
+    let wrapper = function_c_wrapper(name, &wrapper_ident, &spec, pyfn_attrs.pass_module)?;
     Ok(quote! {
         #wrapper
         pub(crate) fn #function_wrapper_ident<'a>(
@@ -223,7 +223,7 @@ fn function_c_wrapper(
     wrapper_ident: &Ident,
     spec: &method::FnSpec<'_>,
     pass_module: bool,
-) -> TokenStream {
+) -> Result<TokenStream> {
     let names: Vec<Ident> = get_arg_names(&spec);
     let cb;
     let slf_module;
@@ -240,8 +240,8 @@ fn function_c_wrapper(
         };
         slf_module = None;
     };
-    let body = pymethod::impl_arg_params(spec, None, cb);
-    quote! {
+    let body = pymethod::impl_arg_params(spec, None, cb)?;
+    Ok(quote! {
         unsafe extern "C" fn #wrapper_ident(
             _slf: *mut pyo3::ffi::PyObject,
             _args: *mut pyo3::ffi::PyObject,
@@ -256,5 +256,5 @@ fn function_c_wrapper(
                 #body
             })
         }
-    }
+    })
 }

pyo3-macros-backend/src/pymethod.rs

@@ -4,8 +4,8 @@ use crate::konst::ConstSpec;
 use crate::method::{FnArg, FnSpec, FnType, SelfType};
 use crate::utils;
 use proc_macro2::{Span, TokenStream};
-use quote::quote;
-use syn::{ext::IdentExt, spanned::Spanned};
+use quote::{quote, quote_spanned};
+use syn::{ext::IdentExt, spanned::Spanned, Result};
 
 pub enum PropertyType<'a> {
     Descriptor(&'a syn::Field),
@@ -22,29 +22,29 @@ pub fn gen_py_method(
     cls: &syn::Type,
     sig: &mut syn::Signature,
     meth_attrs: &mut Vec<syn::Attribute>,
-) -> syn::Result<GeneratedPyMethod> {
+) -> Result<GeneratedPyMethod> {
     check_generic(sig)?;
     let spec = FnSpec::parse(sig, &mut *meth_attrs, true)?;
 
     Ok(match &spec.tp {
         FnType::Fn(self_ty) => GeneratedPyMethod::Method(impl_py_method_def(
             &spec,
-            &impl_wrap(cls, &spec, self_ty, true),
+            &impl_wrap(cls, &spec, self_ty, true)?,
         )),
         FnType::FnNew => {
-            GeneratedPyMethod::New(impl_py_method_def_new(cls, &impl_wrap_new(cls, &spec)))
+            GeneratedPyMethod::New(impl_py_method_def_new(cls, &impl_wrap_new(cls, &spec)?))
         }
         FnType::FnCall(self_ty) => GeneratedPyMethod::Call(impl_py_method_def_call(
             cls,
-            &impl_wrap(cls, &spec, self_ty, false),
+            &impl_wrap(cls, &spec, self_ty, false)?,
         )),
         FnType::FnClass => GeneratedPyMethod::Method(impl_py_method_def_class(
             &spec,
-            &impl_wrap_class(cls, &spec),
+            &impl_wrap_class(cls, &spec)?,
         )),
         FnType::FnStatic => GeneratedPyMethod::Method(impl_py_method_def_static(
             &spec,
-            &impl_wrap_static(cls, &spec),
+            &impl_wrap_static(cls, &spec)?,
         )),
         FnType::ClassAttribute => GeneratedPyMethod::Method(impl_py_method_class_attribute(
             &spec,
@@ -98,7 +98,7 @@ pub fn impl_wrap(
     spec: &FnSpec<'_>,
     self_ty: &SelfType,
     noargs: bool,
-) -> TokenStream {
+) -> Result<TokenStream> {
     let body = impl_call(cls, &spec);
     let slf = self_ty.receiver(cls);
     impl_wrap_common(cls, spec, noargs, slf, body)
@@ -110,10 +110,10 @@ fn impl_wrap_common(
     noargs: bool,
     slf: TokenStream,
     body: TokenStream,
-) -> TokenStream {
+) -> Result<TokenStream> {
     let python_name = &spec.python_name;
     if spec.args.is_empty() && noargs {
-        quote! {
+        Ok(quote! {
             unsafe extern "C" fn __wrap(
                 _slf: *mut pyo3::ffi::PyObject,
                 _args: *mut pyo3::ffi::PyObject,
@@ -126,11 +126,10 @@ fn impl_wrap_common(
                     pyo3::callback::convert(_py, #body)
                 })
             }
-        }
+        })
     } else {
-        let body = impl_arg_params(&spec, Some(cls), body);
-
-        quote! {
+        let body = impl_arg_params(&spec, Some(cls), body)?;
+        Ok(quote! {
             unsafe extern "C" fn __wrap(
                 _slf: *mut pyo3::ffi::PyObject,
                 _args: *mut pyo3::ffi::PyObject,
@@ -146,18 +145,22 @@ fn impl_wrap_common(
                     pyo3::callback::convert(_py, #body)
                 })
             }
-        }
+        })
     }
 }
 
 /// Generate function wrapper for protocol method (PyCFunction, PyCFunctionWithKeywords)
-pub fn impl_proto_wrap(cls: &syn::Type, spec: &FnSpec<'_>, self_ty: &SelfType) -> TokenStream {
+pub fn impl_proto_wrap(
+    cls: &syn::Type,
+    spec: &FnSpec<'_>,
+    self_ty: &SelfType,
+) -> Result<TokenStream> {
     let python_name = &spec.python_name;
     let cb = impl_call(cls, &spec);
-    let body = impl_arg_params(&spec, Some(cls), cb);
+    let body = impl_arg_params(&spec, Some(cls), cb)?;
     let slf = self_ty.receiver(cls);
 
-    quote! {
+    Ok(quote! {
         #[allow(unused_mut)]
         unsafe extern "C" fn __wrap(
             _slf: *mut pyo3::ffi::PyObject,
@@ -173,27 +176,25 @@ pub fn impl_proto_wrap(cls: &syn::Type, spec: &FnSpec<'_>, self_ty: &SelfType) -
                 pyo3::callback::convert(_py, #body)
             })
         }
-    }
+    })
 }
 
 /// Generate class method wrapper (PyCFunction, PyCFunctionWithKeywords)
-pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream {
+pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> Result<TokenStream> {
     let name = &spec.name;
     let python_name = &spec.python_name;
     let names: Vec<syn::Ident> = get_arg_names(&spec);
     let cb = quote! { #cls::#name(#(#names),*) };
-    let body = impl_arg_params(spec, Some(cls), cb);
+    let body = impl_arg_params(spec, Some(cls), cb)?;
 
-    quote! {
+    Ok(quote! {
         #[allow(unused_mut)]
         unsafe extern "C" fn __wrap(
             subtype: *mut pyo3::ffi::PyTypeObject,
             _args: *mut pyo3::ffi::PyObject,
             _kwargs: *mut pyo3::ffi::PyObject) -> *mut pyo3::ffi::PyObject
         {
-            use pyo3::type_object::PyTypeInfo;
             use pyo3::callback::IntoPyCallbackOutput;
-            use std::convert::TryFrom;
 
             const _LOCATION: &'static str = concat!(stringify!(#cls),".",stringify!(#python_name),"()");
             pyo3::callback_body_without_convert!(_py, {
@@ -205,19 +206,19 @@ pub fn impl_wrap_new(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream {
                 Ok(cell as *mut pyo3::ffi::PyObject)
             })
         }
-    }
+    })
 }
 
 /// Generate class method wrapper (PyCFunction, PyCFunctionWithKeywords)
-pub fn impl_wrap_class(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream {
+pub fn impl_wrap_class(cls: &syn::Type, spec: &FnSpec<'_>) -> Result<TokenStream> {
     let name = &spec.name;
     let python_name = &spec.python_name;
     let names: Vec<syn::Ident> = get_arg_names(&spec);
     let cb = quote! { #cls::#name(&_cls, #(#names),*) };
 
-    let body = impl_arg_params(spec, Some(cls), cb);
+    let body = impl_arg_params(spec, Some(cls), cb)?;
 
-    quote! {
+    Ok(quote! {
         #[allow(unused_mut)]
         unsafe extern "C" fn __wrap(
             _cls: *mut pyo3::ffi::PyObject,
@@ -233,19 +234,19 @@ pub fn impl_wrap_class(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream {
                 pyo3::callback::convert(_py, #body)
             })
         }
-    }
+    })
 }
 
 /// Generate static method wrapper (PyCFunction, PyCFunctionWithKeywords)
-pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream {
+pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> Result<TokenStream> {
     let name = &spec.name;
     let python_name = &spec.python_name;
     let names: Vec<syn::Ident> = get_arg_names(&spec);
     let cb = quote! { #cls::#name(#(#names),*) };
 
-    let body = impl_arg_params(spec, Some(cls), cb);
+    let body = impl_arg_params(spec, Some(cls), cb)?;
 
-    quote! {
+    Ok(quote! {
         #[allow(unused_mut)]
         unsafe extern "C" fn __wrap(
             _slf: *mut pyo3::ffi::PyObject,
@@ -260,7 +261,7 @@ pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream {
                 pyo3::callback::convert(_py, #body)
             })
         }
-    }
+    })
 }
 
 /// Generate a wrapper for initialization of a class attribute from a method
@@ -399,11 +400,9 @@ pub fn impl_arg_params(
     spec: &FnSpec<'_>,
     self_: Option<&syn::Type>,
     body: TokenStream,
-) -> TokenStream {
+) -> Result<TokenStream> {
     if spec.args.is_empty() {
-        return quote! {
-            #body
-        };
+        return Ok(body);
     }
 
     let mut positional_parameter_names = Vec::new();
@@ -438,7 +437,7 @@ pub fn impl_arg_params(
     let mut param_conversion = Vec::new();
     let mut option_pos = 0;
     for (idx, arg) in spec.args.iter().enumerate() {
-        param_conversion.push(impl_arg_param(&arg, &spec, idx, self_, &mut option_pos));
+        param_conversion.push(impl_arg_param(&arg, &spec, idx, self_, &mut option_pos)?);
     }
 
     let (mut accept_args, mut accept_kwargs) = (false, false);
@@ -453,25 +452,27 @@ pub fn impl_arg_params(
     }
 
     // create array of arguments, and then parse
-    quote! {{
-        const DESCRIPTION: pyo3::derive_utils::FunctionDescription = pyo3::derive_utils::FunctionDescription {
-            fname: _LOCATION,
-            positional_parameter_names: &[#(#positional_parameter_names),*],
-            // TODO: https://github.com/PyO3/pyo3/issues/1439 - support specifying these
-            positional_only_parameters: 0,
-            required_positional_parameters: #required_positional_parameters,
-            keyword_only_parameters: &[#(#keyword_only_parameters),*],
-            accept_varargs: #accept_args,
-            accept_varkeywords: #accept_kwargs,
-        };
+    Ok(quote! {
+        {
+            const DESCRIPTION: pyo3::derive_utils::FunctionDescription = pyo3::derive_utils::FunctionDescription {
+                fname: _LOCATION,
+                positional_parameter_names: &[#(#positional_parameter_names),*],
+                // TODO: https://github.com/PyO3/pyo3/issues/1439 - support specifying these
+                positional_only_parameters: 0,
+                required_positional_parameters: #required_positional_parameters,
+                keyword_only_parameters: &[#(#keyword_only_parameters),*],
+                accept_varargs: #accept_args,
+                accept_varkeywords: #accept_kwargs,
+            };
 
-        let mut output = [None; #num_params];
-        let (_args, _kwargs) = DESCRIPTION.extract_arguments(_args, _kwargs, &mut output)?;
+            let mut output = [None; #num_params];
+            let (_args, _kwargs) = DESCRIPTION.extract_arguments(_args, _kwargs, &mut output)?;
 
-        #(#param_conversion)*
+            #(#param_conversion)*
 
-        #body
-    }}
+            #body
+        }
+    })
 }
 
 /// Re option_pos: The option slice doesn't contain the py: Python argument, so the argument
@@ -482,13 +483,14 @@ fn impl_arg_param(
     idx: usize,
     self_: Option<&syn::Type>,
     option_pos: &mut usize,
-) -> TokenStream {
+) -> Result<TokenStream> {
     let arg_name = syn::Ident::new(&format!("arg{}", idx), Span::call_site());
 
     if arg.py {
-        return quote! {
+        return Ok(quote_spanned! {
+            arg.ty.span() =>
             let #arg_name = _py;
-        };
+        });
     }
 
     let ty = arg.ty;
@@ -498,29 +500,26 @@ fn impl_arg_param(
     };
 
     if spec.is_args(&name) {
-        return if arg.optional.is_some() {
-            quote! {
-                let #arg_name = _args.map(|args| args.extract())
-                    .transpose()
-                    .map_err(#transform_error)?;
-            }
-        } else {
-            quote! {
-                let #arg_name = _args.unwrap().extract()
-                    .map_err(#transform_error)?;
-            }
-        };
+        ensure_spanned!(
+            arg.optional.is_none(),
+            arg.name.span() => "args cannot be optional"
+        );
+        return Ok(quote_spanned! {
+            arg.ty.span() =>
+            let #arg_name = _args.unwrap().extract()
+                .map_err(#transform_error)?;
+        });
     } else if spec.is_kwargs(&name) {
-        // FIXME: check the below?
-        // ensure_spanned!(
-        //     arg.optional.is_some(),
-        //     arg.name.span() => "kwargs must be Option<_>"
-        // );
-        return quote! {
+        ensure_spanned!(
+            arg.optional.is_some(),
+            arg.name.span() => "kwargs must be Option<_>"
+        );
+        return Ok(quote_spanned! {
+            arg.ty.span() =>
             let #arg_name = _kwargs.map(|kwargs| kwargs.extract())
                 .transpose()
                 .map_err(#transform_error)?;
-        };
+        });
     }
 
     let arg_value = quote!(output[#option_pos]);
@@ -559,20 +558,22 @@ fn impl_arg_param(
             )
         };
 
-        quote! {
+        Ok(quote_spanned! {
+            arg.ty.span() =>
             let #mut_ _tmp: #target_ty = match #arg_value {
                 Some(_obj) => #extract,
                 None => #default,
             };
             let #arg_name = #borrow_tmp;
-        }
+        })
     } else {
-        quote! {
+        Ok(quote_spanned! {
+            arg.ty.span() =>
             let #arg_name = match #arg_value {
                 Some(_obj) => #extract,
                 None => #default,
             };
-        }
+        })
     };
 
     /// Replace `Self`, remove lifetime and get mutability from the type

pyo3-macros-backend/src/pyproto.rs

@@ -65,7 +65,7 @@ fn impl_proto_impl(
                 let fn_spec = FnSpec::parse(&mut met.sig, &mut met.attrs, false)?;
 
                 let method = if let FnType::Fn(self_ty) = &fn_spec.tp {
-                    pymethod::impl_proto_wrap(ty, &fn_spec, &self_ty)
+                    pymethod::impl_proto_wrap(ty, &fn_spec, &self_ty)?
                 } else {
                     bail_spanned!(
                         met.sig.span() => "expected method with receiver for #[pyproto] method"

src/derive_utils.rs

@@ -89,12 +89,12 @@ impl FunctionDescription {
 
         // Check that there's sufficient positional arguments once keyword arguments are specified
         if args_provided < self.required_positional_parameters {
-            let missing_positional_arguments: Vec<_> = self.positional_parameter_names
-                [..self.required_positional_parameters]
+            let missing_positional_arguments: Vec<_> = self
+                .positional_parameter_names
                 .iter()
-                .copied()
+                .take(self.required_positional_parameters)
                 .zip(output.iter())
-                .filter_map(|(param, out)| if out.is_none() { Some(param) } else { None })
+                .filter_map(|(param, out)| if out.is_none() { Some(*param) } else { None })
                 .collect();
             if !missing_positional_arguments.is_empty() {
                 return Err(
@@ -148,33 +148,30 @@ impl FunctionDescription {
             // Compare the keyword name against each parameter in turn. This is exactly the same method
             // which CPython uses to map keyword names. Although it's O(num_parameters), the number of
             // parameters is expected to be small so it's not worth constructing a mapping.
-            for (param, out) in self.keyword_only_parameters.iter().zip(&mut *kwargs_output) {
-                if utf8_string == param.name {
-                    *out = Some(value);
-                    continue 'kwarg_loop;
-                }
+            if let Some(i) = self
+                .keyword_only_parameters
+                .iter()
+                .position(|param| utf8_string == param.name)
+            {
+                kwargs_output[i] = Some(value);
+                continue 'kwarg_loop;
             }
 
             // Repeat for positional parameters
-            for (i, (&param, out)) in self
+            if let Some((i, param)) = self
                 .positional_parameter_names
                 .iter()
-                .zip(&mut *args_output)
                 .enumerate()
+                .find(|&(_, param)| utf8_string == *param)
             {
-                if utf8_string == param {
-                    if i < self.positional_only_parameters {
-                        positional_only_keyword_arguments.push(param);
-                    } else {
-                        match out {
-                            Some(_) => return Err(self.multiple_values_for_argument(param)),
-                            None => {
-                                *out = Some(value);
-                            }
-                        }
+                if i < self.positional_only_parameters {
+                    positional_only_keyword_arguments.push(*param);
+                } else {
+                    if args_output[i].replace(value).is_some() {
+                        return Err(self.multiple_values_for_argument(param));
                     }
-                    continue 'kwarg_loop;
                 }
+                continue 'kwarg_loop;
             }
 
             unexpected_keyword_handler(kwarg_name, value)?;
@@ -229,7 +226,7 @@ impl FunctionDescription {
             "{} got some positional-only arguments passed as keyword arguments: ",
             self.fname
         );
-        write_parameter_list(&mut msg, parameter_names);
+        push_parameter_list(&mut msg, parameter_names);
         PyTypeError::new_err(msg)
     }
 
@@ -246,7 +243,7 @@ impl FunctionDescription {
             argument_type,
             arguments,
         );
-        write_parameter_list(&mut msg, parameter_names);
+        push_parameter_list(&mut msg, parameter_names);
         PyTypeError::new_err(msg)
     }
 }
@@ -392,14 +389,18 @@ impl<'a> From<&'a PyModule> for PyFunctionArguments<'a> {
     }
 }
 
-fn write_parameter_list(msg: &mut String, parameter_names: &[&str]) {
+fn push_parameter_list(msg: &mut String, parameter_names: &[&str]) {
     for (i, parameter) in parameter_names.iter().enumerate() {
-        if i != 0 && parameter_names.len() > 2 {
-            msg.push(',');
-        }
+        if i != 0 {
+            if parameter_names.len() > 2 {
+                msg.push(',');
+            }
 
-        if i == parameter_names.len() - 1 {
-            msg.push_str(" and ")
+            if i == parameter_names.len() - 1 {
+                msg.push_str(" and ")
+            } else {
+                msg.push(' ')
+            }
         }
 
         msg.push('\'');
@@ -407,3 +408,43 @@ fn write_parameter_list(msg: &mut String, parameter_names: &[&str]) {
         msg.push('\'');
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::push_parameter_list;
+
+    #[test]
+    fn push_parameter_list_empty() {
+        let mut s = String::new();
+        push_parameter_list(&mut s, &[]);
+        assert_eq!(&s, "");
+    }
+
+    #[test]
+    fn push_parameter_list_one() {
+        let mut s = String::new();
+        push_parameter_list(&mut s, &["a"]);
+        assert_eq!(&s, "'a'");
+    }
+
+    #[test]
+    fn push_parameter_list_two() {
+        let mut s = String::new();
+        push_parameter_list(&mut s, &["a", "b"]);
+        assert_eq!(&s, "'a' and 'b'");
+    }
+
+    #[test]
+    fn push_parameter_list_three() {
+        let mut s = String::new();
+        push_parameter_list(&mut s, &["a", "b", "c"]);
+        assert_eq!(&s, "'a', 'b', and 'c'");
+    }
+
+    #[test]
+    fn push_parameter_list_four() {
+        let mut s = String::new();
+        push_parameter_list(&mut s, &["a", "b", "c", "d"]);
+        assert_eq!(&s, "'a', 'b', 'c', and 'd'");
+    }
+}