Changed all baseline rustdoc generations to use placeholder manifest

.github/workflows/ci.yml

@@ -373,15 +373,42 @@ jobs:
         with:
           key: clap
 
-      - name: Run semver-checks
+      - name: Run semver-checks with --baseline-root
         continue-on-error: true
         run: |
           cargo run semver-checks check-release --manifest-path="subject-current/Cargo.toml" --baseline-root="subject-baseline/Cargo.toml" > output
-          echo "$?" > return_code
+          touch unexpectedly_did_not_fail
+
+      - name: Check whether it failed
+        run: |
+          if [ -f unexpectedly_did_not_fail ]; then exit 1; else exit 0; fi
+
+      - name: Check output
+        run: |
+          EXPECTED="$(echo -e "--- failure auto_trait_impl_removed: auto trait no longer implemented ---\n--- failure trait_missing: pub trait removed or renamed ---")"
+          RESULT="$(cat output | grep failure)"
+          diff <(echo "$RESULT") <(echo "$EXPECTED")
+
+      - name: Cleanup
+        run: |
+          rm output
+          rm -f unexpectedly_did_not_fail
+
+      - name: Fetch v3.1.18 in subject-current
+        run: |
+          cd subject-current
+          git fetch origin 524e36cf1a67ee6a447d3615a70b065d2b4f5e60
+          cd ..
+
+      - name: Run semver-checks with --baseline-rev
+        continue-on-error: true
+        run: |
+          cargo run semver-checks check-release --manifest-path="subject-current/Cargo.toml" --baseline-rev=524e36cf1a67ee6a447d3615a70b065d2b4f5e60 > output
+          touch unexpectedly_did_not_fail
 
-      - name: Check return code
+      - name: Check whether it failed
         run: |
-          if [[ "$(cat return_code)" == "0" ]]; then exit 1; else exit 0; fi
+          if [ -f unexpectedly_did_not_fail ]; then exit 1; else exit 0; fi
 
       - name: Check output
         run: |

src/baseline.rs

@@ -4,69 +4,95 @@ use anyhow::Context as _;
 use crates_index::Crate;
 
 use crate::dump::RustDocCommand;
+use crate::manifest::Manifest;
 use crate::util::slugify;
 use crate::GlobalConfig;
 
