Add enum variant discriminant changed lint #912

dmatos2012 · 2024-09-03T16:49:47Z

Adds lint a public API enum's variant had its discriminant has changed from its previous value from #898.

PS: Hope no typos or spacing issues this time, only technical things. Thanks :)

obi1kenobi · 2024-09-03T18:05:47Z

Oh wow, we ... might have found a breaking change in clap? 😮

@dmatos2012 could you dig into the source code of clap at the version used in CI, and compare it to the previous clap release to see if the enum discriminant value indeed changed?

If it didn't, that sounds like a false-positive we should investigate. If it did, we'll need to adjust what the CI test expects.

dmatos2012 · 2024-09-03T19:48:18Z

@obi1kenobi Indeed, there are bunch of discriminant changes from clap-v3.1.18 to clap-v3.2.0. For example, the first one:

Failed in:
  variant AppSettings::AllowInvalidUtf8ForExternalSubcommands 13 -> 14

#clap-v3.1.18

pub enum AppSettings {
  IgnoreErrors,
  WaitOnError,
  AllowHyphenValues,
  AllowNegativeNumbers,
  AllArgsOverrideSelf,
  AllowMissingPositional,
  TrailingVarArg,
  DontDelimitTrailingValues,
  InferLongArgs,
  InferSubcommands,
  SubcommandRequired,
  SubcommandRequiredElseHelp,
  AllowExternalSubcommands,
  AllowInvalidUtf8ForExternalSubcommands,
}

Then clap-v3.2.0

pub enum AppSettings {
   IgnoreErrors,
   WaitOnError,
   AllowHyphenValues,
   AllowNegativeNumbers,
   AllArgsOverrideSelf,
   AllowMissingPositional,
   TrailingVarArg,
   DontDelimitTrailingValues,
   InferLongArgs,
   InferSubcommands,
   SubcommandRequired,
   SubcommandRequiredElseHelp,
   AllowExternalSubcommands,
   Multicall,
   AllowInvalidUtf8ForExternalSubcommands,
}

So in this case AllowInvalidUtf8ForExternalSubcommands moved from pos 13 to 14 as the magic error indicates:)

Because MultiCall went from :

    #[deprecated(
       since = "3.1.0",
       note = "Replaced with `Command::multicall` and `Command::is_multicall_set`"
   )]
   #[cfg(feature = "unstable-multicall")]
   Multicall,

to

    /// Deprecated, replaced with [`Command::multicall`] and [`Command::is_multicall_set`]
    #[deprecated(
        since = "3.1.0",
        note = "Replaced with `Command::multicall` and `Command::is_multicall_set`"
    )]
    Multicall,

That being said, I guess should I adjust the CI test? I do want to add that this is the sort of things that a human would never pick up on, so seeing that picked up so flawlessly by csc is so amazing, particularly since I am not a user of csc myself so seeing it in action is suuuper cool

TLDR: MultiCall was enabled only under unstable-multicall feature, but its on by default on clap-v3.2.0

obi1kenobi · 2024-09-03T21:49:43Z

Nice, that looks legit.

Do you think you can edit the clap CI test to expect both lints to trigger, or would you like some help/suggestions?

obi1kenobi · 2024-09-03T21:53:16Z

src/lints/enum_variant_discriminant_changed.ron

@@ -0,0 +1,65 @@
+SemverQuery(
+    id: "enum_variant_discriminant_changed",
+    human_readable_name: "Public enum's variant had its discriminant changed from its previous value",


This field is intended to be a name — it's printed in the headline of the CLI diagnostics we emit when breakage is found — so it should be quite short and it's okay if it doesn't have all the details.

Check out how these fields are used in the CLI ("what gets printed where") if you have a moment. I think that'll give you a better sense of what to put in the various fields going forward.

Suggested change

human_readable_name: "Public enum's variant had its discriminant changed from its previous value",

human_readable_name: "enum variant had its discriminant change value",

dmatos2012 · 2024-09-04T21:02:54Z

Applied the change and changed the CI test as well

obi1kenobi · 2024-09-04T22:16:08Z

Just making sure you've seen this comment: #898 (comment)

We should split this lint into two separate lints based on whether there's an ABI concern or just an API concern. This will let us offer specific feedback to the user that is stronger than "may break ABI." That's important because our users likely have a general expectation that cargo-semver-checks should be the expert in most cases, not them. We want to be able to tell them "you broke ABI compatibility" or "you didn't," not "learn about SemVer of variants and discriminants, then decide if you broke the ABI or not."

Sorry I didn't catch this earlier, hope you don't mind making the tweaks!

dmatos2012 · 2024-09-06T21:17:14Z

Yeah no problem, I dont mind making the tweaks. I have changed everything slightly, so it might as well be kinda like a new lint. I hope that I got all the details correctly, and if I am missing something, feedback is welcomed :)

