Implement more checks in cargo-check-external-types (#1645)

jdisanti · web-flow · commit 0d1dc51b0f95 · 2022-08-18T21:41:35.000Z
Adds support for unions, and marks unstable Rust language features as unsupported.
diff --git a/tools/cargo-check-external-types/Cargo.lock b/tools/cargo-check-external-types/Cargo.lock
diff --git a/tools/cargo-check-external-types/Cargo.toml b/tools/cargo-check-external-types/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "cargo-check-external-types"
-version = "0.1.1"
+version = "0.1.2"
 authors = ["AWS Rust SDK Team <aws-sdk-rust@amazon.com>", "John DiSanti <jdisanti@amazon.com>"]
 description = "Static analysis tool to detect external types exposed in a library's public API."
 edition = "2021"
diff --git a/tools/cargo-check-external-types/src/path.rs b/tools/cargo-check-external-types/src/path.rs
@@ -24,6 +24,7 @@ pub enum ComponentType {
     StructField,
     Trait,
     TypeDef,
+    Union,
 }
 
 /// Represents one component in a [`Path`].
diff --git a/tools/cargo-check-external-types/src/visitor.rs b/tools/cargo-check-external-types/src/visitor.rs
@@ -10,13 +10,23 @@ use crate::path::{ComponentType, Path};
 use anyhow::{anyhow, Context, Result};
 use rustdoc_types::{
     Crate, FnDecl, GenericArgs, GenericBound, GenericParamDef, GenericParamDefKind, Generics, Id,
-    Item, ItemEnum, ItemSummary, Struct, Term, Trait, Type, Variant, Visibility, WherePredicate,
+    Item, ItemEnum, ItemSummary, Struct, Term, Trait, Type, Union, Variant, Visibility,
+    WherePredicate,
 };
 use std::cell::RefCell;
 use std::collections::{BTreeSet, HashMap};
 use tracing::debug;
 use tracing_attributes::instrument;
 
+macro_rules! unstable_rust_feature {
+    ($name:expr, $documentation_uri:expr) => {
+        panic!(
+            "unstable Rust feature '{}' (see {}) is not supported by cargo-check-external-types",
+            $name, $documentation_uri
+        )
+    };
+}
+
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 enum VisibilityCheck {
     /// Check to make sure the item is public before visiting it
@@ -137,7 +147,10 @@ impl Visitor {
                     self.visit_item(&path, self.item(id)?, VisibilityCheck::Default)?;
                 }
             }
-            ItemEnum::ForeignType => unimplemented!("visit_item ItemEnum::ForeignType"),
+            ItemEnum::ForeignType => unstable_rust_feature!(
+                "extern_types",
+                "https://doc.rust-lang.org/beta/unstable-book/language-features/extern-types.html"
+            ),
             ItemEnum::Function(function) => {
                 path.push(ComponentType::Function, item);
                 self.visit_fn_decl(&path, &function.decl)?;
@@ -176,7 +189,7 @@ impl Visitor {
                     }
                 }
             }
