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

Error at compile-time when a non-subclassable class is being subclassed #4453

Merged
merged 4 commits into from
Aug 31, 2024

Conversation

ChayimFriedman2
Copy link
Contributor

Previously this crashed at runtime.

We even get to remove few tests. Nothing more fun than removing a no-longer-needed test!

Fixes #4451.

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 improving this; it's great to improve UX!

I think we might be able to keep the codebase a bit simpler by improving the existing PyClassBaseType trait instead of adding a new trait & generated code. At the moment all #[pyclass] types unconditionally implement it via a blanket impl, and most native types do via the pyobject_native_type_sized! macro. I guess we could add new bounds on the blanket, or even remove it and just emit implementations in the #[pyclass] macro if that improves diagnostics.

@ChayimFriedman2
Copy link
Contributor Author

@davidhewitt Done.

@davidhewitt
Copy link
Member

davidhewitt commented Aug 21, 2024

Overall this looks good to me, however there seem to be some feature combinations which aren't satisfied. I'd suggest running nox -s check-feature-powerset to help identify the broken cases and add #[cfg]s on them.

(Note that in the long run we should be able to remove a lot of the cfgs if we solved #1344. Which I think is very doable but just hasn't managed to top my priorities...)

@ChayimFriedman2
Copy link
Contributor Author

@davidhewitt Fixed that. Those cfgs are a mess!

@ChayimFriedman2 ChayimFriedman2 force-pushed the compile-error-for-subclassing branch 2 times, most recently from 6e0a90f to fa17224 Compare August 21, 2024 15:03
@ChayimFriedman2
Copy link
Contributor Author

@davidhewitt How come this PR decreased coverage when all it did was fiddling with types? Do you have an idea?

@davidhewitt
Copy link
Member

Pretty sure the coverage is due to other changes reducing coverage on main compared to the merge base on this PR. I think it can be fixed by #4472.

Let's merge this - thanks very much!

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

I think the test-debug failure is another manifestation of #4185 (comment) 🤔

src/impl_/pyclass.rs Outdated Show resolved Hide resolved
@ChayimFriedman2
Copy link
Contributor Author

I can't reproduce the CI failure locally. Why does it even run the test under abi3 when it is explicitly cfg'ed out in this case?

@davidhewitt
Copy link
Member

It's a mismatch between how the configuration and features are set up for that job. I have an idea how to make progress here that I'm going to play with tonight.

@davidhewitt
Copy link
Member

@ChayimFriedman2 I pushed a commit which should fix the test-debug issue. Looks like I got clippy wrong; don't worry about fixing here. If test-debug passes we can drop the commit from this branch and merge it in #4497.

@davidhewitt
Copy link
Member

I merged #4497 so I dropped the commit from here; hopefully CI is green and we can merge this shortly!

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 again!

@davidhewitt davidhewitt added this pull request to the merge queue Aug 31, 2024
Merged via the queue into PyO3:main with commit 73fc844 Aug 31, 2024
157 of 159 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect non-subclassable class being subclassed at compile-time
3 participants