From 1c02c08b16466b7ee5dc9f26ab10619730f20704 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Tue, 23 Aug 2022 10:04:31 -0700 Subject: [PATCH] Fix crash for certain re-exports in `cargo-check-external-types` (#1654) The visitor was examining the inner contents of the entire re-exported type, which, for a type that is re-exported within the same crate, is the correct behavior. Its the wrong behavior when exporting types from other crates, however, since it doesn't matter what is inside that external type, and rustdoc doesn't include information about inner pieces of the external type in the JSON output. --- tools/cargo-check-external-types/Cargo.lock | 42 +++++------ tools/cargo-check-external-types/Cargo.toml | 2 +- .../cargo-check-external-types/src/visitor.rs | 74 +++++++++++-------- .../test-workspace/Cargo.toml | 2 +- .../test-workspace/test-crate/Cargo.toml | 1 + .../test-reexports-crate/Cargo.toml | 8 ++ .../test-reexports-crate/src/lib.rs | 16 ++++ .../tests/integration_test.rs | 12 +++ .../tests/test-reexports-expected-output.md | 57 ++++++++++++++ 9 files changed, 162 insertions(+), 52 deletions(-) create mode 100644 tools/cargo-check-external-types/test-workspace/test-reexports-crate/Cargo.toml create mode 100644 tools/cargo-check-external-types/test-workspace/test-reexports-crate/src/lib.rs create mode 100644 tools/cargo-check-external-types/tests/test-reexports-expected-output.md diff --git a/tools/cargo-check-external-types/Cargo.lock b/tools/cargo-check-external-types/Cargo.lock index 9f2762f74..b35e21c09 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 5ec2a686b..9c8dcd3d7 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 aeeae9cc8..d33bead4c 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 4601ec8f8..9f50884ad 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 b5bcfeacd..f80e06286 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 000000000..8790abe4f --- /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 000000000..0b98a0884 --- /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 daa84bc2a..84686dfa5 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 000000000..bfd40d1d0 --- /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 -- GitLab