To reiterate, this is targeting a public API enum's variant had its discriminant has changed from its previous value, on the API only brekage(thus no repr). Thank you!

obi1kenobi · 2024-09-06T21:37:43Z

.github/workflows/ci.yml

+          EXPECTED="$(echo -e "--- failure auto_trait_impl_removed: auto trait no longer implemented ---\n--- failure enum_no_repr_variant_discriminant_changed: enum variant had its discriminant change value ---")"
          RESULT="$(cat output | grep failure)"


See if you can squeeze a sort call after grep failure here? I'm not sure if we guarantee (or want to guarantee) a particular order of lint outputs, and flaky tests are annoying so better avoid the risk whenever possible.

Suggested change

EXPECTED="$(echo -e "--- failure auto_trait_impl_removed: auto trait no longer implemented ---\n--- failure enum_no_repr_variant_discriminant_changed: enum variant had its discriminant change value ---")"

RESULT="$(cat output | grep failure)"

EXPECTED="$(echo -e "--- failure auto_trait_impl_removed: auto trait no longer implemented ---\n--- failure enum_no_repr_variant_discriminant_changed: enum variant had its discriminant change value ---")"

RESULT="$(cat output | grep failure | sort)"

obi1kenobi · 2024-09-06T21:44:27Z

src/lints/enum_no_repr_variant_discriminant_changed.ron

+                        attribute @optional {
+                            content {
+                                base @filter(op: "!=", value: ["$repr"])
+                            }
+                        }


I don't think we want @optional here. Try adding an enum that has two non-repr attributes on it (e.g. #[non_exhaustive] and #[allow(dead_code)]) to see why.

There's a subtle difference in Trustfall between "if it has an attribute, it isn't repr" (AKA @optional and @filter like here) and "it doesn't have repr" (AKA @fold @transform(op: "count") ...). The difference is around what happens on edges with cardinality greater than 1: in the former case we have a "left outer join" in relational terms, and in the latter (because of the @fold aggregation), we don't.

obi1kenobi · 2024-09-06T21:44:35Z

src/lints/enum_no_repr_variant_discriminant_changed.ron

+
+                        variant {
+                            variant_name: name @output @tag
+                            discriminant @optional {


Why @optional here?

obi1kenobi · 2024-09-06T21:45:18Z

src/lints/enum_no_repr_variant_discriminant_changed.ron

+                        attribute @optional {
+                            content {
+                                base @filter(op: "!=", value: ["$repr"])
+                            }
+                        }


It shouldn't matter if the enum now gained a repr, the API is still broken.

obi1kenobi · 2024-09-06T21:47:19Z

src/lints/enum_no_repr_variant_discriminant_changed.ron

+        "repr": "repr",
+        "true": true,
+    },
+    error_message: "The public enum's variant had its discriminant changed from its previous value. This breaks implementors that relied on the discriminant value.",


Let's make sure to be very specific about what broke, and precise with terminology. Enums aren't traits so they don't have implementors.

Suggested change

error_message: "The public enum's variant had its discriminant changed from its previous value. This breaks implementors that relied on the discriminant value.",

error_message: "The enum's variant had its discriminant value change. This breaks downstream code that used its value via a numeric cast like `as isize`.",

obi1kenobi · 2024-09-06T21:50:20Z

src/lints/enum_no_repr_variant_discriminant_changed.ron

@@ -0,0 +1,78 @@
+SemverQuery(
+    id: "enum_no_repr_variant_discriminant_changed",


Naming is hard. This is not the best name, and not the worst. Nothing better comes to my mind, but if you have other suggestions, it would be good to consider them.

obi1kenobi · 2024-09-06T21:51:03Z

Thanks for working on this tricky lint!

dmatos2012 · 2024-09-10T21:30:18Z

Hopefully this addresses all the changes requested :)

But there is still a pending task, the renaming of the actual lint. Ill make the renames in the code once we come up with a good one, but for now i left since didnt want to rename everything too many times.

Possible ones:

enum_variant_discriminant_changed_api_only_breakage
enum_variant_discriminant_changed_no_abi_changes
enum_variant_discriminant_changed_api_only

I dont know as you said, naming is hard and particularly this one. Specially since we would like to keep naming consistent with the other lints.

obi1kenobi

This lint name is fine, let's keep it. I spent a bunch of time thinking about alternatives, and couldn't come up with anything that didn't have worse downsides.

This PR is super close, it needs maybe one more round of polishing. I think there might be a bug in the query (which might be hidden by a feature by the underlying rustdoc plugin). There's also some imprecise language and use of terminology. Otherwise, good to go! 🚀

A quick meta suggestion outside this PR: consider going through past PRs, making a list of the changes I've suggested in reviews in the past, and using it as a checklist for things to self-review more closely before opening a new PR. With that list, you could catch pretty much everything I've commented on so far — and that's a great way to improve your skills and get more PRs done in less time.

obi1kenobi · 2024-09-16T01:32:15Z

src/lints/enum_no_repr_variant_discriminant_changed.ron

+                        span_: span @optional {
+                            filename @output
+                            begin_line @output
+                        }


Can we grab the span of the variant instead of the span of the enum? It's nicer to point to Example::Variant instead of to enum Example.

obi1kenobi · 2024-09-16T01:34:08Z

test_crates/enum_no_repr_variant_discriminant_changed/new/src/lib.rs

+// This now implies that the enum is no longer `well-defined`, which means that a numeric
+// cast is no longer possible on the enum, should not be reported.


Suggested change

// This now implies that the enum is no longer `well-defined`, which means that a numeric

// cast is no longer possible on the enum, should not be reported.

// This now implies that the discriminant is no longer `well-defined`, which means that a numeric

// cast is no longer possible on the enum. Should not be reported.

Please try to use precise, clear language and good grammar as much as possible. Long sentences with lots of commas are hard to read.

obi1kenobi · 2024-09-16T01:35:14Z

test_crates/enum_no_repr_variant_discriminant_changed/new/src/lib.rs

+    Second = 2,
+}
+
+// Discriminant changed to be doc hidden and explicit. Being doc hidden is not relevant


