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

Emit a better error for abi3 inheritance #4185

Merged
merged 2 commits into from
May 17, 2024
Merged

Conversation

mejrs
Copy link
Member

@mejrs mejrs commented May 14, 2024

I'm excited to finally use diagnostic::on_unimplemented :)

@mejrs mejrs added the CI-skip-changelog Skip checking changelog entry label May 14, 2024
@mejrs mejrs force-pushed the diagnostic branch 2 times, most recently from 7e688b5 to 5d267c7 Compare May 14, 2024 16:00
@mejrs mejrs force-pushed the diagnostic branch 7 times, most recently from 108a57e to cee1c7e Compare May 14, 2024 17:19
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.

Nice! I think there's loads of places where we can use this :)

src/impl_/pyclass.rs Outdated Show resolved Hide resolved
tests/test_compile_error.rs Outdated Show resolved Hide resolved
@mejrs mejrs force-pushed the diagnostic branch 3 times, most recently from ef4d978 to 5e96c6a Compare May 15, 2024 12:10
tests/test_compile_error.rs Outdated Show resolved Hide resolved

#[cfg(not(target_arch = "wasm32"))] // Not possible to invoke compiler from wasm
#[test]
fn test_diagnostics() {
Copy link
Member

Choose a reason for hiding this comment

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

Now that we're not gating the above, do we need this separate test?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it would be useful to separate into "we don't want this to compile" and "we want a decent error message for this". I'm happy to combine them if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Usually my hope for all these tests is to have a good error message, so I think it's simpler to combine. 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok :)

@mejrs mejrs force-pushed the diagnostic branch 2 times, most recently from f28b889 to 849d42b Compare May 15, 2024 20:31
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.

Looks great to me, thanks! 👍

I'm looking forward to seeing how this can help with things like FromPyObject and maybe even PyClass<Frozen = False>!

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

I think the test-debug failure is real and my fault for the config file hacks which make that job work; I suspect it's not applying abi3 properly.

@davidhewitt davidhewitt added this pull request to the merge queue May 17, 2024
Merged via the queue into PyO3:main with commit fff053b May 17, 2024
42 of 43 checks passed
@davidhewitt davidhewitt mentioned this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants