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

Conversation

programmerjake
Copy link
Contributor

@programmerjake programmerjake commented Nov 24, 2019

Based on design at #310 (comment)

Just implemented parsing #[text_signature = "..."] and adding to generated doc strings.
Still need to add docs, tests, changelog entries, etc.

Demo code:

/// doc string 1
#[pyclass]
#[text_signature = "(arg1, *, arg2=0)"]
/// doc string 2
struct MyClass {}

#[pymethods]
impl MyClass {
    #[new]
    #[args(arg1, "*", arg2 = "0")]
    fn new(obj: &PyRawObject, arg1: i32, arg2: i32) {
        obj.init(Self {});
    }

    /// doc string 1
    #[text_signature = "($self)"]
    /// doc string 2
    fn my_method(&self) {}

    #[staticmethod]
    #[text_signature = "()"]
    fn method_without_docs() {}
}

/// doc string 1
#[pyfunction]
#[text_signature = "(arg1, arg2)"]
/// doc string 2
fn my_function(arg1: i32, arg2: i32) -> i32 {
    0
}

@kngwyu
Copy link
Member

kngwyu commented Nov 24, 2019

Thank you.

  • We already have 'doc comment to doc str' feature.
    E.g.,
>>> from rustapi_module.buf_and_str import BytesExtractor
>>> b = BytesExtractor()
>>> b.__doc__
'This is for confirming that PyBuffer does not cause memory leak'

So I think what this PR should do is just filling out __text__signature__.

  • Why and when is this feature useful? When we generate docs using sphinx?

@programmerjake
Copy link
Contributor Author

  • We already have 'doc comment to doc str' feature.

Yes, I am building this pull request on top of the existing doc comment machinery (and improving error handling too by using syn::Error instead of panic) since both CPython and PyPy currently get __text_signature__ from the provided doc string.

  • Why and when is this feature useful? When we generate docs using sphinx?

Being able to set __text_signature__ is useful because:

  • it enables function argument auto-completion when using pyls (the Python language server -- like RLS for Python).
  • it works with Python's built-in help() (doesn't currently work on #[new] methods, probably a CPython defect)
  • it makes inspect.signature() work on functions/methods/classes implemented in Rust
    • used in a lot of Python doc generators (at least pdoc3, probably many more)

This feature is useful when you don't want the special __text_signature__ text to always be at the top of your Rust docs, such as:

  • when your API is intended for Rust users as well as just Python users.
  • when you use a auto-formatter on doc comments

This also does some checks to make sure the provided signature is valid enough to actually be put in __text_signature__ instead of __doc__ when CPython parses the doc-string.

This is also useful because the #[text_signature = "..."] annotation doesn't have to be at the top of the doc comments and can be ordered anywhere where PyO3 can parse it (much more useful when generating functions using macros):

/// my really long docs for `my_fn`
/// that cover many
/// many lines
#[pyfunction(b = 2, "*", c = "None")]
#[text_signature = "my_fn(a, b=2, *, c=None)"]
/// more docs here
fn my_fn(a: &str, b: i32, c: Option<u32>) -> i16 {
    unimplemented!()
}

This also is much nicer when you want to have __text_signature__ without __doc__:

#[pyfunction]
#[text_signature = "no_docs()"] // simple
fn no_docs() {}

vs.

// hope this is right -- easy to mess up:
// must have exactly 2 trailing "\n"

/// no_docs()
/// --
///
///
#[pyfunction]
fn no_docs() {}

Additional useful property: should CPython (or any other supported Python interpreter) switch to getting __text_signature__ some other way, all that needs to be changed is PyO3 and not all the code using #[text_signature = "..."].

When the code to implement automatically generating __text_signature__ #310 gets around to being implemented, it should be relatively easy to slot that code into the machinery I'm implementing.

@Alexander-N
Copy link
Member

Alexander-N commented Nov 25, 2019

I think this makes a lot of sense since it gives a clear way to provide __text_signature__. I didn't know that the __text_signature__ attribute is built from the docstring and wouldn't have made the connection when reading a docstring with the manually constructed special string. Just too bad that type annotations are not supported with this approach. It would be nice if we could replace this mechanism by providing a proper __signature__ in the future.

@programmerjake
Copy link
Contributor Author

I think this makes a lot of sense since it gives a clear way to provide __text_signature__. I didn't know that the __text_signature__ attribute is built from the docstring and wouldn't have made the connection when reading a docstring with the manually constructed special string. Just too bad that type annotations are not supported with this approach. It would be nice if we could replace this mechanism by providing a proper __signature__ in the future.

That's part of why I ended up picking #[text_signature] instead of #[signature], figuring that #[signature] can be added to support annotations using __signature__ and leaving #[text_signature] for backward-compatibility.

@kngwyu
Copy link
Member

kngwyu commented Nov 26, 2019

@programmerjake
Thank you for detailed explaination.

it enables function argument auto-completion when using pyls (the Python language server -- like RLS for Python).
it works with Python's built-in help() (doesn't currently work on #[new] methods, probably a CPython defect)
it makes inspect.signature() work on functions/methods/classes implemented in Rust
    used in a lot of Python doc generators (at least pdoc3, probably many more)

These are all good.

