From 0d1dc51b0f954b740c73bcec7d6f4fb92703c55b Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Thu, 18 Aug 2022 14:41:35 -0700 Subject: [PATCH] Implement more checks in `cargo-check-external-types` (#1645) Adds support for unions, and marks unstable Rust language features as unsupported. --- tools/cargo-check-external-types/Cargo.lock | 2 +- tools/cargo-check-external-types/Cargo.toml | 2 +- tools/cargo-check-external-types/src/path.rs | 1 + .../cargo-check-external-types/src/visitor.rs | 51 ++++++++++++++++--- .../test-workspace/external-lib/src/lib.rs | 7 +++ .../test-workspace/test-crate/src/lib.rs | 4 +- .../test-crate/src/test_union.rs | 24 +++++++++ .../allow-some-types-expected-output.txt | 20 +++++++- .../tests/default-config-expected-output.txt | 30 ++++++++++- ...t-format-markdown-table-expected-output.md | 3 ++ 10 files changed, 130 insertions(+), 14 deletions(-) create mode 100644 tools/cargo-check-external-types/test-workspace/test-crate/src/test_union.rs diff --git a/tools/cargo-check-external-types/Cargo.lock b/tools/cargo-check-external-types/Cargo.lock index c034a93937c..9f2762f7424 100644 --- a/tools/cargo-check-external-types/Cargo.lock +++ b/tools/cargo-check-external-types/Cargo.lock @@ -51,7 +51,7 @@ dependencies = [ [[package]] name = "cargo-check-external-types" -version = "0.1.1" +version = "0.1.2" dependencies = [ "anyhow", "cargo_metadata", diff --git a/tools/cargo-check-external-types/Cargo.toml b/tools/cargo-check-external-types/Cargo.toml index 23e0b2773e6..5ec2a686bd3 100644 --- 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 ", "John DiSanti "] 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 index 9ceae049ba1..29123e63c67 100644 --- 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 index 9eff6ea9f43..aeeae9cc893 100644 --- 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 index 45083310f24..d35c9c1cfcd 100644 --- 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 index 8fd3bcb73cd..3b643d226fb 100644 --- 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 new file mode 100644 index 00000000000..9b311ed87cb --- /dev/null +++ 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 { + 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 index c0f42ec87de..f954bcf2e16 100644 --- 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 index 6c897ab4479..5854df7a590 100644 --- 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 { + | ... +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 index 9674fff9d6b..12f1a19ccfa 100644 --- 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 |