Skip to content

Add --no-default flag #1654

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

Closed
wants to merge 2 commits into from
Closed

Add --no-default flag #1654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1111301
Track what can be Impl'ed for derive analysis
fletcherw Oct 23, 2019
1e00d63
Add "--no-default [regex]" flag
fletcherw Oct 23, 2019
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
6 changes: 4 additions & 2 deletions src/codegen/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1790,8 +1790,10 @@ impl CodeGenerator for CompInfo {
if item.can_derive_default(ctx) {
derives.push("Default");
} else {
needs_default_impl =
ctx.options().derive_default && !self.is_forward_declaration();
needs_default_impl = ctx.options().derive_default &&
!self.is_forward_declaration() &&
ctx.lookup_can_derive_default(item.id()) ==
CanDerive::Manually;
}

let all_template_params = item.all_template_params(ctx);
Expand Down
160 changes: 92 additions & 68 deletions src/ir/analysis/derive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,10 @@ impl<'ctx> CannotDerive<'ctx> {
fn constrain_type(&mut self, item: &Item, ty: &Type) -> CanDerive {
if !self.ctx.whitelisted_items().contains(&item.id()) {
trace!(
" cannot derive {} for blacklisted type",
" cannot derive {} for blacklisted type, but it can be implemented",
self.derive_trait
);
return CanDerive::No;
return CanDerive::Manually;
}

if self.derive_trait.not_by_name(self.ctx, &item) {
Expand All @@ -156,15 +156,18 @@ impl<'ctx> CannotDerive<'ctx> {

trace!("ty: {:?}", ty);
if item.is_opaque(self.ctx, &()) {
if !self.derive_trait.can_derive_union() &&
ty.is_union() &&
if ty.is_union() &&
self.ctx.options().rust_features().untagged_union
{
trace!(
" cannot derive {} for Rust unions",
self.derive_trait
);
return CanDerive::No;
let can_derive = self.derive_trait.can_derive_union();
if can_derive != CanDerive::Yes {
trace!(
" cannot derive {} for Rust unions ({:?})",
self.derive_trait,
can_derive,
);
return can_derive;
}
}

let layout_can_derive =
Expand All @@ -179,7 +182,13 @@ impl<'ctx> CannotDerive<'ctx> {
self.derive_trait
);
}
_ => {
CanDerive::Manually => {
trace!(
" we cannot derive {} for the layout, but it can be implemented",
self.derive_trait
);
}
CanDerive::No => {
trace!(
" we cannot derive {} for the layout",
self.derive_trait
Expand Down Expand Up @@ -226,11 +235,12 @@ impl<'ctx> CannotDerive<'ctx> {
if inner_type != CanDerive::Yes {
trace!(
" arrays of T for which we cannot derive {} \
also cannot derive {}",
also cannot derive {}, ({:?})",
self.derive_trait,
self.derive_trait
self.derive_trait,
inner_type
);
return CanDerive::No;
return inner_type;
}

if len == 0 && !self.derive_trait.can_derive_incomplete_array()
Expand Down Expand Up @@ -308,62 +318,74 @@ impl<'ctx> CannotDerive<'ctx> {
}

if info.kind() == CompKind::Union {
if self.derive_trait.can_derive_union() {
if self.ctx.options().rust_features().untagged_union &&
// https://github.com/rust-lang/rust/issues/36640
(!info.self_template_params(self.ctx).is_empty() ||
!item.all_template_params(self.ctx).is_empty())
{
trace!(
" cannot derive {} for Rust union because issue 36640", self.derive_trait
);
return CanDerive::No;
}
// fall through to be same as non-union handling
} else {
if self.ctx.options().rust_features().untagged_union {
trace!(
" cannot derive {} for Rust unions",
self.derive_trait
);
return CanDerive::No;
}

let layout_can_derive =
ty.layout(self.ctx).map_or(CanDerive::Yes, |l| {
l.opaque()
.array_size_within_derive_limit(self.ctx)
});
match layout_can_derive {
CanDerive::Yes => {
match self.derive_trait.can_derive_union() {
CanDerive::Yes => {
if self.ctx.options().rust_features().untagged_union &&
// https://github.com/rust-lang/rust/issues/36640
(!info.self_template_params(self.ctx).is_empty() ||
!item.all_template_params(self.ctx).is_empty())
{
trace!(
" union layout can trivially derive {}",
self.derive_trait
" cannot derive {} for Rust union because issue 36640", self.derive_trait
);
return CanDerive::No;
}
_ => {
// fall through to be same as non-union handling
}
trait_can_derive => {
if self.ctx.options().rust_features().untagged_union
{
trace!(
" union layout cannot derive {}",
self.derive_trait
);
" cannot derive {} for Rust unions ({:?})",
self.derive_trait,
trait_can_derive,
);
return trait_can_derive;
}
};
return layout_can_derive;

let layout_can_derive = ty.layout(self.ctx).map_or(
CanDerive::Yes,
|l| {
l.opaque().array_size_within_derive_limit(
self.ctx,
)
},
);
match layout_can_derive {
CanDerive::Yes => {
trace!(
" union layout can trivially derive {}",
self.derive_trait
);
}
_ => {
trace!(
" union layout cannot derive {}, ({:?})",
self.derive_trait,
layout_can_derive,
);
}
};
return layout_can_derive;
}
}
}

if !self.derive_trait.can_derive_compound_with_vtable() &&
item.has_vtable(self.ctx)
{
trace!(
" cannot derive {} for comp with vtable",
self.derive_trait
);
return CanDerive::No;
if item.has_vtable(self.ctx) {
let can_derive =
self.derive_trait.can_derive_compound_with_vtable();
if can_derive != CanDerive::Yes {
trace!(
" cannot derive {} for comp with vtable ({:?})",
self.derive_trait,
can_derive,
);
return can_derive;
}
}

let pred = self.derive_trait.consider_edge_comp();
return self.constrain_join(item, pred);
self.constrain_join(item, pred)
}

TypeKind::ResolvedTypeRef(..) |
Expand Down Expand Up @@ -432,6 +454,7 @@ impl DeriveTrait {
fn not_by_name(&self, ctx: &BindgenContext, item: &Item) -> bool {
match self {
DeriveTrait::Copy => ctx.no_copy_by_name(item),
DeriveTrait::Default => ctx.no_default_by_name(item),
DeriveTrait::Hash => ctx.no_hash_by_name(item),
DeriveTrait::PartialEqOrPartialOrd => {
ctx.no_partialeq_by_name(item)
Expand Down Expand Up @@ -476,10 +499,11 @@ impl DeriveTrait {
}
}

fn can_derive_union(&self) -> bool {
fn can_derive_union(&self) -> CanDerive {
match self {
DeriveTrait::Copy => true,
_ => false,
DeriveTrait::Copy => CanDerive::Yes,
DeriveTrait::Default => CanDerive::Manually,
_ => CanDerive::No,
}
}

Expand All @@ -490,10 +514,10 @@ impl DeriveTrait {
}
}

fn can_derive_compound_with_vtable(&self) -> bool {
fn can_derive_compound_with_vtable(&self) -> CanDerive {
match self {
DeriveTrait::Default => false,
_ => true,
DeriveTrait::Default => CanDerive::Manually,
_ => CanDerive::Yes,
}
}

Expand Down Expand Up @@ -549,8 +573,8 @@ impl DeriveTrait {
fn can_derive_pointer(&self) -> CanDerive {
match self {
DeriveTrait::Default => {
trace!(" pointer cannot derive Default");
CanDerive::No
trace!(" pointer cannot derive Default, but it may be implemented");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how can it be implemented for a pointer?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Literally speaking we can't implement anything on pointers, but

struct Test {
    field: *mut i32,
}

impl Default for Test {
    fn default() -> Self {
        unsafe { ::std::mem::zeroed() }
    }
}

does seem to compile for composite types that contain pointers.

CanDerive::Manually
}
_ => {
trace!(" pointer can derive {}", self);
Expand All @@ -570,8 +594,8 @@ impl DeriveTrait {
(DeriveTrait::Default, TypeKind::ObjCInterface(..)) |
(DeriveTrait::Default, TypeKind::ObjCId) |
(DeriveTrait::Default, TypeKind::ObjCSel) => {
trace!(" types that always cannot derive Default");
CanDerive::No
trace!(" types that always cannot derive Default, but may implement it manually");
CanDerive::Manually
}
(DeriveTrait::Default, TypeKind::UnresolvedTypeRef(..)) => {
unreachable!(
Expand Down
31 changes: 22 additions & 9 deletions src/ir/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,8 @@ where
T: Copy + Into<ItemId>,
{
fn can_derive_default(&self, ctx: &BindgenContext) -> bool {
ctx.options().derive_default && ctx.lookup_can_derive_default(*self)
ctx.options().derive_default &&
ctx.lookup_can_derive_default(*self) == CanDerive::Yes
}
}

Expand Down Expand Up @@ -401,11 +402,12 @@ pub struct BindgenContext {
/// and is always `None` before that and `Some` after.
cannot_derive_debug: Option<HashSet<ItemId>>,

/// The set of (`ItemId`s of) types that can't derive default.
/// Map from type `ItemId`s to why they can't derive hash. If an id is not
/// in this map, we can assume that its value would be `CanDerive::Yes`.
///
/// This is populated when we enter codegen by `compute_cannot_derive_default`
/// and is always `None` before that and `Some` after.
cannot_derive_default: Option<HashSet<ItemId>>,
cannot_derive_default: Option<HashMap<ItemId, CanDerive>>,

/// The set of (`ItemId`s of) types that can't derive copy.
///
Expand Down Expand Up @@ -2462,16 +2464,16 @@ If you encounter an error missing from this list, please file an issue or a PR!"
assert!(self.cannot_derive_default.is_none());
if self.options.derive_default {
self.cannot_derive_default =
Some(as_cannot_derive_set(analyze::<CannotDerive>((
self,
DeriveTrait::Default,
))));
Some(analyze::<CannotDerive>((self, DeriveTrait::Default)));
}
}

/// Look up whether the item with `id` can
/// derive default or not.
pub fn lookup_can_derive_default<Id: Into<ItemId>>(&self, id: Id) -> bool {
pub fn lookup_can_derive_default<Id: Into<ItemId>>(
&self,
id: Id,
) -> CanDerive {
let id = id.into();
assert!(
self.in_codegen_phase(),
Expand All @@ -2480,7 +2482,12 @@ If you encounter an error missing from this list, please file an issue or a PR!"

// Look up the computed value for whether the item with `id` can
// derive default or not.
!self.cannot_derive_default.as_ref().unwrap().contains(&id)
self.cannot_derive_default
.as_ref()
.unwrap()
.get(&id)
.cloned()
.unwrap_or(CanDerive::Yes)
}

/// Compute whether we can derive copy.
Expand Down Expand Up @@ -2637,6 +2644,12 @@ If you encounter an error missing from this list, please file an issue or a PR!"
let name = item.path_for_whitelisting(self)[1..].join("::");
self.options().no_hash_types.matches(&name)
}

/// Check if `--no-default` flag is enabled for this item.
pub fn no_default_by_name(&self, item: &Item) -> bool {
let name = item.path_for_whitelisting(self)[1..].join("::");
self.options().no_default_types.matches(&name)
}
}

/// A builder struct for configuring item resolution options.
Expand Down
22 changes: 22 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,16 @@ impl Builder {
})
.count();

self.options
.no_default_types
.get_items()
.iter()
.map(|item| {
output_vector.push("--no-default".into());
output_vector.push(item.to_owned());
})
.count();

output_vector
}

Expand Down Expand Up @@ -1507,6 +1517,13 @@ impl Builder {
self.options.wasm_import_module_name = Some(import_name.into());
self
}

/// Don't derive `Default` for a given type. Regular
/// expressions are supported.
pub fn no_default<T: Into<String>>(mut self, arg: T) -> Builder {
self.options.no_default_types.insert(arg.into());
self
}
}

/// Configuration options for generated bindings.
Expand Down Expand Up @@ -1769,6 +1786,9 @@ struct BindgenOptions {

/// Wasm import module name.
wasm_import_module_name: Option<String>,

/// The set of types that we should not derive `Default` for.
no_default_types: RegexSet,
}

/// TODO(emilio): This is sort of a lie (see the error message that results from
Expand Down Expand Up @@ -1798,6 +1818,7 @@ impl BindgenOptions {
&mut self.no_partialeq_types,
&mut self.no_copy_types,
&mut self.no_hash_types,
&mut self.no_default_types,
];
let record_matches = self.record_matches;
for regex_set in &mut regex_sets {
Expand Down Expand Up @@ -1895,6 +1916,7 @@ impl Default for BindgenOptions {
no_hash_types: Default::default(),
array_pointers_in_arguments: false,
wasm_import_module_name: None,
no_default_types: Default::default(),
}
}
}
Expand Down
Loading