Skip to content

Order sensitivity across namespaces #386

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

Closed
dtolnay opened this issue Oct 31, 2020 · 5 comments · Fixed by #392
Closed

Order sensitivity across namespaces #386

dtolnay opened this issue Oct 31, 2020 · 5 comments · Fixed by #392

Comments

@dtolnay
Copy link
Owner

dtolnay commented Oct 31, 2020

The namespace sorting introduced in #370 does not handle alphabetically lesser namespaces referencing items from alphabetically greater namespaces.

#[cxx::bridge]
mod ffi {
    #[namespace = "first"]
    struct First {
        second: Box<Second>,
    }

    #[namespace = "second"]
    struct Second {
        i: i32,
    }
}
target/debug/build/demo-d74fe843f72e57b6/out/cxxbridge/sources/demo/src/main.rs.cc:154:17: error: ‘::second’ has not been declared
  154 |   ::rust::Box<::second::Second> second;
      |                 ^~~~~~
target/debug/build/demo-d74fe843f72e57b6/out/cxxbridge/sources/demo/src/main.rs.cc:154:31: error: template argument 1 is invalid
  154 |   ::rust::Box<::second::Second> second;
      |                               ^
@adetaylor
Copy link
Collaborator

Good spot.

I am assuming that the correct fix is to write out forward declarations across all namespaces, and then iterate all the namespaces again to write everything else. I shall do so when I get a moment.

@adetaylor
Copy link
Collaborator

Actually that is nonsense and won't work at all. I need to first look into how sensitive cxx is to definition order irrespective of namespaces.

@adetaylor
Copy link
Collaborator

There is order sensitivity without namespaces too:

    struct A {
        b: B
    }

    struct B {
        c: i32
    }

works in normal Rust code, but does not work in a #[cxx::bridge] mod.

cargo:warning=/Users/adetaylor/dev/cxx/target/debug/build/demo-0110b4ddc5238254/out/cxxbridge/sources/demo/src/main.rs.cc:205:21: error: field has incomplete type '::org::example::B'
  cargo:warning=  ::org::example::B b;
  cargo:warning=                    ^
  cargo:warning=/Users/adetaylor/dev/cxx/target/debug/build/demo-0110b4ddc5238254/out/cxxbridge/sources/demo/src/main.rs.cc:189:8: note: forward declaration of 'org::example::B'
  cargo:warning=struct B;
  cargo:warning=       ^
  cargo:warning=1 error generated.

I assume this is a known limitation of what can be put in cxx::bridge mods, but I think it probably makes sense to fix this at the same time by doing a topological sort of all the structs then outputting them in that order. This sounds fun. However, it would be nice to use an algorithm which, instead of sorting into a list of types, sorts into a list of lists of types. Then within each inner list we can continue to sort by namespace to avoid outputting too many zillions of namespace declarations.

@dtolnay
Copy link
Owner Author

dtolnay commented Oct 31, 2020

The "field has incomplete type" issue is #292. I should have used second: Box<Second> for this one; fixed the repro at the top. Topological sort could solve both but I would also accept just forward declarations to close this one.

@adetaylor
Copy link
Collaborator

OK cool. I will probably just do the forward declaration thing then. It may not be for a few days; it's proving to be a busy weekend.

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 a pull request may close this issue.

2 participants