Some more imprecise language:

Suggested change

// Discriminant changed to be doc hidden and explicit. Being doc hidden is not relevant

// Variant became doc(hidden) while variant became explicit. Being doc(hidden) is not relevant

obi1kenobi · 2024-09-16T01:35:44Z

test_crates/enum_no_repr_variant_discriminant_changed/new/src/lib.rs

+    Unit = 5,
+}
+
+// Variant `Struct` acquires a field. By doing so, it makes the enum not `well-defined`,


Suggested change

// Variant `Struct` acquires a field. By doing so, it makes the enum not `well-defined`,

// Variant `Struct` acquires a field. By doing so, it makes the discriminant not `well-defined`,

obi1kenobi · 2024-09-16T01:38:54Z

test_crates/enum_no_repr_variant_discriminant_changed/new/src/lib.rs

+#[non_exhaustive]
+pub enum DiscriminantIsChangedButAlreadyNonExhaustive {
+    First,
+    #[non_exhaustive]


Could you add a test case where another variant is #[non_exhaustive], not the one that changes discriminant values?

I think there's a bug in the lint query related to it: the lint is checking for "is the variant with the changed discriminant marked #[non_exhaustive]" when it should be checking for "does the enum have any #[non_exhaustive] variants" — not quite the same thing.

Typos and grammar

dmatos2012 · 2024-09-18T22:20:35Z

Thanks for the detailed feedback, is greatly appreciated. I did the following:

Point span to the actual variant
Add two new test cases to cover the bug you discovered with the non_exhaustive variants
Fix the suggestions on clarity.

Regarding clarity or precision in language: Every new PR either on trustfall itself, or the RustLang nuances, I learn a ton. This means that sometimes I am not clear with the use of terminology, but this might come from the fact that I am not sure myself either, and end up using it plainly wrong or making it unclear. This is where your expertise helps me, so that I can get those right for future PRs and contributions.

Thanks again for the detailed feedback and HOPE that this time is good. This has been a very tricky lint for me, specially if compared with my first two lints :)

obi1kenobi

Go team! Excellent work, I'm thrilled to see you keep improving with every new PR 🚀

obi1kenobi reviewed Sep 3, 2024

View reviewed changes

dmatos2012 added 3 commits September 6, 2024 22:52

Add enum variant discriminant changed lint

0e24e8e

Change human_readable name

583dc61

Change CI test to allow extra failure for this lint

164eff7

dmatos2012 force-pushed the add-enum-variant-discriminant-changed branch 5 times, most recently from f427a45 to b244d6b Compare September 6, 2024 21:01

