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

#[pyclass(frozen)] implementation #2325

Closed
3 of 6 tasks
davidhewitt opened this issue Apr 23, 2022 · 7 comments
Closed
3 of 6 tasks

#[pyclass(frozen)] implementation #2325

davidhewitt opened this issue Apr 23, 2022 · 7 comments
Milestone

Comments

@davidhewitt
Copy link
Member

davidhewitt commented Apr 23, 2022

As of merging #1979 we now have a basic #[pyclass(frozen)] working on main. However, it is still undocumented, and the implementation was a bit rough around the edges. Below is a list of things that I wrote down that would be good to clean up / finish off before the 0.17 release is ready to go:

  • I moved a few associated types from PyClass to PyClassImpl (i.e. make them not public). This was necessary to allow some other trait bounds to be just on PyClassImpl. I think this is fine to do, however we should mention in the CHANGELOG.
  • I added a second associated type in PyClassImpl - Mutability and PyClassMutability. It should be possible to just have one.
  • (Internal) documentation for how the new traits work.
  • Documentation for #[pyclass(frozen)] in the guide.
  • Might be nice to benchmark / optimise these new code paths.
  • Now that it's not actually doing borrow checking in the immutable case, I'd like to propose renaming PyCell<T> to PyClassObject<T>.

@mejrs if you're picking up any of these bits feel free to ping on here, just so we don't duplicate work. I'll do the same.

@davidhewitt davidhewitt added this to the 0.17 milestone Apr 23, 2022
@mejrs
Copy link
Member

mejrs commented May 7, 2022

I was thinking - can we use this to avoid having to acquire the GIL for things like Py<T>'s Display and Serialize impls?

Also, I plan on rewriting the docs around the pointer types, I can probably include some bits about this :)

@davidhewitt
Copy link
Member Author

I think... maybe from a correctness standpoint yes, but I fear that the implementation would require specialization or a big big refactor.

@davidhewitt
Copy link
Member Author

A thought... should this be called #[pyclass(frozen)]? That would match the pattern from dataclasses and attrs libraries e.g. @dataclass(frozen = True).

Just realised this because if we do add #[pyclass(dataclass)] we would then have #[pyclass(dataclass, frozen)] 🤔

@mejrs
Copy link
Member

mejrs commented May 25, 2022

A thought... should this be called #[pyclass(frozen)]?

That mirrors the python api a lot better 👍

What should we name the Mutability/Mutable/Immutable traits and structs then? Those are user-visible.

@davidhewitt davidhewitt changed the title #[pyclass(immutable)] implementation #[pyclass(frozen)] implementation Jun 25, 2022
@davidhewitt
Copy link
Member Author

I added a second associated type in PyClassImpl - Mutability and PyClassMutability. It should be possible to just have one.

So I'm going to call this item done; the current implementation has type Frozen in PyClass trait, and then PyClassImpl does still need PyClassMutability to make the underlying implementation compile. (From experimenting it seems like removing concrete PyClassMutability and asking the compiler to infer it leads to an explosion in where bounds all over the place.)

@davidhewitt
Copy link
Member Author

I moved a few associated types from PyClass to PyClassImpl (i.e. make them not public). This was necessary to allow some other trait bounds to be just on PyClassImpl. I think this is fine to do, however we should mention in the CHANGELOG.

Done in #2572

@davidhewitt
Copy link
Member Author

With this I'm going to close this issue. The rest of the items are nice-to-haves which I expect will happen over time anyway, and this code is now in a state ready to release for 0.17.

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

No branches or pull requests

2 participants