Repository: Enselic/cargo-public-api (rust)

Issue:
Add `--deny` flag for diffs in CI usage situations
Hi there! Cool project 😄 

I'd like to use this as part of a CI pipeline but it doesn't look like there's any way to
programmatically check if something in the public API changed.

RustFmt has a [`--check` flag](https://github.com/rust-lang/rustfmt#checking-style-on-a-ci-server) which exits with `0` if the input would be formatted successfully
and `1` if it wouldn't be.

I think having a similar flag as part of this project would allow it to be used in the context
of CI.

Pull request description:
Add support for --deny to exit with failure if there is an API diff
Closes #63 

This is merely a proposal.

I am not very sure about the actual flag name. `--check` seems to be ambiguous for me, maybe something along `--check-diff-empty` or `--is-empty` is more asppropriate.

Tell me what you think!

Patch:
diff --git a/Cargo.lock b/Cargo.lock
index cd4806b..2da6e80 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -106,6 +106,7 @@ dependencies = [
  "public-api",
  "rustdoc-json",
  "serial_test",
+ "thiserror",
 ]
 
 [[package]]
diff --git a/cargo-public-api/Cargo.toml b/cargo-public-api/Cargo.toml
index 08075c4..c504e2b 100644
--- a/cargo-public-api/Cargo.toml
+++ b/cargo-public-api/Cargo.toml
@@ -16,6 +16,7 @@ anyhow = "1.0.53"
 atty = "0.2.14"
 clap = { version = "3.1.2", features = ["derive"] }
 diff = "0.1.12"
+thiserror = "1.0.29"
 
 [dependencies.rustdoc-json]
 path = "../rustdoc-json"
diff --git a/cargo-public-api/src/arg_types.rs b/cargo-public-api/src/arg_types.rs
index c53f562..8f9ace7 100644
--- a/cargo-public-api/src/arg_types.rs
+++ b/cargo-public-api/src/arg_types.rs
@@ -4,6 +4,13 @@ use std::str::FromStr;
 
 use crate::{markdown::Markdown, output_formatter::OutputFormatter, plain::Plain};
 
+#[derive(Copy, Clone, Debug, Eq, PartialEq, clap::ArgEnum)]
+#[clap(rename_all = "lower")]
+pub enum DenyMethod {
+    /// All forms of API diffs are denied: additions, changes, deletions.
+    All,
+}
+
 #[derive(Debug)]
 pub enum OutputFormat {
     Plain,
diff --git a/cargo-public-api/src/error.rs b/cargo-public-api/src/error.rs
new file mode 100644
index 0000000..8780c47
--- /dev/null
+++ b/cargo-public-api/src/error.rs
@@ -0,0 +1,5 @@
+#[derive(Debug, thiserror::Error)]
+pub enum Error {
+    #[error("The API diff is not allowed as per --deny")]
+    DiffDenied,
+}
diff --git a/cargo-public-api/src/main.rs b/cargo-public-api/src/main.rs
index 2570a95..b79167b 100644
--- a/cargo-public-api/src/main.rs
+++ b/cargo-public-api/src/main.rs
@@ -2,7 +2,9 @@ use std::io::stdout;
 use std::path::{Path, PathBuf};
 
 use anyhow::{anyhow, Context, Result};
+use arg_types::{Color, DenyMethod, OutputFormat};
 use output_formatter::OutputFormatter;
+use public_api::diff::PublicItemsDiff;
 use public_api::{
     public_api_from_rustdoc_json_str, Options, PublicItem, MINIMUM_RUSTDOC_JSON_VERSION,
 };
@@ -11,6 +13,7 @@ use clap::Parser;
 use rustdoc_json::BuildError;
 
 mod arg_types;
+mod error;
 mod git_utils;
 mod markdown;
 mod output_formatter;
@@ -71,15 +74,20 @@ pub struct Args {
     #[clap(long, min_values = 2, max_values = 2)]
     diff_rustdoc_json: Option<Vec<String>>,
 
+    /// Exit with failure if the specified API diff is detected. all = deny
+    /// additions, changes, and removals of public items in the API
+    #[clap(long, arg_enum)]
+    deny: Option<Vec<DenyMethod>>,
+
     /// What output format to use. You can select between "plain" and "markdown".
     #[clap(long, name = "FORMAT", default_value = "plain")]
-    output_format: arg_types::OutputFormat,
+    output_format: OutputFormat,
 
     /// Whether or not to use colors. You can select between "auto", "never", "always".
     /// If "auto" (the default), colors will be used if stdout is a terminal. If you pipe
     /// the output to a file, colors will be disabled by default.
     #[clap(long, default_value = "auto")]
-    color: arg_types::Color,
+    color: Color,
 
     /// Show detailed info about processing. For debugging purposes. The output
     /// is not stable and can change across patch versions.
@@ -95,12 +103,34 @@ pub struct Args {
 fn main_() -> Result<()> {
     let args = get_args();
 
-    if let Some(commits) = &args.diff_git_checkouts {
-        print_public_items_diff_between_two_commits(&args, commits)
+    let diff = if let Some(commits) = &args.diff_git_checkouts {
+        Some(print_diff_between_two_commits(&args, commits)?)
     } else if let Some(files) = &args.diff_rustdoc_json {
-        print_public_items_diff_between_two_rustdoc_json_files(&args, files)
+        Some(print_diff_between_two_rustdoc_json_files(&args, files)?)
     } else {
-        print_public_items_of_current_commit(&args)
+        print_public_items_of_current_commit(&args)?;
+        None
+    };
+
+    check_diff(&args, diff)
+}
+
+fn check_diff(args: &Args, diff: Option<PublicItemsDiff>) -> Result<()> {
+    match (&args.deny, diff) {
+        // We were requested to deny diffs, so make sure there is no diff
+        (Some(_deny), Some(diff)) => {
+            if diff.is_empty() {
+                Ok(())
+            } else {
+                Err(anyhow!(error::Error::DiffDenied))
+            }
+        }
+
+        // We were requested to deny diffs, but we did not calculate a diff!
+        (Some(_), None) => Err(anyhow!("`--deny` can only be used when diffing")),
+
+        // No diff related stuff to care about, all is Ok
+        _ => Ok(()),
     }
 }
 
@@ -113,7 +143,7 @@ fn print_public_items_of_current_commit(args: &Args) -> Result<()> {
     Ok(())
 }
 
-fn print_public_items_diff_between_two_commits(args: &Args, commits: &[String]) -> Result<()> {
+fn print_diff_between_two_commits(args: &Args, commits: &[String]) -> Result<PublicItemsDiff> {
     let old_commit = commits.get(0).expect("clap makes sure first commit exist");
     let old = collect_public_items_from_commit(Some(old_commit))?;
 
@@ -123,10 +153,10 @@ fn print_public_items_diff_between_two_commits(args: &Args, commits: &[String])
     print_diff(args, old, new)
 }
 
-fn print_public_items_diff_between_two_rustdoc_json_files(
+fn print_diff_between_two_rustdoc_json_files(
     args: &Args,
     files: &[String],
-) -> Result<()> {
+) -> Result<PublicItemsDiff> {
     let options = get_options(args);
 
     let old_file = files.get(0).expect("clap makes sure first file exists");
@@ -138,13 +168,13 @@ fn print_public_items_diff_between_two_rustdoc_json_files(
     print_diff(args, old, new)
 }
 
-fn print_diff(args: &Args, old: Vec<PublicItem>, new: Vec<PublicItem>) -> Result<()> {
-    let diff = public_api::diff::PublicItemsDiff::between(old, new);
+fn print_diff(args: &Args, old: Vec<PublicItem>, new: Vec<PublicItem>) -> Result<PublicItemsDiff> {
+    let diff = PublicItemsDiff::between(old, new);
     args.output_format
         .formatter()
         .print_diff(&mut stdout(), args, &diff)?;
 
-    Ok(())
+    Ok(diff)
 }
 
 /// Get CLI args via `clap` while also handling when we are invoked as a cargo
diff --git a/public-api/src/diff.rs b/public-api/src/diff.rs
index c80f6c4..b4a8e24 100644
--- a/public-api/src/diff.rs
+++ b/public-api/src/diff.rs
@@ -106,6 +106,12 @@ impl PublicItemsDiff {
             added,
         }
     }
+
+    /// Check whether the diff is empty
+    #[must_use]
+    pub fn is_empty(&self) -> bool {
+        self.removed.is_empty() && self.changed.is_empty() && self.added.is_empty()
+    }
 }
 
 /// Converts a set (read: bag) of public items into a hash map that maps a given
@@ -139,6 +145,7 @@ mod tests {
             added: vec![],
         };
         assert_eq!(actual, expected);
+        assert!(!actual.is_empty());
     }
 
     #[test]
@@ -153,6 +160,7 @@ mod tests {
             added: vec![item_with_path("foo")],
         };
         assert_eq!(actual, expected);
+        assert!(!actual.is_empty());
     }
 
     #[test]
@@ -171,6 +179,7 @@ mod tests {
             added: vec![item_with_path("2")],
         };
         assert_eq!(actual, expected);
+        assert!(!actual.is_empty());
     }
 
     #[test]
@@ -189,6 +198,7 @@ mod tests {
             added: vec![],
         };
         assert_eq!(actual, expected);
+        assert!(!actual.is_empty());
     }
 
     #[test]
@@ -233,6 +243,7 @@ mod tests {
             added: vec![item_with_path("4"), item_with_path("4")],
         };
         assert_eq!(actual, expected);
+        assert!(!actual.is_empty());
     }
 
     /// Regression test for
@@ -260,6 +271,22 @@ mod tests {
         };
         let actual = PublicItemsDiff::between(old, new);
         assert_eq!(actual, expected);
+        assert!(!actual.is_empty());
+    }
+
+    #[test]
+    fn no_diff_means_empty_diff() {
+        let old = vec![item_with_path("foo")];
+        let new = vec![item_with_path("foo")];
+
+        let actual = PublicItemsDiff::between(old, new);
+        let expected = PublicItemsDiff {
+            removed: vec![],
+            changed: vec![],
+            added: vec![],
+        };
+        assert_eq!(actual, expected);
+        assert!(actual.is_empty());
     }
 
     fn item_with_path(path: &str) -> PublicItem {
diff --git a/public-api/src/item_iterator.rs b/public-api/src/item_iterator.rs
index c8dc557..52ca7a5 100644
--- a/public-api/src/item_iterator.rs
+++ b/public-api/src/item_iterator.rs
@@ -29,12 +29,10 @@ pub struct ItemIterator<'a> {
     /// What items left to visit (and possibly add more items from)
     items_left: Vec<Rc<IntermediatePublicItem<'a>>>,
 
-    /// Normally, an item referenced by item Id is present in the rustdoc JSON.
-    /// If [`Self::crate_.index`] is missing an Id, then we add it here, to aid
-    /// with debugging. It will typically be missing because of bugs (or
-    /// borderline bug such as re-exports of foreign items like discussed in
-    /// <https://github.com/rust-lang/rust/pull/99287#issuecomment-1186586518>)
-    /// We do not report it to users, because they can't do anything about it.
+    /// Normally, a reference in the rustdoc JSON exists. If
+    /// [Self::crate_.index] is missing an id (e.g. if it is for a dependency
+    /// but the rustdoc JSON was built with `--no-deps`) then we track that in
+    /// this field.
     missing_ids: Vec<&'a Id>,
 
     /// `impl`s are a bit special. They do not need to be reachable by the crate
@@ -87,50 +85,53 @@ impl<'a> ItemIterator<'a> {
         parent: Option<Rc<IntermediatePublicItem<'a>>>,
     ) {
         match self.crate_.index.get(id) {
-            Some(item) => {
-                // We handle `impl`s specially, and we don't want to process `impl`
-                // items directly. See [`ItemIterator::impls`] docs for more info.
-                // All other items we can go ahead and add.
-                if !matches!(item.inner, ItemEnum::Impl { .. }) {
-                    self.add_item_to_visit(item, parent);
+            // We handle `impl`s specially, and we don't want to process `impl`
+            // items directly. See [`ItemIterator::impls`] docs for more info.
+            Some(Item {
+                inner: ItemEnum::Impl { .. },
+                ..
+            }) => (),
+
+            // Since public imports are part of the public API, we inline them
+            // in the output as much as we can. If we didn't do inlining of
+            // imports, items would just show up as
+            // ```
+            // pub use ...
+            // ```
+            // which is not sufficient for the use cases of this tool. We want
+            // to show the actual API, not an abstract version of it!
+            //
+            // Note that only partial rustdoc JSON for re-exported items from
+            // other crates are available to us in certain cases. See
+            // <https://github.com/rust-lang/rust/pull/99287#issuecomment-1186586518>.
+            Some(Item {
+                inner: ItemEnum::Import(import),
+                ..
+            }) => {
+                if let Some(imported_id) = &import.id {
+                    if let Some(imported_item) = self.crate_.index.get(imported_id) {
+                        self.add_item_to_visit(imported_item, &import.name, parent);
+                    } else {
+                        self.add_missing_id(imported_id);
+                    }
                 }
             }
 
+            Some(item) => {
+                self.add_item_to_visit(item, item.name.as_deref().unwrap_or("<<no_name>>"), parent);
+            }
+
             None => self.add_missing_id(id),
         }
     }
 
     fn add_item_to_visit(
         &mut self,
-        mut item: &'a Item,
+        item: &'a Item,
+        name: &'a str,
         parent: Option<Rc<IntermediatePublicItem<'a>>>,
     ) {
-        let mut name = item.name.as_deref();
-
-        // Since public imports are part of the public API, we inline them, i.e.
-        // replace the item corresponding to an import with the item that is
-        // imported. If we didn't do this, publicly imported items would show up
-        // as just e.g. `pub use some::function`, which is not sufficient for
-        // the use cases of this tool. We want to show the actual API, and thus
-        // also show type information! There is one exception; for re-exports of
-        // primitive types, there is no item Id to inline with, so they remain
-        // as e.g. `pub use my_i32` in the output.
-        if let ItemEnum::Import(import) = &item.inner {
-            name = Some(&import.name);
-            if let Some(imported_id) = &import.id {
-                match self.crate_.index.get(imported_id) {
-                    Some(imported_item) => item = imported_item,
-                    None => self.add_missing_id(imported_id),
-                }
-            }
-        }
-
-        let public_item = Rc::new(IntermediatePublicItem::new(
-            item,
-            name.unwrap_or("<<no_name>>"),
-            parent,
-        ));
-
+        let public_item = Rc::new(IntermediatePublicItem::new(item, name, parent));
         self.items_left.push(public_item);
     }
 


Test patch:
diff --git a/cargo-public-api/tests/cargo-public-api-bin-tests.rs b/cargo-public-api/tests/cargo-public-api-bin-tests.rs
index 81ae54b..d799a20 100644
--- a/cargo-public-api/tests/cargo-public-api-bin-tests.rs
+++ b/cargo-public-api/tests/cargo-public-api-bin-tests.rs
@@ -6,6 +6,7 @@
 use std::path::{Path, PathBuf};
 
 use assert_cmd::Command;
+use predicates::str::contains;
 use serial_test::serial;
 
 #[serial]
@@ -54,7 +55,7 @@ fn virtual_manifest_error() {
     cmd.arg(current_dir_and("tests/virtual-manifest/Cargo.toml"));
     cmd.assert()
         .stdout("")
-        .stderr(predicates::str::contains(
+        .stderr(contains(
             "Listing or diffing the public API of an entire workspace is not supported.",
         ))
         .failure();
@@ -80,6 +81,64 @@ fn diff_public_items() {
         .success();
 }
 
+#[serial]
+#[test]
+fn deny_when_not_diffing() {
+    ensure_test_crate_is_cloned(); // Because we still list the API
+
+    let mut cmd = Command::cargo_bin("cargo-public-api").unwrap();
+    cmd.arg("--deny=all");
+    cmd.assert()
+        .stderr(contains("`--deny` can only be used when diffing"))
+        .failure();
+}
+
+#[serial]
+#[test]
+fn deny_without_diff() {
+    ensure_test_crate_is_cloned();
+
+    let mut cmd = Command::cargo_bin("cargo-public-api").unwrap();
+    cmd.current_dir(test_crate_path());
+    cmd.arg("--diff-git-checkouts");
+    cmd.arg("v0.2.0");
+    cmd.arg("v0.3.0");
+    cmd.arg("--deny=all");
+    cmd.assert().success();
+}
+
+#[serial]
+#[test]
+fn deny_with_diff() {
+    ensure_test_crate_is_cloned();
+
+    let mut cmd = Command::cargo_bin("cargo-public-api").unwrap();
+    cmd.current_dir(test_crate_path());
+    cmd.arg("--diff-git-checkouts");
+    cmd.arg("v0.0.4");
+    cmd.arg("v0.0.5");
+    cmd.arg("--deny=all");
+    cmd.assert()
+        .stderr(contains("The API diff is not allowed as per --deny"))
+        .failure();
+}
+
+#[serial]
+#[test]
+fn deny_with_invalid_arg() {
+    ensure_test_crate_is_cloned();
+
+    let mut cmd = Command::cargo_bin("cargo-public-api").unwrap();
+    cmd.current_dir(test_crate_path());
+    cmd.arg("--diff-git-checkouts");
+    cmd.arg("v0.0.4");
+    cmd.arg("v0.0.5");
+    cmd.arg("--deny=invalid");
+    cmd.assert()
+        .stderr(contains("\"invalid\" isn't a valid value"))
+        .failure();
+}
+
 #[serial]
 #[test]
 fn diff_public_items_with_manifest_path() {
@@ -238,7 +297,7 @@ fn diff_public_items_missing_one_arg() {
     cmd.arg("--diff-git-checkouts");
     cmd.arg("v0.2.0");
     cmd.assert()
-        .stderr(predicates::str::contains(
+        .stderr(contains(
             "requires at least 2 values but only 1 was provided",
         ))
         .failure();
@@ -265,9 +324,7 @@ fn list_public_items_markdown() {
 fn verbose() {
     let mut cmd = Command::cargo_bin("cargo-public-api").unwrap();
     cmd.arg("--verbose");
-    cmd.assert()
-        .stdout(predicates::str::contains("Processing \""))
-        .success();
+    cmd.assert().stdout(contains("Processing \"")).success();
 }
 
 #[test]
@@ -296,9 +353,9 @@ fn assert_presence_of_own_library_items(mut cmd: Command) {
 
 fn assert_presence_of_args_in_help(mut cmd: Command) {
     cmd.assert()
-        .stdout(predicates::str::contains("--with-blanket-implementations"))
-        .stdout(predicates::str::contains("--manifest-path"))
-        .stdout(predicates::str::contains("--diff-git-checkouts"))
+        .stdout(contains("--with-blanket-implementations"))
+        .stdout(contains("--manifest-path"))
+        .stdout(contains("--diff-git-checkouts"))
         .success();
 }
 
diff --git a/public-api/tests/expected-output/comprehensive_api.txt b/public-api/tests/expected-output/comprehensive_api.txt
index 108659b..ba1aeb4 100644
--- a/public-api/tests/expected-output/comprehensive_api.txt
+++ b/public-api/tests/expected-output/comprehensive_api.txt
@@ -119,5 +119,3 @@ pub type comprehensive_api::typedefs::TypedefPlain = Plain
 pub union comprehensive_api::unions::Basic
 pub unsafe fn comprehensive_api::functions::unsafe_fn()
 pub unsafe trait comprehensive_api::traits::UnsafeTrait
-pub use comprehensive_api::my_i32
-pub use comprehensive_api::u32
diff --git a/test-apis/comprehensive_api/src/lib.rs b/test-apis/comprehensive_api/src/lib.rs
index ee94a73..56c37d1 100644
--- a/test-apis/comprehensive_api/src/lib.rs
+++ b/test-apis/comprehensive_api/src/lib.rs
@@ -1,34 +1,21 @@
 pub extern crate rand;
+
+pub use private::StructInPrivateMod;
 pub use rand::RngCore;
+pub use structs::Plain;
+pub use structs::Plain as RenamedPlain;
 
 mod private;
-pub use private::StructInPrivateMod;
 
 pub mod attributes;
-
 pub mod constants;
-
 pub mod enums;
-
 pub mod functions;
-
 pub mod higher_ranked_trait_bounds;
-
 pub mod impls;
-
 pub mod macros;
-
 pub mod statics;
-
 pub mod structs;
-pub use structs::Plain;
-pub use structs::Plain as RenamedPlain;
-
 pub mod traits;
-
 pub mod typedefs;
-
 pub mod unions;
-
-pub use u32;
-pub use i32 as my_i32;