It would be nice if we could replace this mechanism by providing a proper signature in the future.

Is it a proper/efficient way to provide __annotations__?
CPython built-in types also don't have __signature__.

In [15]: object.__signature__                                                                                             
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-15-e544111936b4> in <module>
----> 1 object.__signature__

AttributeError: type object 'object' has no attribute '__signature__'

In [16]: list.__signature__                                                                                               
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-16-27c4b7eaead5> in <module>
----> 1 list.__signature__

AttributeError: type object 'list' has no attribute '__signature__'

I think what we need for type-annotation is just fill out the __annotations__ field.

And... #[text_signature] looks good, but isn't it better if we can generate __text_signature__ from function declaration?
Do you think we can do this?
If you think we cannot, what is the problem?

@programmerjake
Copy link
Contributor Author

It would be nice if we could replace this mechanism by providing a proper signature in the future.

Yeah, but that's probably quite a bit harder to implement, so #[text_signature] is a stop-gap solution.

Is it a proper/efficient way to provide __annotations__?

I don't think so, it didn't work with argument annotations when I tried; a return type annotation causes CPython to not detect the signature (__text_signature__ unset, signature left at the beginning of __doc__) due to specifically searching the docstring for ")\n--\n\n".

In the latest CPython git, inspect.signature() ignores __attributes__ when __text_signature__ is set. Assuming CPython's implementation doesn't change, the only way that I can see to support annotations that inspect.signature() detects on builtin functions is to set __signature__.

CPython built-in types also don't have __signature__.

Yes, __signature__ is the mechanism used to override what inspect.signature() returns, none of the built-in functions/methods/types need to do that since __text_signature__ is sufficient for them.

I think what we need for type-annotation is just fill out the __annotations__ field.

And... #[text_signature] looks good, but isn't it better if we can generate __text_signature__ from function declaration?
Is it possible?

yes, but it's probably also much harder to implement, hence why #[text_signature] is useful as a stop-gap measure. Once __text_signature__ generation is implemented, #[text_signature] will also be useful for cases where the user wants a different value than is generated by PyO3.

@konstin
Copy link
Member

konstin commented Nov 27, 2019

Is it a proper/efficient way to provide annotations?

Unfortunately not, see #305, python#3208 amd #510 (comment).

I think what we need for type-annotation is just fill out the __annotations__ field.
And... #[text_signature] looks good, but isn't it better if we can generate __text_signature__ from function declaration?
Is it possible?

yes, but it's probably also much harder to implement, hence why #[text_signature] is useful as a stop-gap measure. Once __text_signature__ generation is implemented, #[text_signature] will also be useful for cases where the user wants a different value than is generated by PyO3.

Imo we should eventually autogenerate the text signature, even though this is pratically useful intermediate step.


How does this currently interact with rustdoc, i.e. does the signature show up when running cargo doc?

@programmerjake
Copy link
Contributor Author

Imo we should eventually autogenerate the text signature, even though this is pratically useful intermediate step.

Agreed, though I don't think we should remove #[text_signature] once auto-generation is added since it's useful for overriding the autogenerated result as well as backward-compatibility.

How does this currently interact with rustdoc, i.e. does the signature show up when running cargo doc?

I haven't tested it yet, but the signature shouldn't show up in the Rust doc-string. It will show up as a #[text_signature] annotation if rustdoc shows the pre-macro-expanded code though, which I don't think it does.

@programmerjake programmerjake marked this pull request as ready for review November 29, 2019 21:23
@programmerjake
Copy link
Contributor Author

I finished adding tests and documentation. I switched the syntax for #[text_signature] to mirror the value of __text_signature__ visible in Python:

#[pyfunction(a, b, c=3, "*", d=4, e=5]
// doesn't have function name in text_signature
#[text_signature = "(a, b, c=3, *, d=4, e=5)"]
fn my_function(a: i32, b: i32, c: i32, d: i32, e: i32) -> i32 {
    a + b + c + d + e
}

rather than

#[pyfunction(a, b, c=3, "*", d=4, e=5]
#[text_signature = "my_function(a, b, c=3, *, d=4, e=5)"]
fn my_function(a: i32, b: i32, c: i32, d: i32, e: i32) -> i32 {
    a + b + c + d + e
}

@programmerjake

This comment has been minimized.

pyo3-derive-backend/src/module.rs Show resolved Hide resolved
pyo3-derive-backend/src/module.rs Show resolved Hide resolved
pyo3-derive-backend/src/utils.rs Outdated Show resolved Hide resolved
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

)? {
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__

pyo3cls/src/lib.rs Show resolved Hide resolved
pyo3-derive-backend/src/utils.rs Outdated Show resolved Hide resolved
pyo3-derive-backend/src/utils.rs Show resolved Hide resolved
@kngwyu
Copy link
Member

kngwyu commented Nov 30, 2019

Thank you.
It looks well, but since proc-macro code is often hard to maintain, I think we should be careful not to make the code too complex.

@kngwyu
Copy link
Member

kngwyu commented Dec 7, 2019

Thank you, it's a pretty good feature!

@LucaCappelletti94
Copy link

Is the notation with the default values in the signature actually supported or was it dropped?

@davidhewitt
Copy link
Member

There is a test for that still present here:

fn test_function() {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants