Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
suaviloquence opened this issue Sep 19, 2024 · 4 comments
Open

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

suaviloquence opened this issue Sep 19, 2024 · 4 comments
Labels
C-enhancement Category: raise the bar on expectations

Comments

@suaviloquence
Copy link
Contributor

suaviloquence commented Sep 19, 2024

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 suaviloquence added the C-enhancement Category: raise the bar on expectations label Sep 19, 2024
@suaviloquence
Copy link
Contributor Author

@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
Copy link
Owner

obi1kenobi commented Sep 19, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: raise the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants