Skip to content

Change Record.is_unsafe_type to Record.safety_annotation #389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
copybara-service merged 1 commit into from
Feb 3, 2026
Merged

Change Record.is_unsafe_type to Record.safety_annotation #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions rs_bindings_from_cc/generate_bindings/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,9 +690,15 @@ fn record_field_safety(db: &dyn BindingsGenerator, field: Field) -> Safety {
fn record_safety(db: &dyn BindingsGenerator, record: Rc<Record>) -> 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() {
Expand All @@ -716,7 +722,10 @@ fn record_safety(db: &dyn BindingsGenerator, record: Rc<Record>) -> 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;
}

Expand Down
25 changes: 18 additions & 7 deletions rs_bindings_from_cc/importers/cxx_record.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,16 +439,26 @@ absl::StatusOr<TraitDerives> GetTraitDerives(const clang::Decl& decl) {
return result;
}

absl::StatusOr<bool> IsUnsafeType(const clang::Decl& decl) {
absl::StatusOr<SafetyAnnotation> GetSafetyAnnotation(const clang::Decl& decl) {
CRUBIT_ASSIGN_OR_RETURN(std::optional<AnnotateArgs> 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<bool> 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<Identifier> StringRefToOptionalIdentifier(llvm::StringRef name) {
Expand Down Expand Up @@ -1090,10 +1100,11 @@ std::optional<IR::Item> CXXRecordDeclImporter::Import(
FormattedError::FromStatus(std::move(trait_derives).status()));
}

absl::StatusOr<bool> is_unsafe_type = IsUnsafeType(*record_decl);
if (!is_unsafe_type.ok()) {
absl::StatusOr<SafetyAnnotation> 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{
Expand All @@ -1118,7 +1129,7 @@ std::optional<IR::Item> 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),
Expand Down
2 changes: 1 addition & 1 deletion rs_bindings_from_cc/ir.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down
16 changes: 5 additions & 11 deletions rs_bindings_from_cc/ir.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion rs_bindings_from_cc/ir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion rs_bindings_from_cc/ir_from_cc_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ fn test_struct_with_unsafe_annotation() {
quote! {
Record {
rs_name: "UnsafeType", ...
is_unsafe_type: true, ...
safety_annotation: Unsafe, ...
}
}
);
Expand Down
4 changes: 1 addition & 3 deletions rs_bindings_from_cc/test/struct/unsafe_attributes/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@ use unsafe_attributes::*;
#[deny(unsafe_code)]
fn test_safe_struct() {
UseSafeStructUnannotated(SafeStructUnannotated::default());
UseUnsafeStructAnnotatedSafe(UnsafeStructAnnotatedSafe::default());
}

#[googletest::gtest]
#[deny(unused_unsafe)]
fn test_unsafe_struct() {
unsafe { UseSafeStructAnnotatedUnsafe(SafeStructAnnotatedUnsafe::default()) };
unsafe { UseUnsafeStructUnannotated(UnsafeStructUnannotated::default()) };

// TODO(b/481018055): this should not be unsafe.
unsafe { UseUnsafeStructAnnotatedSafe(UnsafeStructAnnotatedSafe::default()) };
}