Change behavior of lint to lint API specific breakage

5b9a24b

dmatos2012 force-pushed the add-enum-variant-discriminant-changed branch from b244d6b to 5b9a24b Compare September 6, 2024 21:12

obi1kenobi reviewed Sep 6, 2024

View reviewed changes

dmatos2012 added 2 commits September 10, 2024 14:33

Merge branch 'main' into add-enum-variant-discriminant-changed

bd62f74

Make modifications to lint and add extra test cases

9cda225

dmatos2012 force-pushed the add-enum-variant-discriminant-changed branch from 57851e5 to 9cda225 Compare September 10, 2024 21:25

obi1kenobi reviewed Sep 16, 2024

View reviewed changes

obi1kenobi mentioned this pull request Sep 18, 2024

New lints: Changes to enum variant discriminants #898

Closed

3 tasks

dmatos2012 added 2 commits September 19, 2024 00:05

Add two test cases to cover non_exhaustive

907454e

Typos and grammar

Merge branch 'main' into add-enum-variant-discriminant-changed

9cb8353

spacing on lint file

23cd52e

obi1kenobi approved these changes Sep 19, 2024

View reviewed changes

obi1kenobi merged commit 6e95110 into obi1kenobi:main Sep 19, 2024
34 checks passed

dmatos2012 deleted the add-enum-variant-discriminant-changed branch September 19, 2024 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add enum variant discriminant changed lint #912

Add enum variant discriminant changed lint #912

dmatos2012 commented Sep 3, 2024

obi1kenobi commented Sep 3, 2024

dmatos2012 commented Sep 3, 2024 •

edited

Loading

obi1kenobi commented Sep 3, 2024

obi1kenobi Sep 3, 2024

dmatos2012 commented Sep 4, 2024

obi1kenobi commented Sep 4, 2024

dmatos2012 commented Sep 6, 2024

obi1kenobi Sep 6, 2024

obi1kenobi Sep 6, 2024

obi1kenobi Sep 6, 2024

obi1kenobi Sep 6, 2024

obi1kenobi Sep 6, 2024

obi1kenobi Sep 6, 2024

obi1kenobi commented Sep 6, 2024

dmatos2012 commented Sep 10, 2024

obi1kenobi left a comment

obi1kenobi Sep 16, 2024

obi1kenobi Sep 16, 2024

obi1kenobi Sep 16, 2024

obi1kenobi Sep 16, 2024

obi1kenobi Sep 16, 2024

dmatos2012 commented Sep 18, 2024

obi1kenobi left a comment

	human_readable_name: "Public enum's variant had its discriminant changed from its previous value",
	human_readable_name: "enum variant had its discriminant change value",

		EXPECTED="$(echo -e "--- failure auto_trait_impl_removed: auto trait no longer implemented ---\n--- failure enum_no_repr_variant_discriminant_changed: enum variant had its discriminant change value ---")"
		RESULT="$(cat output \| grep failure)"

	error_message: "The public enum's variant had its discriminant changed from its previous value. This breaks implementors that relied on the discriminant value.",
	error_message: "The enum's variant had its discriminant value change. This breaks downstream code that used its value via a numeric cast like `as isize`.",

		@@ -0,0 +1,78 @@
		SemverQuery(
		id: "enum_no_repr_variant_discriminant_changed",

		// This now implies that the enum is no longer `well-defined`, which means that a numeric
		// cast is no longer possible on the enum, should not be reported.

	// Discriminant changed to be doc hidden and explicit. Being doc hidden is not relevant
	// Variant became doc(hidden) while variant became explicit. Being doc(hidden) is not relevant

	// Variant `Struct` acquires a field. By doing so, it makes the enum not `well-defined`,
	// Variant `Struct` acquires a field. By doing so, it makes the discriminant not `well-defined`,

Add enum variant discriminant changed lint #912

Add enum variant discriminant changed lint #912

Conversation

dmatos2012 commented Sep 3, 2024

obi1kenobi commented Sep 3, 2024

dmatos2012 commented Sep 3, 2024 • edited Loading

obi1kenobi commented Sep 3, 2024

Choose a reason for hiding this comment

dmatos2012 commented Sep 4, 2024

obi1kenobi commented Sep 4, 2024

dmatos2012 commented Sep 6, 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

obi1kenobi commented Sep 6, 2024

dmatos2012 commented Sep 10, 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

dmatos2012 commented Sep 18, 2024

obi1kenobi left a comment

Choose a reason for hiding this comment

dmatos2012 commented Sep 3, 2024 •

edited

Loading