diff --git a/rs_bindings_from_cc/generate_bindings/lib.rs b/rs_bindings_from_cc/generate_bindings/lib.rs index afbcc8b9c..bb90e4a85 100644 --- a/rs_bindings_from_cc/generate_bindings/lib.rs +++ b/rs_bindings_from_cc/generate_bindings/lib.rs @@ -690,9 +690,15 @@ fn record_field_safety(db: &dyn BindingsGenerator, field: Field) -> Safety { fn record_safety(db: &dyn BindingsGenerator, record: Rc) -> Safety { let mut doc = String::new(); - if record.is_unsafe_type { - // TODO(b/480191443): allow C++ annotations to provide a specific reason. - doc += "* The C++ type is explicitly annotated as unsafe. Ensure that its safety requirements are upheld."; + match record.safety_annotation { + SafetyAnnotation::DisableUnsafe => { + return Safety::Safe; + } + SafetyAnnotation::Unsafe => { + // TODO(b/480191443): allow C++ annotations to provide a specific reason. + doc += "* The C++ type is explicitly annotated as unsafe. Ensure that its safety requirements are upheld."; + } + SafetyAnnotation::Unannotated => {} } if record.is_union() { @@ -716,7 +722,10 @@ fn record_safety(db: &dyn BindingsGenerator, record: Rc) -> Safety { }) .collect(); - if !record.is_unsafe_type && !record.is_union() && reasons.is_empty() { + if matches!(record.safety_annotation, SafetyAnnotation::Unannotated) + && !record.is_union() + && reasons.is_empty() + { return Safety::Safe; } diff --git a/rs_bindings_from_cc/importers/cxx_record.cc b/rs_bindings_from_cc/importers/cxx_record.cc index 4759c35eb..265edbd1e 100644 --- a/rs_bindings_from_cc/importers/cxx_record.cc +++ b/rs_bindings_from_cc/importers/cxx_record.cc @@ -439,16 +439,26 @@ absl::StatusOr GetTraitDerives(const clang::Decl& decl) { return result; } -absl::StatusOr IsUnsafeType(const clang::Decl& decl) { +absl::StatusOr GetSafetyAnnotation(const clang::Decl& decl) { CRUBIT_ASSIGN_OR_RETURN(std::optional args, GetAnnotateAttrArgs(decl, "crubit_override_unsafe")); - if (!args.has_value()) return false; + if (!args.has_value()) return SafetyAnnotation::kUnannotated; if (args->size() != 1) { return absl::InvalidArgumentError( "`crubit_override_unsafe` annotation must have exactly one argument"); } - return GetExprAsBool(*args->front(), decl.getASTContext()); + absl::StatusOr is_unsafe = + GetExprAsBool(*args->front(), decl.getASTContext()); + if (!is_unsafe.ok()) { + return absl::InvalidArgumentError( + "`crubit_override_unsafe` annotation must have a bool argument"); + } + if (*is_unsafe) { + return SafetyAnnotation::kUnsafe; + } else { + return SafetyAnnotation::kDisableUnsafe; + } } std::optional StringRefToOptionalIdentifier(llvm::StringRef name) { @@ -1090,10 +1100,11 @@ std::optional CXXRecordDeclImporter::Import( FormattedError::FromStatus(std::move(trait_derives).status())); } - absl::StatusOr is_unsafe_type = IsUnsafeType(*record_decl); - if (!is_unsafe_type.ok()) { + absl::StatusOr safety_annotation = + GetSafetyAnnotation(*record_decl); + if (!safety_annotation.ok()) { return unsupported( - FormattedError::FromStatus(std::move(is_unsafe_type).status())); + FormattedError::FromStatus(std::move(safety_annotation).status())); } auto record = Record{ @@ -1118,7 +1129,7 @@ std::optional CXXRecordDeclImporter::Import( .trait_derives = *std::move(trait_derives), .is_derived_class = is_derived_class, .override_alignment = override_alignment, - .is_unsafe_type = *is_unsafe_type, + .safety_annotation = *safety_annotation, .copy_constructor = GetCopyCtorSpecialMemberFunc(ictx_, *record_decl), .move_constructor = GetMoveCtorSpecialMemberFunc(ictx_, *record_decl), .destructor = GetDestructorSpecialMemberFunc(*record_decl), diff --git a/rs_bindings_from_cc/ir.cc b/rs_bindings_from_cc/ir.cc index c24e3c4dd..715c4575b 100644 --- a/rs_bindings_from_cc/ir.cc +++ b/rs_bindings_from_cc/ir.cc @@ -655,7 +655,7 @@ llvm::json::Value Record::ToJson() const { {"trait_derives", trait_derives.ToJson()}, {"is_derived_class", is_derived_class}, {"override_alignment", override_alignment}, - {"is_unsafe_type", is_unsafe_type}, + {"safety_annotation", SafetyAnnotationToString(safety_annotation)}, {"copy_constructor", copy_constructor}, {"move_constructor", move_constructor}, {"destructor", destructor}, diff --git a/rs_bindings_from_cc/ir.h b/rs_bindings_from_cc/ir.h index 0059185fc..be8aacebb 100644 --- a/rs_bindings_from_cc/ir.h +++ b/rs_bindings_from_cc/ir.h @@ -718,18 +718,12 @@ struct Record { // More information: docs/struct_layout bool override_alignment = false; - // True if the C++ class is annotated with `CRUBIT_UNSAFE`, otherwise false. + // Whether the C++ type is explicitly annotated as safe or unsafe, or has no + // safety annotation. // - // Crubit considers some types unsafe, like pointers. If a function - // accepts a value of this type, it's assumed that basically anything it could - // possibly do with that value involves a risk of UB if that value is invalid - // (e.g. dangling). Any pointer-like type which has no lifetime / could be - // dangling/invalid should marked with is_unsafe_type. Some examples: - // - // * string_view - // * span - // * absl::FunctionRef - bool is_unsafe_type = false; + // Note that if the C++ type is not annotated, Crubit will still mark the type + // as unsafe if it is a union or contains unsafe public fields. + SafetyAnnotation safety_annotation = SafetyAnnotation::kUnannotated; // Special member functions. SpecialMemberFunc copy_constructor = SpecialMemberFunc::kUnavailable; diff --git a/rs_bindings_from_cc/ir.rs b/rs_bindings_from_cc/ir.rs index 1c572ed64..e0a88e092 100644 --- a/rs_bindings_from_cc/ir.rs +++ b/rs_bindings_from_cc/ir.rs @@ -1213,7 +1213,7 @@ pub struct Record { pub trait_derives: TraitDerives, pub is_derived_class: bool, pub override_alignment: bool, - pub is_unsafe_type: bool, + pub safety_annotation: SafetyAnnotation, pub copy_constructor: SpecialMemberFunc, pub move_constructor: SpecialMemberFunc, pub destructor: SpecialMemberFunc, diff --git a/rs_bindings_from_cc/ir_from_cc_test.rs b/rs_bindings_from_cc/ir_from_cc_test.rs index 1b8bd5b63..d38455a4e 100644 --- a/rs_bindings_from_cc/ir_from_cc_test.rs +++ b/rs_bindings_from_cc/ir_from_cc_test.rs @@ -744,7 +744,7 @@ fn test_struct_with_unsafe_annotation() { quote! { Record { rs_name: "UnsafeType", ... - is_unsafe_type: true, ... + safety_annotation: Unsafe, ... } } ); diff --git a/rs_bindings_from_cc/test/struct/unsafe_attributes/test.rs b/rs_bindings_from_cc/test/struct/unsafe_attributes/test.rs index 9c5d31310..2f32cd737 100644 --- a/rs_bindings_from_cc/test/struct/unsafe_attributes/test.rs +++ b/rs_bindings_from_cc/test/struct/unsafe_attributes/test.rs @@ -10,6 +10,7 @@ use unsafe_attributes::*; #[deny(unsafe_code)] fn test_safe_struct() { UseSafeStructUnannotated(SafeStructUnannotated::default()); + UseUnsafeStructAnnotatedSafe(UnsafeStructAnnotatedSafe::default()); } #[googletest::gtest] @@ -17,7 +18,4 @@ fn test_safe_struct() { fn test_unsafe_struct() { unsafe { UseSafeStructAnnotatedUnsafe(SafeStructAnnotatedUnsafe::default()) }; unsafe { UseUnsafeStructUnannotated(UnsafeStructUnannotated::default()) }; - - // TODO(b/481018055): this should not be unsafe. - unsafe { UseUnsafeStructAnnotatedSafe(UnsafeStructAnnotatedSafe::default()) }; }