add witness template to lints #893

suaviloquence · 2024-08-29T18:03:52Z

This adds a new optional field witness_template to each lint that can be rendered to make a minimal downstream witness package to show how this change could break downstream code.

Design questions:

What verbosity level should this be printed for. Do we want an opt-in --witness flag or something similar?
Similarly, do we need to make witness code for every instance of a lint occurring? Or just the first?
How can we test the correctness of the witness code?


$ cargo r -- semver-checks --manifest-path new/ --baseline-root old
   Compiling cargo-semver-checks v0.34.0 (/home/m/dev/cargo-semver-checks)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 7.08s
     Running `/home/m/dev/cargo-semver-checks/target/debug/cargo-semver-checks semver-checks --manifest-path new/ --baseline-root old`
     Parsing function_missing v0.1.0 (current)
      Parsed [   0.251s] (current)
     Parsing function_missing v0.1.0 (baseline)
      Parsed [   0.253s] (baseline)
    Checking function_missing v0.1.0 -> v0.1.0 (no change)
     Checked [   0.004s] 81 checks: 80 pass, 1 fail, 0 warn, 0 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/function_missing.ron

Failed in:
  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: the following downstream code would break:
use function_missing::will_be_removed_fn;
will_be_removed_fn(...);
  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: the following downstream code would break:
use function_missing::pub_use_removed_fn;
pub_use_removed_fn(...);

Todo list for me:

