Auto merge of rust-lang#618 - fitzgen:extra-assertions, r=emilio

Extra assertions and cargo features

* Clean up testing-only cargo features

  This commit ensures that all of the cargo features we have that only exist for
  CI/testing purposes, and aren't for external consumption, have a "testing_only_"
  prefix.

* Define extra assertion macros

  This commit defines a new set of assertion macros that are only checked in
  testing/CI when the `testing_only_extra_assertions` feature is enabled. This
  makes it so that *users* of bindgen that happen to be making a debug build don't
  enable all these extra and expensive assertions.

  Additionally, this removes the `testing_only_assert_no_dangling_items` feature,
  and runs the assertions that were previously gated on that feature when the new
  `testing_only_extra_assertions` feature is enabled.

r? @emilio

Author: bors-servo

CONTRIBUTING.md

@@ -65,12 +65,12 @@ $ export LD_LIBRARY_PATH=path/to/clang-3.9/lib # for Linux
 $ export DYLD_LIBRARY_PATH=path/to/clang-3.9/lib # for macOS
 ```
 
-Additionally, you may want to build and test with the `docs_` feature to ensure
-that you aren't forgetting to document types and functions. CI will catch it if
-you forget, but the turn around will be a lot slower ;)
+Additionally, you may want to build and test with the `testing_only_docs`
+feature to ensure that you aren't forgetting to document types and functions. CI
+will catch it if you forget, but the turn around will be a lot slower ;)
 
 ```
