Remove unnecessary restrictions around RTTI from wire.h

[dnfield]

Fixes #6551

Removes the static_asserts that check if the type for a LightTypeID is_polymorphic. AFAICT, LightTypeID works fine with polymorphic types, and provides the same identity guarantees as when RTTI is enabled. Adds a test asserting such code compiles and runs correctly.

I'm still not clear why this restriction was in place - it may have been necessary in previous impelmentations, or perhaps with previous versions of LLVM/Clang. However, as far as I can tell, it's not necessary now and only limits options around doing non-RTTI builds with emscripten.

/cc @kjlubick, @yjbanov - this would help building CanvasKit with -fno-rtti, in part.

[welcome]

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

[kripken]

Thanks! This looks good to me, but like you, I'm not sure why this existed.

@chadaustin do you remember?

Also @jgravelle-google may have insights.

[kripken]

the test failure may be due to this change?

[dnfield]

Ahh whups, my editor was set to trim whitespace at EOL. Reverted that line.

[dnfield]

@chinmaygarde knows lots more about C++ than I do, he might have some insight on this. Or maybe @jason-simmons

[abigailbunyan]

This changes the behaviour in subtle ways, and I think it's potentially incorrect. Consider:

struct Base { virtual ~Base() {} };
struct Derived : Base { ~Derived(); };

TYPEID f() {
    Derived d;
    Base& b = d;
    const auto id = getLightTypeID(b);
};

Previously, f would return the TYPEID of Derived, now it returns the TYPEID of Base. This getLightTypeID function then becomes the getActualType function of the registered class, which looks like it would cause problems down the line.

[dnfield]

@abigailbunyan is that just fine as long as you provide a factory constructor to embind?

Can you think of a sample program that would fail with this change? If nothing else, we should add it as a test to this repo.

[chadaustin]

I had a big comment written about how I didn't recall the context behind the check... but I think it's come back to me. :)

getLightTypeID returns the actual type ID of a potentially-polymorphic value. You can see its use in bind.h's _embind_register_class, which passes an instantiation of getActualType into JavaScript.

I don't know precisely what effect this change would have, but I think it might break code where a polymorphic base class pointer is passed to JavaScript, and JavaScript calls a derived class's bound methods on it.

If you can show that 1) we have tests for that scenario and 2) said tests pass with -fno-rtti, I'd support relaxing this constraint, even though I don't understand how it would work. :)

I seem to recall the cost of RTTI in our use of embind at IMVU not being high when everything was statically linked: only the referenced RTTI tables were include in the binary.

[dnfield]

I'll see if I can make a test case for that. @chadaustin - this change allows Skia CanvasKit to compile as-is with -fno-rtti, which saves ~100kb on the WASM blob. I'm fairly confident there's no actual need for RTTI in that case, but it would still help to prove out the case you're describing.

[dnfield]

@chadaustin - I'm having trouble constructing a case where this causes problems that don't already exist with RTTI. For example:

#include <emscripten/bind.h>
#include <memory>

#include <string>

class Poly {
public:
  virtual ~Poly() {}
  virtual std::string message() = 0;
  virtual Poly *get() = 0;
};

class PolyHi : public Poly {
public:
  std::string message() override { return std::string("Hi"); }
  Poly *get() override { return static_cast<Poly *>(this); }
  std::string nonVirtual() { return std::string("nonVirtual"); }
};

std::shared_ptr<Poly> MakePoly() { return std::make_shared<PolyHi>(); }
std::shared_ptr<PolyHi> MakePolyHi() { return std::make_shared<PolyHi>(); }

EMSCRIPTEN_BINDINGS(interface_tests) {
  emscripten::class_<Poly>("Poly")
      .smart_ptr_constructor("Poly", &MakePoly)
      .function("message", &Poly::message)
      .function("get", &Poly::get, emscripten::allow_raw_pointers());
  emscripten::class_<PolyHi>("PolyHi")
      .smart_ptr_constructor("PolyHi", &MakePolyHi)
      .function("message", &PolyHi::message)
      .function("get", &PolyHi::get, emscripten::allow_raw_pointers())
      .function("nonVirtual", &PolyHi::nonVirtual);
};

nonVirtual is not available on the object returned from get (in JavaScript) with or without RTTI. What am I missing?

[luigi-rosso]

Another 👍 for this PR. We ran into similar problems with this restriction when trying to compile with -fno-rtti. Using @dnfield's fork fixed the issue (compiles and no runtime issues). We have a bunch of abstract classes with pure virtual methods implemented in JS and no need for RTTI. We don't call any virtual methods from JS as @chadaustin was expressing concern over, however.

[kripken]

I haven't understood all the details here, so sorry if this is not helpful, but could we land this change guarded by #if !__has_feature(cxx_rtti)? That is, to change nothing when rtti is enabled, but if the user disabled it, to do what this PR does?

[dnfield]

@kripken - I'm not quite sure I follow what you're suggesting.

This PR removes case where we assert if rtti is disabled. I have not been able to come up with a test that would fail with this change, although if there is one we should add it and could make a decision then about whether to reject this.

I can try to fix up the merge conflict if we want to move forward with this. It would be beneficial to Flutter users though.

[kripken]

@dnfield Oh, right, I had that backwards... yes, ignore my suggestion.

Rereading this, it looks like your attempted testcase should answer the question @chadaustin posed. It would be better to understand this more deeply (which I don't) but I think we have enough to move forward here.

In the worst case, if we did miss something here, and someone files a bug later, we can revert it (and we'd have a testcase). Let's add a mention in the Changelog for this, so that it will be easier to find in case there is such a report.

[dnfield]

I think I fixed the merge conflict, hopefully I did it right :)

[dnfield]

Updated the changelog, tests should all be good now.

[sbc100]

Can you put this code into test/core/test_embind_polymorphic_class_no_rtti.cpp and the expected output in test/core/test_embind_polymorphic_class_no_rtti.out
and then run with self.do_run_in_out_file_test?

[dnfield]

Done