add a default None field on every existing lint (didn't want to blow up git history just yet, it currently works with #[serde(default)]
add to make_new_lint.sh - should this be Some("TODO") or None - not every lint may have a possible witness
I was able to put together syntax highlighting using the library portion of bat like cargo-expand - branch link. Do we want this? It adds some dependencies.
of course, add more witness templates (this can be a good-first-issue)

obi1kenobi · 2024-08-29T18:30:40Z

Let's keep #[serde(default)] on the new field for now. I think it'll let us be a bit more free to change the representation inside the lints as we evolve how the witnesses get generated.

While generating witness code, we may need to run an additional query to collect contextual information that is only applicable to the witness. For example, say we claim that a function definition has gained an extra argument. The witness generator needs to know:

whether the function in question is generic
the names and types of the previous arguments
whether the function has a return value or not (to avoid a "must use" lint on the witness)

A valid witness may look like this:

fn witness<T: Debug>(arg_1: i64, arg_2: std::sync::Arc<T>) {
    let _ = broken_function(arg_1, arg_2);
}

or it may look like this:

fn witness() {
    broken_function();
}

and we need the machinery to distinguish between the two.

What verbosity level should this be printed for. Do we want an opt-in --witness flag or something similar?

To start, yes, I think this should be a separate, explicitly permanently-unstable opt-in flag. We can tackle stabilizing it in the future, once we're happy with it.

Do we need to make witness code for every instance of a lint occurring? Or just the first?

Great question. I'm not sure. We'll have to come up with a few different UI layouts, then see which works best with our users.

How can we test the correctness of the witness code?

Ideally, we should aim to generate valid Rust that we can compile and prove gets affected by the breakage we identified (e.g. it produces a compile error, or triggers new UB, or some other criterion we can readily point to).

Like I mentioned earlier, the witness and the "hint" we show in the UI don't have to be the same. Once we're generating full witnesses, we should have a way to dump them into files/temp projects individually, and have rustc validate the expected breakage does indeed occur.

This would allow us to replicate the SemVer study from last year, at hopefully a fraction of the human labor cost.

obi1kenobi · 2024-08-29T18:33:58Z

src/lints/function_missing.ron

@@ -46,4 +46,8 @@ SemverQuery(
    },
    error_message: "A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.",
    per_result_error_template: Some("function {{join \"::\" path}}, previously in file {{span_filename}}:{{span_begin_line}}"),
+    witness_template: Some(


I think this is a fine starting point, but I wouldn't call this a witness by itself since it won't compile. It's more of a hint toward one. So maybe hint_template or something similar would be a more appropriate name?

I think eventually we'll want to expand upon this to make it easier to generate full witnesses that can generate real working code. While we don't have to implement all that today, it would be lovely to have a sense of the broader design within which today's changes should fit.

For example, I'd recommend adding an entire struct here, with the hint being just one of its fields.

I think this is a fine starting point, but I wouldn't call this a witness by itself since it won't compile. It's more of a hint toward one. So maybe hint_template or something similar would be a more appropriate name?

It's not a witness because it's not an item, so it just needs to be wrapped in a function in this case? What other properties would a witness need to have?

It also elides the arguments to the function by making the function call will_be_removed_fn(...);

The tension here is this:

The full witness is needed since it's the ultimate correctness check.

But the full witness is usually too onerous and heavyweight to show to users inline with regular CLI output. A rustdoc-style (not wrapped in a function, etc.) hint for how the witness works would be preferable in the CLI.

The CLI output could also provide text to the effect of "we generated the full witness in this temp file, you can go check it out if you want to verify it's valid."

obi1kenobi · 2024-08-29T18:36:44Z

Zooming out — I'm very excited to be moving toward making it possible to produce witness code here!

A lot of the rules we check are obscure and complex, and it's not unreasonable to assume that for some of our users, our lints would be the first time they encounter some of those concepts. Saying "your trait isn't object safe" is much more opaque and less user-friendly than saying "look, dyn YourTrait won't work anymore."

So I love this, and I'm excited to see it move forward!

suaviloquence · 2024-08-29T19:16:43Z

Not at computer right now, but something like:

#[derive(Default)]
struct Witness {
  hint_template: Option<String>,
  witness_template: Option<String>,
  witness_query: Option<Query>
}

struct Query {
  query: String,
  values: BTreeMap<String, String>
}

(names notwithstanding and not sure of the value type for query.values).

where if the query is Some, we run it on the crate diff and provide the values to the witness_template.

obi1kenobi · 2024-08-29T19:30:31Z

I think that's a great place to start. I'd rename values to match the name in the lint query itself, and change its type to BTreeMap<String, FieldValue> to reflect all possible types that could go there. Otherwise, it's great!

The one other thing we should consider is that right now, there are cases where we won't be able to generate a witness. For example, for calls to functions whose args changed, we can't currently generate a witness if the function had any arguments (we can't look up their types), or was generic.

It would be good to figure out how the "we can't generate a witness in this specific case" gets detected, acted upon, and propagated.

suaviloquence · 2024-08-30T19:50:19Z

Updated struct and documented the members. I used TransparentValue instead of FieldValue like in SemverQuery.arguments so we don't have to specify types in the serialization.

Design questions that came up, particularly about the additional query:

Should hint_template also be provided the additional query? (I think it would be good for efficiency if we could write hints such that they don't need additional parameters, but I don't know if that's possible).
Should the additional query itself be provided the results of the original SemverQuery? I would say yes, because otherwise we would have to run each query twice when there is a witness query, but this may complicate things e.g., if the witness query does not use all the @output directives (I know this is an error if the SemverQueries have unused arguments, but I don't know where this validation occurs)
How should @outputs be handled when there is one of the same name in the original query and in the additional query. It could be an error, or the additional query could take precedence (I wrote the latter in the docs, but we could also do the other one).
When should we generate witnesses? Should c-s-c show hints by default but opt-in to generate (and test compilation of?) full witness programs? This is my initial choice, but it would depend on whether we run additional queries for hints, because we don't want to slow down c-s-c by default.

obi1kenobi

Good call on using TransparentValue instead!

Should hint_template also be provided the additional query? (I think it would be good for efficiency if we could write hints such that they don't need additional parameters, but I don't know if that's possible).

I'd argue "no" here. If the hint needs something else output, the main query should output it.

Should the additional query itself be provided the results of the original SemverQuery? I would say yes, because otherwise we would have to run each query twice when there is a witness query, but this may complicate things e.g., if the witness query does not use all the @output directives (I know this is an error if the SemverQueries have unused arguments, but I don't know where this validation occurs)

Probably yes. But it might be good to be explicit about which results from the original query it wants.

Perhaps with some serde magic we could specify the query arguments like:

{
    "some_var": "custom value",
    "inherited": Inherit("output_from_other_query"),
}

What do you think about this approach, assuming it's doable? If you think it's good, let's see if it can be done in reasonable fashion.

How should @outputs be handled when there is one of the same name in the original query and in the additional query. It could be an error, or the additional query could take precedence (I wrote the latter in the docs, but we could also do the other one).

Probably safer to make it error, I think. I don't see a lot of value in allowing shadowing, whereas it could result in some unfortunate debugging if it happens by accident.

When should we generate witnesses? Should c-s-c show hints by default but opt-in to generate (and test compilation of?) full witness programs? This is my initial choice, but it would depend on whether we run additional queries for hints, because we don't want to slow down c-s-c by default.

Showing hints by default and generating/testing witnesses when explicitly asked sounds right to me.

src/check_release.rs

obi1kenobi · 2024-08-30T20:42:41Z

src/query.rs

+    /// more information.  Witness implements `Default` for a query that doesn't generate any
+    /// witness code.
+    #[serde(default)]
+    pub witness: Witness,


Couldn't we keep serde(default) and make this Option<Witness>. Otherwise we might end up in a weird situation where we have no hint text (so nothing to show the user) even though we can generate a full witness example. We'd like weird states like that to be unrepresentable.

I'd make this Option<Witness> then make the hint required but the other fields inside still be Option.

obi1kenobi · 2024-08-30T20:43:14Z

src/query.rs

+    /// Notice how this is not a compilable example, but it provides a distilled hint to the user
+    /// of how downstream code would break with this change.


Your doc comments game continues to be excellent 🚀

obi1kenobi · 2024-08-30T20:46:53Z

src/check_release.rs

+
+            config.log_info(|config| {
+                config.shell_note("the following downstream code would break:")?;
+                writeln!(config.stderr(), "{message}")?;


I'm a bit worried about readability here. It's too early to fuss over spacing and the exact CLI output format, but just wanted to flag this for us to revisit as a polish item before shipping this feature.

In terms of spacing between items, or the messages themselves (or both?) I did get syntax highlighting (on the other branch) working for code snippets so that could help for the snippets themselves.

Spacing, mostly, both between the note and the code, and possible indentation of the hint code. We want to do our best to be readable but concise, and match cargo/rustc style to make merging into cargo easier.

It'll be easier to revisit this later once we have some more witnesses that we're showing. For now, let's leave them hidden behind some permanently-unstable CLI flag (e.g. -Zunstable-options --show-witness-hints) so we can iterate freely?

Perhaps adding the infrastructure for such unstable flags would be a good PR in and of itself?

suaviloquence · 2024-08-30T22:42:48Z

Added InheritedValue which (should) deserialize Inherited("...") as Inherited(...) and other values as TransparentValues. I don't know if the witness query itself should go in this PR, but I can add a test to make sure deserialization works.

I'm renaming the Query to WitnessQuery because it now only takes InheritedValues, but it is also an option to make it Query<V> where V is any value type.

obi1kenobi

I like this a lot, well done! 🚀 I think it's safe to start working on a few more witnesses for different lints as stacked PRs, since I don't think our lint file's format will change much from here.

I'm planning to cut a new release next week, and it would be a bit safer if we hid this feature behind a permanently-unstable flag similar to how cargo/rustdoc/rustc etc. hide work-in-progress features. Would you be open to setting up a similar mechanism for us? We'd probably want to mirror their approaches quite closely, down to whether the permanently-unstable flags show up in --help output or not.

obi1kenobi · 2024-08-30T22:52:25Z

src/check_release.rs

+
+            config.log_info(|config| {
+                config.shell_note("the following downstream code would break:")?;
+                writeln!(config.stderr(), "{message}")?;


Spacing, mostly, both between the note and the code, and possible indentation of the hint code. We want to do our best to be readable but concise, and match cargo/rustc style to make merging into cargo easier.

It'll be easier to revisit this later once we have some more witnesses that we're showing. For now, let's leave them hidden behind some permanently-unstable CLI flag (e.g. -Zunstable-options --show-witness-hints) so we can iterate freely?

Perhaps adding the infrastructure for such unstable flags would be a good PR in and of itself?

obi1kenobi · 2024-08-30T22:53:00Z

src/lints/function_missing.ron

@@ -46,10 +46,9 @@ SemverQuery(
    },
    error_message: "A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.",
    per_result_error_template: Some("function {{join \"::\" path}}, previously in file {{span_filename}}:{{span_begin_line}}"),
-    witness: (
-        hint_template: Some(
+    witness: Some((


Could you try serde(flatten) in addition to serde(default) here? It might make the Some unnecessary.

I tried to add it and it broke deserialization of the outer SemverQuery field 🫤

It's possible to use ron's implicit_some option to be able to specify it without the Some, but this would allow (but not require) this behavior on all Option<T> in the SemverQuery. branch link Is that something we would want?

Oh neat, TIL about that extension. I think that's fine, yeah. Maybe good for a separate PR we can merge immediately?

obi1kenobi · 2024-08-30T22:53:35Z

src/query.rs

@@ -257,7 +254,7 @@ pub struct Witness {
    /// Notice how this is not a compilable example, but it provides a distilled hint to the user
    /// of how downstream code would break with this change.
    #[serde(default)]


We might not need #[serde(default)] here since this should always be specified (given the outer option wasn't None) I think, right?

obi1kenobi · 2024-08-30T22:54:32Z

src/query.rs

+/// Represents either a value inherited from a previous query, or a
+/// provided constant value.
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub enum InheritedValue {
+    /// Inherit the value from the previous output whose name is the given `String`.
+    Inherited(String),
+    /// Provide the constant value specified here.
+    #[serde(untagged)]
+    Constant(TransparentValue),
 }


Excellent, this is exactly what I was hoping for.

never mind 😅 it seems like ron has known limitations with untagged/deserialize_any and having nested untagged makes it so the deserialization doesn't work 😔. Maybe we have a list (or map of new_name -> old_name) of inherit_arguments along with a map of arguments? Then it's an error if the maps share keys. Alternately, we could remove the untagged so users would have to express arguments as Inherit("...") or Constant(...), which prevents duplicate keys from being expressed but is more verbose and less consistent with the arguments top-level field.

round trip

println!( "{:?}, ron::from_str::<InheritedValue>( &ron::to_string(&InheritedValue::Inherited("abc".into())).unwrap() ), );

Ok(Constant(List([String("abc")])))

Oof, that's unfortunate 😞 What do you think is the better choice between the list/renaming map and the explicit Inherit(..) vs Constant(..)?

If you feel it's worth writing a few more witness sections before deciding, that's fine by me. We can go the easier route now and revisit based on your experience of using it in the real world.

I think I would prefer the explicit one. I also thought of another idea that might work. It's more jsony than rusty but it does remove the requirement to specify Constant(...):

arguments: { "abc": (inherit: "abc"), "string": "literal_string", "int": -30, }

where we specify inheriting as a one-field struct (inherit: "name") or verbosely, Inherited(inherit: "name"). How do you feel about this. It won't have any conflicts (I think) with TransparentValue because maps and structs in ron have different syntax, so Inherit and Constant variants of the enum should be disjoint.

I pushed a test that shows that the deserialization is what we expect.

obi1kenobi · 2024-08-31T18:19:11Z

That seems reasonable! Can you just check that lists work too?

…

On Sat, Aug 31, 2024, 2:16 PM Max Carr ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/query.rs <#893 (comment)> : > +/// Represents either a value inherited from a previous query, or a +/// provided constant value. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum InheritedValue { + /// Inherit the value from the previous output whose name is the given `String`. + Inherited(String), + /// Provide the constant value specified here. + #[serde(untagged)] + Constant(TransparentValue), } I pushed a test that shows that the deserialization is what we expect. — Reply to this email directly, view it on GitHub <#893 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5MSXXDGJMZI3V3V5FWHDZUICATAVCNFSM6AAAAABNK6B6Z2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENZUGEYTCMRWHE> . You are receiving this because you commented.Message ID: ***@***.***>

suaviloquence · 2024-08-31T18:37:30Z

Added list to test and it works! 👍

obi1kenobi · 2024-09-01T13:39:15Z

src/query.rs

@@ -800,6 +799,7 @@ mod tests {
                "abc": (inherit: "abc"),
                "string": "literal_string",
                "int": -30,
+                "list": [-30, -2, "abc"],


Heterogeneous lists like this aren't something Trustfall supports. This test case might make future refactoring harder if e.g. the TransparentValue type changes in a way that disallows that at the type level — the person doing it will have to also fix this test case too. It's usually not a good idea to exceed the bounds of the spec, because if the implementation of the spec starts enforcing the bounds more strongly then breakage ensues.

I'd recommend removing the string here, and making a separate list entry with a string or two to test strings inside the list.

obi1kenobi · 2024-09-01T13:40:55Z

All looks good other than the tiny nitpick on the test structure!

Btw, is the failing test accurate in saying that we print the error template to stdout and the hint to stderr? That seems strange and probably not desirable. Might be time to look into getting our use of stdout/stderr to be more consistent, or at least maybe introducing the ability to print notes to either stdout/stderr as needed in the caller's context.

suaviloquence · 2024-09-01T19:07:14Z

Rebased on #897 and fixed test.

suaviloquence · 2024-09-01T19:13:08Z

Btw, is the failing test accurate in saying that we print the error template to stdout and the hint to stderr? That seems strange and probably not desirable. Might be time to look into getting our use of stdout/stderr to be more consistent, or at least maybe introducing the ability to print notes to either stdout/stderr as needed in the caller's context.

I think any of the shell_* only prints to stderr. AFAIK, it's just the lint output that goes to stdout. Also, should the tests have witness hints while it's behind a #896 feature gate? That's why I haven't updated the outputs yet.

obi1kenobi · 2024-09-01T20:23:19Z

I think any of the shell_* only prints to stderr. AFAIK, it's just the lint output that goes to stdout.

I think this is the first time we've used note: styled output on something that's relevant to stdout. Previously it was just for telling people that c-s-c failed to run for some reason, for which stderr was probably the right output.

I think it's confusing to direct the witness to stderr while the lint output is on stdout. I think the witness info should be on stdout too, so the user doesn't have to figure out how to interleave the stdout/stderr outputs together to figure out what each witness was referring to.

Also, should the tests have witness hints while it's behind a #896 feature gate? That's why I haven't updated the outputs yet.

Probably a good idea to test with the witnesses present, even though it's an unstable feature. We still want to know if we broke something, or else we'll have a really hard time stabilizing it.

- update behavior for stable flags and test - move bugreport action to the top - add test that all unstable features are hidden

Co-authored-by: Predrag Gruevski <[email protected]>

suaviloquence · 2024-09-06T22:03:11Z

Added configuration to indicate in a Check whether to print witnesses (the WitnessGeneration struct).

Todos:

make the note print to stdout (might have to manually impl it)
set up some sort of test harness per lint that checks if witness is Some, then loads a test_outputs/witnesses/{query_name}.ron and generates witness code/hints and compares expected == actual

suaviloquence · 2024-09-10T19:07:33Z

Added test harness and moved note to stdout, so this should be ready for review.

suaviloquence · 2024-09-10T19:09:33Z

test_outputs/witnesses/function_missing.output.ron

+        (
+            filename: "src/lib.rs",
+            begin_line: 4,
+            hint: "use feature_flags_validation::foo_becomes_gated;\nfoo_becomes_gated(...);",


it's mentioned in the comment in the test code, but prettier (multiline) strings are blocked on the release of ron v0.9 (which although there is an alpha, doesn't seem to be happening anytime soon due to maintainer overburden). it may be worth (albeit hacky), doing a str.replace("\\n", "\n") after serializing to a string for better diffs.

suaviloquence · 2024-09-10T19:11:04Z

src/query.rs

+                                    filename: filename.to_string(),
+                                    begin_line,
+                                    hint: registry
+                                        .render_template(&witness.hint_template, values)


once we have the infrastructure to run witness queries and render full witnesses, code here should call an encapsulated version of that logic to generate the full witness here, if it is Some.

Added a TODO note to our future selves about it.

suaviloquence · 2024-09-10T19:11:52Z

src/main.rs

@@ -627,6 +627,10 @@ impl From<CheckRelease> for cargo_semver_checks::Check {
            check.set_build_target(build_target);
        }

+        let mut witness_generation = WitnessGeneration::new();
+        witness_generation.show_hints = value.unstable_options.witness_hints;


we could also add setters for these instead of pub fields

I don't have strong feelings about it, this is fine for now.

suaviloquence · 2024-09-10T19:14:40Z

src/check_release.rs

+                        .bold();
+                    writeln!(
+                        config.stdout(),
+                        "{note}note:{note:#} downstream code similar to the following would break:\n\


here i just inlined the note code. we can't have a method in GlobalConfig that takes an impl Write and renders a note/error/warning/etc., because .stdout/.stderr mutably borrows the whole GlobalConfig struct. we could:

make (associated) functions

implement them on the streams themselves (using a newtype)

revisit using a crate like annotate-snippets to render for us

make macros

if we want a longer-term solution, if you think it is necessary.

Revisiting with something like annotate-snippets down the line would be great. This is a reasonable solution for the meantime — it doesn't make sense to hold up witness generation over CLI formatting.

suaviloquence · 2024-09-17T23:06:27Z

@obi1kenobi btw this one is ready for review when you get a chance.

obi1kenobi

Looks good! Making some trivial tweaks and merging. Also leaving an action item for you for a subsequent PR on updating contributing docs to explain the witness section.

obi1kenobi · 2024-09-18T14:34:01Z

src/check_release.rs

+                        .bold();
+                    writeln!(
+                        config.stdout(),
+                        "{note}note:{note:#} downstream code similar to the following would break:\n\


Revisiting with something like annotate-snippets down the line would be great. This is a reasonable solution for the meantime — it doesn't make sense to hold up witness generation over CLI formatting.

src/lib.rs

obi1kenobi · 2024-09-18T14:35:21Z

src/main.rs

@@ -627,6 +627,10 @@ impl From<CheckRelease> for cargo_semver_checks::Check {
            check.set_build_target(build_target);
        }

+        let mut witness_generation = WitnessGeneration::new();
+        witness_generation.show_hints = value.unstable_options.witness_hints;


I don't have strong feelings about it, this is fine for now.

src/query.rs

obi1kenobi · 2024-09-18T14:50:22Z

src/query.rs

+                .with_context(|| format!("Query {query_name} has a witness template, but could not load\n\
+                    `test_outputs/witnesses/{query_name}.output.ron` expected witness file, did you forget to add it?"))


In a separate PR, could you amend our contributing docs for adding new lints to add an optional step for "add a witness" and add a small section explaining how to do it?

src/query.rs

obi1kenobi · 2024-09-18T14:58:02Z

I was able to put together syntax highlighting using the library portion of bat like cargo-expand - branch link. Do we want this? It adds some dependencies.

I'd love to see a before-and-after screenshot of the difference if you get a chance! I'm open to adding a dependency if it results in notable improvement in user experience, and syntax highlighting certainly could be it.

obi1kenobi · 2024-09-18T17:01:11Z

Ah unfortunately this has a merge conflict due to the priority logic for lints that merged yesterday. Sorry about that!

suaviloquence force-pushed the witness branch from ad7a94f to 6a06964 Compare August 29, 2024 18:05

obi1kenobi reviewed Aug 29, 2024

View reviewed changes

obi1kenobi reviewed Aug 30, 2024

View reviewed changes

obi1kenobi reviewed Sep 1, 2024

View reviewed changes

suaviloquence mentioned this pull request Sep 1, 2024

enable implicit some feature on query rons. #897

Merged

suaviloquence force-pushed the witness branch from c0d11b5 to 890dd41 Compare September 1, 2024 19:06

suaviloquence marked this pull request as ready for review September 1, 2024 19:07

suaviloquence marked this pull request as draft September 1, 2024 19:10

suaviloquence and others added 9 commits September 3, 2024 10:13

add test and output

4428d8a

unstable feature flags and options

31cb09e

clippy :(

f81db7d

update featureflags, add docs, and...

64312a5

- update behavior for stable flags and test - move bugreport action to the top - add test that all unstable features are hidden

fmt docstring

d63647f

better validation for unstable options

9fc88d5

annotate code paths and consolidate helps

d869817

use stdout

269f70e

error consistency

462be7c

Co-authored-by: Predrag Gruevski <[email protected]>

suaviloquence added 6 commits September 4, 2024 21:03

you didnt see that

8540634

Merge branch 'main' into witness

d5ce8e0

config-gate witness generation

6ff3a9e

Merge branch 'main' into witness

ac35d47

Merge branch 'main' into witness

4d122d4

bless tests

9c01217

suaviloquence force-pushed the witness branch from 8824224 to 9c01217 Compare September 6, 2024 21:58

suaviloquence added 5 commits September 6, 2024 15:06

fix doctests

d29b3dd

doc fix

7f0c21d

test harness

e59673a

print note to stdout

796f3be

Merge branch 'main' into witness

4262323

suaviloquence marked this pull request as ready for review September 10, 2024 19:06

suaviloquence commented Sep 10, 2024

View reviewed changes

Merge branch 'main' into witness

c3473e1

suaviloquence changed the title ~~Draft: add witness template to lints~~ add witness template to lints Sep 17, 2024

obi1kenobi approved these changes Sep 18, 2024

View reviewed changes

obi1kenobi reviewed Sep 18, 2024

View reviewed changes

src/query.rs Show resolved Hide resolved

Apply suggestions from code review

84de46a

Merge branch 'main' into witness

05f70de

obi1kenobi approved these changes Sep 18, 2024

View reviewed changes

obi1kenobi merged commit a056a5a into obi1kenobi:main Sep 18, 2024
34 checks passed

		/// Notice how this is not a compilable example, but it provides a distilled hint to the user
		/// of how downstream code would break with this change.

		.with_context(\|\| format!("Query {query_name} has a witness template, but could not load\n\
		`test_outputs/witnesses/{query_name}.output.ron` expected witness file, did you forget to add it?"))

add witness template to lints #893

add witness template to lints #893

Conversation

suaviloquence commented Aug 29, 2024

obi1kenobi commented Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi commented Aug 29, 2024

suaviloquence commented Aug 29, 2024

obi1kenobi commented Aug 29, 2024 • edited Loading

suaviloquence commented Aug 30, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suaviloquence commented Aug 30, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suaviloquence Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suaviloquence Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi commented Aug 31, 2024 via email

suaviloquence commented Aug 31, 2024

Choose a reason for hiding this comment

obi1kenobi commented Sep 1, 2024 • edited Loading

suaviloquence commented Sep 1, 2024

suaviloquence commented Sep 1, 2024

obi1kenobi commented Sep 1, 2024

suaviloquence commented Sep 6, 2024 • edited Loading

suaviloquence commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suaviloquence commented Sep 17, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi commented Sep 18, 2024

obi1kenobi commented Sep 18, 2024

obi1kenobi commented Aug 29, 2024 •

edited

Loading

obi1kenobi commented Aug 29, 2024 •

edited

Loading

suaviloquence Aug 31, 2024 •

edited

Loading

suaviloquence Aug 31, 2024 •

edited

Loading

obi1kenobi commented Sep 1, 2024 •

edited

Loading

suaviloquence commented Sep 6, 2024 •

edited

Loading