-$ cargo build --features docs_
+$ cargo build --features testing_only_docs
 ```
 
 ## Testing

Cargo.toml

@@ -65,10 +65,12 @@ features = ["with-syntex"]
 version = "0.29"
 
 [features]
-assert_no_dangling_items = []
 default = ["logging"]
-testing_only_llvm_stable = []
 logging = ["env_logger", "log"]
 static = []
-# This feature only exists for CI -- don't use it!
-docs_ = []
+
+# These features only exist for CI testing -- don't use them if you're not hacking
+# on bindgen!
+testing_only_docs = []
+testing_only_extra_assertions = []
+testing_only_llvm_stable = []

ci/assert-docs.sh

@@ -3,4 +3,4 @@
 set -xeu
 cd "$(dirname "$0")/.."
 
-cargo build --features "$BINDGEN_FEATURES docs_"
+cargo build --features "$BINDGEN_FEATURES testing_only_docs"

ci/test.sh

@@ -6,10 +6,13 @@ cd "$(dirname "$0")/.."
 # Regenerate the test headers' bindings in debug and release modes, and assert
 # that we always get the expected generated bindings.
 
-cargo test --features "$BINDGEN_FEATURES assert_no_dangling_items"
+cargo test --features "$BINDGEN_FEATURES"
+./ci/assert-no-diff.sh
+
+cargo test --features "$BINDGEN_FEATURES testing_only_extra_assertions"
 ./ci/assert-no-diff.sh
 
-cargo test --release --features "$BINDGEN_FEATURES assert_no_dangling_items"
+cargo test --release --features "$BINDGEN_FEATURES testing_only_extra_assertions"
 ./ci/assert-no-diff.sh
 
 # Now test the expectations' size and alignment tests.

src/codegen/mod.rs

@@ -2642,7 +2642,7 @@ impl TryToRustTy for TemplateInstantiation {
                 // This can happen if we generated an opaque type for a partial
                 // template specialization, and we've hit an instantiation of
                 // that partial specialization.
-                debug_assert!(ctx.resolve_type_through_type_refs(decl)
+                extra_assert!(ctx.resolve_type_through_type_refs(decl)
                                   .is_opaque());
                 return Err(error::Error::InstantiationOfOpaqueType);
             }

src/extra_assertions.rs

@@ -0,0 +1,30 @@
+//! Macros for defining extra assertions that should only be checked in testing
+//! and/or CI when the `testing_only_extra_assertions` feature is enabled.
+
+#[macro_export]
+macro_rules! extra_assert {
+    ( $cond:expr ) => {
+        if cfg!(feature = "testing_only_extra_assertions") {
+            assert!($cond);
+        }
+    };
+    ( $cond:expr , $( $arg:tt )+ ) => {
+        if cfg!(feature = "testing_only_extra_assertions") {
+            assert!($cond, $( $arg )* )
+        }
+    };
+}
+
+#[macro_export]
+macro_rules! extra_assert_eq {
+    ( $lhs:expr , $rhs:expr ) => {
+        if cfg!(feature = "testing_only_extra_assertions") {
+            assert_eq!($lhs, $rhs);
+        }
+    };
+    ( $lhs:expr , $rhs:expr , $( $arg:tt )+ ) => {
+        if cfg!(feature = "testing_only_extra_assertions") {
+            assert!($lhs, $rhs, $( $arg )* );
+        }
+    };
+}

src/ir/context.rs

@@ -584,9 +584,11 @@ impl<'ctx> BindgenContext<'ctx> {
         ret
     }
 
-    /// This function trying to find any dangling references inside of `items`
+    /// When the `testing_only_extra_assertions` feature is enabled, this
+    /// function walks the IR graph and asserts that we do not have any edges
+    /// referencing an ItemId for which we do not have an associated IR item.
     fn assert_no_dangling_references(&self) {
-        if cfg!(feature = "assert_no_dangling_items") {
+        if cfg!(feature = "testing_only_extra_assertions") {
             for _ in self.assert_no_dangling_item_traversal() {
                 // The iterator's next method does the asserting for us.
             }

src/ir/item.rs

@@ -70,7 +70,7 @@ pub trait ItemAncestors {
 }
 
 cfg_if! {
-    if #[cfg(debug_assertions)] {
+    if #[cfg(testing_only_extra_assertions)] {
         type DebugOnlyItemSet = ItemSet;
     } else {
         struct DebugOnlyItemSet;
@@ -123,7 +123,7 @@ impl<'a, 'b> Iterator for ItemAncestorsIter<'a, 'b>
         } else {
             self.item = item.parent_id();
 
-            debug_assert!(!self.seen.contains(&item.id()));
+            extra_assert!(!self.seen.contains(&item.id()));
             self.seen.insert(item.id());
 
             Some(item.id())
@@ -614,7 +614,7 @@ impl Item {
         let mut item = self;
 
         loop {
-            debug_assert!(!targets_seen.contains(&item.id()));
+            extra_assert!(!targets_seen.contains(&item.id()));
             targets_seen.insert(item.id());
 
             if self.annotations().use_instead_of().is_some() {

src/ir/named.rs

@@ -371,7 +371,7 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> {
     fn constrain(&mut self, id: ItemId) -> bool {
         // Invariant: all hash map entries' values are `Some` upon entering and
         // exiting this method.
-        debug_assert!(self.used.values().all(|v| v.is_some()));
+        extra_assert!(self.used.values().all(|v| v.is_some()));
 
         // Take the set for this id out of the hash map while we mutate it based
         // on other hash map entries. We *must* put it back into the hash map at
@@ -446,7 +446,7 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> {
         // Put the set back in the hash map and restore our invariant.
         debug_assert!(self.used[&id].is_none());
         self.used.insert(id, Some(used_by_this_id));
-        debug_assert!(self.used.values().all(|v| v.is_some()));
+        extra_assert!(self.used.values().all(|v| v.is_some()));
 
         new_len != original_len
     }

src/lib.rs

@@ -37,16 +37,19 @@ extern crate log;
 #[macro_use]
 mod log_stubs;
 
+#[macro_use]
+mod extra_assertions;
+
 // A macro to declare an internal module for which we *must* provide
-// documentation for. If we are building with the "docs_" feature, then the
-// module is declared public, and our `#![deny(missing_docs)]` pragma applies to
-// it. This feature is used in CI, so we won't let anything slip by
+// documentation for. If we are building with the "testing_only_docs" feature,
+// then the module is declared public, and our `#![deny(missing_docs)]` pragma
+// applies to it. This feature is used in CI, so we won't let anything slip by
 // undocumented. Normal builds, however, will leave the module private, so that
 // we don't expose internals to library consumers.
 macro_rules! doc_mod {
     ($m:ident, $doc_mod_name:ident) => {
         cfg_if! {
-            if #[cfg(feature = "docs_")] {
+            if #[cfg(feature = "testing_only_docs")] {
                 pub mod $doc_mod_name {
                     //! Autogenerated documentation module.
                     pub use super::$m::*;