-mod crate_features {
-    /// Sometimes crates ship types with fields or variants that are included
-    /// only when certain features are enabled.
-    ///
-    /// By default, we want to generate rustdoc with `--all-features`,
-    /// but that option isn't available outside of the current crate,
-    /// so we have to implement it ourselves.
-
-    pub(crate) fn get_all_crate_features_from_registry(
-        crate_: &crates_index::Version,
-    ) -> Vec<String> {
-        // Implicit features from optional dependencies have to be added separately
-        // from regular features: https://github.com/obi1kenobi/cargo-semver-checks/issues/265
-        let mut implicit_features: std::collections::BTreeSet<_> = crate_
-            .dependencies()
-            .iter()
-            .filter_map(|dep| dep.is_optional().then_some(dep.name()))
-            .map(|x| x.to_string())
-            .collect();
-        for feature_defn in crate_.features().values().flatten() {
-            // "If you specify the optional dependency with the dep: prefix anywhere
-            //  in the [features] table, that disables the implicit feature."
-            // https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies
-            if let Some(optional_dep) = feature_defn.strip_prefix("dep:") {
-                implicit_features.remove(optional_dep);
-            }
-        }
-        let regular_features: std::collections::BTreeSet<_> =
-            crate_.features().keys().cloned().collect();
-        let mut all_crate_features = implicit_features;
-        all_crate_features.extend(regular_features);
-        all_crate_features.into_iter().collect()
-    }
-
-    #[allow(unused_variables)]
-    pub(crate) fn get_all_crate_features_from_manifest(path: &std::path::Path) -> Vec<String> {
-        unimplemented!()
-    }
-}
-
-#[allow(dead_code)]
 #[derive(Debug, Clone)]
 enum CrateSource<'a> {
     Registry { crate_: &'a crates_index::Version },
-    ManifestPath { path: &'a Path, name: String },
+    ManifestPath { manifest: &'a Manifest },
 }
 
 impl<'a> CrateSource<'a> {
-    fn name(&self) -> &str {
-        match self {
+    fn name(&self) -> anyhow::Result<&str> {
+        Ok(match self {
             Self::Registry { crate_ } => crate_.name(),
-            Self::ManifestPath { .. } => unimplemented!(),
-        }
+            Self::ManifestPath { manifest } => crate::manifest::get_package_name(manifest)?,
+        })
     }
 
-    fn version(&self) -> &str {
-        match self {
+    fn version(&self) -> anyhow::Result<&str> {
+        Ok(match self {
             Self::Registry { crate_ } => crate_.version(),
-            Self::ManifestPath { .. } => unimplemented!(),
+            Self::ManifestPath { manifest } => crate::manifest::get_package_version(manifest)?,
+        })
+    }
+
+    /// Returns features listed in `[features]` section in the manifest
+    /// <https://doc.rust-lang.org/cargo/reference/features.html#the-features-section>
+    fn regular_features(&self) -> Vec<String> {
+        match self {
+            Self::Registry { crate_ } => crate_.features().keys().cloned().into_iter().collect(),
+            Self::ManifestPath { manifest } => manifest
+                .parsed
+                .features
+                .keys()
+                .cloned()
+                .into_iter()
+                .collect(),
+        }
+    }
+
+    /// Returns features implicitly defined by optional dependencies
+    /// <https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies>
+    fn implicit_features(&self) -> std::collections::BTreeSet<String> {
+        let mut implicit_features: std::collections::BTreeSet<_> = match self {
+            Self::Registry { crate_ } => crate_
+                .dependencies()
+                .iter()
+                .filter_map(|dep| dep.is_optional().then_some(dep.name()))
+                .map(|x| x.to_string())
+                .collect(),
+            Self::ManifestPath { manifest } => manifest
+                .parsed
+                .dependencies
+                .iter()
+                .filter_map(|(name, dep)| dep.optional().then_some(name))
+                .map(|x| x.to_string())
+                .collect(),
+        };
+
+        let feature_defns: Vec<&String> = match self {
+            Self::Registry { crate_ } => crate_.features().values().flatten().collect(),
+            Self::ManifestPath { manifest } => {
+                manifest.parsed.features.values().flatten().collect()
+            }
+        };
+
+        for feature_defn in feature_defns {
+            // "If you specify the optional dependency with the dep: prefix anywhere
+            //  in the [features] table, that disables the implicit feature."
+            // https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies
+            if let Some(optional_dep) = feature_defn.strip_prefix("dep:") {
+                implicit_features.remove(optional_dep);
+            }
         }
+        implicit_features
+    }
+
+    /// Sometimes crates ship types with fields or variants that are included
+    /// only when certain features are enabled.
+    ///
+    /// By default, we want to generate rustdoc with `--all-features`,
+    /// but that option isn't available outside of the current crate,
+    /// so we have to implement it ourselves.
+    fn all_features(&self) -> Vec<String> {
+        // Implicit features from optional dependencies have to be added separately
+        // from regular features: https://github.com/obi1kenobi/cargo-semver-checks/issues/265
+        let mut all_crate_features = self.implicit_features();
+        all_crate_features.extend(self.regular_features());
+        all_crate_features.into_iter().collect()
     }
 }
 
@@ -98,26 +124,36 @@ fn create_placeholder_rustdoc_manifest(
                     // give us the latest semver-compatible version which is not we want.
                     // Fixes: https://github.com/obi1kenobi/cargo-semver-checks/issues/261
                     version: Some(format!("={}", crate_.version())),
-                    features: crate_features::get_all_crate_features_from_registry(crate_),
+                    features: crate_source.all_features(),
                     ..DependencyDetail::default()
                 },
-                CrateSource::ManifestPath { path, .. } => DependencyDetail {
-                    path: Some(
+                CrateSource::ManifestPath { manifest } => DependencyDetail {
+                    path: Some({
+                        assert!(
+                            manifest.path.ends_with("Cargo.toml"),
+                            "path {} isn't pointing to a manifest",
+                            manifest.path.display()
+                        );
+                        let dir_path = manifest
+                            .path
+                            .parent()
+                            .context("manifest path doesn't have a parent")?;
                         // The manifest will be saved in some other directory,
                         // so for convenience, we're using absolute paths.
-                        path.canonicalize()
+                        dir_path
+                            .canonicalize()
                             .context("failed to canonicalize manifest path")?
                             .to_str()
                             .context("manifest path is not valid UTF-8")?
-                            .to_string(),
-                    ),
-                    features: crate_features::get_all_crate_features_from_manifest(path),
+                            .to_string()
+                    }),
+                    features: crate_source.all_features(),
                     ..DependencyDetail::default()
                 },
             };
             let mut deps = DepsSet::new();
             deps.insert(
-                crate_source.name().to_string(),
+                crate_source.name()?.to_string(),
                 Dependency::Detailed(project_with_features),
             );
             deps
@@ -152,12 +188,24 @@ fn generate_rustdoc(
     target_root: PathBuf,
     crate_source: CrateSource,
 ) -> anyhow::Result<PathBuf> {
-    let name = crate_source.name();
-    let version = crate_source.version();
-
-    let (build_dir, cache_dir, cached_rustdoc) = match crate_source {
+    let name = crate_source.name()?;
+    let version = crate_source.version()?;
+
+    let crate_identifier = format!(
+        "{}-{}-{}",
+        match crate_source {
+            CrateSource::Registry { .. } => "registry",
+            // Identifiers of manifest-based crates cannot be used for caching,
+            // since they probably correspond to a specific (and unknown) gitrev and git state
+            // so cached entries cannot be checked to see if they are a match or not.
+            CrateSource::ManifestPath { .. } => "local",
+        },
+        slugify(name),
+        slugify(version)
+    );
+    let build_dir = target_root.join(&crate_identifier);
+    let (cache_dir, cached_rustdoc) = match crate_source {
         CrateSource::Registry { .. } => {
-            let crate_identifier = format!("registry-{}-{}", slugify(name), slugify(version));
             let cache_dir = target_root.join("cache");
             let cached_rustdoc = cache_dir.join(format!("{crate_identifier}.json"));
 
@@ -173,12 +221,9 @@ fn generate_rustdoc(
                 return Ok(cached_rustdoc);
             }
 
-            let build_dir = target_root.join(crate_identifier);
-            (build_dir, cache_dir, cached_rustdoc)
-        }
-        CrateSource::ManifestPath { .. } => {
-            unimplemented!()
+            (Some(cache_dir), Some(cached_rustdoc))
         }
+        CrateSource::ManifestPath { .. } => (None, None),
     };
 
     let placeholder_manifest = create_placeholder_rustdoc_manifest(&crate_source)
@@ -200,6 +245,12 @@ fn generate_rustdoc(
     match crate_source {
         CrateSource::Registry { .. } => {
             // Clean up after ourselves.
+            let cache_dir = cache_dir.expect(
+                "when crate_source is Registry a cache_dir was created, so it should be Some",
+            );
+            let cached_rustdoc = cached_rustdoc.expect(
+                "when crate_source is Registry a cached_rustdoc was created, so it should be Some",
+            );
             std::fs::create_dir_all(cache_dir)?;
             std::fs::copy(rustdoc_path, &cached_rustdoc)?;
             std::fs::remove_dir_all(build_dir)?;
@@ -247,29 +298,34 @@ impl BaselineLoader for RustdocBaseline {
 }
 
 pub(crate) struct PathBaseline {
-    root: PathBuf,
-    lookup: std::collections::HashMap<String, (String, PathBuf)>,
+    project_root: PathBuf,
+    lookup: std::collections::HashMap<String, Manifest>,
+    target_root: PathBuf,
 }
 
 impl PathBaseline {
-    pub(crate) fn new(root: &std::path::Path) -> anyhow::Result<Self> {
+    /// # Arguments
+    /// * `project_root` - Path to a directory with the manifest or with subdirectories with the manifests.
+    /// * `target_root` - Path to a directory where the placeholder manifest / rustdoc can be created.
+    pub(crate) fn new(
+        project_root: &std::path::Path,
+        target_root: &std::path::Path,
+    ) -> anyhow::Result<Self> {
         let mut lookup = std::collections::HashMap::new();
-        for result in ignore::Walk::new(root) {
+        for result in ignore::Walk::new(project_root) {
             let entry = result?;
             if entry.file_name() == "Cargo.toml" {
-                if let Ok(manifest) = crate::manifest::Manifest::parse(entry.path()) {
-                    if let (Ok(name), Ok(version)) = (
-                        crate::manifest::get_package_name(&manifest),
-                        crate::manifest::get_package_version(&manifest),
-                    ) {
-                        lookup.insert(name, (version, entry.into_path()));
+                if let Ok(manifest) = crate::manifest::Manifest::parse(entry.into_path()) {
+                    if let Ok(name) = crate::manifest::get_package_name(&manifest) {
+                        lookup.insert(name.to_string(), manifest);
                     }
                 }
             }
         }
         Ok(Self {
-            root: root.to_owned(),
+            project_root: project_root.to_owned(),
             lookup,
+            target_root: target_root.to_owned(),
         })
     }
 }
@@ -282,14 +338,19 @@ impl BaselineLoader for PathBaseline {
         name: &str,
         _version_current: Option<&semver::Version>,
     ) -> anyhow::Result<PathBuf> {
-        let (version, manifest_path) = self
-            .lookup
-            .get(name)
-            .with_context(|| format!("package `{}` not found in {}", name, self.root.display()))?;
-        let version = format!(" v{version}");
-        config.shell_status("Parsing", format_args!("{name}{version} (baseline)"))?;
-        let rustdoc_path = rustdoc.dump(manifest_path.as_path(), None, true)?;
-        Ok(rustdoc_path)
+        let manifest: &Manifest = self.lookup.get(name).with_context(|| {
+            format!(
+                "package `{}` not found in {}",
+                name,
+                self.project_root.display()
+            )
+        })?;
+        generate_rustdoc(
+            config,
+            rustdoc,
+            self.target_root.clone(),
+            CrateSource::ManifestPath { manifest },
+        )
     }
 }
 
@@ -308,13 +369,13 @@ impl GitBaseline {
         let repo = git2::Repository::discover(source)?;
 
         let rev = repo.revparse_single(rev)?;
-        let target = target.join(rev.id().to_string());
+        let rev_dir = target.join(rev.id().to_string());
 
-        std::fs::create_dir_all(&target)?;
+        std::fs::create_dir_all(&rev_dir)?;
         let tree = rev.peel_to_tree()?;
-        extract_tree(&repo, tree, &target)?;
+        extract_tree(&repo, tree, &rev_dir)?;
 
-        let path = PathBaseline::new(&target)?;
+        let path = PathBaseline::new(&rev_dir, target)?;
         Ok(Self { path })
     }
 }