Support linting breaking changes in manifests & add feature_missing…

… lint (#1007) Add support for linting of package manifests, allowing us to scan for breaking changes there as well. For example, deleting a feature is a major breaking change. As of this PR, we can detect and report that: ``` --- failure feature_missing: package feature removed or renamed --- Description: A feature has been removed from this package's Cargo.toml. This will break downstream crates which enable that feature. ref: https://doc.rust-lang.org/cargo/reference/semver.html#cargo-feature-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.37.0/src/lints/feature_missing.ron Failed in: feature going_missing in the package's Cargo.toml feature rand in the package's Cargo.toml Summary semver requires new major version: 1 major and 0 minor checks failed ``` Completes the first checkbox of the 2024H2 Rust Project Goal on cargo-semver-checks: rust-lang/rust-project-goals#104 Unblocks the lints specified in #48.
obi1kenobi · Dec 1, 2024 · e6bed4d · e6bed4d
1 parent 45f0664
commit e6bed4d
Show file tree

Hide file tree

Showing 17 changed files with 461 additions and 178 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -194,7 +194,7 @@ jobs:
         uses: actions/cache@v4
         with:
           path: semver/localdata/test_data/
-          key: test-rustdocs-${{ runner.os }}-${{ steps.toolchain.outputs.cachekey }}-${{ hashFiles('semver/test_crates/**/*.rs') }}
+          key: test-rustdocs-and-meta-${{ runner.os }}-${{ steps.toolchain.outputs.cachekey }}-${{ hashFiles('semver/test_crates/**/*.rs') }}
 
       - name: Regenerate test data
         if: steps.cache-test-rustdocs.outputs.cache-hit != 'true'
@@ -406,11 +406,17 @@ jobs:
 
           # Show what's in the cache.
           ls subject-new/target/semver-checks/cache/
-          EXPECTED_CACHED_RUSTDOC='subject-new/target/semver-checks/cache/libp2p_core-0_37_0-ccce455725cbab73.json'
+          EXPECTED_PREFIX='subject-new/target/semver-checks/cache/libp2p_core-0_37_0-ccce455725cbab73'
+          EXPECTED_CACHED_METADATA="${EXPECTED_PREFIX}.metadata.json"
+          EXPECTED_CACHED_RUSTDOC="${EXPECTED_PREFIX}.json"
 
-          # Ensure the previous cached rustdoc file still exists.
+          # Ensure the previous cached rustdoc and metadata files still exist.
+          [ -f "$EXPECTED_CACHED_METADATA" ] || { \
+            echo "could not find libp2p-core metadata cache file"; \
+            exit 1;
+          }
           [ -f "$EXPECTED_CACHED_RUSTDOC" ] || { \
-            echo "could not find libp2p-core cache file"; \
+            echo "could not find libp2p-core rustdoc cache file"; \
             exit 1;
           }
 
@@ -527,20 +533,30 @@ jobs:
           ls subject/target/semver-checks/cache/
 
           # The previous cached rustdoc should continue to exist.
-          if [ -f subject/target/semver-checks/cache/serde-1_0_162-5900ebf8bb9b9f8b.json ]; then
-            exit 0
-          else
+          PREVIOUS_PREFIX='subject/target/semver-checks/cache/serde-1_0_162-5900ebf8bb9b9f8b'
+          PREVIOUS_RUSTDOC="${PREVIOUS_PREFIX}.json"
+          PREVIOUS_METADATA="${PREVIOUS_PREFIX}.metadata.json"
+          if [ ! -f "$PREVIOUS_RUSTDOC" ]; then
             echo "Older rustdoc JSON not found in cache!"
             exit 1
           fi
+          if [ ! -f "$PREVIOUS_METADATA" ]; then
+            echo "Older metadata file not found in cache!"
+            exit 1
+          fi
 
           # There should also be a new cached rustdoc file for the new feature settings.
-          if [ -f subject/target/semver-checks/cache/serde-1_0_162-6999ae87ca463ab3.json ]; then
-            exit 0
-          else
+          NEW_PREFIX='subject/target/semver-checks/cache/serde-1_0_162-6999ae87ca463ab3'
+          NEW_RUSTDOC="${NEW_PREFIX}.json"
+          NEW_METADATA="${NEW_PREFIX}.metadata.json"
+          if [ ! -f "$NEW_RUSTDOC" ]; then
             echo "New rustdoc JSON for new feature combo not found in cache!"
             exit 1
           fi
+          if [ ! -f "$NEW_METADATA" ]; then
+            echo "New metadata file for new feature combo not found in cache!"
+            exit 1
+          fi
 
       - name: Cleanup
         run: |

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -111,7 +111,7 @@ To generate this data, please run `./scripts/regenerate_test_rustdocs.sh`.
 To use a specific toolchain, like beta or nightly, pass it as
 an argument: `./scripts/regenerate_test_rustdocs.sh +nightly`.
 
-## What are those `.snap` or `.snap.new` files generated via `cargo test` 
+## What are those `.snap` or `.snap.new` files generated via `cargo test`
 
 As part of our overall testing strategy, we use a technique called "snapshot testing."
 The tool we use for this ([`insta`](https://insta.rs/docs/)) is user friendly and has mutliple ways to interact with it:
@@ -138,7 +138,7 @@ Reviewing them is possible via these options:
    ```
 3. **Manually**: If you can't (or don't want to) use `cargo-insta`, you can verify the snapshot
    file manually.  There should be a file called `test_outputs/<some_path>/<lint_name>.snap.new`.
-   Open it, and verify that its contents match what you expected: all expected data is present, and no unexpected data is included. 
+   Open it, and verify that its contents match what you expected: all expected data is present, and no unexpected data is included.
    Once you've checked it, remove the `.new` suffix so that the file's new path
    is `test_outputs/<some_path>/<lint_name>.snap`
 
@@ -172,14 +172,16 @@ We'll use the [`scripts/make_new_lint.sh`](https://github.com/obi1kenobi/cargo-s
 
 Now it's time to fill in these files!
 - Define the lint in `src/lints/<lint_name>.ron`.
-- Make sure your lint outputs `span_filename` and `span_begin_line` for it
-  to be a valid lint. The pattern we commonly use is:
+- For almost all lints, make sure your lint outputs `span_filename` and `span_begin_line`
+  in order to be a valid lint. The pattern we commonly use is:
   ```
   span_: span @optional {
     filename @output
     begin_line @output
   }
   ```
+  <details><summary>Exception: lints over Cargo.toml information (click to expand)</summary>If you are writing a lint over manifest (<code>Cargo.toml</code>) information such as "a feature was deleted," you won't be able to find span data from the manifest file. To proceed, pick a value unique to each result produced by your lint query and output it as <code>ordering_key</code> instead.</details>
+
 - Demonstrate the semver issue your lint is looking for by adding suitable code in
   the `test_crates/<lint_name>/old` and `test_crates/<lint_name>/new` crates.
 - Add code to the test crates that aims to catch for false-positives
@@ -269,8 +271,8 @@ Inspect the generated "actual" output in the `.snap.new` file:
   If so, ensure the reported code is indeed violating semver and is not being flagged
   by any other lint.
 
-If everything looks okay, either run `cargo insta review` (see 
-the [snapshot instructions](#what-are-those-snap-or-snapnew-files-generated-via-cargo-test) 
+If everything looks okay, either run `cargo insta review` (see
+the [snapshot instructions](#what-are-those-snap-or-snapnew-files-generated-via-cargo-test)
 for context) or manually move `test_outputs/query_execution/<lint_name>.snap.new`
 to  `test_outputs/query_execution/<lint_name>.snap`.
 Then re-run `cargo test` and make sure everything passes.
@@ -342,7 +344,7 @@ to update the test outputs.
 > It may contain output for other test crates — this is not necessarily an error:
 > See the [troubleshooting section](#troubleshooting) for more info.
 
-To update the output, please refer to the section 
+To update the output, please refer to the section
 on [snapshot testing](#what-are-those-snap-or-snapnew-files-generated-via-cargo-test)
 
 Once you've update the test output, run `cargo test` again and the `<lint_name>` test should pass!
@@ -356,40 +358,40 @@ otherwise the test will fail in CI.
 ### Troubleshooting
 
 - <details><summary>A valid query must output <code>span_filename</code> and/or <code>span_begin_line</code> (click to expand)</summary>
-  
+
   If your lint fails with an error similar to the following:
   ```
   ---- query::tests_lints::enum_missing stdout ----
   thread 'query::tests_lints::enum_missing' panicked at 'A valid query must output both `span_filename` and `span_begin_line`. See https://github.com/obi1kenobi/cargo-semver-checks/blob/main/CONTRIBUTING.md for how to do this.', src/query.rs:395:26
   note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
   ```
-  
+
   It likely means that your lint does not specify the `span_filename` and `span_begin_line` of where the error occurs. To fix this, add the following to the part of query that catches the error:
   ```
   span_: span @optional {
     filename @output
     begin_line @output
   }
   ```
-  
+
   </details>
 - <details><summary>Other lints' tests failed too (click to expand)</summary>
 
   This is not always a problem! In process of testing a lint, it's frequently desirable to include
   test code that contains a related semver issue in order to ensure the lint differentiates between
   them.
-  
+
   For example, say one is testing a lint for pub field removals from a struct. Its test crate code
 may then include removals of the entire struct, in order to make sure that the lint *does not*
 report those. But those struct removals *will* get reported by the lint that looks for semver
 violations due to struct removal!
-  
+
   So if you added code to a test crate, and it caused other lints to report new findings, consider:
   - whether your code indeed contains the reported semver issue;
   - whether the same semver issue is being reported only once, and not multiple times
     by different lints,
   - and whether the new reported lint result points to the correct item and span information.
-  
+
   If the answer to all is yes, then everything is fine! Just edit those other lints'
 expected output files to include the new items, and you can get back on track.
   </details>