diff --git a/tools/cargo-check-external-types/Cargo.lock b/tools/cargo-check-external-types/Cargo.lock index 9f2762f7424f491eea586b9fd3f0e521de4f3b15..b35e21c09dc6c13012b87182eea33048b836ab84 100644 --- a/tools/cargo-check-external-types/Cargo.lock +++ b/tools/cargo-check-external-types/Cargo.lock @@ -13,9 +13,9 @@ dependencies = [ [[package]] name = "anyhow" -version = "1.0.59" +version = "1.0.62" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c91f1f46651137be86f3a2b9a8359f9ab421d04d941c62b5982e1ca21113adf9" +checksum = "1485d4d2cc45e7b201ee3767015c96faa5904387c9d87c6efdd0fb511f12d305" [[package]] name = "atty" @@ -42,16 +42,16 @@ checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" [[package]] name = "camino" -version = "1.0.9" +version = "1.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "869119e97797867fd90f5e22af7d0bd274bd4635ebb9eb68c04f3f513ae6c412" +checksum = "88ad0e1e3e88dd237a156ab9f571021b8a158caa0ae44b1968a241efb5144c1e" dependencies = [ "serde", ] [[package]] name = "cargo-check-external-types" -version = "0.1.2" +version = "0.1.3" dependencies = [ "anyhow", "cargo_metadata", @@ -204,9 +204,9 @@ checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" [[package]] name = "libc" -version = "0.2.126" +version = "0.2.132" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "349d5a591cd28b49e1d1037471617a32ddcda5731b99419008085f72d5a53836" +checksum = "8371e4e5341c3a96db127eb2465ac681ced4c433e01dd0e938adbef26ba93ba5" [[package]] name = "log" @@ -228,15 +228,15 @@ dependencies = [ [[package]] name = "once_cell" -version = "1.13.0" +version = "1.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "18a6dbe30758c9f83eb00cbea4ac95966305f5a7772f3f42ebfc7fc7eddbd8e1" +checksum = "074864da206b4973b84eb91683020dbefd6a8c3f0f38e054d93954e891935e4e" [[package]] name = "os_str_bytes" -version = "6.2.0" +version = "6.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "648001efe5d5c0102d8cea768e348da85d90af8ba91f0bea908f157951493cd4" +checksum = "9ff7415e9ae3fff1225851df9e0d9e4e5479f947619774677a63572e55e80eff" [[package]] name = "output_vt100" @@ -249,18 +249,18 @@ dependencies = [ [[package]] name = "owo-colors" -version = "3.4.0" +version = "3.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "decf7381921fea4dcb2549c5667eda59b3ec297ab7e2b5fc33eac69d2e7da87b" +checksum = "c1b04fb49957986fdce4d6ee7a65027d55d4b6d2265e5848bbb507b58ccfdb6f" dependencies = [ "supports-color", ] [[package]] name = "pest" -version = "2.2.1" +version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "69486e2b8c2d2aeb9762db7b4e00b0331156393555cff467f4163ff06821eef8" +checksum = "4b0560d531d1febc25a3c9398a62a71256c0178f2e3443baedd9ad4bb8c9deb4" dependencies = [ "thiserror", "ucd-trie", @@ -376,18 +376,18 @@ dependencies = [ [[package]] name = "serde" -version = "1.0.142" +version = "1.0.144" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e590c437916fb6b221e1d00df6e3294f3fccd70ca7e92541c475d6ed6ef5fee2" +checksum = "0f747710de3dcd43b88c9168773254e809d8ddbdf9653b84e2554ab219f17860" dependencies = [ "serde_derive", ] [[package]] name = "serde_derive" -version = "1.0.142" +version = "1.0.144" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "34b5b8d809babe02f538c2cfec6f2c1ed10804c0e5a6a041a049a4f5588ccc2e" +checksum = "94ed3a816fb1d101812f83e789f888322c34e291f894f19590dc310963e87a00" dependencies = [ "proc-macro2", "quote", @@ -396,9 +396,9 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.83" +version = "1.0.85" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "38dd04e3c8279e75b31ef29dbdceebfe5ad89f4d0937213c53f7d49d01b3d5a7" +checksum = "e55a28e3aaef9d5ce0506d0a14dbba8054ddc7e499ef522dd8b26859ec9d4a44" dependencies = [ "itoa", "ryu", diff --git a/tools/cargo-check-external-types/Cargo.toml b/tools/cargo-check-external-types/Cargo.toml index 5ec2a686bd352ade6726e7517d64c81c96ea77cc..9c8dcd3d76b4af3eb399386916ebd0b8ec4d902b 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.2" +version = "0.1.3" 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/visitor.rs b/tools/cargo-check-external-types/src/visitor.rs index aeeae9cc8934717eca73b531f28b13a20b6ac5b3..d33bead4c0a8c6bb32395d01879f78002d112047 100644 --- a/tools/cargo-check-external-types/src/visitor.rs +++ b/tools/cargo-check-external-types/src/visitor.rs @@ -128,23 +128,23 @@ impl Visitor { } => { path.push(ComponentType::AssocType, item); if let Some(typ) = default { - self.visit_type(&path, &ErrorLocation::AssocType, typ)?; + self.visit_type(&path, &ErrorLocation::AssocType, typ).context(here!())?; } - self.visit_generic_bounds(&path, bounds)?; - self.visit_generics(&path, generics)?; + self.visit_generic_bounds(&path, bounds).context(here!())?; + self.visit_generics(&path, generics).context(here!())?; } ItemEnum::Constant(constant) => { path.push(ComponentType::Constant, item); - self.visit_type(&path, &ErrorLocation::Constant, &constant.type_)?; + self.visit_type(&path, &ErrorLocation::Constant, &constant.type_).context(here!())?; } ItemEnum::Enum(enm) => { path.push(ComponentType::Enum, item); - self.visit_generics(&path, &enm.generics)?; + self.visit_generics(&path, &enm.generics).context(here!())?; for id in &enm.impls { - self.visit_impl(&path, self.item(id)?)?; + self.visit_impl(&path, self.item(id).context(here!())?)?; } for id in &enm.variants { - self.visit_item(&path, self.item(id)?, VisibilityCheck::Default)?; + self.visit_item(&path, self.item(id).context(here!())?, VisibilityCheck::Default).context(here!())?; } } ItemEnum::ForeignType => unstable_rust_feature!( @@ -153,15 +153,18 @@ impl Visitor { ), ItemEnum::Function(function) => { path.push(ComponentType::Function, item); - self.visit_fn_decl(&path, &function.decl)?; - self.visit_generics(&path, &function.generics)?; + self.visit_fn_decl(&path, &function.decl).context(here!())?; + self.visit_generics(&path, &function.generics).context(here!())?; } ItemEnum::Import(import) => { if let Some(target_id) = &import.id { - if let Ok(target_item) = self.item(target_id) { + if self.in_root_crate(target_id) { // Override the visibility check for re-exported items - self.visit_item(&path, target_item, VisibilityCheck::AssumePublic) - .context(here!())?; + self.visit_item( + &path, + self.item(target_id).context(here!())?, + VisibilityCheck::AssumePublic + ).context(here!())?; } path.push_raw(ComponentType::ReExport, &import.name, item.span.as_ref()); self.check_external(&path, &ErrorLocation::ReExport, target_id) @@ -170,33 +173,33 @@ impl Visitor { } ItemEnum::Method(method) => { path.push(ComponentType::Method, item); - self.visit_fn_decl(&path, &method.decl)?; - self.visit_generics(&path, &method.generics)?; + self.visit_fn_decl(&path, &method.decl).context(here!())?; + self.visit_generics(&path, &method.generics).context(here!())?; } ItemEnum::Module(module) => { if !module.is_crate { path.push(ComponentType::Module, item); } for id in &module.items { - let module_item = self.item(id)?; + let module_item = self.item(id).context(here!())?; // Re-exports show up twice in the doc json: once as an `ItemEnum::Import`, // and once as the type as if it were originating from the root crate (but // with a different crate ID). We only want to examine the `ItemEnum::Import` // for re-exports since it includes the correct span where the re-export occurs, // and we don't want to examine the innards of the re-export. if module_item.crate_id == self.root_crate_id { - self.visit_item(&path, module_item, VisibilityCheck::Default)?; + self.visit_item(&path, module_item, VisibilityCheck::Default).context(here!())?; } } } 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_)?; + self.visit_type(&path, &ErrorLocation::Static, &sttc.type_).context(here!())?; } ItemEnum::Struct(strct) => { path.push(ComponentType::Struct, item); - self.visit_struct(&path, strct)?; + self.visit_struct(&path, strct).context(here!())?; } ItemEnum::StructField(typ) => { path.push(ComponentType::StructField, item); @@ -205,13 +208,13 @@ impl Visitor { } ItemEnum::Trait(trt) => { path.push(ComponentType::Trait, item); - self.visit_trait(&path, trt)?; + self.visit_trait(&path, trt).context(here!())?; } ItemEnum::Typedef(typedef) => { path.push(ComponentType::TypeDef, item); self.visit_type(&path, &ErrorLocation::TypeDef, &typedef.type_) .context(here!())?; - self.visit_generics(&path, &typedef.generics)?; + self.visit_generics(&path, &typedef.generics).context(here!())?; } ItemEnum::TraitAlias(_) => unstable_rust_feature!( "trait_alias", @@ -219,11 +222,11 @@ impl Visitor { ), ItemEnum::Union(unn) => { path.push(ComponentType::Union, item); - self.visit_union(&path, unn)?; + self.visit_union(&path, unn).context(here!())?; } ItemEnum::Variant(variant) => { path.push(ComponentType::EnumVariant, item); - self.visit_variant(&path, variant)?; + self.visit_variant(&path, variant).context(here!())?; } ItemEnum::ExternCrate { .. } | ItemEnum::Impl(_) @@ -238,11 +241,11 @@ impl Visitor { fn visit_struct(&self, path: &Path, strct: &Struct) -> Result<()> { self.visit_generics(path, &strct.generics)?; for id in &strct.fields { - let field = self.item(id)?; + let field = self.item(id).context(here!())?; self.visit_item(path, field, VisibilityCheck::Default)?; } for id in &strct.impls { - self.visit_impl(path, self.item(id)?)?; + self.visit_impl(path, self.item(id).context(here!())?)?; } Ok(()) } @@ -251,11 +254,11 @@ impl Visitor { 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)?; + let field = self.item(id).context(here!())?; self.visit_item(path, field, VisibilityCheck::Default)?; } for id in &unn.impls { - self.visit_impl(path, self.item(id)?)?; + self.visit_impl(path, self.item(id).context(here!())?)?; } Ok(()) } @@ -265,7 +268,7 @@ impl Visitor { self.visit_generics(path, &trt.generics)?; self.visit_generic_bounds(path, &trt.bounds)?; for id in &trt.items { - let item = self.item(id)?; + let item = self.item(id).context(here!())?; self.visit_item(path, item, VisibilityCheck::Default)?; } Ok(()) @@ -280,7 +283,11 @@ impl Visitor { } self.visit_generics(path, &imp.generics)?; for id in &imp.items { - self.visit_item(path, self.item(id)?, VisibilityCheck::Default)?; + self.visit_item( + path, + self.item(id).context(here!())?, + VisibilityCheck::Default, + )?; } if let Some(trait_) = &imp.trait_ { self.visit_type(path, &ErrorLocation::ImplementedTrait, trait_) @@ -495,7 +502,11 @@ impl Visitor { } Variant::Struct(ids) => { for id in ids { - self.visit_item(path, self.item(id)?, VisibilityCheck::Default)?; + self.visit_item( + path, + self.item(id).context(here!())?, + VisibilityCheck::Default, + )?; } } } @@ -545,6 +556,11 @@ impl Visitor { Ok(Self::root(package)?.crate_id) } + /// Returns true if the given `id` belongs to the root crate + fn in_root_crate(&self, id: &Id) -> bool { + id.0.starts_with(&format!("{}:", self.root_crate_id)) + } + fn root_crate_name(package: &Crate) -> Result { Ok(Self::root(package)? .name diff --git a/tools/cargo-check-external-types/test-workspace/Cargo.toml b/tools/cargo-check-external-types/test-workspace/Cargo.toml index 4601ec8f8df245fd43957bf0a90eb5b1cf1b6cb4..9f50884ad596997044099a108bb44e8f18c500a1 100644 --- a/tools/cargo-check-external-types/test-workspace/Cargo.toml +++ b/tools/cargo-check-external-types/test-workspace/Cargo.toml @@ -1,2 +1,2 @@ [workspace] -members = ["external-lib", "test-crate"] +members = ["external-lib", "test-crate", "test-reexports-crate"] diff --git a/tools/cargo-check-external-types/test-workspace/test-crate/Cargo.toml b/tools/cargo-check-external-types/test-workspace/test-crate/Cargo.toml index b5bcfeacdd2ceb12ae1a5084b1bcaff80be4d4bb..f80e0628608407dddffe0147ac173b9d8388c01d 100644 --- a/tools/cargo-check-external-types/test-workspace/test-crate/Cargo.toml +++ b/tools/cargo-check-external-types/test-workspace/test-crate/Cargo.toml @@ -2,6 +2,7 @@ name = "test-crate" version = "0.1.0" edition = "2021" +publish = false [dependencies] external-lib = { path = "../external-lib" } diff --git a/tools/cargo-check-external-types/test-workspace/test-reexports-crate/Cargo.toml b/tools/cargo-check-external-types/test-workspace/test-reexports-crate/Cargo.toml new file mode 100644 index 0000000000000000000000000000000000000000..8790abe4ff1d567243990bc6980c8efc5499dd2d --- /dev/null +++ b/tools/cargo-check-external-types/test-workspace/test-reexports-crate/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "test-reexports-crate" +version = "0.1.0" +edition = "2021" +publish = false + +[dependencies] +external-lib = { path = "../external-lib" } diff --git a/tools/cargo-check-external-types/test-workspace/test-reexports-crate/src/lib.rs b/tools/cargo-check-external-types/test-workspace/test-reexports-crate/src/lib.rs new file mode 100644 index 0000000000000000000000000000000000000000..0b98a08849018946e4b6910af5dfe4111986d140 --- /dev/null +++ b/tools/cargo-check-external-types/test-workspace/test-reexports-crate/src/lib.rs @@ -0,0 +1,16 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +pub use external_lib::AssociatedGenericTrait; +pub use external_lib::ReprCType; +pub use external_lib::SimpleTrait; + +pub mod Something { + pub use external_lib::SimpleGenericTrait; + pub use external_lib::SimpleNewType; +} + +pub use external_lib::SomeOtherStruct; +pub use external_lib::SomeStruct; diff --git a/tools/cargo-check-external-types/tests/integration_test.rs b/tools/cargo-check-external-types/tests/integration_test.rs index daa84bc2a31248680d94e5e8538513550a15d8da..84686dfa5cce3103fbc56fe3dd21116c147d57af 100644 --- a/tools/cargo-check-external-types/tests/integration_test.rs +++ b/tools/cargo-check-external-types/tests/integration_test.rs @@ -63,3 +63,15 @@ fn with_output_format_markdown_table() { ); assert_str_eq!(expected_output, actual_output); } + +// Make sure that the visitor doesn't attempt to visit the inner items of re-exported external types. +// Rustdoc doesn't include these inner items in its JSON output, which leads to obtuse crashes if they're +// referenced. It's also just the wrong behavior to look into the type being re-exported, since if it's +// approved, then it doesn't matter what it referenced. If it's not approved, then the re-export itself +// is the violation. +#[test] +fn test_reexports() { + let expected_output = fs::read_to_string("tests/test-reexports-expected-output.md").unwrap(); + let actual_output = run_with_args("test-workspace/test-reexports-crate", &[]); + assert_str_eq!(expected_output, actual_output); +} diff --git a/tools/cargo-check-external-types/tests/test-reexports-expected-output.md b/tools/cargo-check-external-types/tests/test-reexports-expected-output.md new file mode 100644 index 0000000000000000000000000000000000000000..bfd40d1d02c0cd166c955844b5481095ee6d1773 --- /dev/null +++ b/tools/cargo-check-external-types/tests/test-reexports-expected-output.md @@ -0,0 +1,57 @@ +error: Unapproved external type `external_lib::AssociatedGenericTrait` referenced in public API + --> test-reexports-crate/src/lib.rs:6:1 + | +6 | pub use external_lib::AssociatedGenericTrait; + | ^-------------------------------------------^ + | + = in re-export named `test_reexports_crate::AssociatedGenericTrait` + +error: Unapproved external type `external_lib::ReprCType` referenced in public API + --> test-reexports-crate/src/lib.rs:7:1 + | +7 | pub use external_lib::ReprCType; + | ^------------------------------^ + | + = in re-export named `test_reexports_crate::ReprCType` + +error: Unapproved external type `external_lib::SimpleTrait` referenced in public API + --> test-reexports-crate/src/lib.rs:8:1 + | +8 | pub use external_lib::SimpleTrait; + | ^--------------------------------^ + | + = in re-export named `test_reexports_crate::SimpleTrait` + +error: Unapproved external type `external_lib::SimpleGenericTrait` referenced in public API + --> test-reexports-crate/src/lib.rs:11:5 + | +11 | pub use external_lib::SimpleGenericTrait; + | ^---------------------------------------^ + | + = in re-export named `test_reexports_crate::Something::SimpleGenericTrait` + +error: Unapproved external type `external_lib::SimpleNewType` referenced in public API + --> test-reexports-crate/src/lib.rs:12:5 + | +12 | pub use external_lib::SimpleNewType; + | ^----------------------------------^ + | + = in re-export named `test_reexports_crate::Something::SimpleNewType` + +error: Unapproved external type `external_lib::SomeOtherStruct` referenced in public API + --> test-reexports-crate/src/lib.rs:15:1 + | +15 | pub use external_lib::SomeOtherStruct; + | ^------------------------------------^ + | + = in re-export named `test_reexports_crate::SomeOtherStruct` + +error: Unapproved external type `external_lib::SomeStruct` referenced in public API + --> test-reexports-crate/src/lib.rs:16:1 + | +16 | pub use external_lib::SomeStruct; + | ^-------------------------------^ + | + = in re-export named `test_reexports_crate::SomeStruct` + +7 errors emitted