Skip to content
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.

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

add #[text_signature = "..."] attribute #675

Merged
merged 8 commits into from
Dec 7, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## Unreleased

### Added

* Support for `#[text_signature]` attribute. [#675](https://github.com/PyO3/pyo3/pull/675)

## [0.8.3]

### Fixed
Expand Down
74 changes: 71 additions & 3 deletions guide/src/function.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,72 @@ fn module_with_functions(py: Python, m: &PyModule) -> PyResult<()> {
# fn main() {}
```

### Making the function signature available to Python
## Making the function signature available to Python

In order to make the function signature available to Python to be retrieved via
`inspect.signature`, simply make sure the first line of your docstring is
formatted like in the example below. Please note that the newline after the
`inspect.signature`, use the `#[text_signature]` annotation as in the example
below. The `/` signifies the end of positional-only arguments.

```rust
use pyo3::prelude::*;

/// This function adds two unsigned 64-bit integers.
#[pyfunction]
#[text_signature = "(a, b, /)"]
fn add(a: u64, b: u64) -> u64 {
a + b
}
```

This also works for classes and methods:

```rust
use pyo3::prelude::*;
use pyo3::types::PyType;

// it works even if the item is not documented:

#[pyclass]
#[text_signature = "(c, d, /)"]
struct MyClass {}

#[pymethods]
impl MyClass {
// the signature for the constructor is attached
// to the struct definition instead.
#[new]
fn new(obj: &PyRawObject, c: i32, d: &str) {
obj.init(Self {});
}
// the self argument should be written $self
#[text_signature = "($self, e, f)"]
fn my_method(&self, e: i32, f: i32) -> i32 {
e + f
}
#[classmethod]
#[text_signature = "(cls, e, f)"]
fn my_class_method(cls: &PyType, e: i32, f: i32) -> i32 {
e + f
}
#[staticmethod]
#[text_signature = "(e, f)"]
fn my_static_method(e: i32, f: i32) -> i32 {
e + f
}
}
```

### Making the function signature available to Python (old method)

Alternatively, simply make sure the first line of your docstring is
formatted like in the following example. Please note that the newline after the
`--` is mandatory. The `/` signifies the end of positional-only arguments. This
is not a feature of this library in particular, but the general format used by
CPython for annotating signatures of built-in functions.

`#[text_signature]` should be preferred, since it will override automatically
generated signatures when those are added in a future version of PyO3.

```rust
use pyo3::prelude::*;

Expand All @@ -94,6 +151,17 @@ use pyo3::prelude::*;
fn add(a: u64, b: u64) -> u64 {
a + b
}

// a function with a signature but without docs. Both blank lines after the `--` are mandatory.

/// sub(a, b, /)
/// --
///
///
#[pyfunction]
fn sub(a: u64, b: u64) -> u64 {
a - b
}
```

When annotated like this, signatures are also correctly displayed in IPython.
Expand Down
2 changes: 1 addition & 1 deletion pyo3-derive-backend/src/method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl<'a> FnSpec<'a> {
pub fn parse(
name: &'a syn::Ident,
sig: &'a syn::Signature,
meth_attrs: &'a mut Vec<syn::Attribute>,
meth_attrs: &mut Vec<syn::Attribute>,
) -> syn::Result<FnSpec<'a>> {
let (mut fn_type, fn_attrs) = parse_attributes(meth_attrs)?;

Expand Down
11 changes: 9 additions & 2 deletions pyo3-derive-backend/src/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ fn function_wrapper_ident(name: &Ident) -> Ident {
/// Generates python wrapper over a function that allows adding it to a python module as a python
/// function
pub fn add_fn_to_module(
func: &syn::ItemFn,
func: &mut syn::ItemFn,
python_name: &Ident,
pyfn_attrs: Vec<pyfunction::Argument>,
) -> TokenStream {
Expand All @@ -157,7 +157,14 @@ pub fn add_fn_to_module(
let function_wrapper_ident = function_wrapper_ident(&func.sig.ident);

let wrapper = function_c_wrapper(&func.sig.ident, &spec);
let doc = utils::get_doc(&func.attrs, true);
let text_signature = match utils::parse_text_signature_attrs(&mut func.attrs, python_name) {
programmerjake marked this conversation as resolved.
Show resolved Hide resolved
Ok(text_signature) => text_signature,
Err(err) => return err.to_compile_error(),
};
let doc = match utils::get_doc(&func.attrs, text_signature, true) {
programmerjake marked this conversation as resolved.
Show resolved Hide resolved
Ok(doc) => doc,
Err(err) => return err.to_compile_error(),
};

let tokens = quote! {
fn #function_wrapper_ident(py: pyo3::Python) -> pyo3::PyObject {
Expand Down
18 changes: 13 additions & 5 deletions pyo3-derive-backend/src/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,11 @@ impl PyClassArgs {
}

pub fn build_py_class(class: &mut syn::ItemStruct, attr: &PyClassArgs) -> syn::Result<TokenStream> {
let doc = utils::get_doc(&class.attrs, true);
let text_signature = utils::parse_text_signature_attrs(
&mut class.attrs,
&get_class_python_name(&class.ident, attr),
)?;
let doc = utils::get_doc(&class.attrs, text_signature, true)?;
let mut descriptors = Vec::new();

check_generics(class)?;
Expand Down Expand Up @@ -245,16 +249,20 @@ fn impl_inventory(cls: &syn::Ident) -> TokenStream {
}
}

fn get_class_python_name(cls: &syn::Ident, attr: &PyClassArgs) -> TokenStream {
match &attr.name {
Some(name) => quote! { #name },
None => quote! { #cls },
}
}

fn impl_class(
cls: &syn::Ident,
attr: &PyClassArgs,
doc: syn::Lit,
descriptors: Vec<(syn::Field, Vec<FnType>)>,
) -> TokenStream {
let cls_name = match &attr.name {
Some(name) => quote! { #name }.to_string(),
None => cls.to_string(),
};
let cls_name = get_class_python_name(cls, attr).to_string();

let extra = {
if let Some(freelist) = &attr.freelist {
Expand Down
52 changes: 50 additions & 2 deletions pyo3-derive-backend/src/pymethod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,56 @@ pub fn gen_py_method(
) -> syn::Result<TokenStream> {
check_generic(name, sig)?;

let doc = utils::get_doc(&meth_attrs, true);
let spec = FnSpec::parse(name, sig, meth_attrs)?;
let spec = FnSpec::parse(name, sig, &mut *meth_attrs)?;

let text_signature = match &spec.tp {
FnType::Fn | FnType::PySelf(_) | FnType::FnClass | FnType::FnStatic => {
utils::parse_text_signature_attrs(&mut *meth_attrs, name)?
}
FnType::FnNew => {
Copy link
Member

Choose a reason for hiding this comment

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

These three patterns do almost the same thing and thus we should shorten the code.
E.g.,

FnType::FnNew | FnType::FnCall |.. => {
    if let Some(type_signature) = utils::parse_text_signature_attrs(
        &mut *meth_attrs,
        name
    )? {
        return Err(syn::Error::new_spanned(
            type_signature,
            "type_signature not allowed to use here",
        ));
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the name does need some translation for __new__ and __call__, so it would probably be just as many lines and be more complex to understand.

Copy link
Member

Choose a reason for hiding this comment

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

Then you can use macro or closure to construct an error.
I didn't want to say 'you should treat all errors as the same', but this kind of common error throwing code should be packed compactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

// try to parse anyway to give better error messages
if let Some(type_signature) = utils::parse_text_signature_attrs(
&mut *meth_attrs,
&syn::Ident::new("__new__", name.span()),
)? {
return Err(syn::Error::new_spanned(
type_signature,
"type_signature not allowed on __new__, put it on the struct definition instead",
Copy link
Member

Choose a reason for hiding this comment

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

put it on the struct definition instead
What does this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it means that instead of writing:

#[pyclass]
struct MyClass {
}

#[pymethods]
impl MyClass {
    #[new]
    #[text_signature = "(a, b, c)"]
    fn new(...) {...}
}

This should be written instead:

#[pyclass]
#[text_signature = "(a, b, c)"]
struct MyClass {
}

#[pymethods]
impl MyClass {
    #[new]
    fn new(...) {...}
}

Do you have any suggestions for how to rephrase the error message?

Copy link
Member

@kngwyu kngwyu Dec 4, 2019

Choose a reason for hiding this comment

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

#[pyclass]
#[text_signature = "(a, b, c)"]
struct MyClass {
}

So this (a, b, c) is a signature of __init__?
Then I suggest adding 'If you want to add a signature to __init__, '

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, though I used __new__ instead of __init__ since PyO3 doesn't support __init__

));
}
None
}
FnType::FnCall => {
// try to parse anyway to give better error messages
if let Some(type_signature) = utils::parse_text_signature_attrs(
&mut *meth_attrs,
&syn::Ident::new("__call__", name.span()),
)? {
return Err(syn::Error::new_spanned(
type_signature,
"type_signature not allowed on __call__, put it on the struct definition instead",
));
}
None
}
FnType::Getter(get_set_name) | FnType::Setter(get_set_name) => {
// try to parse anyway to give better error messages
let get_set_name = match get_set_name {
None => name.clone(),
Some(get_set_name) => syn::Ident::new(get_set_name, name.span()),
};
if let Some(type_signature) =
utils::parse_text_signature_attrs(&mut *meth_attrs, &get_set_name)?
{
return Err(syn::Error::new_spanned(
type_signature,
"type_signature not allowed on getters/setters",
));
}
None
}
};
let doc = utils::get_doc(&meth_attrs, text_signature, true)?;

Ok(match spec.tp {
FnType::Fn => impl_py_method_def(name, doc, &spec, &impl_wrap(cls, name, &spec, true)),
Expand Down
113 changes: 110 additions & 3 deletions pyo3-derive-backend/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use proc_macro2::Span;
use proc_macro2::TokenStream;
use std::fmt::Display;

pub fn print_err(msg: String, t: TokenStream) {
println!("Error: {} in '{}'", msg, t.to_string());
Expand All @@ -20,9 +21,110 @@ pub fn if_type_is_python(ty: &syn::Type) -> bool {
}
}

pub fn is_text_signature_attr(attr: &syn::Attribute) -> bool {
attr.path.is_ident("text_signature")
}

fn parse_text_signature_attr<T: Display + quote::ToTokens + ?Sized>(
attr: &syn::Attribute,
python_name: &T,
text_signature_line: &mut Option<syn::LitStr>,
programmerjake marked this conversation as resolved.
Show resolved Hide resolved
) -> syn::Result<Option<()>> {
if !is_text_signature_attr(attr) {
return Ok(None);
}
if text_signature_line.is_some() {
return Err(syn::Error::new_spanned(
attr,
"text_signature attribute already specified previously",
));
}
let python_name_str = python_name.to_string();
let python_name_str = python_name_str
.rsplit('.')
.next()
.map(str::trim)
.filter(|v| !v.is_empty())
.ok_or_else(|| {
syn::Error::new_spanned(
&python_name,
format!("failed to parse python name: {}", python_name),
)
})?;
match attr.parse_meta()? {
syn::Meta::NameValue(syn::MetaNameValue {
lit: syn::Lit::Str(lit),
..
}) => {
let value = lit.value();
if !value.starts_with('(') {
return Err(syn::Error::new_spanned(
lit,
"text_signature must start with \"(\"",
));
}
if !value.ends_with(')') {
return Err(syn::Error::new_spanned(
lit,
"text_signature must end with \")\"",
));
}
*text_signature_line = Some(syn::LitStr::new(
&(python_name_str.to_owned() + &value),
lit.span(),
));
}
meta => {
return Err(syn::Error::new_spanned(
meta,
"text_signature must be of the form #[text_signature = \"\"]",
));
}
};
Ok(Some(()))
}

pub fn parse_text_signature_attrs<T: Display + quote::ToTokens + ?Sized>(
attrs: &mut Vec<syn::Attribute>,
programmerjake marked this conversation as resolved.
Show resolved Hide resolved
python_name: &T,
) -> syn::Result<Option<syn::LitStr>> {
let mut parse_error: Option<syn::Error> = None;
let mut text_signature = None;
attrs.retain(|attr| {
match parse_text_signature_attr(attr, python_name, &mut text_signature) {
Ok(None) => return true,
Ok(Some(_)) => {}
Err(err) => {
if let Some(parse_error) = &mut parse_error {
programmerjake marked this conversation as resolved.
Show resolved Hide resolved
parse_error.combine(err);
} else {
parse_error = Some(err);
}
}
}
false
});
if let Some(parse_error) = parse_error {
return Err(parse_error);
}
Ok(text_signature)
}

// FIXME(althonos): not sure the docstring formatting is on par here.
pub fn get_doc(attrs: &[syn::Attribute], null_terminated: bool) -> syn::Lit {
pub fn get_doc(
attrs: &[syn::Attribute],
text_signature: Option<syn::LitStr>,
null_terminated: bool,
) -> syn::Result<syn::Lit> {
let mut doc = Vec::new();
let mut needs_terminating_newline = false;

if let Some(text_signature) = text_signature {
doc.push(text_signature.value());
doc.push("--".to_string());
doc.push(String::new());
needs_terminating_newline = true;
}

// TODO(althonos): set span on produced doc str literal
// let mut span = None;
Expand All @@ -38,17 +140,22 @@ pub fn get_doc(attrs: &[syn::Attribute], null_terminated: bool) -> syn::Lit {
} else {
d
});
needs_terminating_newline = false;
} else {
panic!("Invalid doc comment");
return Err(syn::Error::new_spanned(metanv, "Invalid doc comment"));
}
}
}
}

if needs_terminating_newline {
doc.push(String::new());
}

let mut docstr = doc.join("\n");
if null_terminated {
docstr.push('\0');
}

syn::Lit::Str(syn::LitStr::new(&docstr, Span::call_site()))
Ok(syn::Lit::Str(syn::LitStr::new(&docstr, Span::call_site())))
}
Loading