Skip to content

Commit

Permalink
Make experimental Kotlin features optional for consumers.
Browse files Browse the repository at this point in the history
Previously, we were using two experimental features of Kotlin,
in different ways:

* Unsigned types, both as API-level types and for some internal
  helpers.
* The `buildMap` function, for easily building a Map.

Both of these were emitted as part of the generated Kotlin code,
regardless of whether the component actually needed them.

In the first case, consumers would get a warning that they need
to opt-in to using experimental unsigned types, even if the API
they were consuming did not use any unsigned types.

In the second case, we were silently opting in to an experimental
API, meaning that consumers might find the generated code broken
by a future Kotlin release without any warning.

This commit removes default-on experimental Kotlin APIs. Now,
the only time Kotlin consumers will hear about experimental APIs
is if they're using a component with unsigned integers in its
public API, in which case they will need to explicitly opt in
(and hence will be aware of the potential for bustage if the
feature gets removed or changed in future Kotlin releases).

The key here was to refactor the ways the `ComponentInterface`
deals with types, so that we can inspect the set of types that
are actually used by the interface and emit only the code required
for those types. This is encapsulated in a new `TypeUniverse`
struct whose job is to maintain that whole-interface view of the
set of types in use.
  • Loading branch information
rfk committed Sep 12, 2020
1 parent 5fe0cff commit 1f338f4
Show file tree
Hide file tree
Showing 8 changed files with 561 additions and 416 deletions.
47 changes: 25 additions & 22 deletions uniffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,10 +197,10 @@ impl_via_ffi_for_num_primitive! {

/// Support for passing boolean values via the FFI.
///
/// Booleans are passed as a `u8` in order to avoid problems with handling
/// Booleans are passed as an `i8` in order to avoid problems with handling
/// C-compatible boolean values on JVM-based languages.
unsafe impl ViaFfi for bool {
type FfiType = u8;
type FfiType = i8;

fn lower(self) -> Self::FfiType {
if self {
Expand All @@ -219,12 +219,12 @@ unsafe impl ViaFfi for bool {
}

fn write<B: BufMut>(&self, buf: &mut B) {
buf.put_u8(ViaFfi::lower(*self));
buf.put_i8(ViaFfi::lower(*self));
}

fn try_read<B: Buf>(buf: &mut B) -> Result<Self> {
check_remaining(buf, 1)?;
ViaFfi::try_lift(buf.get_u8())
ViaFfi::try_lift(buf.get_i8())
}
}

Expand All @@ -236,8 +236,9 @@ unsafe impl ViaFfi for bool {
/// *must* be a valid `RustBuffer` and it *must* contain valid utf-8 data (in other
/// words, it *must* be a `Vec<u8>` suitable for use as an actual rust `String`).
///
/// When serialized in a buffer, strings are represented as a u32 byte length
/// followed by utf8-encoded bytes.
/// When serialized in a buffer, strings are represented as a i32 byte length
/// followed by utf8-encoded bytes. (It's a signed integer because unsigned types are
/// currently experimental in Kotlin).
unsafe impl ViaFfi for String {
type FfiType = RustBuffer;

Expand All @@ -263,14 +264,14 @@ unsafe impl ViaFfi for String {
fn write<B: BufMut>(&self, buf: &mut B) {
// N.B. `len()` gives us the length in bytes, not in chars or graphemes.
// TODO: it would be nice not to panic here.
let len = u32::try_from(self.len()).unwrap();
buf.put_u32(len); // We limit strings to u32::MAX bytes
let len = i32::try_from(self.len()).unwrap();
buf.put_i32(len); // We limit strings to u32::MAX bytes
buf.put(self.as_bytes());
}

fn try_read<B: Buf>(buf: &mut B) -> Result<Self> {
check_remaining(buf, 4)?;
let len = buf.get_u32() as usize;
let len = usize::try_from(buf.get_i32())?;
check_remaining(buf, len)?;
let bytes = &buf.bytes()[..len];
let res = String::from_utf8(bytes.to_vec())?;
Expand Down Expand Up @@ -301,17 +302,17 @@ unsafe impl<T: ViaFfi> ViaFfi for Option<T> {

fn write<B: BufMut>(&self, buf: &mut B) {
match self {
None => buf.put_u8(0),
None => buf.put_i8(0),
Some(v) => {
buf.put_u8(1);
buf.put_i8(1);
ViaFfi::write(v, buf);
}
}
}

fn try_read<B: Buf>(buf: &mut B) -> Result<Self> {
check_remaining(buf, 1)?;
Ok(match buf.get_u8() {
Ok(match buf.get_i8() {
0 => None,
1 => Some(<T as ViaFfi>::try_read(buf)?),
_ => bail!("unexpected tag byte for Option"),
Expand All @@ -322,7 +323,8 @@ unsafe impl<T: ViaFfi> ViaFfi for Option<T> {
/// Support for passing vectors of values via the FFI.
///
/// Vectors are currently always passed by serializing to a buffer.
/// We write a `u32` item count followed by each item in turn.
/// We write a `i32` item count followed by each item in turn.
/// (It's a nsigned type due to limits of the JVM).
///
/// Ideally we would pass `Vec<u8>` directly as a `RustBuffer` rather
/// than serializing, and perhaps even pass other vector types using a
Expand All @@ -340,17 +342,17 @@ unsafe impl<T: ViaFfi> ViaFfi for Vec<T> {

fn write<B: BufMut>(&self, buf: &mut B) {
// TODO: would be nice not to panic here :-/
let len = u32::try_from(self.len()).unwrap();
buf.put_u32(len); // We limit arrays to u32::MAX items
let len = i32::try_from(self.len()).unwrap();
buf.put_i32(len); // We limit arrays to i32::MAX items
for item in self.iter() {
ViaFfi::write(item, buf);
}
}

fn try_read<B: Buf>(buf: &mut B) -> Result<Self> {
check_remaining(buf, 4)?;
let len = buf.get_u32();
let mut vec = Vec::with_capacity(len as usize);
let len = usize::try_from(buf.get_i32())?;
let mut vec = Vec::with_capacity(len);
for _ in 0..len {
vec.push(<T as ViaFfi>::try_read(buf)?)
}
Expand All @@ -363,8 +365,9 @@ unsafe impl<T: ViaFfi> ViaFfi for Vec<T> {
/// the key must always be of the String type.
///
/// HashMaps are currently always passed by serializing to a buffer.
/// We write a `u32` entries count followed by each entry (string
/// We write a `i32` entries count followed by each entry (string
/// key followed by the value) in turn.
/// (It's a signed type due to limits of the JVM).
unsafe impl<V: ViaFfi> ViaFfi for HashMap<String, V> {
type FfiType = RustBuffer;

Expand All @@ -378,8 +381,8 @@ unsafe impl<V: ViaFfi> ViaFfi for HashMap<String, V> {

fn write<B: BufMut>(&self, buf: &mut B) {
// TODO: would be nice not to panic here :-/
let len = u32::try_from(self.len()).unwrap();
buf.put_u32(len); // We limit HashMaps to u32::MAX entries
let len = i32::try_from(self.len()).unwrap();
buf.put_i32(len); // We limit HashMaps to i32::MAX entries
for (key, value) in self.iter() {
ViaFfi::write(key, buf);
ViaFfi::write(value, buf);
Expand All @@ -388,8 +391,8 @@ unsafe impl<V: ViaFfi> ViaFfi for HashMap<String, V> {

fn try_read<B: Buf>(buf: &mut B) -> Result<Self> {
check_remaining(buf, 4)?;
let len = buf.get_u32();
let mut map = HashMap::with_capacity(len as usize);
let len = usize::try_from(buf.get_i32())?;
let mut map = HashMap::with_capacity(len);
for _ in 0..len {
let key = String::try_read(buf)?;
let value = <V as ViaFfi>::try_read(buf)?;
Expand Down
1 change: 0 additions & 1 deletion uniffi_bindgen/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,3 @@ anyhow = "1"
askama = "0.10"
heck = "0.3"
clap = "2"
serde = "1"
64 changes: 10 additions & 54 deletions uniffi_bindgen/src/bindings/kotlin/gen_kotlin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,22 +111,9 @@ mod filters {
pub fn lower_kt(nm: &dyn fmt::Display, type_: &Type) -> Result<String, askama::Error> {
let nm = var_name_kt(nm)?;
Ok(match type_ {
Type::Optional(t) => format!(
"lowerOptional({}, {{ v, buf -> {} }})",
nm,
write_kt(&"v", &"buf", t)?
),
Type::Sequence(t) => format!(
"lowerSequence({}, {{ v, buf -> {} }})",
nm,
write_kt(&"v", &"buf", t)?
),
Type::Map(t) => format!(
"lowerMap({}, {{ k, v, buf -> {}; {} }})",
nm,
write_kt(&"k", &"buf", &Type::String)?,
write_kt(&"v", &"buf", t)?
),
Type::Optional(_) | Type::Sequence(_) | Type::Map(_) => {
format!("lower{}({})", class_name_kt(&type_.canonical_name())?, nm,)
}
_ => format!("{}.lower()", nm),
})
}
Expand All @@ -142,24 +129,11 @@ mod filters {
) -> Result<String, askama::Error> {
let nm = var_name_kt(nm)?;
Ok(match type_ {
Type::Optional(t) => format!(
"writeOptional({}, {}, {{ v, buf -> {} }})",
nm,
target,
write_kt(&"v", &"buf", t)?
),
Type::Sequence(t) => format!(
"writeSequence({}, {}, {{ v, buf -> {} }})",
Type::Optional(_) | Type::Sequence(_) | Type::Map(_) => format!(
"write{}({}, {})",
class_name_kt(&type_.canonical_name())?,
nm,
target,
write_kt(&"v", &"buf", t)?
),
Type::Map(t) => format!(
"writeMap({}, {}, {{ k, v, buf -> {}; {} }})",
nm,
target,
write_kt(&"k", &"buf", &Type::String)?,
write_kt(&"v", &"buf", t)?
),
_ => format!("{}.write({})", nm, target),
})
Expand All @@ -172,18 +146,9 @@ mod filters {
pub fn lift_kt(nm: &dyn fmt::Display, type_: &Type) -> Result<String, askama::Error> {
let nm = nm.to_string();
Ok(match type_ {
Type::Optional(t) => {
format!("liftOptional({}, {{ buf -> {} }})", nm, read_kt(&"buf", t)?)
Type::Optional(_) | Type::Sequence(_) | Type::Map(_) => {
format!("lift{}({})", class_name_kt(&type_.canonical_name())?, nm,)
}
Type::Sequence(t) => {
format!("liftSequence({}, {{ buf -> {} }})", nm, read_kt(&"buf", t)?)
}
Type::Map(t) => format!(
"liftMap({}, {{ buf -> Pair({}, {}) }})",
nm,
read_kt(&"buf", &Type::String)?,
read_kt(&"buf", t)?
),
_ => format!("{}.lift({})", type_kt(type_)?, nm),
})
}
Expand All @@ -195,18 +160,9 @@ mod filters {
pub fn read_kt(nm: &dyn fmt::Display, type_: &Type) -> Result<String, askama::Error> {
let nm = nm.to_string();
Ok(match type_ {
Type::Optional(t) => {
format!("readOptional({}, {{ buf -> {} }})", nm, read_kt(&"buf", t)?)
}
Type::Sequence(t) => {
format!("readSequence({}, {{ buf -> {} }})", nm, read_kt(&"buf", t)?)
Type::Optional(_) | Type::Sequence(_) | Type::Map(_) => {
format!("read{}({})", class_name_kt(&type_.canonical_name())?, nm,)
}
Type::Map(t) => format!(
"readMap({}, {{ buf -> Pair({}, {}) }})",
nm,
read_kt(&"buf", &Type::String)?,
read_kt(&"buf", t)?
),
_ => format!("{}.read({})", type_kt(type_)?, nm),
})
}
Expand Down
1 change: 0 additions & 1 deletion uniffi_bindgen/src/bindings/kotlin/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ pub fn compile_bindings(ci: &ComponentInterface, out_dir: &Path) -> Result<()> {
let mut jar_file = PathBuf::from(out_dir);
jar_file.push(format!("{}.jar", ci.namespace()));
let status = Command::new("kotlinc")
.arg("-Xopt-in=kotlin.RequiresOptIn")
.arg("-Xopt-in=kotlin.ExperimentalUnsignedTypes")
.arg("-classpath")
.arg(env::var("CLASSPATH").unwrap_or_else(|_| "".to_string()))
Expand Down
Loading

0 comments on commit 1f338f4

Please sign in to comment.