Compiled headers fail on `-Wundefined-func-template` if we use `Vec`

[jmcclellan-figma]

Right now Vec for specific types is implemented with template specializations here, however this scheme doesn't work with -Wundefined-func-template warning enabled in Clang, since e.g. the Vec<MyType>::drop function is left undeclared in the header.

While clang does seem to compile this correctly, it's not actually allowed by C++ - see cppreference, specifically:

Specialization must be declared before the first use that would cause implicit instantiation, in every translation unit where such use occurs

[jmcclellan-figma]

I think a fix would be to forward declare these specializations in the headers

[anforowicz]

FWIW I tried to repro and try forward-declaring, but it didn't really help. Still, let me share what I got, in case it helps any future attempts.

I note that the CI runs not only with -Wall but also with -Wpedantic (8f067ad), but it seems that to get -Wundefined-func-template one needs to use -Weverything (although in that case the CI explicitly passes -Wno-undefined-func-template).

I can repro at f0a0e24, when running CXX=clang CXXFLAGS="-Wundefined-func-template -Werror" cargo test --workspace:

In file included from /home/lukasza/src/github/cxx/target/debug/build/demo-4b4fc931f198a6dd/out/cxxbridge/sources/demo/src/main.rs.cc:1:
In file included from /home/lukasza/src/github/cxx/target/debug/build/demo-4b4fc931f198a6dd/out/cxxbridge/crate/demo/include/blobstore.h:2:
/home/lukasza/src/github/cxx/target/debug/build/demo-4b4fc931f198a6dd/out/cxxbridge/include/rust/cxx.h:887:9: error: instantiation of function 'rust::Vec<rust::String>::drop' required here, but no definition is available [-Werror,-Wundefined-func-template]
  887 |   this->drop();
      |         ^
/home/lukasza/src/github/cxx/target/debug/build/demo-4b4fc931f198a6dd/out/cxxbridge/sources/demo/src/main.rs.cc:855:8: note: in instantiation of member function 'rust::Vec<rust::String>::~Vec' requested here
  855 | struct BlobMetadata final {
      |        ^
/home/lukasza/src/github/cxx/target/debug/build/demo-4b4fc931f198a6dd/out/cxxbridge/include/rust/cxx.h:395:8: note: forward declaration of template entity is here
  395 |   void drop() noexcept;
      |        ^
/home/lukasza/src/github/cxx/target/debug/build/demo-4b4fc931f198a6dd/out/cxxbridge/include/rust/cxx.h:887:9: note: add an explicit instantiation declaration to suppress this warning if 'rust::Vec<rust::String>::drop' is explicitly instantiated in another translation unit
  887 |   this->drop();
      |         ^
1 error generated.

I tried to forward-declare the template in 6e2c9d9, but it was only partially successful. It did help to avoid the -Wundefined-func-template error, but my attempt to declare Vec<rust::String>::drop and other private methods didn't duplicate all the definitions from the main template (e.g. emplace_back) and ultimately failed with the following error:

/home/lukasza/src/github/cxx/target/debug/build/cxx-test-suite-f70c7ddc7bb449f2/out/cxxbridge/sources/tests/ffi/lib.rs.cc:1819:27: error: no matching constructor for initialization of '::rust::Vec< ::rust::String>'
 1819 |   c_take_rust_vec_string$(::rust::Vec<::rust::String>(::rust::unsafe_bitcopy, *v));
      |                           ^                           ~~~~~~~~~~~~~~~~~~~~~~~~~~
...
/home/lukasza/src/github/cxx/target/debug/build/cxx-test-suite-f70c7ddc7bb449f2/out/cxxbridge/include/rust/cxx.h:1151:3: note: candidate constructor not viable: requires 0 arguments, but 2 were provided
 1151 |   Vec() noexcept;
      |   ^
1 error generated.
src/blobstore.cc:61:48: error: no member named 'emplace_back' in 'rust::Vec<rust::String>'
   61 |                   [&](auto &t) { metadata.tags.emplace_back(t); });
      |                                  ~~~~~~~~~~~~~ ^
1 error generated.

[anforowicz]

Oops. I think in my previous comment I didn't C++ quite right... :-/ The following snippet is the right way to forward-declare those methods in include/cxx.h:

template<> Vec<rust::String>::Vec() noexcept;
template<> std::size_t Vec<rust::String>::size() const noexcept;
template<> std::size_t Vec<rust::String>::capacity() const noexcept;
template<> const rust::String* Vec<rust::String>::data() const noexcept;
template<> void Vec<rust::String>::truncate(std::size_t len);
template<> void Vec<rust::String>::reserve_total(std::size_t new_cap) noexcept;
template<> void Vec<rust::String>::set_len(std::size_t len) noexcept;
template<> void Vec<rust::String>::drop() noexcept;

Instead of doing the above manually, I guess it may be possible to hoist FOR_EACH_RUST_VEC (and consequently also FOR_EACH_NUMERIC) from src/cxx.cc to include/cxx.h (probably prefixing the macros with CXXBRIDGE1_ to avoid naming conflicts).

And in addition to that I guess something similar can be done in fn write_generic_instantiations in gen/src/write.rs which currently doesn't generate anything if out.header:

cxx/gen/src/write.rs

Lines 1553 to 1556 in 6c3e01d

	fn write_generic_instantiations(out: &mut OutFile) {
	if out.header {
	return;
	}

[anforowicz]

FWIW after 5d36773, CXX=clang CXXFLAGS="-Wundefined-func-template -Werror" cargo test --workspace gets further, but still fails - this time in linking:

  = note: rust-lld: error: undefined symbol: rust::cxxbridge1::Vec<bool>::Vec(std::initializer_list<bool>)
          >>> referenced by tests.cc:194
          >>>               a1edd97dd51cd48d-tests.o:(tests::c_return_rust_vec_bool()) in archive /home/lukasza/src/github/cxx/target/debug/deps/libcxx_test_suite-b6df211d48834f0a.rlib

[anforowicz]

(and also the latest attempt ignores rust::detail::char_if_unique for now - probably can't land like this)

[jmcclellan-figma]

Amazing! Thanks so much for digging in here - I think this is really close. The linker problem seems to be the lines like extern template class Vec<CXX_TYPE> - these prevent the.cc files from actually outputting the template functions defined in the header such as the initializer_list constructor, and we should probably just remove these lines.

The forward declarations just of the specializations of functions we don't have in the header, like drop, will be enough to satisfy the warning (and the standard!)

[anforowicz]

Thanks! After 1cb8b94:

• CXX=clang CXXFLAGS="-Wundefined-func-template -Werror" cargo test --workspace passes
• CI passes: https://github.com/anforowicz/cxx/actions/runs/19577489455

One thing that I am not quite sure about is Opt::cxx_impl_annotations. IIUC "normal" (free, non-template) function declarations are written via fn write_rust_function_shim_impl which calls into fn write_rust_function_shim_decl which calls into fn begin_function_definition which writes out.opt.cxx_impl_annotations. And so it seems that this should be replicated somehow, but:

• I don't quite understand why in some cases it is okay if only .cc files get an annotation - e.g. Chromium's third_party/rust/chromium_crates_io/patches/cxx/0004-Define-exported-cxx-symbols.patch only injects CXX_RS_EXPORT into cxx.cc
• Since the tests+CI pass without changing this, ideally we would figure out how to add a regression test against forgetting to propagate cxx_impl_annotations. Testing linker attributes in an end-to-end test seems difficult, so maybe gen/src/write.rs should also grow a mini-framework for unit testing (simlar to unit tests that are present at the end of macro/src/expand.rs).
• It seems quite nice that in the latest fix attempt fn write_rust_vec_header can just reuse/call CXXBRIDGE1_RUST_VEC_DECLS({inner}).
  • Maybe we should just inline CXXBRIDGE1_RUST_VEC_DECLS into fn write_rust_vec_header?
  • Alternatively, maybe CXXBRIDGE1_RUST_VEC_DECLS can take an extra macro argument representing cxx_impl_annotations.

[jmcclellan-figma]

I haven't used the annotation feature so I don't have too much to add here - but I think whether it's okay to skip the annotation in the declaration depends deeply on the semantics of the annotation, for example, I believe GCC-style visibility annotations are only strictly required on definitions, and can be skipped on declarations (see here).