From d375bedd000e984b74ef8dfeb66f3d26db443ecd Mon Sep 17 00:00:00 2001 From: m Date: Wed, 18 Sep 2024 13:52:56 -0700 Subject: [PATCH 1/7] add doc for adding witness templates --- CONTRIBUTING.md | 118 +++++++++++++++++++++++++++++++++++++++ scripts/make_new_lint.sh | 3 + 2 files changed, 121 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c7ff1eba..c5ef544e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -210,6 +210,124 @@ the "actual" output, then re-run `cargo test` and make sure everything passes. Congrats on the new lint! +#### Adding a witness + +**Witnesses** are a new, unstable feature of `cargo-semver-checks` that let us create witness +code, a minimal compilable example of potential downstream breakage\. They are configured via the +`witness` field in the lint file `src/lints/.ron`: + +If it is `None` (or the field is omitted entirely), `cargo-semver-checks` will not be able +to generate witness code for this lint\. This can be the right choice, sometimes: +for example, if this lint is a warn- or allow-by-default lint that hints at potential +breakage, but won't cause breaking changes directly\. Additionally, if it's not currently +possible to write a Trustfall query that gets the necessary information to generate +witnesses, leave this field as `None`, but leave a `// TODO` comment explaining +what would unblock this lint from being able to generate a witness. + +##### Hint templates + +When the `witness` field is not `None`, it must have the `hint_template` field\. This is a +`handlebars` template that generates a small (1-3 line) human-readable message that +explains the idea of how downstream code would break\. For example, for the `function_missing` lint: + +```rust +use {{join "::" path}; +{{name}}(...); +``` + +which could render to something like: + +```ron +witness: ( + hint_template: r#"use function_missing::will_be_removed_fn;\n\ + will_be_removed_fn(...);"# +) +``` + +A witness hint like this may not be buildable (e.g., we elide the function arguments here, +and the function call is not inside a block), but a witness hint should be a distilled +example of breakage, and should not require extra information beyond the original +lint's query. + +##### Templating + +We use the `handlebars` crate for writing these templates\. [More information +more information about the syntax can be found here](https://handlebarsjs.com/guide/#simple-expressions), +and [here is where `cargo-semver-checks` defines custom helpers +](https://github.com/obi1kenobi/cargo-semver-checks/blob/main/src/templating.rs). + +All fields marked with `@output` in the `query` in `.ron` are available +to access with `{{name}}` in the `hint_template`, like in the example above. + +##### Testing + +When the `witness` field is not `None`, `cargo-semver-checks` tests the witness generation +of the lint similarly to how it tests the `query` itself\. If you run `cargo test` after adding +a witness for the first time, the test will fail, because it expects a file containing the +expected witness output. + +To solve this, create a file called `test_outputs/witnesses/.output.ron`. Make the +contents `{}` currently, but we will change this\. Run `cargo test` again\. The test +`` should fail (because we told it we expect no witnesses to be generated, +and this is no longer true). + +The failed test should output a message like: + +``` +---- query::tests_lints::function_missing stdout ---- +--- actual output --- +{ + // cut down for readibility + "./test_crates/function_missing/": [ + ( + filename: "src/lib.rs", + begin_line: 1, + hint: "use function_missing::will_be_removed_fn;\nwill_be_removed_fn(...);", + ), + ( + filename: "src/lib.rs", + begin_line: 4, + hint: "use function_missing::pub_use_removed_fn;\npub_use_removed_fn(...);", + ), + ], +} +thread 'query::tests_lints::function_missing' panicked at src/query.rs:751:17: +Witness output for function_missing did not match expected values: +Differences (-expected|+actual): +-{} ++{ ++ // cut down for readibility ++ "./test_crates/function_missing/": [ ++ ( ++ filename: "src/lib.rs", ++ begin_line: 1, ++ hint: "use function_missing::will_be_removed_fn;\nwill_be_removed_fn(...);", ++ ), ++ ( ++ filename: "src/lib.rs", ++ begin_line: 4, ++ hint: "use function_missing::pub_use_removed_fn;\npub_use_removed_fn(...);", ++ ), ++ ], ++} + +Update the `test_outputs/witnesses/function_missing.output.ron` file if + the new test results are correct. +``` + +Check the actual result under `--- actual ---` and make sure that it outputted +the correct witness hints\. Note that it may contain output for other test crates - this +not necessarily an error: see the troubleshooting section for more info\. Once you've +verified that the `--- actual ---` results are as expected, copy the actual results +into the `test_outputs/witnesses/.output.ron` file and save\. Running +`cargo test` should pass the `` test now\. **Make sure to commit and push the +`test_outputs/witnesses/.output.ron` into git**, otherwise the test will +fail in CI. + +##### Full witness templates + +*TODO: @suaviloquence will write this once the feature has been implemented.* + ### Troubleshooting #### A valid query must output span_filename and/or span_begin_line If your lint fails with an error similar to the following: diff --git a/scripts/make_new_lint.sh b/scripts/make_new_lint.sh index 63474c08..77ca9e41 100755 --- a/scripts/make_new_lint.sh +++ b/scripts/make_new_lint.sh @@ -52,6 +52,9 @@ SemverQuery( }, error_message: "TODO", per_result_error_template: Some("TODO"), + // TODO: see https://github.com/obi1kenobi/cargo-semver-checks/blob/main/CONTRIBUTING.md#adding-a-witness + // for information about this field. + witness: None, ) EOF echo ' done!' From da9d35f68b177210e1602fcec244163777c8342c Mon Sep 17 00:00:00 2001 From: Max Carr Date: Wed, 18 Sep 2024 22:20:46 +0000 Subject: [PATCH 2/7] Apply suggestions from code review Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> --- CONTRIBUTING.md | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c5ef544e..f70361e7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -212,8 +212,8 @@ Congrats on the new lint! #### Adding a witness -**Witnesses** are a new, unstable feature of `cargo-semver-checks` that let us create witness -code, a minimal compilable example of potential downstream breakage\. They are configured via the +**Witnesses** are a new, unstable feature of `cargo-semver-checks` that let us create +minimal compile-able examples of potential downstream breakage\. They are configured via the `witness` field in the lint file `src/lints/.ron`: If it is `None` (or the field is omitted entirely), `cargo-semver-checks` will not be able @@ -228,11 +228,14 @@ what would unblock this lint from being able to generate a witness. When the `witness` field is not `None`, it must have the `hint_template` field\. This is a `handlebars` template that generates a small (1-3 line) human-readable message that -explains the idea of how downstream code would break\. For example, for the `function_missing` lint: +explains the idea of how downstream code would break. + +This example code is meant to be small and illustrative, and does not have to pass `cargo check`. It should give the reader a sense of the kind of breakage in one glance. + +For example, for the `function_missing` lint, a witness template may look like this: ```rust -use {{join "::" path}; -{{name}}(...); +{{join "::" path}(...); ``` which could render to something like: @@ -244,22 +247,21 @@ witness: ( ) ``` -A witness hint like this may not be buildable (e.g., we elide the function arguments here, -and the function call is not inside a block), but a witness hint should be a distilled -example of breakage, and should not require extra information beyond the original -lint's query. +This hint will not pass `cargo check` (e.g. the function call arguments are elided), +and that's okay. The hint is a distilled example of breakage, and shouldn't require +additional information beyond what the lint query retrieved. ##### Templating We use the `handlebars` crate for writing these templates\. [More information -more information about the syntax can be found here](https://handlebarsjs.com/guide/#simple-expressions), +about the syntax can be found here](https://handlebarsjs.com/guide/#simple-expressions), and [here is where `cargo-semver-checks` defines custom helpers ](https://github.com/obi1kenobi/cargo-semver-checks/blob/main/src/templating.rs). All fields marked with `@output` in the `query` in `.ron` are available to access with `{{name}}` in the `hint_template`, like in the example above. -##### Testing +##### Testing witnesses When the `witness` field is not `None`, `cargo-semver-checks` tests the witness generation of the lint similarly to how it tests the `query` itself\. If you run `cargo test` after adding @@ -317,7 +319,7 @@ Update the `test_outputs/witnesses/function_missing.output.ron` file if Check the actual result under `--- actual ---` and make sure that it outputted the correct witness hints\. Note that it may contain output for other test crates - this -not necessarily an error: see the troubleshooting section for more info\. Once you've +is not necessarily an error: see the troubleshooting section for more info\. Once you've verified that the `--- actual ---` results are as expected, copy the actual results into the `test_outputs/witnesses/.output.ron` file and save\. Running `cargo test` should pass the `` test now\. **Make sure to commit and push the From d118ea7140990e43c2dea6801b13ef28e8440f5f Mon Sep 17 00:00:00 2001 From: m Date: Wed, 18 Sep 2024 15:24:15 -0700 Subject: [PATCH 3/7] fixes --- CONTRIBUTING.md | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f70361e7..7e04ac5b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -213,20 +213,20 @@ Congrats on the new lint! #### Adding a witness **Witnesses** are a new, unstable feature of `cargo-semver-checks` that let us create -minimal compile-able examples of potential downstream breakage\. They are configured via the +minimal compile-able examples of potential downstream breakage. They are configured via the `witness` field in the lint file `src/lints/.ron`: If it is `None` (or the field is omitted entirely), `cargo-semver-checks` will not be able -to generate witness code for this lint\. This can be the right choice, sometimes: +to generate witness code for this lint. This can be the right choice, sometimes: for example, if this lint is a warn- or allow-by-default lint that hints at potential -breakage, but won't cause breaking changes directly\. Additionally, if it's not currently +breakage, but won't cause breaking changes directly. Additionally, if it's not currently possible to write a Trustfall query that gets the necessary information to generate witnesses, leave this field as `None`, but leave a `// TODO` comment explaining what would unblock this lint from being able to generate a witness. ##### Hint templates -When the `witness` field is not `None`, it must have the `hint_template` field\. This is a +When the `witness` field is not `None`, it must have the `hint_template` field. This is a `handlebars` template that generates a small (1-3 line) human-readable message that explains the idea of how downstream code would break. @@ -234,42 +234,41 @@ This example code is meant to be small and illustrative, and does not have to pa For example, for the `function_missing` lint, a witness template may look like this: -```rust -{{join "::" path}(...); +```ron +witness: ( + hint_template: r#"{{join "::" path}(...);"#, +), ``` which could render to something like: -```ron -witness: ( - hint_template: r#"use function_missing::will_be_removed_fn;\n\ - will_be_removed_fn(...);"# -) +```rust +function_missing::will_be_removed_fn(...); ``` -This hint will not pass `cargo check` (e.g. the function call arguments are elided), +This hint will not pass `cargo check` (e.g. the function call arguments are elided), and that's okay. The hint is a distilled example of breakage, and shouldn't require additional information beyond what the lint query retrieved. ##### Templating -We use the `handlebars` crate for writing these templates\. [More information +We use the `handlebars` crate for writing these templates. [More information about the syntax can be found here](https://handlebarsjs.com/guide/#simple-expressions), and [here is where `cargo-semver-checks` defines custom helpers ](https://github.com/obi1kenobi/cargo-semver-checks/blob/main/src/templating.rs). All fields marked with `@output` in the `query` in `.ron` are available -to access with `{{name}}` in the `hint_template`, like in the example above. +to access with `{{output_name}}` in the `hint_template`, like in the example above. ##### Testing witnesses When the `witness` field is not `None`, `cargo-semver-checks` tests the witness generation -of the lint similarly to how it tests the `query` itself\. If you run `cargo test` after adding +of the lint similarly to how it tests the `query` itself. If you run `cargo test` after adding a witness for the first time, the test will fail, because it expects a file containing the expected witness output. To solve this, create a file called `test_outputs/witnesses/.output.ron`. Make the -contents `{}` currently, but we will change this\. Run `cargo test` again\. The test +contents `{}` currently, but we will change this. Run `cargo test` again. The test `` should fail (because we told it we expect no witnesses to be generated, and this is no longer true). @@ -318,11 +317,11 @@ Update the `test_outputs/witnesses/function_missing.output.ron` file if ``` Check the actual result under `--- actual ---` and make sure that it outputted -the correct witness hints\. Note that it may contain output for other test crates - this -is not necessarily an error: see the troubleshooting section for more info\. Once you've +the correct witness hints. Note that it may contain output for other test crates - this +is not necessarily an error: see the troubleshooting section for more info. Once you've verified that the `--- actual ---` results are as expected, copy the actual results -into the `test_outputs/witnesses/.output.ron` file and save\. Running -`cargo test` should pass the `` test now\. **Make sure to commit and push the +into the `test_outputs/witnesses/.output.ron` file and save. Running +`cargo test` should pass the `` test now. **Make sure to commit and push the `test_outputs/witnesses/.output.ron` into git**, otherwise the test will fail in CI. From e5ac45412389d0f708b471c40b56a936fa3fe6a1 Mon Sep 17 00:00:00 2001 From: m Date: Wed, 18 Sep 2024 15:33:01 -0700 Subject: [PATCH 4/7] update witness examples --- CONTRIBUTING.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7e04ac5b..56fd31ad 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -283,12 +283,12 @@ The failed test should output a message like: ( filename: "src/lib.rs", begin_line: 1, - hint: "use function_missing::will_be_removed_fn;\nwill_be_removed_fn(...);", + hint: "function_missing::will_be_removed_fn(...);", ), ( filename: "src/lib.rs", begin_line: 4, - hint: "use function_missing::pub_use_removed_fn;\npub_use_removed_fn(...);", + hint: "function_missing::pub_use_removed_fn(...);", ), ], } @@ -302,12 +302,12 @@ Differences (-expected|+actual): + ( + filename: "src/lib.rs", + begin_line: 1, -+ hint: "use function_missing::will_be_removed_fn;\nwill_be_removed_fn(...);", ++ hint: "function_missing::will_be_removed_fn(...);", + ), + ( + filename: "src/lib.rs", + begin_line: 4, -+ hint: "use function_missing::pub_use_removed_fn;\npub_use_removed_fn(...);", ++ hint: "function_missing::pub_use_removed_fn(...);", + ), + ], +} From a0de3f67edbfed403f040661217d6baa10587b00 Mon Sep 17 00:00:00 2001 From: m Date: Wed, 18 Sep 2024 17:17:26 -0700 Subject: [PATCH 5/7] update docs to be about insta tests --- CONTRIBUTING.md | 82 +++++++++++++------------------------------------ 1 file changed, 21 insertions(+), 61 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 56fd31ad..8bf7ec6b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -263,67 +263,27 @@ to access with `{{output_name}}` in the `hint_template`, like in the example abo ##### Testing witnesses When the `witness` field is not `None`, `cargo-semver-checks` tests the witness generation -of the lint similarly to how it tests the `query` itself. If you run `cargo test` after adding -a witness for the first time, the test will fail, because it expects a file containing the -expected witness output. - -To solve this, create a file called `test_outputs/witnesses/.output.ron`. Make the -contents `{}` currently, but we will change this. Run `cargo test` again. The test -`` should fail (because we told it we expect no witnesses to be generated, -and this is no longer true). - -The failed test should output a message like: - -``` ----- query::tests_lints::function_missing stdout ---- ---- actual output --- -{ - // cut down for readibility - "./test_crates/function_missing/": [ - ( - filename: "src/lib.rs", - begin_line: 1, - hint: "function_missing::will_be_removed_fn(...);", - ), - ( - filename: "src/lib.rs", - begin_line: 4, - hint: "function_missing::pub_use_removed_fn(...);", - ), - ], -} -thread 'query::tests_lints::function_missing' panicked at src/query.rs:751:17: -Witness output for function_missing did not match expected values: -Differences (-expected|+actual): --{} -+{ -+ // cut down for readibility -+ "./test_crates/function_missing/": [ -+ ( -+ filename: "src/lib.rs", -+ begin_line: 1, -+ hint: "function_missing::will_be_removed_fn(...);", -+ ), -+ ( -+ filename: "src/lib.rs", -+ begin_line: 4, -+ hint: "function_missing::pub_use_removed_fn(...);", -+ ), -+ ], -+} - -Update the `test_outputs/witnesses/function_missing.output.ron` file if - the new test results are correct. -``` - -Check the actual result under `--- actual ---` and make sure that it outputted -the correct witness hints. Note that it may contain output for other test crates - this -is not necessarily an error: see the troubleshooting section for more info. Once you've -verified that the `--- actual ---` results are as expected, copy the actual results -into the `test_outputs/witnesses/.output.ron` file and save. Running -`cargo test` should pass the `` test now. **Make sure to commit and push the -`test_outputs/witnesses/.output.ron` into git**, otherwise the test will -fail in CI. +of the lint similarly to how it tests the `query` itself. After adding a witness for the first +time, run `cargo test` to start generating the snapshots. The first time you run this test, +it will fail, because there's no expected result to compare to. Let's make the test pass: + +We use `insta` for snapshot testing witness results, so after adding/changing a witness, we need +to update the test outputs. Note that it may contain output for other test crates - this +is not necessarily an error: see the troubleshooting section for more info. + +There are two ways to update the output: + +1. **With `cargo insta`**: If you install (or have already installed) the `insta` CLI with + `cargo install --locked cargo-insta`, you can run `cargo insta review`. Check that the + new output is what you expect, and accept it in the TUI. +2. **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/witnesses/.snap.new`. + Open it, and verify that the witnesses generated as expected. Once you've checked it, move it + to `test_outputs/witnesses/.snap` (remove the `.new`) + +Once you've update the test output, run `cargo test` again and the `` test should pass! +**Make sure to commit and push the `test_outputs/witnesses/.output.ron` into git**; +otherwise the test will fail in CI. ##### Full witness templates From c20c5db461b50f09cd40c8e921558abf0347a66c Mon Sep 17 00:00:00 2001 From: m Date: Wed, 18 Sep 2024 17:19:34 -0700 Subject: [PATCH 6/7] fix file name --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8bf7ec6b..687cedf5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -282,7 +282,7 @@ There are two ways to update the output: to `test_outputs/witnesses/.snap` (remove the `.new`) Once you've update the test output, run `cargo test` again and the `` test should pass! -**Make sure to commit and push the `test_outputs/witnesses/.output.ron` into git**; +**Make sure to commit and push the `test_outputs/witnesses/.snap` into git**; otherwise the test will fail in CI. ##### Full witness templates From 2a209bd2facd902de0baf481f444eee13918de9d Mon Sep 17 00:00:00 2001 From: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> Date: Wed, 18 Sep 2024 22:04:38 -0400 Subject: [PATCH 7/7] Update CONTRIBUTING.md --- CONTRIBUTING.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 687cedf5..c8c8ef29 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -230,7 +230,8 @@ When the `witness` field is not `None`, it must have the `hint_template` field. `handlebars` template that generates a small (1-3 line) human-readable message that explains the idea of how downstream code would break. -This example code is meant to be small and illustrative, and does not have to pass `cargo check`. It should give the reader a sense of the kind of breakage in one glance. +This example code is meant to be small and illustrative, and does not have to pass `cargo check`. +It should give the reader a sense of the kind of breakage in one glance. For example, for the `function_missing` lint, a witness template may look like this: