Merge pull request #1026 from google/unsafe-dialect

Unsafe dialect

Author: adetaylor

engine/src/conversion/codegen_rs/fun_codegen.rs

@@ -21,7 +21,7 @@ use syn::{
 use super::{
     function_wrapper_rs::RustParamConversion,
     unqualify::{unqualify_params, unqualify_ret_type},
-    ImplBlockDetails, RsCodegenResult, TraitImplBlockDetails, Use,
+    ImplBlockDetails, MaybeUnsafeStmt, RsCodegenResult, TraitImplBlockDetails, Use,
 };
 use crate::{
     conversion::{
@@ -30,6 +30,7 @@ use crate::{
             TraitMethodDetails,
         },
         api::UnsafetyNeeded,
+        codegen_rs::maybe_unsafes_to_tokens,
     },
     types::{Namespace, QualifiedName},
 };
@@ -253,24 +254,21 @@ impl<'a> FnGenerator<'a> {
         let mut local_variables = Vec::new();
         let mut arg_list = Vec::new();
         let mut ptr_arg_name = None;
-        let wrap_unsafe_calls = self.should_wrap_unsafe_calls();
         let ret_type = Cow::Borrowed(ret_type);
         for pd in self.param_details {
             let wrapper_arg_name = if pd.self_type.is_some() && !avoid_self {
                 parse_quote!(self)
             } else {
                 pd.name.clone()
             };
-            let rust_for_param = pd
-                .conversion
-                .rust_conversion(wrapper_arg_name.clone(), wrap_unsafe_calls);
+            let rust_for_param = pd.conversion.rust_conversion(wrapper_arg_name.clone());
             let RustParamConversion {
                 ty,
                 conversion,
-                local_variables: these_local_variables,
+                local_variables: mut these_local_variables,
             } = rust_for_param;
             arg_list.push(conversion.clone());
-            local_variables.extend(these_local_variables.into_iter());
+            local_variables.append(&mut these_local_variables);
             if pd.is_placement_return_destination {
                 ptr_arg_name = Some(conversion);
             } else {
@@ -280,7 +278,6 @@ impl<'a> FnGenerator<'a> {
                 ));
             }
         }
-        let local_variables = quote! { #(#local_variables);* };
         if let Some(parameter_reordering) = &parameter_reordering {
             wrapper_params = Self::reorder_parameters(wrapper_params, parameter_reordering);
         }
@@ -293,34 +290,34 @@ impl<'a> FnGenerator<'a> {
         );
 
         let cxxbridge_name = self.cxxbridge_name;
-        let call_body = quote! {
-            cxxbridge::#cxxbridge_name ( #(#arg_list),* )
-        };
-        let call_body = if let Some(ptr_arg_name) = ptr_arg_name {
+        let call_body = MaybeUnsafeStmt::maybe_unsafe(
             quote! {
-                autocxx::moveit::new::by_raw(move |#ptr_arg_name| {
-                    let #ptr_arg_name = #ptr_arg_name.get_unchecked_mut().as_mut_ptr();
-                    #call_body
+                cxxbridge::#cxxbridge_name ( #(#arg_list),* )
+            },
+            !matches!(self.unsafety, UnsafetyNeeded::None),
+        );
+        let call_stmts = if let Some(ptr_arg_name) = ptr_arg_name {
+            let mut closure_stmts = local_variables;
+            closure_stmts.push(MaybeUnsafeStmt::binary(
+                quote! { let #ptr_arg_name = unsafe { #ptr_arg_name.get_unchecked_mut().as_mut_ptr() };},
+                quote! { let #ptr_arg_name = #ptr_arg_name.get_unchecked_mut().as_mut_ptr();},
+            ));
+            closure_stmts.push(call_body);
+            let closure_stmts = maybe_unsafes_to_tokens(closure_stmts, true);
+            vec![MaybeUnsafeStmt::needs_unsafe(parse_quote! {
+               autocxx::moveit::new::by_raw(move |#ptr_arg_name| {
+                    #closure_stmts
                 })
-            }
+            })]
         } else {
-            quote! {
-                #call_body
-            }
-        };
-        let call_body = if self.should_wrap_unsafe_calls() {
-            quote! {
-                unsafe {
-                    #call_body
-                }
-            }
-        } else {
-            call_body
-        };
-        let call_body = quote! {
-            #local_variables
-            #call_body
+            let mut call_stmts = local_variables;
+            call_stmts.push(call_body);
+            call_stmts
         };
+
+        let context_is_unsafe = matches!(self.unsafety, UnsafetyNeeded::Always)
+            || self.always_unsafe_due_to_trait_definition;
+        let call_body = maybe_unsafes_to_tokens(call_stmts, context_is_unsafe);
         (lifetime_tokens, wrapper_params, ret_type, call_body)
     }
 
@@ -413,11 +410,6 @@ impl<'a> FnGenerator<'a> {
         })
     }
 
-    fn should_wrap_unsafe_calls(&self) -> bool {
-        matches!(self.unsafety, UnsafetyNeeded::JustBridge)
-            || self.always_unsafe_due_to_trait_definition
-    }
-
     fn reorder_parameters(
         params: Punctuated<FnArg, Comma>,
         parameter_ordering: &[usize],

engine/src/conversion/codegen_rs/function_wrapper_rs.rs

@@ -16,25 +16,27 @@ use crate::{
 use quote::quote;
 use syn::parse_quote;
 
+use super::MaybeUnsafeStmt;
+
 /// Output Rust snippets for how to deal with a given parameter.
 pub(super) struct RustParamConversion {
     pub(super) ty: Type,
-    pub(super) local_variables: Option<TokenStream>,
+    pub(super) local_variables: Vec<MaybeUnsafeStmt>,
     pub(super) conversion: TokenStream,
 }
 
 impl TypeConversionPolicy {
     /// If returns `None` then this parameter should be omitted entirely.
-    pub(super) fn rust_conversion(&self, var: Pat, wrap_in_unsafe: bool) -> RustParamConversion {
+    pub(super) fn rust_conversion(&self, var: Pat) -> RustParamConversion {
         match self.rust_conversion {
             RustConversionType::None => RustParamConversion {
                 ty: self.converted_rust_type(),
-                local_variables: None,
+                local_variables: Vec::new(),
                 conversion: quote! { #var },
             },
             RustConversionType::FromStr => RustParamConversion {
                 ty: parse_quote! { impl ToCppString },
-                local_variables: None,
+                local_variables: Vec::new(),
                 conversion: quote! ( #var .into_cpp() ),
             },
             RustConversionType::ToBoxedUpHolder(ref sub) => {
@@ -45,7 +47,7 @@ impl TypeConversionPolicy {
                 };
                 RustParamConversion {
                     ty,
-                    local_variables: None,
+                    local_variables: Vec::new(),
                     conversion: quote! {
                         Box::new(#holder_type(#var))
                     },
@@ -61,7 +63,7 @@ impl TypeConversionPolicy {
                 };
                 RustParamConversion {
                     ty,
-                    local_variables: None,
+                    local_variables: Vec::new(),
                     conversion: quote! {
                         #var.get_unchecked_mut().as_mut_ptr()
                     },
@@ -77,7 +79,7 @@ impl TypeConversionPolicy {
                 };
                 RustParamConversion {
                     ty,
-                    local_variables: None,
+                    local_variables: Vec::new(),
                     conversion: quote! {
                         { let r: &mut _ = ::std::pin::Pin::into_inner_unchecked(#var.as_mut());
                             r
@@ -93,7 +95,7 @@ impl TypeConversionPolicy {
                 let ty = parse_quote! { &mut #ty };
                 RustParamConversion {
                     ty,
-                    local_variables: None,
+                    local_variables: Vec::new(),
                     conversion: quote! {
                         #var
                     },
@@ -106,29 +108,28 @@ impl TypeConversionPolicy {
                     panic!("Unexpected non-ident parameter name");
                 };
                 let space_var_name = make_ident(format!("{}_space", var_name));
-                let call = quote! { #space_var_name.as_mut().populate(#var_name);  };
-                let call = if wrap_in_unsafe {
-                    quote! {
-                        unsafe {
-                            #call
-                        }
-                    }
-                } else {
-                    call
-                };
                 let ty = &self.unwrapped_type;
                 let ty = parse_quote! { impl autocxx::ValueParam<#ty> };
                 // This is the usual trick to put something on the stack, then
                 // immediately shadow the variable name so it can't be accessed or moved.
                 RustParamConversion {
                     ty,
-                    local_variables: Some(quote! {
-                        let mut #space_var_name = autocxx::ValueParamHandler::default();
-                        let mut #space_var_name = unsafe {
-                            std::pin::Pin::new_unchecked(&mut #space_var_name)
-                        };
-                        #call
-                    }),
+                    local_variables: vec![
+                        MaybeUnsafeStmt::new(
+                            quote! { let mut #space_var_name = autocxx::ValueParamHandler::default(); },
+                        ),
+                        MaybeUnsafeStmt::binary(
+                            quote! { let mut #space_var_name =
+                                unsafe { std::pin::Pin::new_unchecked(&mut #space_var_name) };
+                            },
+                            quote! { let mut #space_var_name =
+                                std::pin::Pin::new_unchecked(&mut #space_var_name);
+                            },
+                        ),
+                        MaybeUnsafeStmt::needs_unsafe(
+                            quote! { #space_var_name.as_mut().populate(#var_name); },
+                        ),
+                    ],
                     conversion: quote! {
                         #space_var_name.get_ptr()
                     },

engine/src/conversion/codegen_rs/mod.rs

@@ -1206,3 +1206,138 @@ struct RsCodegenResult {
     trait_impl_entry: Option<Box<TraitImplBlockDetails>>,
     materializations: Vec<Use>,
 }
+
+/// An [`Item`] that always needs to be in an unsafe block.
+#[derive(Clone)]
+enum MaybeUnsafeStmt {
+    // This could almost be a syn::Stmt, but that doesn't quite work
+    // because the last stmt in a function is actually an expression
+    // thus lacking a semicolon.
+    Normal(TokenStream),
+    NeedsUnsafe(TokenStream),
+    Binary {
+        in_safe_context: TokenStream,
+        in_unsafe_context: TokenStream,
+    },
+}
+
+impl MaybeUnsafeStmt {
+    fn new(stmt: TokenStream) -> Self {
+        Self::Normal(stmt)
+    }
+
+    fn needs_unsafe(stmt: TokenStream) -> Self {
+        Self::NeedsUnsafe(stmt)
+    }
+
+    fn maybe_unsafe(stmt: TokenStream, needs_unsafe: bool) -> Self {
+        if needs_unsafe {
+            Self::NeedsUnsafe(stmt)
+        } else {
+            Self::Normal(stmt)
+        }
+    }
+
+    fn binary(in_safe_context: TokenStream, in_unsafe_context: TokenStream) -> Self {
+        Self::Binary {
+            in_safe_context,
+            in_unsafe_context,
+        }
+    }
+}
+
+fn maybe_unsafes_to_tokens(
+    items: Vec<MaybeUnsafeStmt>,
+    context_is_already_unsafe: bool,
+) -> TokenStream {
+    if context_is_already_unsafe {
+        let items = items.into_iter().map(|item| match item {
+            MaybeUnsafeStmt::Normal(stmt)
+            | MaybeUnsafeStmt::NeedsUnsafe(stmt)
+            | MaybeUnsafeStmt::Binary {
+                in_unsafe_context: stmt,
+                ..
+            } => stmt,
+        });
+        quote! {
+            #(#items)*
+        }
+    } else {
+        let mut currently_unsafe_list = None;
+        let mut output = Vec::new();
+        for item in items {
+            match item {
+                MaybeUnsafeStmt::NeedsUnsafe(stmt) => {
+                    if currently_unsafe_list.is_none() {
+                        currently_unsafe_list = Some(Vec::new());
+                    }
+                    currently_unsafe_list.as_mut().unwrap().push(stmt);
+                }
+                MaybeUnsafeStmt::Normal(stmt)
+                | MaybeUnsafeStmt::Binary {
+                    in_safe_context: stmt,
+                    ..
+                } => {
+                    if let Some(currently_unsafe_list) = currently_unsafe_list.take() {
+                        output.push(quote! {
+                            unsafe {
+                                #(#currently_unsafe_list)*
+                            }
+                        })
+                    }
+                    output.push(stmt);
+                }
+            }
+        }
+        if let Some(currently_unsafe_list) = currently_unsafe_list.take() {
+            output.push(quote! {
+                unsafe {
+                    #(#currently_unsafe_list)*
+                }
+            })
+        }
+        quote! {
+            #(#output)*
+        }
+    }
+}
+
+#[test]
+fn test_maybe_unsafes_to_tokens() {
+    let items = vec![
+        MaybeUnsafeStmt::new(quote! { use A; }),
+        MaybeUnsafeStmt::new(quote! { use B; }),
+        MaybeUnsafeStmt::needs_unsafe(quote! { use C; }),
+        MaybeUnsafeStmt::needs_unsafe(quote! { use D; }),
+        MaybeUnsafeStmt::new(quote! { use E; }),
+        MaybeUnsafeStmt::needs_unsafe(quote! { use F; }),
+    ];
+    assert_eq!(
+        maybe_unsafes_to_tokens(items.clone(), false).to_string(),
+        quote! {
+            use A;
+            use B;
+            unsafe {
+                use C;
+                use D;
+            }
+            use E;
+            unsafe {
+                use F;
+            }
+        }
+        .to_string()
+    );
+    assert_eq!(
+        maybe_unsafes_to_tokens(items, true).to_string(),
+        quote! {
+            use A;
+            use B;
+            use C;
+            use D;
+            use E;
+            use F;
+        }
+        .to_string()
+    );
+}