Skip to content

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented May 9, 2021

This closes #865.

This accidentally also allows arbitrarily nesting Option and Pin. On emitting code, it's always normalized to Option<Pin<ref>>. I don't know how to avoid this. You could already arbitrarily nest Pins (which would be normalized to one Pin), though that seems less problematic than allowing someone to write e.g. Pin<Option<ref>> and getting out Option<Pin<ref>>.

I don't know how to fix Pin<Option<ref>> without also breaking Pin<Pin<ref>>. Pin<Pin<ref>> is certainly bad practice and misleading, though, so perhaps breaking it is desirable.

@CAD97
Copy link
Contributor Author

CAD97 commented May 9, 2021

Side note: cxx is awesome and I can't thank you enough for setting it up. I'm just running into all the rough edges because I'm trying to push it really far.

@CAD97
Copy link
Contributor Author

CAD97 commented May 9, 2021

Added a simple smoke test, fixed the fallout. Ready for review.

@CAD97 CAD97 marked this pull request as ready for review May 9, 2021 17:56
@CAD97
Copy link
Contributor Author

CAD97 commented May 9, 2021

I'm happy with the state of this, pending review. I hope the semantics match what you want out of cxx, because this patch is very useful for the use case I'm pursuing currently.

@CAD97
Copy link
Contributor Author

CAD97 commented May 14, 2021

I suppose this is also blocked on #682, as it's adding a new builtin type which someone could have defined. Interestingly, I think the resolution to #682 will make the problem of Pin<Pin<ref>> and Pin<Option<ref>> easier to solve.

@CAD97 CAD97 force-pushed the option-ref-is-ptr branch from 204c4a0 to 06c8ab7 Compare May 14, 2021 07:17
@CAD97 CAD97 force-pushed the option-ref-is-ptr branch from 06c8ab7 to 2d4bc0c Compare May 14, 2021 07:17
@CAD97
Copy link
Contributor Author

CAD97 commented May 14, 2021

More testing found two edge cases I hadn't handled; handled those now.

@CAD97
Copy link
Contributor Author

CAD97 commented May 14, 2021

(why can't I unmark the PR as ready for review)

So I added a test for Option<Option<ref>> to make sure that it's not allowed, and it turns out that it is. This is definitely a problem, as Option<&T> and Option<Option<&T>> are very distinct types, much more so than Pin<&T> vs Pin<Pin<&T>>. So this PR is not viable until that's fixed.

I think that fixing that should wait for resolving #682, though, as that will majorly impact the handling of builtin types 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.

Support for using Option<&mut T> on the Rust side of T*

1 participant