-            ItemEnum::OpaqueTy(_) => unimplemented!("visit_item ItemEnum::OpaqueTy"),
+            ItemEnum::OpaqueTy(_) => unstable_rust_feature!("type_alias_impl_trait", "https://doc.rust-lang.org/beta/unstable-book/language-features/type-alias-impl-trait.html"),
             ItemEnum::Static(sttc) => {
                 path.push(ComponentType::Static, item);
                 self.visit_type(&path, &ErrorLocation::Static, &sttc.type_)?;
@@ -200,10 +213,14 @@ impl Visitor {
                     .context(here!())?;
                 self.visit_generics(&path, &typedef.generics)?;
             }
-            // Trait aliases aren't stable:
-            // https://doc.rust-lang.org/beta/unstable-book/language-features/trait-alias.html
-            ItemEnum::TraitAlias(_) => unimplemented!("unstable trait alias support"),
-            ItemEnum::Union(_) => unimplemented!("union support"),
+            ItemEnum::TraitAlias(_) => unstable_rust_feature!(
+                "trait_alias",
+                "https://doc.rust-lang.org/beta/unstable-book/language-features/trait-alias.html"
+            ),
+            ItemEnum::Union(unn) => {
+                path.push(ComponentType::Union, item);
+                self.visit_union(&path, unn)?;
+            }
             ItemEnum::Variant(variant) => {
                 path.push(ComponentType::EnumVariant, item);
                 self.visit_variant(&path, variant)?;
@@ -230,6 +247,19 @@ impl Visitor {
         Ok(())
     }
 
+    #[instrument(level = "debug", skip(self, path, unn), fields(path = %path))]
+    fn visit_union(&self, path: &Path, unn: &Union) -> Result<()> {
+        self.visit_generics(path, &unn.generics)?;
+        for id in &unn.fields {
+            let field = self.item(id)?;
+            self.visit_item(path, field, VisibilityCheck::Default)?;
+        }
+        for id in &unn.impls {
+            self.visit_impl(path, self.item(id)?)?;
+        }
+        Ok(())
+    }
+
     #[instrument(level = "debug", skip(self, path, trt), fields(path = %path))]
     fn visit_trait(&self, path: &Path, trt: &Trait) -> Result<()> {
         self.visit_generics(path, &trt.generics)?;
@@ -321,7 +351,12 @@ impl Visitor {
                     }
                 }
             }
-            Type::Infer => unimplemented!("visit_type Type::Infer"),
+            Type::Infer => {
+                unimplemented!(
+                    "visit_type for Type::Infer: not sure what Rust code triggers this. \
+                    If you encounter this, please report it with a link to the code it happens with."
+                )
+            }
             Type::RawPointer { type_, .. } => {
                 self.visit_type(path, what, type_).context(here!())?
             }
diff --git a/tools/cargo-check-external-types/test-workspace/external-lib/src/lib.rs b/tools/cargo-check-external-types/test-workspace/external-lib/src/lib.rs
@@ -33,3 +33,10 @@ pub trait AssociatedGenericTrait {
 }
 
 pub struct SimpleNewType(pub u32);
+
+#[repr(C)]
+#[derive(Copy, Clone)]
+pub struct ReprCType {
+    i: i32,
+    f: f32,
+}
diff --git a/tools/cargo-check-external-types/test-workspace/test-crate/src/lib.rs b/tools/cargo-check-external-types/test-workspace/test-crate/src/lib.rs
@@ -9,6 +9,8 @@
 //! This crate is used to test the cargo-check-external-types by exercising the all possible
 //! exposure of external types in a public API.
 
+pub mod test_union;
+
 use external_lib::{
     AssociatedGenericTrait,
     SimpleNewType,
@@ -23,8 +25,6 @@ use external_lib::{
     // Remove this comment if more lines are needed for imports in the future to preserve line numbers
     // Remove this comment if more lines are needed for imports in the future to preserve line numbers
     // Remove this comment if more lines are needed for imports in the future to preserve line numbers
-    // Remove this comment if more lines are needed for imports in the future to preserve line numbers
-    // Remove this comment if more lines are needed for imports in the future to preserve line numbers
 };
 
 pub struct LocalStruct;
diff --git a/tools/cargo-check-external-types/test-workspace/test-crate/src/test_union.rs b/tools/cargo-check-external-types/test-workspace/test-crate/src/test_union.rs
@@ -0,0 +1,24 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+use external_lib::{ReprCType, SimpleTrait};
+
+#[repr(C)]
+pub union SimpleUnion {
+    pub repr_c: ReprCType,
+    pub something_else: u64,
+}
+
+impl SimpleUnion {
+    pub fn repr_c(&self) -> &ReprCType {
+        &self.repr_c
+    }
+}
+
+#[repr(C)]
+pub union GenericUnion<T: Copy + SimpleTrait> {
+    pub repr_c: T,
+    pub something_else: u64,
+}
diff --git a/tools/cargo-check-external-types/tests/allow-some-types-expected-output.txt b/tools/cargo-check-external-types/tests/allow-some-types-expected-output.txt
@@ -18,4 +18,22 @@ error: Unapproved external type `external_lib::AssociatedGenericTrait` reference
     |
     = in trait bound of `test_crate::SomeTraitWithExternalDefaultTypes::OtherThing`
 
-2 errors emitted
+error: Unapproved external type `external_lib::ReprCType` referenced in public API
+  --> test-crate/src/test_union.rs:10:5
+   |
+10 |     pub repr_c: ReprCType,
+   |     ^-------------------^
+   |
+   = in struct field of `test_crate::test_union::SimpleUnion::repr_c`
+
+error: Unapproved external type `external_lib::ReprCType` referenced in public API
+  --> test-crate/src/test_union.rs:15:5
+   |
+15 |     pub fn repr_c(&self) -> &ReprCType {
+   | ...
+17 |     }␊
+   |     ^
+   |
+   = in return value of `test_crate::test_union::SimpleUnion::repr_c`
+
+4 errors emitted
diff --git a/tools/cargo-check-external-types/tests/default-config-expected-output.txt b/tools/cargo-check-external-types/tests/default-config-expected-output.txt
@@ -338,4 +338,32 @@ error: Unapproved external type `external_lib::SimpleNewType` referenced in publ
     |
     = in struct field of `test_crate::AssocConstStruct::OTHER_CONST`
 
-39 errors emitted
+error: Unapproved external type `external_lib::ReprCType` referenced in public API
+  --> test-crate/src/test_union.rs:10:5
+   |
+10 |     pub repr_c: ReprCType,
+   |     ^-------------------^
+   |
+   = in struct field of `test_crate::test_union::SimpleUnion::repr_c`
+
+error: Unapproved external type `external_lib::ReprCType` referenced in public API
+  --> test-crate/src/test_union.rs:15:5
+   |
+15 |     pub fn repr_c(&self) -> &ReprCType {
+   | ...
+17 |     }␊
+   |     ^
+   |
+   = in return value of `test_crate::test_union::SimpleUnion::repr_c`
+
+error: Unapproved external type `external_lib::SimpleTrait` referenced in public API
+  --> test-crate/src/test_union.rs:21:1
+   |
+21 | pub union GenericUnion<T: Copy + SimpleTrait> {
+   | ...
+24 | }␊
+   | ^
+   |
+   = in trait bound of `test_crate::test_union::GenericUnion`
+
+42 errors emitted
diff --git a/tools/cargo-check-external-types/tests/output-format-markdown-table-expected-output.md b/tools/cargo-check-external-types/tests/output-format-markdown-table-expected-output.md
@@ -2,6 +2,8 @@
 | ---   | ---  | ---     |
 | external_lib | external_lib::AssociatedGenericTrait | test-crate/src/lib.rs:125:0 |
 | external_lib | external_lib::AssociatedGenericTrait | test-crate/src/lib.rs:136:4 |
+| external_lib | external_lib::ReprCType | test-crate/src/test_union.rs:10:4 |
+| external_lib | external_lib::ReprCType | test-crate/src/test_union.rs:15:4 |
 | external_lib | external_lib::SimpleNewType | test-crate/src/lib.rs:158:4 |
 | external_lib | external_lib::SimpleTrait | test-crate/src/lib.rs:104:4 |
 | external_lib | external_lib::SimpleTrait | test-crate/src/lib.rs:122:0 |
@@ -13,6 +15,7 @@
 | external_lib | external_lib::SimpleTrait | test-crate/src/lib.rs:47:0 |
 | external_lib | external_lib::SimpleTrait | test-crate/src/lib.rs:89:4 |
 | external_lib | external_lib::SimpleTrait | test-crate/src/lib.rs:92:8 |
+| external_lib | external_lib::SimpleTrait | test-crate/src/test_union.rs:21:0 |
 | external_lib | external_lib::SomeOtherStruct | test-crate/src/lib.rs:125:0 |
 | external_lib | external_lib::SomeOtherStruct | test-crate/src/lib.rs:136:4 |
 | external_lib | external_lib::SomeOtherStruct | test-crate/src/lib.rs:72:4 |

Original file line number	Diff line number	Diff line change
`@@ -24,6 +24,7 @@ pub enum ComponentType {`
`24`	`24`	`StructField,`
`25`	`25`	`Trait,`
`26`	`26`	`TypeDef,`
	`27`	`+ Union,`
`27`	`28`	`}`
`28`	`29`
`29`	`30`	/// Represents one component in a [`Path`].