Skip to content
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

Implement glob import semantics for builtin type names #682

Open
dtolnay opened this issue Jan 16, 2021 · 5 comments
Open

Implement glob import semantics for builtin type names #682

dtolnay opened this issue Jan 16, 2021 · 5 comments

Comments

@dtolnay
Copy link
Owner

dtolnay commented Jan 16, 2021

Currently we disallow any user-defined type name that collides with one of the builtin type names.

error[cxxbridge]: reserved name
  ┌─ src/main.rs:3:12

3 │     struct UniquePtr { broken: bool }
  │            ^^^^^^^^^ reserved name

error[cxxbridge]: reserved name
  ┌─ src/main.rs:5:12

5 │     struct c_char { broken: bool }
  │            ^^^^^^ reserved name

However, this is bad because it makes it a breaking change for us to add new builtin bindings, such as #681. If the user already had their own type with the same name as the new binding, that code needs to continue to work.

We need to implement basically the same semantics that Rust has for prelude types and glob imports — a user-defined type replaces any builtin binding of the same name.

@adetaylor
Copy link
Collaborator

Design sketch:

Constraints:

  • We will need to behave differently in both C++ and Rust code generation. If c_char is std::os::raw::c_char, it must be so prefixed in impl ToTokens, whilst if it's a type declared in the cxx::bridge mod then it shouldn't be. Similar changes are needed in the generated C++.
  • We could potentially pass more global information into the C++ codegen, but not (readily) into the Rust codegen because we're implementing the existing signatures within the ToTokens trait.
  • We need to support referring to types which are declared later:
    #[cxx::bridge]
    mod ffi {
       extern "C++" {
          fn needs_a_type_declared_later(a: c_char);
       }
       struct c_char {
          // ...
        };
     }

Design proposal:

  • Split Type::Ident into two different enum variants, Type::BuiltIn versus Type::Ident.
  • parse.rs continues always to generate Type::Ident.
  • Within generate, we continue to do Types::collect as now, unchanged.
  • But then we call a new function immediately after Types::collect, called Types::spot_builtins(apis: &mut [Api], types: &Types).
  • That function visits all Types anywhere in the APIs, and replaces Type::Ident with Type::BuiltIn in all cases where the type is not known to types.
  • The checking phase, and Rust and C++ code generation, are all modified to assume that Type::Ident is never an Atom and is always a type defined in the cxx::bridge mod; whilst Type::Builtin is always an Atom.

Does that sound about right @dtolnay?

@CAD97
Copy link
Contributor

CAD97 commented May 14, 2021

If you aren't going to get to it soon @adetaylor, I could potentially take a good stab at implementing the above design this coming week.

@adetaylor
Copy link
Collaborator

That would be great, yes. I don't think I'm realistically going to get to it in the next few days. If I start, I'll post here - but it's extremely unlikely :) (Either way we should wait for @dtolnay to tell me that my design sketch is nonsense)

@CAD97
Copy link
Contributor

CAD97 commented Aug 2, 2021

(Had some free time, decided to take a look at this again.)

@adetaylor
Design proposal:

  • [...]
  • But then we call a new function immediately after Types::collect, called Types::spot_builtins(apis: &mut [Api], types: &Types).
  • That function visits all Types anywhere in the APIs, and replaces Type::Ident with Type::BuiltIn in all cases where the type is not known to types.
  • [...]

This can't work as-written, as Types borrows from apis. If we want to fix-up the builtin Types in-place, there are three simple-ish options that I see:

  • restructure Types::collect to not borrow from apis (do the fixup, then actually borrow everything to continue);
  • do Types::collect, record which Types to fixup, do the fixups, re-Types::collect; or
  • use internal mutability (e.g. Types::Ident(NamedType, Cell<bool>)) to do the fixup.

The other approach I can think of is to not do an inplace fixup of Types, but to instead have an extra list of Types in Types of "hey this is a Type::Ident of a builtin, not a user-defined type." I don't know how much that would impact downstream.

@CAD97
Copy link
Contributor

CAD97 commented Aug 3, 2021

#908 attempts to implement this via keeping a new mapping of types to atoms, which is used by downstream passes instead of Atom::from. It needs significant testing to ensure that it actually works, but the approach seems sound. However, it only handles atoms (e.g. c_char) at the moment, and not builtins (e.g. UniquePtr). Since builtins are unique kinds of Types, this approach of mapping Type::Ident to Atom won't work there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants