cxx icon indicating copy to clipboard operation
cxx copied to clipboard

Improvements to namespace support

Open dtolnay opened this issue 5 years ago • 16 comments

This issue is just to track the implementation plan laid out at the bottom of https://github.com/dtolnay/cxx/pull/298#issuecomment-703817493.

  • [x] ~Emit all mentions of a type in generated C++ code (in function arguments and so forth) as a fully qualified path ::name::space::of::Type, even if it currently always happens to be within the same module.~ #370

  • [x] ~Store a distinct Namespace on each FFI item rather than having a single global namespace for the whole code generation.~ #370

  • [x] ~Implement #[namespace = ...] attribute to determine namespace of individual items.~ #370 #380

  • [x] ~Implement hierarchical handling of namespace which prioritizes item-level attribute > extern block > module.~ #444

  • [x] ~Emit ExternType impls for shared structs.~ #341

  • [x] ~Safe ExternType derive for extern Rust items. extern "Rust" { #[derive(ExternType)] type TheType; }~ 16e2620fe51032d789770b489e27a9c6c27719ef

  • [ ] Permit use inside of cxx::bridge.

dtolnay avatar Oct 10 '20 05:10 dtolnay

The second and third items on the list are probably #166.

I'm going to need to get at least half way down this list before succeeding in https://github.com/google/autocxx/issues/50.

adetaylor avatar Oct 21 '20 17:10 adetaylor

@adetaylor are you already working on one or more of these? If not I'm going to start working through them again.

sbrocket avatar Oct 22 '20 22:10 sbrocket

Yes. I have started with the second item. I'll aim to post my current progress somewhere by the end of today, assuming I can eke out any time at all (today seems very distraction-filled)

adetaylor avatar Oct 22 '20 22:10 adetaylor

OK, here's my current progress on item 2 of the list. https://github.com/dtolnay/cxx/compare/master...adetaylor:always-emit-namespaces

Depending on work and family commitments I may have a crack at 1, 3 and 4 tomorrow (I hope I can pinch some of #298 for the third item on the list).

Meanwhile @dtolnay if you have a chance to tell me if I'm along the right lines that'd be great!

adetaylor avatar Oct 23 '20 01:10 adetaylor

Ok, I'd prefer if you let me know which ones you're definitely going to take on and which you aren't as you know, so that I can take them on otherwise.

sbrocket avatar Oct 23 '20 21:10 sbrocket

I have pretty much done 2 and then 1. Both are now pushed to the branch mentioned just above. Would you like to branch that branch and start to work on 3 and 4 (and most importantly, some tests for 3 and 4)? All the work on those should be in parse.rs, just filling in the existing QualifiedIdent structs from a finer-grained namespace. In theory things should now continue to work if different QualifiedIdents have different namespaces. In practice, I'm sure there will be problems!

(The last thing I'm going to be doing here is trying to remove some duplication between QualifiedIdent and Symbol, which should be orthogonal to all the rest of the work).

adetaylor avatar Oct 23 '20 21:10 adetaylor

More progress here from a bit of tinkering this evening (a mess of commits which I will gradually rebase and clean up). @sbrocket please post here if you're likely to start any part of this, so we don't collide.

90% of this is tests (which of course fail horribly). The remaining 10% is that I took some of the attribute parsing code from your previous work in #298 and have plumbed it in per items 3 and 4 on the list at the top. I ended up requiring the syntax #[namespace(namespace = A::B)] rather than #[namespace = A::B] because my IDE got mildly grumpy about the former.

https://github.com/adetaylor/cxx/compare/always-emit-namespaces...adetaylor:allow-namespace-override

Next steps:

  • Get these tests to pass (which would validate all the work so far)
  • Rebase and clean up these commits and raise a pull request (or append them to #369)
  • Add tests for namespacing functions, which isn't in any of these commits yet. Then make that work, clean up and PR.
  • Finally, add tests for methods on namespaced classes, clean up and PR.
  • Consider cloning Namespace objects less often.

adetaylor avatar Oct 25 '20 05:10 adetaylor

I was planning on working on 3 and 4 based on your previous message but not over the weekend. If you want to just finish it that’s fine with me, I’ll check where you are on Monday.

sbrocket avatar Oct 25 '20 05:10 sbrocket

OK great.

adetaylor avatar Oct 25 '20 05:10 adetaylor

Hm. I think I'm going to have to take a slightly different approach here anyhow. The various maps inside Types probably need to be keyed by the un-namespaced Rust name, which I think means that the Struct and Enum types will need to store a Pair rather than a QualifiedIdent alone.

adetaylor avatar Oct 25 '20 06:10 adetaylor

I've closed #369 for now as I think it needs some rework.

When we encounter a type declaration/definition, we know its C++ namespace and can fully qualify its name (that's what that PR did). However, when we encounter a type reference e.g. inside a struct, we don't know its C++ namespace. We were previously invalidly qualifying it with the currently applicable namespace.

We therefore need to do this:

  • At parse time, when we encounter a type reference, record just the Rust ident of the type.
  • Once we've parsed all types and APIs, resolve all the type references to retrieve their fully qualified C++ names. (This can be likely done in the pass we already do in types.rs)

This requires more changes to the various data structures in syntax/mod.rs which is really the core of #369, hence closing it for now.

adetaylor avatar Oct 25 '20 21:10 adetaylor

OK. I got the tests to pass (bearing in mind I have only added tests for types in various namespaces, not yet functions). At the moment the branch referenced above contains many muddled commits and duplication which needs a lot of clearing up, so don't take a look yet!

adetaylor avatar Oct 26 '20 00:10 adetaylor

Progress update: I've added all the tests that I said I would, and they all pass. I'm now gradually rebasing and squashing the branch into a coherent series of commits. To reiterate, this will cover points 1-4 of the plan at the top here.

adetaylor avatar Oct 26 '20 05:10 adetaylor

OK, if you've got 1-4 covered then I'll wait to review the PR for those, since I think that should cover everything I need for my use. I'll confirm that after you post a PR and see if there's any gaps still, and then afterwards I can loop back and consider doing 6 and/or 7 since those aren't required (for me, anyway).

sbrocket avatar Oct 26 '20 20:10 sbrocket

@dtolnay Would you like help with the last item in the list? I'd like to take a crack at it if no one else is working on it to get this issue closed out.

casimcdaniels avatar Sep 21 '22 21:09 casimcdaniels

The last item is required for codebases that use both autocxx to generate bindings to C++ functions using extern_rust_type!(), and also manually use cxx to expose Rust functions using the same type to C++. autocxx generates a type MyRustType; in its own bridge's extern "Rust" {} block, which means you can't put type MyRustType; in the handwritten bridge's extern "Rust" {} block.

Xiretza avatar Feb 18 '23 14:02 Xiretza