Skip to content

Derive + blacklisted types #950

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
merged 9 commits into from
Sep 6, 2017
Merged

Derive + blacklisted types #950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
931edfc
Do not derive Copy for blacklisted types
fitzgen Sep 5, 2017
a50055a
Document that blacklisted types are pessimistic with `derive`
fitzgen Sep 5, 2017
c1965e7
Be pessimistic about `derive(Debug)` and blacklisted types
fitzgen Sep 5, 2017
310890c
Don't try and debug format blacklisted types when generating `impl De…
fitzgen Sep 5, 2017
aab7413
Reflect whitelisted status via color in graphviz dot files
fitzgen Sep 5, 2017
a3a4635
Pessimistically assume that blacklisted types do not implement Default
fitzgen Sep 5, 2017
2644a5d
Add a test for simple blacklisting + derive(Hash)
fitzgen Sep 5, 2017
641337d
Add a test for blacklisting and deriving PartialEq
fitzgen Sep 5, 2017
74e2aea
Do not derive things on opaque unions
fitzgen Sep 6, 2017
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
4 changes: 4 additions & 0 deletions book/src/blacklisting.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ appear in the bindings at
all, [make it opaque](./opaque.html) instead of
blacklisting it.)

Blacklisted types are pessimistically assumed not to be able to `derive` any
traits, which can transitively affect other types' ability to `derive` traits or
not.

### Library

* [`bindgen::Builder::hide_type`](https://docs.rs/bindgen/0.23.1/bindgen/struct.Builder.html#method.hide_type)
Expand Down
10 changes: 8 additions & 2 deletions src/codegen/derive_debug.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ pub fn gen_debug_impl(
impl X {
fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
write!(f, $format_string $tokens)
}
}
});

