Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New test crate: method moved to trait #[must_use] added #282

Merged

Conversation

SmolSir
Copy link
Collaborator

@SmolSir SmolSir commented Jan 10, 2023

The purpose of this test crate

is to avoid duplicate lints. In this crate, there could be reports about both #[must_use] being added to the methods, as well as the methods themselves being moved to a Trait. Because both of these are minor changes, and the #[must_use] added violation is impossible to achieve here without the method_moved_to_trait check failing, we want the #[must_use] checks to not find any changes, but expect a failure on the method_moved_to_trait check.

From the #[must_use] attribute addition point of view:

This test crate mainly contains cases where a breaking change happens:

  • in the old version, the method Foo is a public, inherent method of a public Struct, with or without the #[must_use] attribute,
  • in the new version, the method Foo is moved to a public Trait, with addition, deletion or change of the #[must_use] attribute inside the Trait.

There ale also selective test cases where the change is not breaking:

  • the same scenario as above, but with private Struct & Trait,
  • the same scenario as above, but the attribute is not present inside the Trait; instead #[must_use] modification takes place inside an impl Trait for Struct block, (1)
  • adding a new public Trait that implements a new public Struct (both are not present in the old crate version).

(1) - this is a separate test case because such addition does not actually cause the compiler to give a warning when the return value is not used. Only the #[must_use] attribute modifications that are done inside the Trait affect it, thus these are technically non-breaking and were separated into their own test case.

From the method_moved_to_trait point of view:

Once the lint is implemented, all of the test cases should be reported except for:

  • the private Struct & Trait scenario from above,
  • adding a new public Trait that implements a new public Struct (both are not present in the old crate version).

@SmolSir SmolSir linked an issue Jan 10, 2023 that may be closed by this pull request
@SmolSir SmolSir marked this pull request as ready for review January 10, 2023 21:23
@SmolSir SmolSir marked this pull request as draft January 13, 2023 18:23
@SmolSir
Copy link
Collaborator Author

SmolSir commented Jan 13, 2023

This was converted to draft because of the problem mentioned here #283 (comment) (enums & unions also can have methods, this test crate only contains struct tests). I would like to add similar tests for the two missing types once Union is added to the schema. Should I split the PR into smaller per-type PRs with smaller crates to allow faster, smaller merges, or just add the missing types here?

@obi1kenobi
Copy link
Owner

Should I split the PR into smaller per-type PRs with smaller crates to allow faster, smaller merges, or just add the missing types here?

Either way is fine by me. As a hybrid option, you could split the structs / enums / unions test cases into different files in the same test crate, which will make the overall structure simpler to review while keeping these conceptually-related tests in the same crate and PR.

@obi1kenobi
Copy link
Owner

Check out #290 for something else that's good to add to new test crates: just a little safeguard against an erroneous cargo publish accidentally publishing them to crates.io.

@SmolSir SmolSir marked this pull request as ready for review January 14, 2023 22:01
@obi1kenobi obi1kenobi merged commit 5c92270 into obi1kenobi:main Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding #[must_use] on an item
2 participants