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

Improve error messages for #[pyfunction] defined inside #[pymethods] #4349

Merged

Conversation

csernazs
Copy link
Contributor

@csernazs csernazs commented Jul 14, 2024

Make error message more specific when #[pyfunction] is used in #[pymethods].

Effectively, this replaces the error message:

error: static method needs #[staticmethod] attribute

To:

functions inside #[pymethods] do not need to be annotated with #[pyfunction]

Fixes #4340

@csernazs csernazs force-pushed the pyfunction-inside-pymethods-error-improvement branch 2 times, most recently from 0740069 to 9c54a0b Compare July 14, 2024 08:08
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks very much for helping with this! I think there's a couple of further tweaks we should apply, and then potentially we can ship this in #4333

if meth
.attrs
.iter()
.any(|attr| attr.path().is_ident("pyfunction"))
Copy link
Member

Choose a reason for hiding this comment

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

I think if you make this check remove the attribute rather than just check for it, it will improve the error output by removing all the gibberish related to the pyfunction expansion.

Also, see what we recently did in #4288 - we should check for the the "namespaced" attribute here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi David,

Thanks for the review!

I added clearing attrs and it removes the other error messages, but honestly I'm not sure if this is the correct solution. What do you think?

Regarding the namespaces, I'm reading the PR you linked.

Thanks again,
Zsolt

ps: this PR was created during europython 2024 sprint weekend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about namespaces and I found the has_attribute function, so at the end the "if" can be updated to:

if has_attribute(&meth.attrs, "pyfunction")`

But this function is not public so I cannot use it in pyimpl.rs. I checked the references and it seems no code path runs through module.rs.

So if this is the way to solve (I mean, calling has_attribute). I think it can be done by:

  • making this function public
  • moving this function to some common source file (and making it public there).

Or something else. What do you think?

Zsolt

Copy link
Member

Choose a reason for hiding this comment

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

I think you want both has_attribute and has_attribute_with_namespace. Perhaps move them to utils.rs and make them pub(crate)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved both functions, and I also had to move

pub(crate) enum IdentOrStr<'a> {
    Str(&'a str),
    Ident(syn::Ident),
}

as well to utils.rs.

@csernazs csernazs force-pushed the pyfunction-inside-pymethods-error-improvement branch from 9c54a0b to 5428792 Compare July 14, 2024 20:35
.iter()
.any(|attr| attr.path().is_ident("pyfunction"))
{
meth.attrs.clear();
Copy link
Member

Choose a reason for hiding this comment

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

IMO .clear() is a bit too much because it might introduce other compile errors by removing needed attributes.

One option is to use .retain(), something like this pseudocode:

let mut error = None;
meth.attrs.retain(|attr| {
    if attr is pyfunction { 
        error = Some(err_spanned!(...));
        false
    } else {
        true
    }
});
error.map_or(Ok(()), Err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pseudo code, it helped a lot!

I make a copy of attrs to construct a 1-element slice here:

        let attrs = [attr.clone()];

idk what is the idiomatic way in rust to solve this issue ,but this is the best I could do.

There could be two additional function for has_attribute_with_namespace and has_attribute accepting just a single attribute but I felt it is easier to construct a one-element slice here.

Span needs to be adjusted a little bit as in retain the attrs is not yet removed at that point so rust points to the attribute instead if I use meth.span(), like this:

error: functions inside #[pymethods] do not need to be annotated with #[pyfunction]
  --> src/lib.rs:10:9
   |
10 |         #[pyfunction]
   |         ^

Make error message more specific when `#[pyfunction]` is used in
`#[pymethods]`.

Effectively, this replaces the error message:

```
error: static method needs #[staticmethod] attribute
```

To:
```
functions inside #[pymethods] do not need to be annotated with #[pyfunction]
```

...and also removes the other misleading error messages to the function in
question.

Fixes PyO3#4340

Co-authored-by: László Vaskó <[email protected]>
@csernazs csernazs force-pushed the pyfunction-inside-pymethods-error-improvement branch from 5428792 to 956046a Compare July 16, 2024 21:02
@csernazs csernazs force-pushed the pyfunction-inside-pymethods-error-improvement branch from 956046a to 7eba98c Compare July 16, 2024 21:10
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me now!

@davidhewitt davidhewitt added this pull request to the merge queue Jul 24, 2024
Merged via the queue into PyO3:main with commit 3bd8777 Jul 24, 2024
42 checks passed
@csernazs csernazs deleted the pyfunction-inside-pymethods-error-improvement branch July 25, 2024 16:18
davidhewitt pushed a commit that referenced this pull request Sep 3, 2024
…4349)

* Improve error messages for #[pyfunction] defined inside #[pymethods]

Make error message more specific when `#[pyfunction]` is used in
`#[pymethods]`.

Effectively, this replaces the error message:

```
error: static method needs #[staticmethod] attribute
```

To:
```
functions inside #[pymethods] do not need to be annotated with #[pyfunction]
```

...and also removes the other misleading error messages to the function in
question.

Fixes #4340

Co-authored-by: László Vaskó <[email protected]>

* review fixes

---------

Co-authored-by: László Vaskó <[email protected]>
davidhewitt pushed a commit that referenced this pull request Sep 15, 2024
…4349)

* Improve error messages for #[pyfunction] defined inside #[pymethods]

Make error message more specific when `#[pyfunction]` is used in
`#[pymethods]`.

Effectively, this replaces the error message:

```
error: static method needs #[staticmethod] attribute
```

To:
```
functions inside #[pymethods] do not need to be annotated with #[pyfunction]
```

...and also removes the other misleading error messages to the function in
question.

Fixes #4340

Co-authored-by: László Vaskó <[email protected]>

* review fixes

---------

Co-authored-by: László Vaskó <[email protected]>
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.

#[pyfunction] inside #[pymethods] could emit a better error
2 participants