match impl_.unwrap().node {
Expand Down Expand Up @@ -127,6 +127,12 @@ impl<'a> ImplDebug<'a> for Item {
) -> Option<(String, Vec<TokenTree>)> {
let name_ident = ctx.rust_ident_raw(name);

// We don't know if blacklisted items `impl Debug` or not, so we can't
// add them to the format string we're building up.
if !ctx.whitelisted_items().contains(&self.id()) {
return None;
}

let ty = match self.as_type() {
Some(ty) => ty,
None => {
Expand Down Expand Up @@ -168,7 +174,7 @@ impl<'a> ImplDebug<'a> for Item {
} else {
debug_print(ctx, name, name_ident)
}
}
}

// The generic is not required to implement Debug, so we can not debug print that type
TypeKind::TypeParam => {
Expand Down
32 changes: 24 additions & 8 deletions src/ir/analysis/derive_copy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,13 @@ impl<'ctx, 'gen> CannotDeriveCopy<'ctx, 'gen> {

ConstrainResult::Changed
}

/// A type is not `Copy` if we've determined it is not copy, or if it is
/// blacklisted.
fn is_not_copy(&self, id: ItemId) -> bool {
self.cannot_derive_copy.contains(&id) ||
!self.ctx.whitelisted_items().contains(&id)
}
}

impl<'ctx, 'gen> MonotoneFramework for CannotDeriveCopy<'ctx, 'gen> {
Expand Down Expand Up @@ -118,6 +125,15 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveCopy<'ctx, 'gen> {
return ConstrainResult::Same;
}

// If an item is reachable from the whitelisted items set, but isn't
// itself whitelisted, then it must be blacklisted. We assume that
// blacklisted items are not `Copy`, since they are presumably
// blacklisted because they are too complicated for us to understand.
if !self.ctx.whitelisted_items().contains(&id) {
trace!(" blacklisted items are assumed not to be Copy");
return ConstrainResult::Same;
}

let item = self.ctx.resolve_item(id);
let ty = match item.as_type() {
Some(ty) => ty,
Expand Down Expand Up @@ -163,7 +179,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveCopy<'ctx, 'gen> {
}

TypeKind::Array(t, len) => {
let cant_derive_copy = self.cannot_derive_copy.contains(&t);
let cant_derive_copy = self.is_not_copy(t);
if cant_derive_copy {
trace!(
" arrays of T for which we cannot derive Copy \
Expand All @@ -184,7 +200,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveCopy<'ctx, 'gen> {
TypeKind::ResolvedTypeRef(t) |
TypeKind::TemplateAlias(t, _) |
TypeKind::Alias(t) => {
let cant_derive_copy = self.cannot_derive_copy.contains(&t);
let cant_derive_copy = self.is_not_copy(t);
if cant_derive_copy {
trace!(
" arrays of T for which we cannot derive Copy \
Expand Down Expand Up @@ -237,7 +253,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveCopy<'ctx, 'gen> {

let bases_cannot_derive =
info.base_members().iter().any(|base| {
self.cannot_derive_copy.contains(&base.ty)
self.is_not_copy(base.ty)
});
if bases_cannot_derive {
trace!(
Expand All @@ -250,11 +266,11 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveCopy<'ctx, 'gen> {
let fields_cannot_derive =
info.fields().iter().any(|f| match *f {
Field::DataMember(ref data) => {
self.cannot_derive_copy.contains(&data.ty())
self.is_not_copy(data.ty())
}
Field::Bitfields(ref bfu) => {
bfu.bitfields().iter().any(|b| {
self.cannot_derive_copy.contains(&b.ty())
self.is_not_copy(b.ty())
})
}
});
Expand All @@ -270,7 +286,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveCopy<'ctx, 'gen> {
TypeKind::TemplateInstantiation(ref template) => {
let args_cannot_derive =
template.template_arguments().iter().any(|arg| {
self.cannot_derive_copy.contains(&arg)
self.is_not_copy(*arg)
});
if args_cannot_derive {
trace!(
Expand All @@ -284,8 +300,8 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveCopy<'ctx, 'gen> {
!template.template_definition().is_opaque(self.ctx, &()),
"The early ty.is_opaque check should have handled this case"
);
let def_cannot_derive = self.cannot_derive_copy.contains(
&template.template_definition(),
let def_cannot_derive = self.is_not_copy(
template.template_definition(),
);
if def_cannot_derive {
trace!(
Expand Down
38 changes: 28 additions & 10 deletions src/ir/analysis/derive_debug.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ impl<'ctx, 'gen> CannotDeriveDebug<'ctx, 'gen> {

ConstrainResult::Changed
}

/// A type is not `Debug` if we've determined it is not debug, or if it is
/// blacklisted.
fn is_not_debug(&self, id: ItemId) -> bool {
self.cannot_derive_debug.contains(&id) ||
!self.ctx.whitelisted_items().contains(&id)
}
}

impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDebug<'ctx, 'gen> {
Expand Down Expand Up @@ -120,6 +127,15 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDebug<'ctx, 'gen> {
return ConstrainResult::Same;
}

// If an item is reachable from the whitelisted items set, but isn't
// itself whitelisted, then it must be blacklisted. We assume that
// blacklisted items are not `Copy`, since they are presumably
// blacklisted because they are too complicated for us to understand.
if !self.ctx.whitelisted_items().contains(&id) {
trace!(" blacklisted items are assumed not to be Debug");
return ConstrainResult::Same;
}

let item = self.ctx.resolve_item(id);
let ty = match item.as_type() {
Some(ty) => ty,
Expand All @@ -129,11 +145,13 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDebug<'ctx, 'gen> {
}
};

if ty.is_opaque(self.ctx, item) {
if item.is_opaque(self.ctx, &()) {
let layout_can_derive = ty.layout(self.ctx).map_or(true, |l| {
l.opaque().can_trivially_derive_debug()
});
return if layout_can_derive {
return if layout_can_derive &&
!(ty.is_union() &&
self.ctx.options().rust_features().untagged_union()) {
trace!(" we can trivially derive Debug for the layout");
ConstrainResult::Same
} else {
Expand Down Expand Up @@ -176,7 +194,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDebug<'ctx, 'gen> {
}

TypeKind::Array(t, len) => {
if self.cannot_derive_debug.contains(&t) {
if self.is_not_debug(t) {
trace!(
" arrays of T for which we cannot derive Debug \
also cannot derive Debug"
Expand All @@ -196,7 +214,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDebug<'ctx, 'gen> {
TypeKind::ResolvedTypeRef(t) |
TypeKind::TemplateAlias(t, _) |
TypeKind::Alias(t) => {
if self.cannot_derive_debug.contains(&t) {
if self.is_not_debug(t) {
trace!(
" aliases and type refs to T which cannot derive \
Debug also cannot derive Debug"
Expand Down Expand Up @@ -237,7 +255,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDebug<'ctx, 'gen> {

let bases_cannot_derive =
info.base_members().iter().any(|base| {
self.cannot_derive_debug.contains(&base.ty)
self.is_not_debug(base.ty)
});
if bases_cannot_derive {
trace!(
Expand All @@ -250,11 +268,11 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDebug<'ctx, 'gen> {
let fields_cannot_derive =
info.fields().iter().any(|f| match *f {
Field::DataMember(ref data) => {
self.cannot_derive_debug.contains(&data.ty())
self.is_not_debug(data.ty())
}
Field::Bitfields(ref bfu) => {
bfu.bitfields().iter().any(|b| {
self.cannot_derive_debug.contains(&b.ty())
self.is_not_debug(b.ty())
})
}
});
Expand Down Expand Up @@ -287,7 +305,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDebug<'ctx, 'gen> {
TypeKind::TemplateInstantiation(ref template) => {
let args_cannot_derive =
template.template_arguments().iter().any(|arg| {
self.cannot_derive_debug.contains(&arg)
self.is_not_debug(*arg)
});
if args_cannot_derive {
trace!(
Expand All @@ -301,8 +319,8 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDebug<'ctx, 'gen> {
!template.template_definition().is_opaque(self.ctx, &()),
"The early ty.is_opaque check should have handled this case"
);
let def_cannot_derive = self.cannot_derive_debug.contains(
&template.template_definition(),
let def_cannot_derive = self.is_not_debug(
template.template_definition(),
);
if def_cannot_derive {
trace!(
Expand Down
79 changes: 40 additions & 39 deletions src/ir/analysis/derive_default.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ impl<'ctx, 'gen> CannotDeriveDefault<'ctx, 'gen> {

ConstrainResult::Changed
}

fn is_not_default(&self, id: ItemId) -> bool {
self.cannot_derive_default.contains(&id) ||
!self.ctx.whitelisted_items().contains(&id)
}
}

impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDefault<'ctx, 'gen> {
Expand Down Expand Up @@ -153,6 +158,11 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDefault<'ctx, 'gen> {
return ConstrainResult::Same;
}

if !self.ctx.whitelisted_items().contains(&id) {
trace!(" blacklisted items pessimistically cannot derive Default");
return ConstrainResult::Same;
}

let item = self.ctx.resolve_item(id);
let ty = match item.as_type() {
Some(ty) => ty,
Expand All @@ -166,7 +176,9 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDefault<'ctx, 'gen> {
let layout_can_derive = ty.layout(self.ctx).map_or(true, |l| {
l.opaque().can_trivially_derive_default()
});
return if layout_can_derive {
return if layout_can_derive &&
!(ty.is_union() &&
self.ctx.options().rust_features().untagged_union()) {
trace!(" we can trivially derive Default for the layout");
ConstrainResult::Same
} else {
Expand Down Expand Up @@ -213,7 +225,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDefault<'ctx, 'gen> {
}

TypeKind::Array(t, len) => {
if self.cannot_derive_default.contains(&t) {
if self.is_not_default(t) {
trace!(
" arrays of T for which we cannot derive Default \
also cannot derive Default"
Expand All @@ -233,7 +245,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDefault<'ctx, 'gen> {
TypeKind::ResolvedTypeRef(t) |
TypeKind::TemplateAlias(t, _) |
TypeKind::Alias(t) => {
if self.cannot_derive_default.contains(&t) {
if self.is_not_default(t) {
trace!(
" aliases and type refs to T which cannot derive \
Default also cannot derive Default"
Expand Down Expand Up @@ -280,7 +292,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDefault<'ctx, 'gen> {
let bases_cannot_derive =
info.base_members().iter().any(|base| {
!self.ctx.whitelisted_items().contains(&base.ty) ||
self.cannot_derive_default.contains(&base.ty)
self.is_not_default(base.ty)
});
if bases_cannot_derive {
trace!(
Expand All @@ -296,14 +308,14 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDefault<'ctx, 'gen> {
!self.ctx.whitelisted_items().contains(
&data.ty(),
) ||
self.cannot_derive_default.contains(&data.ty())
self.is_not_default(data.ty())
}
Field::Bitfields(ref bfu) => {
bfu.bitfields().iter().any(|b| {
!self.ctx.whitelisted_items().contains(
&b.ty(),
) ||
self.cannot_derive_default.contains(&b.ty())
self.is_not_default(b.ty())
})
}
});
Expand All @@ -319,45 +331,34 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveDefault<'ctx, 'gen> {
}

TypeKind::TemplateInstantiation(ref template) => {
if self.ctx.whitelisted_items().contains(
&template.template_definition(),
)
{
let args_cannot_derive =
template.template_arguments().iter().any(|arg| {
self.cannot_derive_default.contains(&arg)
});
if args_cannot_derive {
trace!(
" template args cannot derive Default, so \
insantiation can't either"
);
return self.insert(id);
}

assert!(
!template.template_definition().is_opaque(self.ctx, &()),
"The early ty.is_opaque check should have handled this case"
let args_cannot_derive =
template.template_arguments().iter().any(|arg| {
self.is_not_default(*arg)
});
if args_cannot_derive {
trace!(
" template args cannot derive Default, so \
insantiation can't either"
);
let def_cannot_derive =
self.cannot_derive_default.contains(&template
.template_definition());
if def_cannot_derive {
trace!(
" template definition cannot derive Default, so \
insantiation can't either"
);
return self.insert(id);
}
return self.insert(id);
}

trace!(" template instantiation can derive Default");
ConstrainResult::Same
} else {
assert!(
!template.template_definition().is_opaque(self.ctx, &()),
"The early ty.is_opaque check should have handled this case"
);
let def_cannot_derive =
self.is_not_default(template.template_definition());
if def_cannot_derive {
trace!(
" blacklisted template instantiation cannot derive default"
" template definition cannot derive Default, so \
insantiation can't either"
);
return self.insert(id);
}

trace!(" template instantiation can derive Default");
ConstrainResult::Same
}

TypeKind::Opaque => {
Expand Down
4 changes: 3 additions & 1 deletion src/ir/analysis/derive_hash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,9 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveHash<'ctx, 'gen> {
let layout_can_derive = ty.layout(self.ctx).map_or(true, |l| {
l.opaque().can_trivially_derive_hash()
});
return if layout_can_derive {
return if layout_can_derive &&
!(ty.is_union() &&
self.ctx.options().rust_features().untagged_union()) {
trace!(" we can trivially derive Hash for the layout");
ConstrainResult::Same
} else {
Expand Down
Loading