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 error messages for unsupported macro features on compilation #4194

Conversation

codeguru42
Copy link
Contributor

@codeguru42 codeguru42 commented May 20, 2024

Continues the work from #3993

Error messages for weakref and dict.

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 for working on this at the PyCon sprints! 🚀

It looks like there's a compile error on the pipeline still:

error: `weakref` requires >= python 3.9 with the `abi3` feature
  --> src/tests/hygiene/pyclass.rs:16:5
   |
16 |     weakref,
   |     ^^^^^^^

I'd suggest having a look at that file and moving the weakref flag to a separate #[cfg_attr(any(Py_3_9, not(Py_LIMITED_API)), pyo3(weakref))].

.gitignore Outdated Show resolved Hide resolved
newsfragments/4194.added.md Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyclass.rs Outdated Show resolved Hide resolved
@codeguru42
Copy link
Contributor Author

@davidhewitt this is ready for another review

@davidhewitt
Copy link
Member

It looks like there's a compile error on the pipeline still:

It looks like the pipeline is still failing with this same error. I think you should be able to reproduce by running cargo test --lib --features=abi3-py37

@codeguru42 codeguru42 force-pushed the add-error-messages-for-unsupported-macro-features-on-compilation branch from bc9a80e to 3f5d1bc Compare May 26, 2024 14:29
@codeguru42
Copy link
Contributor Author

codeguru42 commented May 28, 2024

After making the suggested change, we are getting the following error:

error: cannot find attribute pyo3 in this scope
--> src/tests/hygiene/pyclass.rs:12:46
|
12 | #[cfg_attr(any(Py_3_9, not(Py_LIMITED_API)), pyo3(weakref))]

How do I fix this? Do I need to add an import? Rust Rover doesn't give any suggestions.

@davidhewitt
Copy link
Member

Ungh, I realise now that with that suggestion I've blundered right into #2786 (and similar pains around cfg_attr).

I think the solution for now here would be to still use cfg_attr but wrap the whole pyclass in the attribute.

So something like

#[cfg_attr(any(Py_3_9, not(Py_LIMITED_API)), crate::pyclass(name = "ActuallyBar", ..., weakref))]
#[cfg_attr(not(any(Py_3_9, not(Py_LIMITED_API))), crate::pyclass(name = "ActuallyBar", ...))]
pub struct Bar {

@codeguru42
Copy link
Contributor Author

Thanks for working on this at the PyCon sprints! 🚀

It looks like there's a compile error on the pipeline still:

error: `weakref` requires >= python 3.9 with the `abi3` feature
  --> src/tests/hygiene/pyclass.rs:16:5
   |
16 |     weakref,
   |     ^^^^^^^

I'd suggest having a look at that file and moving the weakref flag to a separate #[cfg_attr(any(Py_3_9, not(Py_LIMITED_API)), pyo3(weakref))].

I made this change and it gives a compiler error. I don't know rust well enough to even guess how to fix it. Do you have any suggestions?

codeguru42 and others added 3 commits June 3, 2024 23:18
…ror-messages-for-unsupported-macro-features-on-compilation

# Conflicts:
#	pyo3-macros-backend/src/pyclass.rs
@davidhewitt
Copy link
Member

I pushed a commit to apply something which should work as per #4194 (comment)

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.

I think this should now (hopefully) merge, many thanks!

@davidhewitt
Copy link
Member

Ah, there was a lot of adjustment to conditional compilation to follow up where our own test suite had been silently ignoring the missing behaviour. Good to find that this makes us in a better place! 😂

Copy link
Member

Choose a reason for hiding this comment

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

I merged these tests into test_class_basics.

@davidhewitt davidhewitt added this pull request to the merge queue Jun 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 6, 2024
@davidhewitt
Copy link
Member

I think the CI failure arises due to divergence between how we check for abi3 here and how pyo3-build-config activate the limited API for PyPy / GraalPy. I opened #4237 to simplify.

@davidhewitt davidhewitt added this pull request to the merge queue Jun 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 10, 2024
@codeguru42
Copy link
Contributor Author

Where are we at with this PR? What can I do to get it over the line?

@davidhewitt
Copy link
Member

@codeguru42 great question! After the last updates I pushed, I think this PR should in principle be good to merge.

But naturally, making these error messages emerge properly has uncovered two pain points that I've been fixing separately. First there was #4237 which fixed a divergence between how we were setting the abi3 feature here in macro code versus in pyo3-ffi.

And then now the tests have picked up an old bug that's my fault with the 3.9 implementation, which I opened this morning in #4251.

My hope is that once that PR is merged, we'll be able to merge this one without further modifications 👀

@davidhewitt davidhewitt mentioned this pull request Jun 15, 2024
5 tasks
…ror-messages-for-unsupported-macro-features-on-compilation

# Conflicts:
#	tests/test_class_basics.rs
@codeguru42
Copy link
Contributor Author

codeguru42 commented Jun 16, 2024

@davidhewitt I merged main into this branch and resolved conflicts. But I have one question for clarification. As you can see in 3c7e898. main has Py_3_9 as the version for cfg but this branch has Py_3_10. It looks like the change on this branch comes from 7177d53 which you pushed to this PR. I accepted the change from main assuming that it is probably correct, but I want to double-check with you to be sure.

@davidhewitt
Copy link
Member

Yep I think main is correct; let's try merging again. Thanks for fixing conflicts 🙏

@davidhewitt davidhewitt added this pull request to the merge queue Jun 16, 2024
Merged via the queue into PyO3:main with commit 79591f2 Jun 16, 2024
38 of 39 checks passed
@davidhewitt
Copy link
Member

Hooray, success finally! 🎉 Thanks again @codeguru42 and great to meet you at PyCon 👍

@codeguru42
Copy link
Contributor Author

codeguru42 commented Jun 16, 2024

Closes #3945.

I probably don't have permissions to do this...but thought I'd see if a comment would do it. Maybe needed it in the description before merging?

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.

5 participants