Fix improper_ctypes warnings in Rust Nightly

[SimonSapin]

Steps:

• Make a branch up to date with origin/master
• Run ./mach rustup, which updates the rust-toolchain file
• Run ./mach build
• See many warnings like

  warning: `extern` fn uses type `script_runtime::JSContext`, which is not FFI-safe
     --> components/script/script_runtime.rs:829:56
      |
  829 | unsafe extern "C" fn report_stream_error_callback(_cx: *mut JSContext, error_code: usize) {
      |                                                        ^^^^^^^^^^^^^^ not FFI-safe
      |
      = note: `#[warn(improper_ctypes)]` on by default
      = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
      = note: this struct has unspecified layout

• Find a way to fix them. This may involve upgrading rust-bindgen, or making changes rust-bindgen and then upgrading to a version with those changes
• Revert the rust-toolchain change
• Submit a PR

We don’t need to upgrade the Rust version yet, but the next time we do those warnings would block landing the upgrade unless we change CI to allow build warnings.

Is suspect that rust-lang/rust#65134 is what introduced these warnings.

[jdm]

JSContext is indeed not FFI-safe. It should be a jsapi::JSContext instead, and #24653 fixes that.

[SimonSapin]

The Rust team is (kindly) waiting on us to be able to upgrade before landing rust-lang/rust#66344. I have a branch ready for that, but it requires a Nightly more recent than these new warnings.

[SimonSapin]

I’ve looked into this. Unfortunately there is no accidental bug here: the lint was intentionally changed to warn against a pattern that is in pervasive use in our DOM bindings.

Specifically: when an extern fn function (meant to be called by C++ or JIT’ed code in SpiderMonkey) takes as parameter a raw pointer to a Rust struct that is not FFI-safe.

Our DOM objects are based on many Rust structs that are not “FFI-safe”: non-Rust code cannot relay on their exact memory layout. Making a struct FFI-safe involves using #[repr(C)], but it shouldn’t be necessary for DOM objects that are only manipulated directly in Rust code, or through pointers in non-Rust code.

Currently, raw pointers to a non-FFI-safe type are themselves not considered FFI-safe by the improper_ctypes lint. I’ve commented in rust-lang/rust#66220 (comment) about potentially reconsidering this.

In the meantime, we could unblock the upgrade by adding #[allow(improper_ctypes)] to large parts of our FFI, but that seems rather counter-productive.

[SimonSapin]

we could unblock the upgrade by adding #[allow(improper_ctypes)] to large parts of our FFI

@jdm, @nox, how would you feel about doing this and keeping this issue open to track fixing it later? Though there’s a risk it falls through the cracks…

[jdm]

I'm fine with being pragmatic when we're blocking the Rust project from making progress. That being said, our DOM objects should already be repr(C). Which types are actually being warned about?

[SimonSapin]

We use repr(C) to preserve field order (and have the "parent" at the same memory address through being the first field), but many of these structs contain fields that are not FFI-safe themeselves: HashMap, RefCell, etc.

[nox]

One other solution would be to make the various extern "C" functions for which the lint is triggered take some sort of *mut c_void instead, which we then cast to the proper type inside the function.

[SimonSapin]

It’s possible. I started doing it for the this parameter of methods generated by CGSpecializedMethod in components/script/dom/bindings/codegen/CodegenRust.py, which cut the number of warnings in ~half.

But there are a number of other cases. It’s hard to say without doing it, but I wonder if pushing this to completion would lead us to having c_void almost everywhere, reducing type safety.

[CryZe]

Yeah while that would work, that would be pretty unfortunate. At the moment I‘m making heavy use of stuff like Option<&mut SomeRustType> as parameters in my C FFI, as it both allows for super clean safe handling from Rust‘s side and also nice automatically generated high level bindings for the other side. Reducing it to raw void pointers only would significantly hurt both.

[SimonSapin]

(Did you mean that comment for rust-lang/rust#66220?)

[camelid]

Is this still an issue?

[jdm]

We do not appear to have blanket allow_ctypes in most of our code (based on https://github.com/servo/servo/search?q=improper_ctypes&unscoped_q=improper_ctypes), so I think the answer is no.

[SimonSapin]

This was an issue because we run CI with RUSTFLAGS="-D warnings", but it’s not anymore because this new warning was reverted in rust-lang/rust#66378.

There is ongoing work at rust-lang/rust#72700 to add it again, but this time:

• Under a dedicated improper_ctypes_definitions lint name, separate from improper_ctypes, so we could #[allow] it without disabling previously-fine lints.
• It considers *const T, *mut T, &T and &mut T to be FFI-safe if T: Sized, even if T is not FFI-safe

With the latter change this lint should not be as much of an issue for as the initial version was.