servo / rust-mozjs

DEPRECATED - moved to servo/mozjs instead.
Mozilla Public License 2.0
293 stars 117 forks source link

Clean up the glue #484

Open nox opened 5 years ago

nox commented 5 years ago

So this crate has a jsglue.cpp file, yet another one, with more stuff than the one in servo/mozjs, but mostly duplicated stuff. It has corresponding bindings in glue.rs, but this file is not generated by bindgen during the build, it's just sitting there in the repo, never regenerated, so it may not even be in sync anymore. The instructions to build it are a comment with hardcoded paths in the otherwise 8 years old file gen.py.

We then wrap those with additional Rust methods, through macro calls generated in glue_wrappers.in (and jsapi_wrappers.in because, why not?). Those files are generated by the shell script generate_wrappers.sh, which is just a sed script in disguise.

Those two .in files aren't generated during build-time either, and the jsapi one is generated from the generated bindings from servo/mozjs_sys, but the script is hardcoded to read the debug bindings, so who knows if that's correct when building in release mode…

How did we let things become that out of hand? What is actually relevant, and what is not?

nox commented 5 years ago

Cc @jdm @asajeffrey

asajeffrey commented 5 years ago

Well the big picture "why have a glue.cpp file at all" is because bindgen has a hard time with C++ APIs, especially on windows where the ABI is :man_shrugging:. Since jsapi is very keen on C++ idioms we then have to put them back into C-like function calls that bindgen is happier with.

That said, the split between the mozjs_sys crate and the mozjs crate is rather arbitrary, and probably in need of a spring clean.

IIRC the .in files are there to bridge between mozjs_sys's Handle (which doesn't take a lifetime) and mozjs's (which does). Yes, those files are checked in, which is odd since we don't check in the result of running bindgen.

So yes, it looks like we have let some technical debt accumulate here.

nox commented 5 years ago

IIRC the .in files are there to bridge between mozjs_sys's Handle (which doesn't take a lifetime) and mozjs's (which does). Yes, those files are checked in, which is odd since we don't check in the result of running bindgen.

I feel like this script using grep and sed and generating macro calls is a failed experiment, and the technical debt wouldn't exist if we finally reached the conclusion that those bridges and safe wrappers (e.g. those additional lifetimes) just cannot be automated.

Would it really be that horrible if we just wrote such safe wrappers on demand, whenever we need a new API, rather than trying to mechanise that stuff weirdly?

jdm commented 5 years ago

My experience updating spidermonkey with the checked in wrapper files is that the compiler complains when there are invalid wrappers, so there is not actually a risk of drift.

asajeffrey commented 5 years ago

Do you mean get rid of the macros, or get rid of the shell script?

nox commented 5 years ago

My experience updating spidermonkey with the checked in wrapper files is that the compiler complains when there are invalid wrappers, so there is not actually a risk of drift.

But those wrappers aren't actually idiomatic, so if we actually wanted to have bindings that don't lead to a miserable experience, we would still need to write code by hand, at which point why do this weird grep/sed stuff?

I guess a good compromise would be to retcon the .in files as if they had always been written by hand, delete the shell script, and add future wrappers by hand in there?

nox commented 4 years ago

Doing the smup, I realise that all these wrappers are hindering me in CodegenRust.py, they are not helping.

nox commented 4 years ago

Like, wrappers for JS_DefineProperty, on one side we need to wrap RawHandleObject as HandleObject, and on the other we need to unwrap SafeJSContext as *mut JSContext, I do not like this.