Skip to content

Remove pin-project dependencies#738

Merged
mitsuhiko merged 1 commit intomasterfrom
feature/remove-pin-project
Mar 22, 2025
Merged

Remove pin-project dependencies#738
mitsuhiko merged 1 commit intomasterfrom
feature/remove-pin-project

Conversation

@mitsuhiko
Copy link
Copy Markdown
Owner

Reverts #711 and #737.

It's for a singular unsafe on a rather uncommon path, that is not worth pulling dependencies in.

@mitsuhiko mitsuhiko merged commit 1989931 into master Mar 22, 2025
25 checks passed
@mitsuhiko mitsuhiko deleted the feature/remove-pin-project branch March 22, 2025 22:09
@Manishearth
Copy link
Copy Markdown
Contributor

@mitsuhiko I think having this crate be 100% safe is rather nice: it avoids the need for unsafe review/etc. You're welcome to make a different call here, but Pin unsafe is hard to review and unsafe that is "hard to review" in my experience has a much higher chance of being subtly broken.

At the very least, would you consider adding real safety comments that explain why it is safe? At the moment it's just a line of completely unexplained unsafe, dealing with a rather tricky corner of Rust safety.

insta is typically a test only dep, so I don't perceive the deptree size to be a huge deal for insta users. I could be wrong.

@mitsuhiko
Copy link
Copy Markdown
Owner Author

I care a lot about dependencies and compile time performance and this showed up when I updated insta in my own project as a regression. Particularly because this is a compile time crate trading a single use of unsafe in an uncommon code path for a dependency to me is a bad tradeoff.

I will add a comment about this use. I did not find it particularly offensive given that it's the example from the documentation. At the time this code was written pin-project did not even exist from what I remember and that pattern was generally widespread.

@Manishearth
Copy link
Copy Markdown
Contributor

I think pinning is one of the more tricky bits of unsafe Rust; since you need to understand both unsafe Rust and async pinning to really handle it. It's not a particularly offensive use of unsafe Rust, but it requires more specialized knowledge than usual to review. There's basically no harm done in writing copious amounts of unsafe documentation, especially when the crate does not actually use unsafe often.

pin-project is a common enough ecosystem crate that I didn't think it would be a big deal, but if it is, it is.

@mitsuhiko
Copy link
Copy Markdown
Owner Author

I added a SAFETY comment in 0b2d273

@Manishearth
Copy link
Copy Markdown
Contributor

I think that's not much, but I'm not going to push it, I might try to write a proper safety comment later.

(full safety comments, where all the necessary invariants are listed and demonstrated, are rare in the ecosystem anyway)

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.

2 participants