obi1kenobi / cargo-semver-checks

Scan your Rust crate for semver violations.
Apache License 2.0
1.18k stars 75 forks source link

Add more witness hints to currently-existing lints. #937

Open suaviloquence opened 1 month ago

suaviloquence commented 1 month ago

Describe your use case

With the new unstable flag cargo semver-checks -Z unstable-options --witness-hints, cargo-semver-checks is now capable of printing 1-3 line witness hints that give a brief example of how downstream breakage could occur for each instance of a lint finding breakage of a SemVer guarantee.

For example, with the function_missing lint:

Failed in:
  function function_missing::pub_use_removed_fn, previously in file /home/m/dev/cargo-semver-checks/test_crates/function_missing/old/src/lib.rs:4
note: downstream code similar to the following would break:
function_missing::pub_use_removed_fn(...);

  function function_missing::will_be_removed_fn, previously in file /home/m/dev/cargo-semver-checks/test_crates/function_missing/old/src/lib.rs:1
note: downstream code similar to the following would break:
function_missing::will_be_removed_fn(...);

Describe the solution you'd like

For each of the currently existing lints in cargo-semver-checks, we want to either add a witness hint or determine that we can't write one.

The process for adding a witness hint is in the contributor docs. Briefly, once you've determined that you can write a witness hint, add a witness key to the <lint_name>.ron file:

SemverQuery(
     // --- rest of lint declaration --- 
    witness: (
         hint_template: "write the hint here as a {{handlebars}} template",
    ),
)

More details can be found in the CONTRIBUTING.md section linked above.

If it's not possible to write a witness hint, it would be great to explicitly set witness: None in the <lint_name>.ron file, accompanied by a comment explaining why it is not (currently) possible to write a hint.

Alternatives, if applicable

No response

Additional Context

This is a new feature, and if any questions or bugs come up when adding a witness hint, please ask me @suaviloquence so we can help improve the docs and contributor experience for the future!

suaviloquence commented 1 month ago

@obi1kenobi I think it would be helpful to call for contributors here, do you see any issues with this/have any objections before I send it out?

obi1kenobi commented 1 month ago

Not opposed to it, just curious about your thoughts in two areas:

Is it worth attempting to add both the hint and the full witness at the same time, as opposed to doing it in two passes? Should we first attempt to validate the full witness code, and then visually inspect the hint for "sufficient similarity" to the validated witness so as to be confident in its correctness? Or do we feel that the risk of incorrect witness hints is low enough for this to be fine?

Should we first iterate on the CLI to make the output look nicer and provide stronger motivation for adding these hints? For example, right now the indentation is very confusing since it goes from function XYZ two-space indented, to a non-indented note: which is then followed by an also-non-indented code example. Then follows a blank line (the same as after a lint's results are done printing), and yet another indented function XYZ line seemingly out of nowhere. This isn't ideal.

suaviloquence commented 1 month ago

Should we first iterate on the CLI to make the output look nicer and provide stronger motivation for adding these hints? For example, right now the indentation is very confusing since it goes from function XYZ two-space indented, to a non-indented note: which is then followed by an also-non-indented code example. Then follows a blank line (the same as after a lint's results are done printing), and yet another indented function XYZ line seemingly out of nowhere. This isn't ideal.

I can definitely work on this first.

Is it worth attempting to add both the hint and the full witness at the same time, as opposed to doing it in two passes?

Since every full witness needs a hint, but not every lint with a hint will have a full witness, I was thinking it makes sense to see which lints we can write the hints for first, which can serve as a starting point to writing the full witnesses once those are implemented.

obi1kenobi commented 1 month ago

Makes sense, let's do that!

It might be interesting to examine how rustc and clippy point out issues about code: both real code in file and hypothetical code that might be affected. In an ideal world, we'd match their style as much as it makes sense to do so, since UI familiarity and consistency are good user experience.