Unverified Commit 1c02c08b authored by John DiSanti's avatar John DiSanti Committed by GitHub
Browse files

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.
parent 70252531

tools/cargo-check-external-types/Cargo.lock

+21 −21
Original line number Diff line number Diff line
@@ -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",

tools/cargo-check-external-types/Cargo.toml

+1 −1
Original line number Diff line number Diff line 
[package]

name = "cargo-check-external-types"

version = "0.1.2"

version = "0.1.3"

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"

tools/cargo-check-external-types/src/visitor.rs

+45 −29
Original line number Diff line number Diff line
@@ -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<String> {

        Ok(Self::root(package)?

            .name

tools/cargo-check-external-types/test-workspace/Cargo.toml

+1 −1
Original line number Diff line number Diff line 
[workspace]

members = ["external-lib", "test-crate"]
 
members = ["external-lib", "test-crate", "test-reexports-crate"]

tools/cargo-check-external-types/test-workspace/test-crate/Cargo.toml

+1 −0
Original line number Diff line number Diff line
@@ -2,6 +2,7 @@ 
name = "test-crate"

version = "0.1.0"

edition = "2021"

publish = false



[dependencies]

external-lib = { path = "../external-lib" }