Add lint for trait default impl removed

[dmatos2012]

Should add another lint for #870, particularly non-sealed trait removed a default impl for a trait method. Fixes #294.

I am not sure if the object_safe @filter(op: "=", value: ["$true"]) line is needed, since otherwise it would raise another lint. I thought that it could be a False Positive, since at least if i try it locally, I only get the warning if it returns Self, like so:

struct Test;
pub trait Trait {
    // fn method(&self) -> Self; #cargo complains about trait obj safety
    fn method(&self); # cargo is fine with it
}
impl Trait for Test {}

Anyways, otherwise any feedback happy to change. Thanks!

[obi1kenobi]

This is very close, and I'm excited to merge it!

Re: the object safety issue you mentioned, I suspect you might accidentally have had the -> Self method in the new arm of the test, and a method that returns nothing in the old arm of the test. That is consistent with the object-safety lint you observed, and would have been eliminated by the extra clause you added in the lint.

The object_safe clause is not necessary, and in fact would prevent this lint from triggering in valid cases where a non-object-safe trait had a removed default impl.

A few action item suggestions:

• add a test case where a non-object-safe lint has the default impl removed, and make sure the lint triggers correctly
• re-read your PR in the GitHub code review UI, to help you spot and fix little typos that are easy to miss when we look at code in our own editor
• try to improve the readability and consistency of the lint query with other lint queries, with respect to spacing, trailing spaces, etc.

[dmatos2012]

Yeah, thanks for those suggestions, my bad. I normally do these things, and tend to be careful but this time I was actually in a rush to get it in, before I got busy with something else that I really overlooked doing those details

• I have added the test case for obj_safe to make sure it triggers properly
• Fix typos and spacing etc in lint.

Thanks again for feedback!

[obi1kenobi]

We're super close here! 🙌

I thought of an interesting edge case while reviewing the lint query, and I think we want to make a small tweak in approach to handle it properly.

There are also a few more typos, test case improvements, and general little points of friction that you could practice catching on your own by self-reviewing your code in the GitHub UI. The fewer comments and fixes I need to apply in code review, the faster and more efficiently we'll both ship code. Fixing things in code review is very inefficient results in fewer/slower merged PRs, so it can be frustrating and wasteful for you, me, and the community as a whole.

That said, don't worry about things that have already happened, and just seek to improve going forward! Becoming better every day is the goal, everything else is icing on the cake. As long as you're interested in improving your skills, I'll be happy to point you in the right direction to do so quickly and efficiently.

Go team! I think we're one last iteration away from shipping this 🚀

[dmatos2012]

I am 100% willing to keep improving my skills, so I appreciate the feedback. I will be more careful on the next PRs , so that the content of discussion should be the technical sides, not the typos as i can imagine that's extremely frustrating. Funny because normally I contribute code, and here is mostly wording thus making it more difficult 😛

But on test cases from your suggestions, I added:

• Trait becomes sealed and has default impl removed
• Trait is partially sealed and has default impl removed
• Trait is fully sealed and has default impl removed

I hope that this one should be the one. Thanks for the patience!!

[obi1kenobi]

Excellent work! Merging 🚀 Thanks for putting this together!

[obi1kenobi]

This is an excellent explanation, well done!

[obi1kenobi]

Btw, I recently created a Discord server to make it easier to coordinate cargo-semver-checks development. If you'd like to join, we'd love to have you there! Here's an invite link (active for 7 days max): https://discord.gg/s7Cfmp3P