Reusing rust type binding #2
Conversation
| trusted: bool, | ||
| namespace: &Namespace, | ||
| ) -> Vec<Api> { | ||
| pub(crate) fn parse_items(cx: &mut Errors, bridge: &mut Module) -> Vec<Api> { |
There was a problem hiding this comment.
... I think I'm fine with this, but I don't love that we're passing a Module here but just ignoring attrs.
... is there a way to logically group the three fields needed by this into a struct and then just pass the struct here? Then we don't have to use std::mem::take() at all, I think.
There was a problem hiding this comment.
Thank you for the feedback. I think this is a valid feedback. But I struggle to act on it and I think I'll just submit the PR as-is.
Extracting sub-structs
Extracting/grouping sub-structs is tricky. We could in theory have 3 sub-structs: attrs, namespace+unsafety+contents, all-other-tokens. But this split is a bit arbitrary (unsafe token influences parsing of APIs, but other than that why would we split it from all the other tokens; and namespace is kind of sort-of parsed from attrs, except a bit differently in proc-macro case vs cmdbridge case).
Namespace parsing in tests
When reading the code I've discovered that syntax/test_support.rs doesn't parse the namespace in #[cxx::bridge(namespace = "foo")]. The fact that namespace is not parsed by syn/parse of syntax::file::Module caught me by surprise, but it seems to make sense.
I've added an extra commit to add namespace parsing to test_support.rs.
Passing &Module by reference
If we want to avoid this icky business of leaving Module in a partially empty/taken/restructured state, then I think we could consider making attrs::parse and syntax::parse_items take their arguments by reference (rather than by value). Maybe this is something to pursue as a follow-up? This would have cascading effects (probably requiring .clone() deeper in syntax/parse.rs and syntax/attrs.rs) so I don't plan to proactively try tackling this.
There was a problem hiding this comment.
I don't love the partial consumption. If it were up to me, I'd probably just copy and paste the manual destructuring into more places if we can't think of a logical grouping. But I'm not strongly against this either.
`syntax::parse::parse_items` is currently called from two places: from `macro/src/expand.rs` and from `gen/src/mod.rs`. (In the future it may be also called from a `syntax/test_support.rs` helper.) Before this commit, all those places had to destructure/interpret a `syntax::file::Module` into `content`, `namespace`, and `trusted`. After this commit, this destructuring/interpretation is deduplicated into `syntax::parse::parse_items`. This requires some minor gymnastics: * `gen/src/mod.rs` has to call `std::mem::take(&mut bridge.attrs)` instead of doing a partial destructuring and passing `bridge.attrs` directly. This is an extra complexity, but we already do the same thing in `macro/src/expand.rs` before this commit, so hopefully this is ok. * `fn parse_items` takes `&mut Module` rather than `Module` and "consumes" / `std::mem::take`s `module.content`, because `macro/src/expand.rs` needs to retain ownership of other `Module` fields. This also seems like an unfortunate extra complexity, but (again) before this commit we already do this for `bridge.attrs` in `macro/src/expand.rs`.
anforowicz
force-pushed
the
reusing-rust-type-binding
branch
from
c311114 to
dce0ba6
Compare
|
@zetafunction, can you PTAL at the new commit at 4288f63? Other than that, I think I addressed most of your feedback (other than the one about partially-taking/destructuring |
anforowicz
force-pushed
the
reusing-rust-type-binding
branch
from
dce0ba6 to
2b8eade
Compare
|
FWIW I've also wondered whether we should also keep the convention from |
| trusted: bool, | ||
| namespace: &Namespace, | ||
| ) -> Vec<Api> { | ||
| pub(crate) fn parse_items(cx: &mut Errors, bridge: &mut Module) -> Vec<Api> { |
There was a problem hiding this comment.
I don't love the partial consumption. If it were up to me, I'd probably just copy and paste the manual destructuring into more places if we can't think of a logical grouping. But I'm not strongly against this either.
Before this commit `fn parse_apis` in `syntax/test_support.rs` would
ignore the namespace in `#[cxx::bridge(namespace = "ignored")]`.
In other words, the new test added in `syntax/namespace.rs` would
fail before this commit. This commit fixes this.
At a high-level this commit:
* Moves two pieces of code from `gen/src/file.rs` into `syntax/...`
to make them reusable from `syntax/test_support.rs`:
- `fn find_cxx_bridge_attr` (moved into `syntax/attrs.rs`)
- `Namespace::parse_attr` (moved into
`syntax/namespace.rs`)
* Reuses these pieces of code from `syntax/test_support.rs`
* Renames `Namespace::parse_bridge_attr_namespace` to
`Namespace::parse_stream` so that all 3 parse methods are named
after their input: `parse_attr`, `parse_stream`, `parse_meta`.
* Adds a `syntax/`-level unit test that verifies that the namespace
is indeed getting correctly parsed and propagated
anforowicz
force-pushed
the
reusing-rust-type-binding
branch
from
2b8eade to
8dbb7b2
Compare
|
Thank you for the review feedback. I've now opened an upstream PR at dtolnay#1539. So let me close this PR. |
@zetafunction, can you PTAL and do an overall review from your side before I submit this as an upstream PR?
We are still trying to figure out the process of contributing to
cxx:WDYT?