add lint trait_associated_const_added

[mrnossiom]

Lint for "non-sealed trait added associated constant without default value"

See #870

This is my first semver-checks lint. I'm unsure how to proceed to check how a default value has disappeared for a given associated constant.

I think I need to check if the default value of the assoc const is still null for the current crate. Looking at other lints, I should be able to do that with fold and filter?

Could someone provide a little guidance, so I can better understand how diff queries works?

Thanks

[obi1kenobi]

a default value has disappeared for a given associated constant

Just to make sure, is this lint looking for "the const's default has disappeared" or "there's a new const that didn't exist before, and doesn't have a default"?

Your query is very close either way, and I'll be happy to point you in the right direction.

There are also a couple of filter operators that might be useful: "is_null" and "is_not_null", which you can use like this:

property @filter(op: "is_null")

[mrnossiom]

Lint is looking for "there's a new const that didn't exist before, and doesn't have a default".

How can I check the addition of an assoc const?

[obi1kenobi]

Try framing the query like this:

• in the new version, there exists a const without a default
• the old version has no const by that name on that trait

For the second part: "X does not exist" is a @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) clause sequence on the edge that produces the X. We specifically want to say "the same trait exists but the named const doesn't exist" so be mindful of where those clauses go. This is important because we don't want the lint to trigger if the trait is new and didn't exist before — adding a new trait with a const is not a problem.

I think you already have the first part just about working in your PR.

So minimal changes needed — you're super close.

[obi1kenobi]

Lmk if you'd like more in-depth suggestions than I gave in my last post!

No pressure to move this forward, it's totally fine if you had other things to do etc. I just want to make sure you aren't blocked because I did a bad job of explaining stuff :)

Feel free to ping me with more questions anytime!

[mrnossiom]

Just didn't have time these last days. Simply switch the query parts made the tests pass with logical results.

[obi1kenobi]

Nice! The new query looks good.

A few remaining things to look into:

• Figure out the remaining TODOs in the query file.
• Consider using @filter(op: "is_null") and @filter(op: "is_not_null") instead of @filter(op: "=", value: ["$null"]). They both work, but the former is a bit more idiomatic.
• Add some more test cases, making sure that the lint always triggers when expected and never triggers when not expected. For example:
  • if the trait was previously sealed, adding a const doesn't trigger the lint
  • if the trait was not previously sealed, sealing it in the new version + adding a const still triggers the lint
  • adding a const with a default value doesn't trigger the lint, regardless of whether the trait was sealed
  • if the trait already had at least one const, adding a second one still triggers the lint when appropriate

[mrnossiom]

I don't know where to look for the reference_link.

[obi1kenobi]

The cargo SemVer reference, and the API evolution RFC 1105, are the two authoritative documents on SemVer in Rust.

I suggested a link to the "trait added non-defaulted item" section, which encompasses this lint.

[obi1kenobi]

If a trait is being annoying in tests and you want to make it not object-safe when it doesn't have a const already, just make a method that returns -> Self. That also makes it not object-safe, and adding a const to it won't cause the extra SemVer breakage.

[mrnossiom]

I would love to be able to bless tests

[mrnossiom]

I'll remove the comments in test_crates/trait_associated_const_added/new/src/lib.rs, but can you check that I've annotated the traits that should trigger the lint?

[obi1kenobi]

These are actually making the trait stop being object-safe, just like having a const does.

Perhaps a better name:

Suggested change

	fn make_me_object_safe() -> Self;
	fn make_me_non_object_safe() -> Self;

[obi1kenobi]

I would love to be able to bless tests

We have an open issue for switching to insta for lint snapshot testing: #688

When I originally started cargo-semver-checks, insta wasn't yet available nor widespread 😅 Would love your help with it, if that sounds interesting!

[obi1kenobi]

A couple of minor typo fixes, plus some edge cases with the lint I just noticed. Sorry I missed these earlier!

[obi1kenobi]

Awesome! 🚀