Merge pull request rustwasm#1199 from chinedufn/ref-slice

Whitelist for slice args that do not need to be mutable

crates/web-sys/tests/wasm/element.js

@@ -151,6 +151,11 @@ export function new_title() {
   return document.createElement("title");
 }
 
+export function new_webgl_rendering_context() {
+  const canvas = document.createElement('canvas');
+  return canvas.getContext('webgl');
+}
+
 export function new_xpath_result() {
     let xmlDoc = new DOMParser().parseFromString("<root><value>tomato</value></root>", "application/xml");
     let xpathResult = xmlDoc.evaluate("/root//value", xmlDoc, null, XPathResult.ANY_TYPE, null);

crates/web-sys/tests/wasm/main.rs

@@ -56,6 +56,7 @@ pub mod style_element;
 pub mod table_element;
 pub mod title_element;
 pub mod xpath_result;
+pub mod whitelisted_immutable_slices;
 
 #[wasm_bindgen_test]
 fn deref_works() {

crates/web-sys/tests/wasm/whitelisted_immutable_slices.rs

@@ -0,0 +1,57 @@
+//! When generating our web_sys APIs we default to setting slice references that
+//! get passed to JS as mutable in case they get mutated in JS.
+//!
+//! In certain cases we know for sure that the slice will not get mutated - for
+//! example when working with the WebGlRenderingContext APIs.
+//!
+//! These tests ensure that whitelisted methods do indeed accept mutable slices.
+//! Especially important since this whitelist is stringly typed and currently
+//! maintained by hand.
+//!
+//! @see https://github.com/rustwasm/wasm-bindgen/issues/1005
+
+use wasm_bindgen::prelude::*;
+use wasm_bindgen_test::*;
+use web_sys::WebGlRenderingContext;
+
+#[wasm_bindgen(module = "./tests/wasm/element.js")]
+extern "C" {
+    fn new_webgl_rendering_context() -> WebGlRenderingContext;
+    // TODO: Add a function to create another type to test here.
+    // These functions come from element.js
+}
+
+// TODO: Uncomment WebGlRenderingContext test. Every now and then we can check if this works
+// in the latest geckodriver.
+//
+// Currently commented out because WebGl isn't working in geckodriver.
+//
+// It currently works in chromedriver so if you need to run this in the meantime you can
+// uncomment this block and run it in using chromedriver.
+//
+// CHROMEDRIVER=chromedriver cargo test --manifest-path crates/web-sys/Cargo.toml --target wasm32-unknown-unknown --all-features test_webgl_rendering_context_immutable_slices
+
+// Ensure that our whitelisted WebGlRenderingContext methods work
+//#[wasm_bindgen_test]
+//fn test_webgl_rendering_context_immutable_slices() {
+//    let gl = new_webgl_rendering_context();
+//
+//    gl.vertex_attrib1fv_with_f32_array(0, &[1.]);
+//    gl.vertex_attrib2fv_with_f32_array(0, &[1.]);
+//    gl.vertex_attrib3fv_with_f32_array(0, &[1.]);
+//    gl.vertex_attrib4fv_with_f32_array(0, &[1.]);
+//
+//    gl.uniform1fv_with_f32_array(None, &[1.]);
+//    gl.uniform2fv_with_f32_array(None, &[1.]);
+//    gl.uniform3fv_with_f32_array(None, &[1.]);
+//    gl.uniform4fv_with_f32_array(None, &[1.]);
+//
+//    gl.uniform_matrix2fv_with_f32_array(None, false, &[1.]);
+//    gl.uniform_matrix3fv_with_f32_array(None, false, &[1.]);
+//    gl.uniform_matrix4fv_with_f32_array(None, false, &[1.]);
+//}
+
+// TODO:
+//#[wasm_bindgen_test]
+//fn test_another_types_immutable_slices_here() {
+//}

crates/webidl-tests/lib.rs

@@ -1 +1 @@
-// intentionally left blank
+// Intentionally left blank in order for Cargo to work

crates/webidl/src/first_pass.rs

@@ -37,6 +37,7 @@ pub(crate) struct FirstPassRecord<'src> {
     pub(crate) dictionaries: BTreeMap<&'src str, DictionaryData<'src>>,
     pub(crate) callbacks: BTreeSet<&'src str>,
     pub(crate) callback_interfaces: BTreeMap<&'src str, CallbackInterfaceData<'src>>,
+    pub(crate) immutable_f32_whitelist: BTreeSet<&'static str>
 }
 
 /// We need to collect interface data during the first pass, to be used later.
@@ -83,6 +84,9 @@ pub(crate) struct CallbackInterfaceData<'src> {
 #[derive(PartialEq, Eq, PartialOrd, Ord, Debug, Clone, Copy)]
 pub(crate) enum OperationId<'src> {
     Constructor(IgnoreTraits<&'src str>),
+    /// The name of a function in crates/web-sys/webidls/enabled/*.webidl
+    ///
+    /// ex: Operation(Some("vertexAttrib1fv"))
     Operation(Option<&'src str>),
     IndexingGetter,
     IndexingSetter,

crates/webidl/src/idl_type.rs

@@ -41,7 +41,10 @@ pub(crate) enum IdlType<'a> {
     Uint16Array,
     Int32Array,
     Uint32Array,
-    Float32Array,
+    Float32Array {
+        /// Whether or not the generated web-sys function should use an immutable slice
+        immutable: bool
+    },
     Float64Array,
     ArrayBufferView,
     BufferSource,
@@ -324,14 +327,23 @@ impl<'a> ToIdlType<'a> for Identifier<'a> {
     }
 }
 
+// We default to Float32Array's being mutable, but in certain cases where we're certain that
+// slices won't get mutated on the JS side (such as the WebGL APIs) we might, later in the flow,
+// instead use the immutable version.
+impl<'a> ToIdlType<'a> for term::Float32Array {
+    fn to_idl_type(&self, _record: &FirstPassRecord<'a>) -> IdlType<'a> {
+        IdlType::Float32Array {immutable: false}
+    }
+}
+
 macro_rules! terms_to_idl_type {
     ($($t:tt => $r:tt)*) => ($(
         impl<'a> ToIdlType<'a> for term::$t {
             fn to_idl_type(&self, _record: &FirstPassRecord<'a>) -> IdlType<'a> {
                 IdlType::$r
             }
         }
-    )*)
+    )*);
 }
 
 terms_to_idl_type! {
@@ -358,7 +370,6 @@ terms_to_idl_type! {
     Uint16Array => Uint16Array
     Uint32Array => Uint32Array
     Uint8ClampedArray => Uint8ClampedArray
-    Float32Array => Float32Array
     Float64Array => Float64Array
     ArrayBufferView => ArrayBufferView
     BufferSource => BufferSource
@@ -396,7 +407,7 @@ impl<'a> IdlType<'a> {
             IdlType::Uint16Array => dst.push_str("u16_array"),
             IdlType::Int32Array => dst.push_str("i32_array"),
             IdlType::Uint32Array => dst.push_str("u32_array"),
-            IdlType::Float32Array => dst.push_str("f32_array"),
+            IdlType::Float32Array { .. } => dst.push_str("f32_array"),
             IdlType::Float64Array => dst.push_str("f64_array"),
             IdlType::ArrayBufferView => dst.push_str("array_buffer_view"),
             IdlType::BufferSource => dst.push_str("buffer_source"),
@@ -501,16 +512,16 @@ impl<'a> IdlType<'a> {
 
             IdlType::ArrayBuffer => js_sys("ArrayBuffer"),
             IdlType::DataView => None,
-            IdlType::Int8Array => Some(array("i8", pos)),
-            IdlType::Uint8Array => Some(array("u8", pos)),
-            IdlType::Uint8ArrayMut => Some(array("u8", pos)),
-            IdlType::Uint8ClampedArray => Some(clamped(array("u8", pos))),
-            IdlType::Int16Array => Some(array("i16", pos)),
-            IdlType::Uint16Array => Some(array("u16", pos)),
-            IdlType::Int32Array => Some(array("i32", pos)),
-            IdlType::Uint32Array => Some(array("u32", pos)),
-            IdlType::Float32Array => Some(array("f32", pos)),
-            IdlType::Float64Array => Some(array("f64", pos)),
+            IdlType::Int8Array => Some(array("i8", pos, false)),
+            IdlType::Uint8Array => Some(array("u8", pos, false)),
+            IdlType::Uint8ArrayMut => Some(array("u8", pos, false)),
+            IdlType::Uint8ClampedArray => Some(clamped(array("u8", pos, false))),
+            IdlType::Int16Array => Some(array("i16", pos, false)),
+            IdlType::Uint16Array => Some(array("u16", pos, false)),
+            IdlType::Int32Array => Some(array("i32", pos, false)),
+            IdlType::Uint32Array => Some(array("u32", pos, false)),
+            IdlType::Float32Array {immutable} => Some(array("f32", pos, *immutable)),
+            IdlType::Float64Array => Some(array("f64", pos, false)),
 
             IdlType::ArrayBufferView | IdlType::BufferSource => js_sys("Object"),
             IdlType::Interface(name)

crates/webidl/src/lib.rs

@@ -82,6 +82,8 @@ fn parse(webidl_source: &str, allowed_types: Option<&[&str]>) -> Result<Program>
 
     let mut first_pass_record: FirstPassRecord = Default::default();
     first_pass_record.builtin_idents = builtin_idents();
+    first_pass_record.immutable_f32_whitelist = immutable_f32_whitelist();
+
     definitions.first_pass(&mut first_pass_record, ())?;
     let mut program = Default::default();
     let mut submodules = Vec::new();
@@ -179,6 +181,26 @@ fn builtin_idents() -> BTreeSet<Ident> {
     )
 }
 
+fn immutable_f32_whitelist() -> BTreeSet<&'static str> {
+    BTreeSet::from_iter(
+        vec![
+            // WebGlRenderingContext
+            "uniform1fv",
+            "uniform2fv",
+            "uniform3fv",
+            "uniform4fv",
+            "uniformMatrix2fv",
+            "uniformMatrix3fv",
+            "uniformMatrix4fv",
+            "vertexAttrib1fv",
+            "vertexAttrib2fv",
+            "vertexAttrib3fv",
+            "vertexAttrib4fv",
+            // TODO: Add another type's functions here. Leave a comment header with the type name
+        ]
+    )
+}
+
 /// Run codegen on the AST to generate rust code.
 fn compile_ast(mut ast: Program) -> String {
     // Iteratively prune all entries from the AST which reference undefined

crates/webidl/src/util.rs

@@ -67,13 +67,13 @@ pub fn mdn_doc(class: &str, method: Option<&str>) -> String {
     format!("[MDN Documentation]({})", link).into()
 }
 
-// Array type is borrowed for arguments (`&[T]`) and owned for return value (`Vec<T>`).
-pub(crate) fn array(base_ty: &str, pos: TypePosition) -> syn::Type {
+// Array type is borrowed for arguments (`&mut [T]` or `&[T]`) and owned for return value (`Vec<T>`).
+pub(crate) fn array(base_ty: &str, pos: TypePosition, immutable: bool) -> syn::Type {
     match pos {
         TypePosition::Argument => {
             shared_ref(
                 slice_ty(ident_ty(raw_ident(base_ty))),
-                /*mutable =*/ true,
+                /*mutable =*/ !immutable,
             )
         }
         TypePosition::Return => vec_ty(ident_ty(raw_ident(base_ty))),
@@ -433,7 +433,10 @@ impl<'src> FirstPassRecord<'src> {
                     );
                     signatures.push((signature, idl_args.clone()));
                 }
-                idl_args.push(arg.ty.to_idl_type(self));
+
+                let mut idl_type = arg.ty.to_idl_type(self);
+                let idl_type = self.maybe_adjust(idl_type, id);
+                idl_args.push(idl_type);
             }
             signatures.push((signature, idl_args));
         }
@@ -627,6 +630,34 @@ impl<'src> FirstPassRecord<'src> {
         }
         return ret;
     }
+
+
+    /// When generating our web_sys APIs we default to setting slice references that
+    /// get passed to JS as mutable in case they get mutated in JS.
+    ///
+    /// In certain cases we know for sure that the slice will not get mutated - for
+    /// example when working with the WebGlRenderingContext APIs.
+    ///
+    /// Here we implement a whitelist for those cases. This whitelist is currently
+    /// maintained by hand.
+    ///
+    /// When adding to this whitelist add tests to crates/web-sys/tests/wasm/whitelisted_immutable_slices.rs
+    fn maybe_adjust<'a>(&self, mut idl_type: IdlType<'a>, id: &'a OperationId) -> IdlType<'a> {
+        let op = match id {
+            OperationId::Operation(Some(op)) => op,
+            _ => return idl_type
+        };
+
+        if self.immutable_f32_whitelist.contains(op) {
+            flag_slices_immutable(&mut idl_type)
+        }
+
+        // TODO: Add other whitelisted slices here, such as F64 or u8..
+
+        idl_type
+    }
+
+
 }
 
 /// Search for an attribute by name in some webidl object's attributes.
@@ -707,3 +738,22 @@ pub fn public() -> syn::Visibility {
         pub_token: Default::default(),
     })
 }
+
+fn flag_slices_immutable(ty: &mut IdlType) {
+    match ty {
+        IdlType::Float32Array { immutable } => *immutable = true,
+        IdlType::Nullable(item) => flag_slices_immutable(item),
+        IdlType::FrozenArray(item) => flag_slices_immutable(item),
+        IdlType::Sequence(item) => flag_slices_immutable(item),
+        IdlType::Promise(item) => flag_slices_immutable(item),
+        IdlType::Record(item1, item2) => {
+            flag_slices_immutable(item1);
+            flag_slices_immutable(item2);
+        },
+        IdlType::Union(list) => {
+            for item in list { flag_slices_immutable(item); }
+        }
+        // catch-all for everything else like Object
+        _ => {}
+    }
+}